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 for Shaka 2.2.x to be able to switch languages #43

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tchakabam
Copy link
Contributor

@tchakabam tchakabam commented Sep 21, 2017

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

…" 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)
Copy link
Member

@leandromoreira leandromoreira left a 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
Copy link
Member

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?

Copy link
Contributor Author

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? :)

Copy link
Member

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.

Copy link
Contributor Author

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) {
Copy link
Member

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?

Copy link
Contributor Author

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 ;)

Copy link
Member

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.

Copy link
Contributor Author

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 :)

Copy link
Member

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
}
Copy link
Member

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?

Copy link
Contributor Author

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 :)

@tchakabam
Copy link
Contributor Author

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 ... :)

@tchakabam
Copy link
Contributor Author

tchakabam commented Sep 22, 2017

Thanks already for all the reviews :)

Problem remaining in our API here as I see it, in one sentence:

Currently setting a variant track obtained from "videoTracks" might also switch the language :)

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!

@tchakabam
Copy link
Contributor Author

Our workaround here would be like:

videoTracks
            .filter((track) => track.language === dashShakaPlayback.activeAudioLanguage)

@leandromoreira
Copy link
Member

leandromoreira commented Sep 22, 2017

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)

@tchakabam
Copy link
Contributor Author

@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 🎆

@joeyparrish
Copy link

joeyparrish commented Dec 19, 2017

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 getAudioLanguages() and selectAudioLanguage() methods allow you to change languages explicitly, rather than using the tracks API for that. There are corresponding methods for text tracks. If you expose language selection through your API, you should definitely filter the track list for the current language, as we do in our demo. We do not recommend using selectVariant() to change languages, although it may be appropriate for some applications in some circumstances.

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 getAudioLanguagesAndRoles() and getTextLanguagesAndRoles(). The track-filtering code for our upcoming v2.3 release is here: https://github.com/google/shaka-player/blob/c78e4a258b8c2924de0ad90d18d16526c74ee1af/demo/info_section.js#L137

Earlier in the conversation, there was some concern over detecting audio-video tracks vs. audio-only vs. video-only. Instead of using mimeType, you can look at the audioCodec and videoCodec fields. In audio-video variants, both fields will be non-null. For audio-only content, the videoCodec will be null, and for video-only content, the audioCodec will be null. The isAudioOnly() method on player may also be useful for this.

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!

@leandromoreira
Copy link
Member

Thanks, @tchakabam for your effort and @joeyparrish for your teachings. Happy holidays! 🎄

@tchakabam
Copy link
Contributor Author

tchakabam commented Dec 21, 2017

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 getAudioLanguagesAndRolesAndChannelsAndCodec ? :)

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:

  • first figure what is the active language
  • then filter all the variants with this language
  • only then i get the video bitrates that would be currently available with this language

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants