Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix program date time handling #45

Merged
merged 4 commits into from Mar 2, 2018

Conversation

mjneil
Copy link
Contributor

@mjneil mjneil commented Feb 24, 2018

The previous version of m3u8-parser had a bug that would associate the last EXT-X-PROGRAM-DATE-TIME tag with the start of the playlist.

This could cause problems in live streams with the following properties

  • ONE of:
    1. EXT-X-PLAYLIST-TYPE:EVENT
    2. a playlist with a sliding window, but viewed as the initial window is being filled, i.e. the playlist is still growing and segments are not yet being removed from the front
  • Every segment has its own EXT-X-PROGRAM-DATE-TIME tag

When these conditions are met, each playlist refresh causes the SyncController to believe that there is a new sync point at the start of the playlist shifted by an amount that is equal to the amount of time added by new segments at the end. Since the playlist window is not actually sliding yet, the sync point for the start of the playlist should not be changed. This could cause issues when seeking backwards and then forwards. This is because seeking backwards causes a new timestampOffset to be set (because the segment that is being seeked to starts before the currently set timestampOffset). When timestampOffset changes, the SyncController also updates its PTS to Display time mapping. Since the SyncController has an incorrect sync point as described above, the mapping that gets set is incorrect. Seeking forward again after this can cause the player to select a segment that is beyond the playhead and stall indefinitely.

This PR updates the m3u8-parser version, which attaches date time info on each segment for each tag, instead of just on the playlist object. The ProgramDateTime strategy in SyncController will now look for the nearest segment to currentTime with date time information for an accurate sync point.

Copy link
Contributor

@gesinger gesinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to have a test with a segment's dateTimeObject where the segment isn't the first one and one with multiple segments with program-date-time tags.

if (!this.datetimeToDisplayTime &&
playlist.segments &&
playlist.segments.length &&
playlist.segments[0].dateTimeObject) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we only want to consider the first segment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yes. The intention of this date time mapping that is being stored is supposed to be the offset of real time to display time for the start of stream (at time of join, i.e. first segment in first playlist). If a segment other than the first is used, you'd have to account for the duration of the segments that came before it in the playlist when trying to calculate sync points from it. This has the potential for inaccuracies

break;
}

if (!syncPoint || lastDistance === null || lastDistance >= distance) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this check here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied this logic from the Segment strategy below, but looking it over again, it doesn't look like it.

@mjneil mjneil merged commit 2609d01 into videojs:master Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants