-
Notifications
You must be signed in to change notification settings - Fork 27
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 for Shaka 2.2.x to be able to switch languages #43
base: master
Are you sure you want to change the base?
Conversation
…" type tracks for audio payload. We just get a list of unique languages. Therefore we need to extend the plugin API to be able to select languages that are not "variant". - add selectLanguage method with a "role" parameter. this can allow to choose between "variants" of one language (if there are any, they can be find in the audioTracks property) - improve selectTrack (use switch-case)
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.
Thank you so much 👍
@@ -261,6 +294,9 @@ class DashShakaPlayback extends HTML5Video { | |||
this._options.shakaConfiguration && this._player.configure(this._options.shakaConfiguration) | |||
this._options.shakaOnBeforeLoad && this._options.shakaOnBeforeLoad(this._player) | |||
|
|||
const preferredAudioLanguage = this._player.getConfiguration().preferredAudioLanguage |
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.
Isn't this something that can be overridden via shakaConfiguration
or _options
?
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.
just by _options
not, but it is actually setup by the this._player.configure(this._options.shakaConfiguration)
call, two lines above ^
now we need to initialize the state of _activeAudioLanguage
because the initial language will be the one set in preferredAudioLanguage
if it is set there.
otherwise me start out with saying language A is active because it's first in the list, but actually language B is because it has been configured as preferred.
not sure i got the question right, but does that answer your question actually? :)
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 think so, this this._options.shakaConfiguration.preferredAudioLanguage
is equivalent to this this._player.getConfiguration().preferredAudioLanguage
.
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.
exactly
} | ||
} else { | ||
throw new Error('Unhandled track type:', track.type); | ||
selectTrack (track, clearBuffer) { |
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 don't see too much value by changing an if else if else
by a switch, there was another reason for this refactoring?
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.
hm well no there is no change in logic.
from a code-style pov i would say that a switch is preferrable here since the condition is solely around the value of a single variable and no other conditions, and there are more than two cases :) so perfect time for a "switch"
so my answer is: this shouldn't have been an if-else in the first place. although i had written this code right there, just correcting myself now ;)
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.
Yes, I see your points :) Later I'll try to break this single class into at least 4 different components.
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 think it's still quite maintainable like this.
we should however add tests + enforce JSdocs with the eslint plugin for that on each method :)
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.
Yeah, tests are way up on the priority list. 🥂
selectTrack (track, clearBuffer) { | ||
if (this.isReady) { | ||
return false | ||
} |
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.
Should we check if isReady
or !isReady
?
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.
nice catch! didn't test selecting variant tracks here again. thanks!!
my next aim on this codebase is to add some unit tests :)
Hey guys, actually i understood that my understanding about the Shaka 2.2 API was not complete :) Basically, to explain really simply, audio-and video tracks are under one hood of the "variant" type tracks. What we currently do (checking for mimeType) is really going to give us only audio-only tracks when we look for "audio/..". The tracks that contain both a/v have the mimeType "video/..". Now when i have say 2 languages and 7 video qualies, that results in 14 variant tracks with respective properties in width/height/language. The new Shaka language API simplifies to deal with this. Now the question is: Should the plugin API here try to deal with this differently that what we do now? I may make sense on the one hand to filter out only the variant tracks that correspond to the currently active language? Or really expose everything Shaka gives us? Or we try to make a more convenient API? I would rather really prefer an API that matches the data-model one would need in a player UI. So just the video variants available under a certain language chosen for example. Discussion is open ... :) |
Thanks already for all the reviews :) Problem remaining in our API here as I see it, in one sentence:
From the way the current API is designed, this behavior is very counter-intuitive. Also I wonder how we can further integrate such pattern into Clappr ... Looking forward to see how we can do this! |
Our workaround here would be like:
|
I'll put @joeyparrish into the discussion maybe he can also give his valuable input and I'll do it later as well. About #43 (comment) |
@leandromoreira Hey guys, unfortunately we haven't been able to continue the conversation. Let's hope we do that next year! I'm a bit outside of the topic right now, but it will become important soon ;) Cheers 🎆 |
Hi all, The v2.2 API exposes all variants to the application so that we don't arbitrarily limit applications, but our demo app only shows the variants for the current language to the user. Here is the code for that in our latest release (v2.2.8): https://github.com/google/shaka-player/blob/5c992c734870fade36dfdf4751b4b2cf49c0cf93/demo/info_section.js#L128 The In v2.3, we introduce a new method that lists language/role combos. In the latest nightly build of our demo app, we only show the variants that match the current language and role, and we allow the user to change languages and roles at the same time. The new methods are Earlier in the conversation, there was some concern over detecting audio-video tracks vs. audio-only vs. video-only. Instead of using Shaka Player will never expose a mix of video-only, audio-only, and audio-video tracks. All variant tracks will be the same type, because we don't dynamically add or remove SourceBuffers and can't switch between audio-video and audio-only, for example. I hope this helps! Happy holidays! |
Thanks, @tchakabam for your effort and @joeyparrish for your teachings. Happy holidays! 🎄 |
Hey! :) Thanks for getting back and for those explanations. It's useful to be in contact about this topic. And good to know that v2.3 exposes some convenience methods. It just seems a bit confusing to access the combinations "folded" together in a way. Logically it's not wrong of course, and I understand why you do it, as in principle a DASH stream may impose inter-dependent audio/video combinations, in theory. At this point I don't have any more questions about Shaka. But we have to figure how Clappr can integrate with it :) I would like to kick off a discussion with Clappr people about how to have Clappr best work with adaptive media engines @leandromoreira Maybe we can have that discussion best around a Clappr player PR I am currently prep'ing in order to add generic adaptive-playback config and API to Clappr. Currently none of the concepts of adaptive audio/video selection or languages is defined/abstracted in the Clappr base components/interfaces. Plugins like level-selector use inofficial methods that HLS.js and Shaka playbacks expose. It could be great to come down to something meaningful here. The Shaka API is a good example for how to do that, even if that one may not be the easiest for apps/UIs to build against it. But I would say on the other side, Hls.js API can learn a bit from it, things are also confusing in a different way there. Clappr could take a chance at generalizing these quality-adaptation and language-selection problems into a nice set of config and interfaces on the Playback base component. What do you think guys? @leandromoreira @vagnervjs @towerz 🙌 Concerning the Shaka API, I find it cumbersome from an application point of view to get all the possible resolution/language combinations flattened, when you actually usually want to give the user or application logic independent choice over both audio and video tracks available (yes I know, they are not always and by definiton indepedent, they can be coupled 😬 ) Now how the DASH streams we usually deal with are conceived, they have independent audio/video fragment, so you can switch both things independently. That's also the 99% use-case of video applications btw I would say. For example, even if MPEG-DASH allows it, really when will I have a video-resolution with say "pt-BR" available as audio language, and another resolution, where that is not available? The only meaningful thing that comes to mind is when you have on the server-side fragments muxed together audio&video, with say HD quality muxed only with 5.1-channels audio, so you can have the stereo track only with the lower resolutions (just an example). So I wonder if that is also covered in Shaka API, as it is about languages and roles. Shouldn't you actually need a method then like Switching audio languages and video bitrates independently is obviously a thing that Shaka API allows to do in principle, when the stream allows it. It just seems it's going to be a bit the same problem every application will have to solve in order to use your API to solve the 99%-use-case. For example to expose all available video bitrates, I need to:
@joeyparrish One would really like to have a method that just does that from the Shaka side :) Or am I missing something here? Same as you already do for the language/role combinations, but for video qualities. But then again, it's easy to have that in the ShakaPlayback here as well, kind of like we do in this PR. And also, what do you think about a method that also exposes the available channel/codec feature-set ? Most of the applications I know usually offer both AAC stereo and Dolby 5.1, for example. |
Fix for Shaka 2.2.x to be able to switch languages
The 2.2.x API basically makes variant tracks such that they are the combination of all possible audio/video combinations and appear as mime-type "video/..". only audio-only tracks will appear as mime-type "audio/..". an MPD with 7 video qualities and 2 languages will result in 14 Shaka renditions all with video mime-type. To make things simpler, Shaka has now a language-switching API that will not need us to deal with all the variants when switching the language.
This PR aims to make it possible to use the language switching.
Moreover it should be clarified how we expose the "variant" type tracks exactly.
Currently setting a variant track obtained from "videoTracks" might also switch the language :)
Changes:
add selectLanguage method with a "role" parameter. this can allow to choose between "variants" of one language (if there are any, they can be find in the audioTracks property)
improve selectTrack (use switch-case)
add isAudioOnly implementation to plugin
fix lint script calls
add .editorconfig file