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: Linear DASH multiperiod label issue #1352

Merged
merged 8 commits into from
Dec 20, 2022

Conversation

adrums86
Copy link
Contributor

@adrums86 adrums86 commented Dec 3, 2022

Description

Fixes Linear DASH SSAI Label Issue
Linear multi-period DASH with <Label> elements that roll in and out of continuous AdaptationSets cause the stream to stall because we use the value of the <Label> as the key for the mediaGroups.type.group.<labelKey> object and the associated mediaGroup group/playlist ID. When the manifest updates and reaches the period either with a <Label> or omitting the <Label> we can no longer access that media group or playlists to update the playlist as we rely on the labelKey set from the previous manifest/period to look for new segments in the update.

Specific Changes proposed

  • Change the way we create groupIDs for mediaGroup playlists to allow for DASH playlists to use the representation ID rather than the label from the parser. Per DASH IF, continuous media in separate periods share the same representation IDs across period boundaries, so this change can easily utilize that feature to ensure our playlistIDs are consistent between continuous periods.
  • Add the new labelKey to the mediaGroups.type.group object and add the playlist reference.
  • Remove any old labelKeys from the mediaGroups.type.group object that no longer exist in the new manifest update.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

@adrums86 adrums86 self-assigned this Dec 3, 2022
@welcome
Copy link

welcome bot commented Dec 3, 2022

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@codecov
Copy link

codecov bot commented Dec 3, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@afb1ff7). Click here to learn what that means.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1352   +/-   ##
=======================================
  Coverage        ?   86.01%           
=======================================
  Files           ?       40           
  Lines           ?     9842           
  Branches        ?     2294           
=======================================
  Hits            ?     8466           
  Misses          ?     1376           
  Partials        ?        0           
Impacted Files Coverage Δ
src/dash-playlist-loader.js 90.33% <100.00%> (ø)
src/manifest.js 93.57% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

* The new mpd object
*/
const removeOldMediaGroupLabels = (update, newMain) => {
forEachMediaGroup(update, (_, type, group, label) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change _ to properties to be consistent with other forEachMediaGroup() calls in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I have an old lint-driven habit of using _ for any unused parameters. Consistency > arbitrary lint rules IMO.

Copy link
Contributor

@wseymour15 wseymour15 left a comment

Choose a reason for hiding this comment

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

Looks good! Just have that one optional change.

src/dash-playlist-loader.js Outdated Show resolved Hide resolved
@adrums86 adrums86 merged commit d7e8713 into videojs:main Dec 20, 2022
@welcome
Copy link

welcome bot commented Dec 20, 2022

Congrats on merging your first pull request! 🎉🎉🎉

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.

Linear DASH SSAI Label Issue
4 participants