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.

Sign up for GitHub

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

Jump to bottom

fix: Fix multi-codec filtering on DASH live #6647

Merged
merged 2 commits into from
May 30, 2024
Merged

fix: Fix multi-codec filtering on DASH live #6647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d94cd62
fix: Fix multi-codec filtering on DASH live
avelad May 22, 2024
e7aded0
Update
avelad May 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 8 additions & 12 deletions lib/media/manifest_filterer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,27 +41,23 @@ shaka.media.ManifestFilterer = class {
}

/**
* Filters a manifest, removing unplayable streams/variants.
* Filters a manifest, removing unplayable streams/variants and choosing
* the codecs.
*
* @param {?shaka.extern.Manifest} manifest
* @return {!Promise.<boolean>} tracksChanged
*/
async filterManifest(manifest) {
await this.filterManifestWithStreamUtils_(manifest);
return this.filterManifestWithRestrictions(manifest);
}

/**
* Filters a manifest, removing unplayable streams/variants.
*
* @param {?shaka.extern.Manifest} manifest
* @private
*/
async filterManifestWithStreamUtils_(manifest) {
goog.asserts.assert(manifest, 'Manifest should exist!');
avelad marked this conversation as resolved.
Show resolved Hide resolved
await shaka.util.StreamUtils.filterManifest(this.drmEngine_, manifest,
avelad marked this conversation as resolved.
Show resolved Hide resolved
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
Member 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
Contributor

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.

manifest,
this.config_.preferredVideoCodecs,
this.config_.preferredAudioCodecs,
this.config_.preferredDecodingAttributes);
this.checkPlayableVariants_(manifest);
return this.filterManifestWithRestrictions(manifest);
}


Expand Down
13 changes: 3 additions & 10 deletions lib/media/preload_manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,9 @@ shaka.media.PreloadManager = class extends shaka.util.FakeEventTarget {

const tracksChangedInitial = this.manifestFilterer_.applyRestrictions(
this.manifest_);
if (tracksChangedInitial) {
const tracksChangedBefore = await this.manifestFilterer_.filterManifest(
this.manifest_);
if (tracksChangedInitial || tracksChangedBefore) {
const event = this.makeEvent_(
shaka.util.FakeEvent.EventName.TracksChanged);
await Promise.resolve();
Expand All @@ -568,7 +570,6 @@ shaka.media.PreloadManager = class extends shaka.util.FakeEventTarget {

// Now that we have drm information, filter the manifest (again) so that
// we can ensure we only use variants with the selected key system.
// const tracksChanged =
const tracksChangedAfter = await this.manifestFilterer_.filterManifest(
this.manifest_);
if (tracksChangedAfter) {
Expand Down Expand Up @@ -630,14 +631,6 @@ shaka.media.PreloadManager = class extends shaka.util.FakeEventTarget {
this.config_.preferredVideoLabel,
this.config_.mediaSource.codecSwitchingStrategy,
this.config_.manifest.dash.enableAudioGroups);

// If the content is multi-codec and the browser can play more than one of
// them, choose codecs now before we initialize streaming.
shaka.util.StreamUtils.chooseCodecsAndFilterManifest(
this.manifest_,
this.config_.preferredVideoCodecs,
this.config_.preferredAudioCodecs,
this.config_.preferredDecodingAttributes);
}

// Make the ABR manager.
Expand Down
Loading