-
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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. |
||
|
||
// Parse Representations into Streams. | ||
var representations = XmlUtils.findChildren(elem, 'Representation'); | ||
var streams = representations | ||
.map(this.parseRepresentation_.bind( | ||
this, context, contentProtection, kind, language, main)) | ||
this, context, contentProtection, kind, language, label, main)) | ||
.filter(function(s) { return !!s; }); | ||
|
||
if (streams.length == 0) { | ||
|
@@ -1018,6 +1020,7 @@ shaka.dash.DashParser.prototype.parseAdaptationSet_ = function(context, elem) { | |
* @param {shaka.dash.ContentProtection.Context} contentProtection | ||
* @param {(string|undefined)} kind | ||
* @param {string} language | ||
* @param {string} label | ||
* @param {boolean} isPrimary | ||
* @param {!Element} node | ||
* @return {?shakaExtern.Stream} The Stream, or null when there is a | ||
|
@@ -1026,7 +1029,7 @@ shaka.dash.DashParser.prototype.parseAdaptationSet_ = function(context, elem) { | |
* @private | ||
*/ | ||
shaka.dash.DashParser.prototype.parseRepresentation_ = function( | ||
context, contentProtection, kind, language, isPrimary, node) { | ||
context, contentProtection, kind, language, label, isPrimary, node) { | ||
var XmlUtils = shaka.util.XmlUtils; | ||
var ContentType = shaka.util.ManifestParserUtils.ContentType; | ||
|
||
|
@@ -1105,6 +1108,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 commentThe reason will be displayed to describe this comment to others. Learn more. Please add this new field to the definition of the |
||
type: context.adaptationSet.contentType, | ||
primary: isPrimary, | ||
trickModeVideo: null, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1408,14 +1408,15 @@ shaka.Player.prototype.getStats = function() { | |
* | ||
* @param {string} uri | ||
* @param {string} language | ||
* @param {?string} label | ||
* @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 commentThe 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. |
||
if (!this.streamingEngine_) { | ||
shaka.log.error( | ||
'Must call load() and wait for it to resolve before adding text ' + | ||
|
@@ -1466,6 +1467,7 @@ shaka.Player.prototype.addTextTrack = function( | |
encrypted: false, | ||
keyId: null, | ||
language: language, | ||
label: label, | ||
type: ContentType.TEXT, | ||
primary: false, | ||
trickModeVideo: null, | ||
|
@@ -1494,6 +1496,7 @@ shaka.Player.prototype.addTextTrack = function( | |
type: ContentType.TEXT, | ||
bandwidth: 0, | ||
language: language, | ||
label: label, | ||
kind: kind, | ||
width: null, | ||
height: null | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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. |
||
var tracks = variants.map(function(variant) { | ||
var isActive; | ||
if (variant.video && variant.audio) { | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. This "if" is unnecessary. Just set label = variant.audio.label. |
||
label = variant.audio.label; | ||
} | ||
} | ||
|
||
var audioCodec = variant.audio ? variant.audio.codecs : null; | ||
|
@@ -251,6 +255,7 @@ shaka.util.StreamUtils.getVariantTracks = | |
type: 'variant', | ||
bandwidth: variant.bandwidth, | ||
language: variant.language, | ||
label: label, | ||
kind: kind || null, | ||
width: variant.video ? variant.video.width : null, | ||
height: variant.video ? variant.video.height : null, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for updating the tests! |
||
' <Role value="caption" />', | ||
' <Role value="main" />', | ||
' <Representation bandwidth="100" />', | ||
|
@@ -176,6 +176,7 @@ describe('DashParser Manifest', function() { | |
.primary() | ||
.addTextStream(jasmine.any(Number)) | ||
.language('es') | ||
.label('spanish') | ||
.primary() | ||
.anySegmentFunctions() | ||
.anyInitSegment() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -162,6 +162,18 @@ shaka.test.ManifestGenerator.prototype.language = function(language) { | |
}; | ||
|
||
|
||
/** | ||
* Sets the label of the language of the most recent variant or text stream. | ||
* | ||
* @param {string} label | ||
* @return {!shaka.test.ManifestGenerator} | ||
*/ | ||
shaka.test.ManifestGenerator.prototype.label = function(label) { | ||
this.currentStream_().label = label; | ||
return this; | ||
}; | ||
|
||
|
||
/** | ||
* Sets that the most recent variant or text stream is primary. | ||
* | ||
|
@@ -320,7 +332,7 @@ shaka.test.ManifestGenerator.prototype.addVideo = function(id) { | |
} | ||
|
||
if (!stream) | ||
stream = this.createStream_(id, ContentType.VIDEO, 'und'); | ||
stream = this.createStream_(id, ContentType.VIDEO, 'und', null); | ||
|
||
variant.video = stream; | ||
this.lastStreamAdded_ = stream; | ||
|
@@ -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 commentThe 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. |
||
|
||
variant.audio = stream; | ||
this.lastStreamAdded_ = stream; | ||
|
@@ -372,7 +385,7 @@ shaka.test.ManifestGenerator.prototype.addAudio = function(id) { | |
shaka.test.ManifestGenerator.prototype.addTextStream = function(id) { | ||
var ContentType = shaka.util.ManifestParserUtils.ContentType; | ||
var period = this.currentPeriod_(); | ||
var stream = this.createStream_(id, ContentType.TEXT, 'und'); | ||
var stream = this.createStream_(id, ContentType.TEXT, 'und', ''); | ||
period.textStreams.push(stream); | ||
this.lastObjectAdded_ = stream; | ||
this.lastStreamAdded_ = stream; | ||
|
@@ -416,11 +429,12 @@ shaka.test.ManifestGenerator.prototype.isIdUsed_ = function(id) { | |
* @param {number} id | ||
* @param {string} type | ||
* @param {string} language | ||
* @param {?string} label | ||
* @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 commentThe reason will be displayed to describe this comment to others. Learn more. Since you have the |
||
goog.asserts.assert(!this.isIdUsed_(id), | ||
'Streams should have unique ids!'); | ||
|
||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Default to null. |
||
type: type, | ||
primary: false, | ||
trickModeVideo: null, | ||
|
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.