Add ms:laurl and mspr:pro parsing #1010
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
Signed the CLA |
|
CLAs look good, thanks! |
| */ | ||
| shaka.dash.ContentProtection.parsePro_ = function(bytes) { | ||
| var length = bytes[0] | bytes[1]; | ||
| var recordCount = bytes[2]; |
There was a problem hiding this comment.
Is there a guarantee that there will be at least 3 elements in bytes?
| * @private | ||
| */ | ||
| shaka.dash.ContentProtection.getLaurl_ = function(xml) { | ||
| var laurlNode = xml.getElementsByTagName('LA_URL')[0]; |
There was a problem hiding this comment.
Is there a guarantee that there will be an element with the given tag name?
| shaka.dash.ContentProtection.getLaurl_ = function(xml) { | ||
| var laurlNode = xml.getElementsByTagName('LA_URL')[0]; | ||
| if (laurlNode) { | ||
| var laurl = laurlNode.childNodes[0].nodeValue; |
There was a problem hiding this comment.
Is there a guarantee that there will be a child node?
| if (laurl) { | ||
| return laurl; | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you reduce this down to:
var laurlNode = ...
return (laurlNode ? laurlNode.childNodes[0].nodeValue : null) || '';
|
|
||
| var records = parsedPro.records; | ||
| var parser = new DOMParser(); | ||
| for (var i = 0; i < records.length; i++) { |
There was a problem hiding this comment.
I would prefer it if we could avoid indexed array iteration. Can we do something like:
var record = records.filter(function(record) {
return PLAYREADY_RECORD_TYPES[record.type] == 'RIGHTS_MANAGEMENT';
}).pop();
if (record) {
var xml = ...
return ...
} else {
return ...
}
| var recordRaw = recordData.subarray(head); | ||
|
|
||
| var type = recordRaw[0]; | ||
| var length = recordRaw[1]; |
There was a problem hiding this comment.
Here too, there are unchecked indexes. Can we add some checks to insure that the right amount of data is present before indexing?
| var drmInfo = ManifestParserUtils.createDrmInfo(keySystem, initData); | ||
|
|
||
| // extract Widevine license URL | ||
| if (keySystem === 'com.widevine.alpha') { |
There was a problem hiding this comment.
To make it easier for us to add support for other schemes, could we structure this like:
var license_url_parsers = {
'com.widevine.alpha': shaka.dash...
'com.microsoft.playready': shaka.dash...
};
var license_parser = license_parsers[keySystem];
if (license_parser) {
drmInfo.licenseServerUri = license_parser(element);
}
|
Great thanks for the comments! |
| @@ -250,6 +250,179 @@ shaka.dash.ContentProtection.parseFromRepresentation = function( | |||
| return repContext.defaultKeyId || context.defaultKeyId; | |||
| }; | |||
|
|
|||
| /** | |||
| * Gets a Widevine license URL from a content protection element | |||
There was a problem hiding this comment.
Unless I am mistaken, 'ms:laurl' is a Microsoft specific element. I don't believe Widevine has a standard way of embedding URLs in the manifest.
| var parser = new DOMParser(); | ||
| for (var i = 0; i < records.length; i++) { | ||
| var record = records[i]; | ||
| if (PLAYREADY_RECORD_TYPES[record.type] === 'RIGHTS_MANAGEMENT') { |
There was a problem hiding this comment.
if (record.type == PLAYREADY_RECORD_TYPES.RIGHTS_MANAGEMENT)| var PLAYREADY_RECORD_TYPES = { | ||
| 0x0001 : 'RIGHTS_MANAGEMENT', | ||
| 0x0002 : 'RESERVED', | ||
| 0x0003 : 'EMBEDDED_LICENSE' |
There was a problem hiding this comment.
It would be better to reverse this enum:
/** @enum {number} */
var PLAYREADY_RECORD_TYPES = {
RIGHTS_MANAGEMENT: 0x001,
RESERVED: 0x002,
EMBEDDED_LICENSE: 0x003
};If you look at my suggestion below, it allows us to use the compiler to check we don't misspell the name. Your enum here just converts a magic number to a magic string. Also, our compiler will remove the enum completely, so it avoids an object lookup at runtime.
| * @return {Array.<shaka.dash.ContentProtection.PlayReadyRecord>} | ||
| * @private | ||
| */ | ||
| shaka.dash.ContentProtection.parseRecords_ = function(recordData, recordCount) { |
There was a problem hiding this comment.
How about renaming this so it is clear it is a 'mspr:pro' record? Something like parseMsProRecords_?
| * @property {Array.<shaka.dash.ContentProtection.PlayReadyRecord>} records | ||
| * Contains a variable number of records that contain license acquisition information. | ||
| */ | ||
| shaka.dash.ContentProtection.PlayReadyHeaderObject; |
There was a problem hiding this comment.
You never use the length and recordCount members; how about just returning the array of records directly?
| } | ||
|
|
||
| var bytes = shaka.util.Uint8ArrayUtils.fromBase64(proNode.textContent); | ||
| var parsedPro = shaka.dash.ContentProtection.parsePro_(new Uint16Array(bytes.buffer)); |
There was a problem hiding this comment.
If I remember correctly, Uint16Array uses the host byte order, whereas PlayReady uses little-endian. You may need to use DataView to read the data with the correct byte order.
|
|
||
| // subarray end is exclusive | ||
| var rawValue = recordRaw.subarray(offset, end); | ||
| var value = String.fromCharCode.apply(null, rawValue); |
There was a problem hiding this comment.
Please use shaka.util.StringUtils.fromUTF8 to parse UTF-8 correctly and avoid a possible stack overflow with large strings.
| * Size of the PlayReady header object in bytes. | ||
| * @property {number} recordCount | ||
| * Number of records in the PlayReady object. | ||
| * @property {Array.<shaka.dash.ContentProtection.PlayReadyRecord>} records |
There was a problem hiding this comment.
For object types like Array and Uint16Array, use a ! to make it non-nullable. E.g. {!Array.<T>}.
| * @private | ||
| */ | ||
| shaka.dash.ContentProtection.parsePro_ = function(bytes) { | ||
| var length = bytes[0] | bytes[1]; |
There was a problem hiding this comment.
Did you forget a shift here? (bytes[0] << 16) | bytes[1]
joeyparrish
added
the
status: waiting on response
|
I would like to see some form of this get in..... What do we think the status of this is? Are we just waiting for the contributor to make the requested changes. Also I am curious about what appears to be adding ms:laurl support for widevine. Is this specifically something that is likely to make it into shaka? |
|
@TheJohnBowers, the status is that @TheModMaker requested changes to this PR from @forbesjo, and we are still waiting on an updated PR. Adding ms:laurl support is a low priority item for the Shaka Player team at this time, but we are always happy to have features contributed by the community. |
|
This PR is still on my radar but other work has taken priority |
|
@forbesjo Are you still interested in this PR? If so, you'll need to rebase and make some style changes like using |
|
Thanks for the ping, I'll be taking a look at this PR and cleaning it up some time soon |
|
Update on this PR, I don't think I'll have time to clean up the PR. If any other user would like to implement this feature in Shaka feel free to use this as a starting point. Feel free to close if necessary. |
joeyparrish
removed
the
status: waiting on response
|
Closing in favor of #1644, which is being actively worked on. Thanks! |
github-actions
bot
added
the
status: archived
This PR closes #484 and parses license urls from
<ms:laurl>and<mspr:pro>in the manifest.