-
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
Add support for non-standard DASH label attribute #811
Add support for non-standard DASH label attribute #811
Conversation
@itaykinnrot, can you provide us with some idea of what this feature is? You didn't put a description in the PR. |
Sure - we want to allow customization of the label that the player show in the menu (same as HLS) and I think that the right way to do it is using the DASH manifest since the packager that create it already has all this data. |
@joeyparrish, to add to what @itaykinnrot wrote - when playing a stream with multiple audio tracks, currently the only way to identify the different tracks is using the lang attribute of AdaptationSet. We have customers who would like to customize the label that is displayed to the end user for selecting the audio track. Also, multiple audio tracks are sometimes used for uses cases which are not multiple languages, for example - one audio track with background music and one without it. |
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 for the contribution! I have some feedback for you to before we merge.
lib/dash/dash_parser.js
Outdated
@@ -957,11 +957,14 @@ shaka.dash.DashParser.prototype.parseAdaptationSet_ = function(context, elem) { | |||
var language = | |||
shaka.util.LanguageUtils.normalize(elem.getAttribute('lang') || 'und'); | |||
|
|||
var label = | |||
shaka.util.LanguageUtils.normalize(elem.getAttribute('label') || 'und'); |
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 think it makes sense to pass the label through language tools or default to the language code 'und'. Label, as I understand it from @erankor's explanation, is a free-form string and may reasonably be missing (null).
@@ -1105,6 +1109,7 @@ shaka.dash.DashParser.prototype.parseRepresentation_ = function( | |||
encrypted: contentProtection.drmInfos.length > 0, | |||
keyId: keyId, | |||
language: language, | |||
label: label, |
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.
Please add this new field to the definition of the shakaExtern.Stream
structure in externs/shaka/manifest.js.
lib/util/stream_utils.js
Outdated
@@ -233,6 +233,9 @@ shaka.util.StreamUtils.getVariantTracks = | |||
if (variant.audio) { | |||
if (codecs != '') codecs += ', '; | |||
codecs += variant.audio.codecs; | |||
if (variant.audio.label) { | |||
variant.label = variant.audio.label; |
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.
It doesn't make sense to push it from variant.audio (Stream
structure) to variant (Variant
structure) in this utility. If you need this on variant, you should add it when the variant is generated in the DASH parser.
Instead, though, I recommend just storing the label in a local var. I don't see any good reason to push it up to the variant.
@@ -119,7 +119,7 @@ describe('DashParser Manifest', function() { | |||
' <Representation bandwidth="50" width="576" height="432" />', | |||
' </AdaptationSet>', | |||
' <AdaptationSet mimeType="text/vtt"', | |||
' lang="es">', | |||
' lang="es" label="spanish">', |
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 for updating the tests!
lib/util/stream_utils.js
Outdated
@@ -251,6 +254,7 @@ shaka.util.StreamUtils.getVariantTracks = | |||
type: 'variant', | |||
bandwidth: variant.bandwidth, | |||
language: variant.language, | |||
label: variant.label, |
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.
Please add this new field to the definition of the shakaExtern.Track
structure in externs/shaka/player.js.
demo/info_section.js
Outdated
@@ -198,7 +197,11 @@ shakaDemo.updateLanguageOptions_ = | |||
// Populate list with new options. | |||
languages.forEach(function(lang) { | |||
var option = document.createElement('option'); | |||
option.textContent = lang; | |||
var currentTrack = tracks.filter(function(track) { |
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.
There are two things wrong with this part:
- currentTrack is a misleading name. It is not the currently-selected track you've identified, it's just any track that matches the language.
- You shouldn't annotate the languages with track labels. Instead, annotate the track list.
For example, if you have two tracks of the same language, as @erankor suggested is common for this feature, you would have these tracks:
[
{ id: 1, lang: 'en', label: 'with music' },
{ id: 2, lang: 'en', label: 'without music' },
]
But the only language in the language list will be "en". With what you've written, your language options in HTML would wind up as:
<select>
<option value="en">with music</option>
</select>
While the variant tracks would still both show "language: en" with no differentiation.
I think the method you really want to modify here is shakaDemo.updateTrackOptions_
, specifically the formatters
callbacks.
test/test/util/manifest_generator.js
Outdated
* @return {!shaka.test.ManifestGenerator} | ||
*/ | ||
shaka.test.ManifestGenerator.prototype.label = function(label) { | ||
this.currentStreamOrVariant_().label = label; |
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.
Please use currentStream_() instead, since this attribute should only live at the stream level.
shakaExtern.Track and shakaExtern.Stream and update project according to it
@joeyparrish thanks for the review - code is update - let me know if anything is missing - Thanks! |
demo/info_section.js
Outdated
var tracks = player.getTextTracks(); | ||
var languages = tracks.map(function(track) { | ||
return { id: track.id, lang: track.language, label: track.label}; |
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.
The variable name languages
is now a misnomer. You're not passing an array of language strings, but an array of objects with track info.
In fact, these objects are a subset of what is already in tracks
. So you don't need this at all.
At a higher level, what you're doing here in the UI is labeling the languages. Instead, you should be labeling the tracks, since labels are attached to tracks.
The language list should stay as it is.
lib/player.js
Outdated
* @param {string} kind | ||
* @param {string} mime | ||
* @param {string=} opt_codec | ||
* @return {!Promise.<shakaExtern.Track>} | ||
* @export | ||
*/ | ||
shaka.Player.prototype.addTextTrack = function( | ||
uri, language, kind, mime, opt_codec) { | ||
uri, language, label, kind, mime, opt_codec) { |
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.
Please do not add additional parameters to the middle of an exported method. This would break backward compatibility. Instead, make it an optional parameter after opt_codec.
lib/util/stream_utils.js
Outdated
@@ -233,6 +234,9 @@ shaka.util.StreamUtils.getVariantTracks = | |||
if (variant.audio) { | |||
if (codecs != '') codecs += ', '; | |||
codecs += variant.audio.codecs; | |||
if (variant.audio.label) { |
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.
This "if" is unnecessary. Just set label = variant.audio.label.
lib/util/stream_utils.js
Outdated
@@ -219,6 +219,7 @@ shaka.util.StreamUtils.getVariantTracks = | |||
function(period, activeAudioId, activeVideoId) { | |||
var StreamUtils = shaka.util.StreamUtils; | |||
var variants = StreamUtils.getPlayableVariants(period.variants); | |||
var label; |
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.
Initialize this to null, since the type allows for null instead of undefined. A minor difference, I know.
test/test/util/manifest_generator.js
Outdated
@@ -353,7 +365,8 @@ shaka.test.ManifestGenerator.prototype.addAudio = function(id) { | |||
} | |||
|
|||
if (!stream) | |||
stream = this.createStream_(id, ContentType.AUDIO, variant.language); | |||
stream = this.createStream_(id, ContentType.AUDIO, variant.language, | |||
variant.audio.label); |
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.
This looks like it will throw an exception, since variant.audio doesn't exist yet.
test/test/util/manifest_generator.js
Outdated
* @return {!shakaExtern.Stream} | ||
* @private | ||
*/ | ||
shaka.test.ManifestGenerator.prototype.createStream_ = | ||
function(id, type, language) { | ||
function(id, type, language, label) { |
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.
Since you have the label()
method to set the label of the current stream, I think we don't need to add this parameter.
test/test/util/manifest_generator.js
Outdated
@@ -456,6 +470,7 @@ shaka.test.ManifestGenerator.prototype.createStream_ = | |||
encrypted: false, | |||
keyId: null, | |||
language: language, | |||
label: label, |
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.
Default to null.
demo/info_section.js
Outdated
@@ -196,11 +195,11 @@ shakaDemo.updateLanguageOptions_ = | |||
var selectedTrack = activeTracks[0]; | |||
|
|||
// Populate list with new options. | |||
languages.forEach(function(lang) { | |||
tracks.forEach(function(track) { |
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.
updateLanguageOptions_
updates the list of languages in the demo. You should not be changing this method.
Instead, you should be changing the formatting in the list of tracks.
Everything else looks good, so I'm going to go ahead and run tests on the build bot. If all tests pass, I will fix the demo UI myself when I merge.
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.
Tests are failing, so I will leave it to you to make updates.
For the demo app, please revert changes in this file and look at the formatters
map in updateTrackOptions_
. That is where you should add labels to the track list. Here's a suggestion:
shakaDemo.updateTrackOptions_ = function(list, tracks, language) {
var formatters = {
variant: function(track) {
var trackInfo = '';
if (track.language) trackInfo += 'language: ' + track.language + ', ';
if (track.label) trackInfo += 'label: ' + track.label + ', ';
if (track.width && track.height)
trackInfo += track.width + 'x' + track.height + ', ';
trackInfo += track.bandwidth + ' bits/s';
return trackInfo;
},
text: function(track) {
var trackInfo = 'language: ' + track.language + ', ';
if (track.label) trackInfo += 'label: ' + track.label + ', ';
trackInfo += 'kind: ' + track.kind;
return trackInfo;
}
};
Test Failure:
|
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.
Please fix the failing tests. You can run the tests yourself locally using python build/test.py
(no arguments required).
demo/info_section.js
Outdated
@@ -196,11 +195,11 @@ shakaDemo.updateLanguageOptions_ = | |||
var selectedTrack = activeTracks[0]; | |||
|
|||
// Populate list with new options. | |||
languages.forEach(function(lang) { | |||
tracks.forEach(function(track) { |
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.
Tests are failing, so I will leave it to you to make updates.
For the demo app, please revert changes in this file and look at the formatters
map in updateTrackOptions_
. That is where you should add labels to the track list. Here's a suggestion:
shakaDemo.updateTrackOptions_ = function(list, tracks, language) {
var formatters = {
variant: function(track) {
var trackInfo = '';
if (track.language) trackInfo += 'language: ' + track.language + ', ';
if (track.label) trackInfo += 'label: ' + track.label + ', ';
if (track.width && track.height)
trackInfo += track.width + 'x' + track.height + ', ';
trackInfo += track.bandwidth + ' bits/s';
return trackInfo;
},
text: function(track) {
var trackInfo = 'language: ' + track.language + ', ';
if (track.label) trackInfo += 'label: ' + track.label + ', ';
trackInfo += 'kind: ' + track.kind;
return trackInfo;
}
};
lib/dash/dash_parser.js
Outdated
@@ -957,11 +957,13 @@ shaka.dash.DashParser.prototype.parseAdaptationSet_ = function(context, elem) { | |||
var language = | |||
shaka.util.LanguageUtils.normalize(elem.getAttribute('lang') || 'und'); | |||
|
|||
var label = elem.getAttribute('label'); |
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 was unable to find any mention of this attribute in the DASH spec (2014) or the latest DASH-IF-IOP (v4.0). I don't object to you adding it, though.
Please add a comment that this is a non-standard attribute supported by Kaltura.
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.
Only one small nit. Thanks!
lib/hls/hls_parser.js
Outdated
@@ -526,12 +528,13 @@ shaka.hls.HlsParser.prototype.createStreamInfoFromVariantTag_ = | |||
* @param {?number} timeOffset | |||
* @param {!string} language | |||
* @param {boolean} primary | |||
* @param {?string=} opt_label |
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.
This probably doesn't need to be both optional and nullable. For an internal method like this, all the arguments should be required, I think.
lib/hls/hls_parser.js
Outdated
@@ -480,6 +480,8 @@ shaka.hls.HlsParser.prototype.createStreamInfoFromMediaTag_ = | |||
var LanguageUtils = shaka.util.LanguageUtils; | |||
var langAttr = tag.getAttribute('LANGUAGE'); | |||
var language = langAttr ? LanguageUtils.normalize(langAttr.value) : 'und'; | |||
var labelAttr = tag.getAttribute('NAME'); |
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 for adding HLS support in this PR as well!
@joeyparrish please review - thanks! |
All tests passed! |
Thank you for your contribution! |
Change-Id: Iaed56bdbc2937006746ee433e2f13af08e3e585d
* add support for non-standard DASH label attribute * add support for HLS NAME attribute Closes #825
Change-Id: Iaed56bdbc2937006746ee433e2f13af08e3e585d
Cherry-picked to v2.1.3. |
No description provided.