fix: handle rollover and don't set wrong timing info for segments with high PTS/DTS values #1040
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
Add test for baseStartTime
gkatsev
reviewed
Jan 5, 2021
| @@ -41,6 +41,83 @@ Copy only the first two audio frames, leave out video. | |||
| $ ffmpeg -i index0.ts -aframes 2 -vn -acodec copy audio.ts | |||
| ``` | |||
|
|
|||
| ### videoMinOffset.ts | |||
| @@ -2067,26 +2067,31 @@ export default class SegmentLoader extends videojs.EventTarget { | |||
| }); | |||
| } | |||
|
|
|||
| handleVideoSegmentTimingInfo_(requestId, videoSegmentTimingInfo) { | |||
| handleSegmentTimingInfo_(type, requestId, segmentTimingInfo) { | |||
There was a problem hiding this comment.
It took me a second to realize that type got passed in here as the first argument. Was wondering how it was being used below 😆
| previousSegment.end && | ||
| previousSegment.timeline === segment.timeline) { | ||
| simpleSegment.baseStartTime = previousSegment.end + segmentInfo.timestampOffset; | ||
| const previousSegment = segmentInfo.playlist.segments[segmentInfo.mediaIndex - 1]; |
There was a problem hiding this comment.
have we never used the previous segment here then? oops
gkatsev
approved these changes
Jan 6, 2021
|
I've tests this, and it's good to go! |
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.
Note that this requires: videojs/mux.js#364
Description
Originally, when the sync-controller was written, and TS segments were probed for timing information, the player used a state variable called
inspectCache_for the last DTS time in either audio or video info, to use in the next probe. This value would be cleared on sync-controllerreset, called whenever the timeline changed (crossing a discontinuity), or the timestampOffset of the new segment was less than the current one being requested.See the use of
inspectCache_in: https://github.com/videojs/videojs-contrib-hls/blob/1298f5192953366d86d98d5660a16cc1fa718521/src/sync-controller.jsWhen TBA was added (see: #494), and probing started happening within media-segment-request instead of sync-controller, no inspectCache_ was kept, but instead
baseStartTimewas added to the segment object passed to media-segment-request to be used in the probe. However, the value, instead of reflecting the end DTS of the previous segment, started using the previousSegment end time + the timestamp offset of the new segment. The value was then multiplied by 90,000 for a 90khz clock, since the value was in seconds. In theory, this value should be OK, as the previous segment end time + the timestamp offset should reflect the original media's timestamp. The problem is that the timestamp offset will not be set when the timeline isn't changing and will instead be null. So then rollover will be detected, and the timestamp of the segment incorrectly changed.For example:
segment 1:
player time: 100 => 110
media time: 100,000 => 100,010
timestampOffset (media time minus player time): (100,000 - 100) = 99,900
segment 2:
player time: 110 => 120
media time: 100,100 => 100,020
timestampOffset: null
If we are requesting segment 2, then we grab the previous segment end time of 110. If we added the timestampOffset of 99,900 to it, we'd have 100,100, which would be correct. However, since timestampOffset is null for standard walk forwards, the value would be passed as 110. The rollover check uses a threshhold of 4294967296, or 47,721.8588444 in seconds (see: https://github.com/videojs/mux.js/blob/main/lib/m2ts/timestamp-rollover-stream.js#L18) as its check to tell whether the timestamps of the media should be adjusted. Since the media time for these segments was a large value, and the timestamp offset was not included, the rollover detection will adjust timestamps improperly, leading to negative values. This is why we only see this issue with streams containing large PTS/DTS values.
One approach would be to always provide a timestampOffset value, however, this might also run as into trouble depending on whether that value is a decode time or a presentation time, and we sometimes estimate a timestampOffset.
Instead, since we already have access to the actual segment media time values passed up from the transmuxer in segment-loader, it's easiest to use those true values when available. This PR uses those. This PR also makes use of videojs/mux.js#364, which adds audio timing information (we already had video timing information) from the transmuxer, in order to handle audio only segments.
One additional fix made was an incorrect previousSegment reference in the old code. This has been updated to actually reflect the previous segment.
Requirements Checklist