-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix: Fix multi-codec filtering on DASH live #6647
Conversation
Incremental code coverage: 100.00% |
@@ -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( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I Updated the description
There was a problem hiding this comment.
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.
@@ -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( |
There was a problem hiding this comment.
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.
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.
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.