Remux according to initial PTS so that streams start at 0 #2030
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robwalch
previously approved these changes
Dec 6, 2018
There was a problem hiding this comment.
The other issue this fixes is hlsjs.currentLevel will always return the level of the first fragment buffered when currentTime = 0. We need media to be buffered at 0 and not at the difference between the first frame's pts and dts.
The alternate solution would be to change all helpers that look for fragments based on pts range to use dts range, and to append based on start dts not pts so that we always have VOD content buffered at 0.
michaelcunningham19
requested changes
Dec 6, 2018
There was a problem hiding this comment.
Looks like the required functional tests on Travis are failing on a few scenarios.
Chrome 70 / Windows 10:
68 passing (24m)
3 failing
1) testing hls.js playback in the browser on "chrome (latest), Windows 10" should receive video loadeddata event for DK Turntable, PTS shifted by 2.3s:
2) testing hls.js playback in the browser on "chrome (latest), Windows 10" should play DK Turntable, PTS shifted by 2.3s:
3) testing hls.js playback in the browser on "chrome (latest), Windows 10" should seek 5s from end and receive video ended event for DK Turntable, PTS shifted by 2.3s:
Each failed test has a large amount of the following logs - looks like it's stuck:
2018-12-06 00:01:20.783: "[log] > Loading 0 of [0 ,40],level 3, currentTime:0.000,bufferEnd:0.000"
2018-12-06 00:01:20.867: "[log] > Loading 0 of [0 ,40],level 3, currentTime:0.000,bufferEnd:0.000"
2018-12-06 00:01:20.967: "[log] > Loading 0 of [0 ,40],level 3, currentTime:0.000,bufferEnd:0.000"
johnBartos
force-pushed
the
bugfix/start-stream-at-pts-0
branch
from
954c360
to
1e464c3
Compare
|
@michaelcunningham19 Thanks for the review, I'll check it out rn |
|
@michaelcunningham19 Must be a flaky test, it plays fine on a physical W10/Chrome 70 machine |
|
@johnBartos cool! |
michaelcunningham19
previously approved these changes
Dec 6, 2018
|
Not so fast! //cdn-videos.akamaized.net/btv/desktop/fastly/us/live/primary.m3u8 |
tchakabam
reviewed
Dec 7, 2018
| // } | ||
| // logger.log(avcSample.pts + '/' + avcSample.dts + ',' + unitsString + avcSample.units.length); | ||
| // } | ||
| let offset = 8; |
tchakabam
previously requested changes
Dec 7, 2018
There was a problem hiding this comment.
@johnBartos looks like a fix! :) but i would like to check with some test streams and look a bit more in detail before it gets merged if that's ok?
|
@tchakabam Sure! I've been testing these changes for the past few days; I ran a live stream with known PTS/DTS issues overnight and it was fine in the morning. |
|
it definitely totally makes sense. two streams can have different packet/frame order at the DTS levels relative to their decoding specifics but in the end what matters to sync is the presentation time. also one can't rely at all actually on two streams having similar gop dependency structure i.e the same PTS/DTS diffs i.e CTOs ("composition time offsets"), for example low quality streams usually don't use B-frames (making re-ording unnecessary for them ie they have DTS=PTS), but high-profile encoding does usually need it. what i mean is: DTS isn't a way to sync/compare anything across streams, PTS is. And it works even less when comparing timestamps of audio and video streams, since audio doesn't use predictive frames at all usually. So, yeah great catch really!! 👍 👍 👍 might explain quite a few reliability issues! |
5 tasks
|
@tchakabam We've been testing this for a few days and no issues have cropped up. Have you found anything in your tests? |
johnBartos
dismissed
tchakabam’s stale review
Need to cut the release; happy to address any failures found after merging
|
@johnBartos All good, sorry for lacking the response! |
4 tasks
johnBartos
added a commit
to jwplayer/hls.js
that referenced
this pull request
Feb 22, 2019
* isCodecSupportedInMp4 test audio codecs as audio Fixes video-dev#1824. * Do not show `undefined` * package: rm npm-run-all as it contains "event-stream" (and maybe other dormant exploits?). see "flatmap-stream" * rewrite package-lock: removed event-stream and all deps got not fixed versions instead of a hat * update lock-file generated with npm v6.4.1 * package deps: npm remove --save mversion jshint * Added OpenPlayerJS to the list of players that support HLS.js (video-dev#2023) * NIT: fix typos in readme 1. Fix spelling of interdependent and environment 2. Correct the agreement in the phrase: 'imported/required by each other.' * Revert video-dev#2004 (video-dev#2027) * Fix 0.11.1 Regressions (video-dev#2028) * Create sourceBuffers if the max of 2 tracks has been reached, regardless of buffer codec events received * Replace Object.values with [].slice.call * Remux according to initial PTS so that streams start at 0 (video-dev#2030) * Buffer streams starting at 0 initPTS * Use PTS to determine if segments are contiguous * Remove code setting startPosition to buffer start * Remove seek to buffered start logic * Simplify EOS check by using frag states, move logic into shared superclass (video-dev#2040) * Simplify EOS check by using frag states, move logic into shared superclass * Move shared seek logic into base, clear fragCurrent on seek out of buffered range * Revert PTS wrapping code which produces incorrect cue times (video-dev#2044) * deploy demo/docs to netlify, not gh-pages * update demo links in issue template * format commit as code because it is slightly more readable * remove accidental "/" in readme * update note to access specific version api docs * include tag in idShort * escape ` * Check if PTS has wrapped after applying PTS time offset (video-dev#2056) * Check if PTS has wrapped after applying PTS time offset * Fix bug accessing buffered track array * Intialize vttCCs map with cc 0 (video-dev#2058) * Intiialize vttCCs map with cc 0 * use the version from package.json when deploying and include this in the deployments readme * fix: abrFactor greater than in docs * Changes clone function used in webpack config to be the webpack recommended merging library. * prod: add bandwidthEstimate method * docs: add bandwidthEstimate * test: add autotest * Convert buffer-controller.js to TypeScript Thank you to Tom Jenkinson for acting as a reviewer for this work, including writing some suggestions via the GitHub PR github.com/video-dev/pull/2073. The commit history has been squashed as it was a fairly lengthly process to get everything working to support the conversion to TypeScript. Early in the history of this commit, we converted properties to be initialized to `null`. This was incorrect, as it changes the behaviour of code that uses `Object.keys` calls to check if an object has any keys on it. Before, they would be empty, but if the properties are set to null, this check is no longer valid and it changes the behaviour. This is something to look out for in the future of other conversions, as the values themselves are often times reset to the `null`, but begin as undefined. Added a common `types` folder for interfaces where the ownership of the type wasn't clear for the individual file. The default no-op logger which is the inferred usage did not have any arguments in it's method signature. By adding an argument, TypeScript is able to infer the correct interface. As well the Karma and Webpack configuration had to be updated to enable building TypeScript, and getting testing coverage information from Istanbul. Define lib "es2015" and "dom" to add HTMLMediaElement, SourceBuffer and Number.isFinite definitions to the TypeScript binary when it is doing type-checking. When attempting to lint the older version of `typescript-eslint-parser` would emit `no-undef` errors for TypeScript interfaces. Updating the package fixes this issue. Tom pointed out that we should not use global module declarations to extend default HTML types since users will eventually be importing *.d.ts files from the hls.js project and this would also extend their types. Due to this feedback switched to defining a module local type `ExtendedSourceBuffer`. Which instead extends with the `ended` prop. Event Handlers were set to be part of the private scope. While this has no effect now, it will be important to ensure other modules do not directly invoke the event handlers. Other methods that were prefixed with a `_` were also set into the private scope. Where possible, code was switched to early-exit to lower the cognitive load in conditional statements, an example of this was the doAppend method. Another cleanup statement was switching handling of error codes and the comments to be closer to the conditional to which they applied. Switched to using the Record<Key, Value> type to help support access via the index signature [key: T] syntax in the future. An issue that was fixed in this commit was that The controller previous exposed the local `pendingTracks` property through the event and did not set the buffer on `this.tracks` instead opting to set it on the individual track, which bubbled up to the pendingTracks object, whose reference was lost in the buffer-controller module. This commit changes this behaviour so that the controller publishes `tracks` from the public variable on the controller rather than the pendingTracks which are passed into createSourceBuffers. This also means that this fixes the `.buffer.buffered` being undefined on the demo page. Added unit tests to cover the new behaviour of throwing an error if createSourceBuffer is called without an attached media element. * test: review changes * docs: review fix * prod: add review fix * docs: update Co-Authored-By: Beraliv <beraliv.spb@gmail.com> * docs: update 2 * docs: fix link * test: equal => strictEqual * test: add mocked _bwEstimator * test: change description * test: (() => { ... }) => function () { ... } * Increase BufferHelper test coverage to 100% (video-dev#2052) * Increase adts.js test coverage to 100% (video-dev#2053) * Increase adts.js test coverage to 100% * Convert eme-controller to TypeScript. Added TypeScript types to the code. Handled media detached and removing the encrypted event listener. * Listen to media detached Clears the encrypted event listener from the HTMLMediaElement in controller, and releases the stored reference. * Choose bitrate in Bps (video-dev#2091) Simplify bandwidth calculation by making; change docs to correctly reference bandwidth in bits/s * Upgrade tests to Chai BDD & optimize runner (video-dev#2037) * Convert tests to Chai BDD * Invoke mocha with exit so that travis doesnt hang on func test completion * Fix video-dev#2061 * configure netlify for PR's this follows the same process that runs for canary releases * check if repo is shallow before fetching because on netlify it clones the whole thing * handle 'netlifyPr' in set-package-version * tabs => spaces * add commit to version when PR * always trim command output * Update Fragment and LevelKey to typescript. * Update usage of Fragment and LevelKey. * Don't pass prevFrag into EXT-X-MAP handling. * Add tests for parsing byte range as seperate call. * Only parse byte range if it exists. * Apply suggestions from code review Co-Authored-By: itsjamie <1956521+itsjamie@users.noreply.github.com> * Review changes. parseByteRange -> setByteRange - removed it returning the parsed value since its usage was like a setter. * Review comments pt.2 * Change tests to call setByteRange * Explicit handling of 'initSegment' conversion for bitwise operation. * Fix lint warnings in m3u8-parser.js in prep for TS conversion. (video-dev#2106) * fix console null reference in logger (in webworkers) (video-dev#2095) Fix null reference in logger when console is not available * (misc) Adding ace editor to demo to enable advanced customization (video-dev#2077) * Add handling for AES-128 encrypted initialization segments needing IV. * Change IV handling for initSegments to just log a warning. * Update webpack to use babel loader with support for TS. (video-dev#2119) * Upgrade to webpack 4, babel 7 * Use straight git link for webworkify dep * Use simpler git url * Re-add globalObject workaround * Bump karma dep versions * Dont preprocess non-test files * Use @babel/register over ts-node * Update demo `const` usage. * Use webpack debug config in Karma configuration rather than duplication. * Delete duplicate declaration
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will...
initPTSinstead ofinitDTSinitPTSstartPTSinstead ofstartDTSas a timeoffset when availableWhy is this Pull Request needed?
So that all streams begin at their intended start position. This PR ensures that audio/video elementary streams begin at the same time by fixing starting video at
initPTS, and flushing audio to video by inserting silent audio or dropping extra frames. Since all video now starts at the expected start time, we don't need to setthis.startPosition = buffered.start(0).Are there any points in the code the reviewer needs to double check?
@dsparacio Tomorrow I'll do some testing on Edge to make sure that the bug you fixed is still OK.
Looking back through the annotations, the original reason for this work was to fix Safari issues with audio/video sync. I tested https://d2u3lzih3uhb7x.cloudfront.net/ubicasttv/680f426d3b4448f8955a051247473f2d/v125867c07c77ws1reji_720.m3u8 on this branch and master and it doesn't seem any different.
Resolves issues:
Found during regression testing of 0.11.1. Because videos were not starting at 0, certain ad plugins were not functioning correctly with preroll ads.
#1880
Checklist