fix: use a separate ProgramDateTime mapping to player time per timeline #1063
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
…nd of a timeline Previously, when the end of a timeline was reached (i.e., the last segment in a given timeline was appended), `endTimeline` would be called, but the loader would not wait for the event to finish. This lead to issues where segment information came back after a segment is considered done with processing, and segment information could be dropped. This fix waits for the `endedtimeline` message from the transmuxer before it considers the transmux complete.
… existsi n program date time
Previously, the ProgramDateTime would map to time 0 at the start of playback, and that mapping would be used for the rest of playback. In case of a discontinuity though, ProgramDateTime can jump (as in, can be a larger difference in time than a single segment). This led to errors while seeking. This change uses a mapping per timeline, allowing those "jumps" in time on discontinuities.
gkatsev
reviewed
Feb 9, 2021
| @@ -161,7 +167,7 @@ export default class SyncController extends videojs.EventTarget { | |||
| // ...for synching across variants | |||
| this.timelines = []; | |||
| this.discontinuities = []; | |||
| this.datetimeToDisplayTime = null; | |||
| this.timelineToDatetimeMappings = {}; | |||
There was a problem hiding this comment.
if segment.timeline is a number, should this just be an array?
There was a problem hiding this comment.
Are the timelines contiguous or would there be a danger of a sparse array? Just thinking that maybe it'll be easier to have it be an array since you could just .length it, but if a segment.timeline could have values like, 0, 1, 100, then it won't work since the length would then be 101.
There was a problem hiding this comment.
Exactly: originally had it as an array, but it can be sparse so changed it.
There was a problem hiding this comment.
It's mainly an issue when either updating a playlist before starting, updating while paused, or if on some refreshes you miss a timeline (a short one).
gkatsev
approved these changes
Feb 10, 2021
brandonocasey
approved these changes
Feb 10, 2021
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.
Description
Previously, the ProgramDateTime would map to time 0 at the start of playback, and that mapping would be used for the rest of playback.
However, in the case of a discontinuity, ProgramDateTime can jump (as in, can be a larger difference in time than a single segment duration, e.g., if the encoder went down for a period of time). Because the ProgramDateTime to player time mapping never changed, this led to seeking errors.
This fix uses a mapping of ProgramDateTime to player time per timeline, allowing those "jumps" in time on discontinuities.
Requirements Checklist