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: Fix multi-codec filtering on DASH live #6647

Merged
merged 2 commits into from
May 30, 2024

Conversation

avelad
Copy link
Collaborator

@avelad avelad commented May 22, 2024

Regression of Preload API

In HLS the definition of variants is only done once and it is the master playlist.
In DASH, the definition of variants is done in each manifest update.
If we make the codec choice only at the beginning, in HLS it works, but in DASH in the updates we lose this choice.

@avelad avelad requested a review from theodab May 22, 2024 11:02
@avelad avelad added type: bug Something isn't working correctly priority: P1 Big impact or workaround impractical; resolve before feature release component: DASH The issue involves the MPEG DASH manifest format labels May 22, 2024
@avelad avelad added this to the v4.9 milestone May 22, 2024
@shaka-bot
Copy link
Collaborator

Incremental code coverage: 100.00%

@avelad avelad requested a review from joeyparrish May 22, 2024 15:52
@@ -61,6 +61,11 @@ shaka.media.ManifestFilterer = class {
goog.asserts.assert(manifest, 'Manifest should exist!');
await shaka.util.StreamUtils.filterManifest(this.drmEngine_, manifest,
this.config_.drm.preferredKeySystems);
shaka.util.StreamUtils.chooseCodecsAndFilterManifest(
Copy link
Member

Choose a reason for hiding this comment

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

I am very much concerned about the overall state of filtering. I can't understand it any more. I can't follow all the different classes and methods.

For example:

  • ManifestFilterer calls StreamUtils methods to do the actual filtering
  • filterManifestWithStreamUtils_ calls both filterManifest and chooseCodecsAndFilterManifest, so it does more than just filter now
  • We call applyRestrictions, which presumably sets flags based on restrictions, but also resolution, which we can't see here because it's in StreamUtils, then we call filterManifest right after, which calls filterManifestWithStreamUtils_, which calls StreamUtils.filterManifest anad chooseCodecsAndFilterManifest, then calls filterManifestWithRestrictions, which presumably repeats the filtering done by applyRestrictions?

I'm very confused.

This is not the fault of this PR, but this PR does bring it to mind for me.

Long term, I think we need to massively clean up this whole system. It's too messy to be understood.

Short term, you need a much better description for the PR, including details like:

  • The root cause of the problem
  • Why this is the right fix
  • Why it's not a problem for any caller of ManifestFilterer.filterManifest that we also choose codecs now inside that function
  • Why we should choose codecs earlier than chooseInitialVariantInner_()
  • Anything else you want to add to give confidence that we don't have to revert this change later because of a cascade of subtle problems caused by the complexity of this system

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I Updated the description

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the sentiment.
It'd be a good idea to take inventory on what types of filtering we perform on manifests, and when each needs to happen in the loading process. There are enough steps that it's hard to keep track of. If we can lay them all out in a "flat" way, one call after the other, and avoided having one method call another inside itself, it'd be a lot easier to tell what's going on.

I made this ManifestFilterer class to encapsulate the manifest filtering related code that was previously inside the Player, but perhaps making it would have been a good opportunity to streamline the process. Well, it's not too late.

@avelad avelad requested a review from joeyparrish May 22, 2024 19:12
lib/media/manifest_filterer.js Show resolved Hide resolved
lib/media/manifest_filterer.js Show resolved Hide resolved
@@ -61,6 +61,11 @@ shaka.media.ManifestFilterer = class {
goog.asserts.assert(manifest, 'Manifest should exist!');
await shaka.util.StreamUtils.filterManifest(this.drmEngine_, manifest,
this.config_.drm.preferredKeySystems);
shaka.util.StreamUtils.chooseCodecsAndFilterManifest(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the sentiment.
It'd be a good idea to take inventory on what types of filtering we perform on manifests, and when each needs to happen in the loading process. There are enough steps that it's hard to keep track of. If we can lay them all out in a "flat" way, one call after the other, and avoided having one method call another inside itself, it'd be a lot easier to tell what's going on.

I made this ManifestFilterer class to encapsulate the manifest filtering related code that was previously inside the Player, but perhaps making it would have been a good opportunity to streamline the process. Well, it's not too late.

@avelad avelad requested a review from theodab May 23, 2024 08:07
@avelad avelad dismissed joeyparrish’s stale review May 30, 2024 08:01

Reviewed by Theodore

@avelad avelad merged commit 9071002 into shaka-project:main May 30, 2024
16 checks passed
@avelad avelad deleted the multi-codec-filter-dash-live branch May 30, 2024 08:02
avelad added a commit that referenced this pull request May 30, 2024
Regression of Preload API

In HLS the definition of variants is only done once and it is the master
playlist.
In DASH, the definition of variants is done in each manifest update.
If we make the codec choice only at the beginning, in HLS it works, but
in DASH in the updates we lose this choice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: DASH The issue involves the MPEG DASH manifest format priority: P1 Big impact or workaround impractical; resolve before feature release type: bug Something isn't working correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants