-
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
custom preference based criteria #7768
Comments
I'm open to this. Can you give a more concrete proposal? I'd like to see what the factory/constructor interface will look like, and an example of criteria logic. (It doesn't have to be the logic you're using in your app, but something plausible to illustrate the benefit.) |
Yeah, the quick answer is that it'll be very similar to the ABR Manager Factory, since moving to the config will make the constructor pretty simple. I'll write something more in-depth up in a bit. |
Essentially, we want to filter down to a specific codec under certain conditions. This is part of a legal requirement. The current filters that Shaka provides don't really give us the flexibility we need. I think the default () => new shaka.media.PreferenceBasedCriteria() Then usage would be something like: this.currentAdaptationSetCriteria_ =
this.config_.preferenceBasedCriteriaFactory();
this.currentAdaptationSetCriteria_.config({
language: this.config_.preferredAudioLanguage,
role: this.config_.preferredVariantRole,
channelCount: this.config_.preferredAudioChannelCount,
hdrLevel: this.config_.preferredVideoHdrLevel,
spatialAudio: this.config_.preferSpatialAudio,
videoLayout: this.config_.preferredVideoLayout,
audioLabel: this.config_.preferredAudioLabel,
videoLabel: this.config_.preferredVideoLabel,
codecSwitchingStrategy: this.config_.mediaSource.codecSwitchingStrategy,
audioCodec: '',
}); Additionally, if the preference based criteria was moved into a separate file, it would be easier for folks having custom-builds to exclude the built-in one and only include a custom one. |
… own files Related to shaka-project#7768
I moved this to a different file is that enough for you or do you still want to make a factory? (1f3af8b) |
Appreciate the move to separate files! I think we would still prefer to also have a factory. It'll make things a lot more flexible for us, as we won't necessarily need this custom code to live in our shaka fork. With the switch to a config object for it, it'll make extending much easier. |
Have you read the FAQ and checked for duplicate open issues?
Yes
Is your feature request related to a problem? Please describe.
In our shaka fork, we have few changes to the preference based criteria that is too specific to upstream. However, since those changes are inside the preference based criteria, we either have to edit the file in-place or create a new file and edit the build types. In either case, the maintenance isn't ideal.
Describe the solution you'd like
We want to be able to provide shaka with a custom preference based criteria. This should work similarly to the ABR Manager or the Text Display, where a factory function is passed to shaka and shaka uses the factory to create the ABR Manager and Text Display instances. The built-in classes will be used by default.
Additionally, all the options passed to the preference based criteria should be moved to a config object. Having everything be a config option would make it easier for the custom criteria to extend with additional options, since we won't need to add arguments to the constructor and deal with merge conflicts around ordering.
Describe alternatives you've considered
The main two alternatives are to edit the file in-place or use the build system to include a separate file for it at build time. The former makes maintenance a lot worse. The latter has been working, but it still makes it harder to maintain the fork.
Additionally, we could subclass it in the shaka code, but that isn't much different than using a separate file and have the build system include that. It would be nicer to keep the custom criteria separate.
Additional context
Are you planning to send a PR to add it?
Yes, assuming there's buy-in on proceeding with this work.
The text was updated successfully, but these errors were encountered: