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

Bugfix/live start bugs #4303

Merged
merged 4 commits into from
Sep 20, 2021
Merged

Conversation

cjpillsbury
Copy link
Collaborator

@cjpillsbury cjpillsbury commented Aug 31, 2021

This PR will...

Resolve edge case issues in initial load for live content, which are more likely with LL-HLS:

If any Media Playlist in a Master Playlist contains an EXT-X-PROGRAM-DATE-TIME tag, then all Media Playlists in that Master Playlist MUST contain EXT-X-PROGRAM-DATE-TIME tags with consistent mappings of date and time to media timestamps.

Are there any points in the code the reviewer needs to double check?

If possible, confirm there aren't any discontinuity-related issues with new alignMediaPlaylistByPDT , or at least none that wouldn't also occur with the original alignPDT usage in onAudioTrackLoaded & onSubtitleTrackLoaded

Resolves issues:

#4302 (Startup on Live Streams sometimes hangs based on requesting the wrong audio and/or video segment)

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md (N/A)

@itsjamie itsjamie self-requested a review August 31, 2021 18:38
Copy link
Collaborator

@itsjamie itsjamie left a comment

Choose a reason for hiding this comment

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

If the problem exists on audio tracks, it will likely exist also on subtitle tracks that exist.

https://github.com/video-dev/hls.js/blob/master/src/controller/subtitle-stream-controller.ts#L254

I'd check if you need to add similar changes on the subtitle stream controller above to ensure these also align using the new mechanism.

* @param details - The details of the rendition you'd like to time-align (e.g. an audio rendition).
* @param refDetails - The details of the reference rendition with start and PDT times for alignment.
*/
export function alignByPDT(details: LevelDetails, refDetails: LevelDetails) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you document the difference between alignByPDT and alignPDT above. Given the closeness in the implementation, having a description for them would be helpful.

That or a better name for each.

Perhaps alignByPDT could be something like; alignMediaPlaylistByPDT.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup can do both.

@cjpillsbury
Copy link
Collaborator Author

If the problem exists on audio tracks, it will likely exist also on subtitle tracks that exist.

https://github.com/video-dev/hls.js/blob/master/src/controller/subtitle-stream-controller.ts#L254

I'd check if you need to add similar changes on the subtitle stream controller above to ensure these also align using the new mechanism.

Good callout. Not sure if I have a stream available to test, but the reasoning tracks.

…ng further description, and updating subtitle-track-controller to also use for initial load, per PR feedback.
@cjpillsbury
Copy link
Collaborator Author

@itsjamie @robwalch PR approved but still blocked pending "test_functional_required (chrome-win_10)" task/check. Let me know if there's anything I can do to help get that unblocked.

@itsjamie
Copy link
Collaborator

I'm not sure, when I run the job, the sauce labs username/access key isn't present so the check is skipped. @tjenkinson thoughts? I'm not sure where the CI was left here.

@cjpillsbury
Copy link
Collaborator Author

Thanks for the quick response, @itsjamie

@tjenkinson
Copy link
Member

Yeh unfortunately sauce can’t run on forks because it doesn’t get the secrets, so the options are admin override and merge and have the checks run on master or merge this into a separate branch in the repo and open a new pr off that one into master, where the secret will then be there.

@itsjamie
Copy link
Collaborator

Ran the functional tests in #4321 existing tests all passed except for the LL-HLS due to the test stream being down. Going to merge. Thanks @cjpillsbury.

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.

Startup on Live Streams sometimes hangs based on requesting the wrong audio and/or video segment
4 participants