Add license url parsing #1644

tylerdaines · 2018-10-27T06:09:57Z

This PR is based off #1010. It closes #484 and parses license urls from <ms:laurl> and <mspr:pro> in the DASH manifest. I believe I have addressed all the suggestions made in the original PR.

/cc @forbesjo

googlebot · 2018-10-27T06:10:04Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

tylerdaines · 2018-10-29T18:55:32Z

I signed it!

googlebot · 2018-10-29T18:55:35Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

… Tests need to be fixed still.

…de getWideviceLicenseUrl() and getPlayReadyLicenseServerUrl() public so they could be tested

googlebot · 2018-10-29T20:31:45Z

CLAs look good, thanks!

tylerdaines · 2018-11-06T17:52:58Z

Any chance I can get some feedback on this? I'm hoping this can make it in 2.5.

ismena · 2018-11-06T18:01:37Z

@tylerdaines Definitely! Thanks for the ping and apologies for the delay.

@TheModMaker @vaage Guys, could I ask you to take a look since you both worked on the #1010

lib/dash/content_protection.js

…nction

tylerdaines · 2018-11-15T21:09:49Z

@vaage I think I have addressed all your concerns and reverted my ill-advised white space changes. Anything else I need to look at?

lib/dash/content_protection.js

…de review. Simplified LA_URL xml parsing

lib/dash/content_protection.js

test/dash/dash_parser_content_protection_unit.js

…ng fixes

…eadyRecord value to be Uint16Array

tdaines · 2018-11-29T22:54:03Z

@vaage Anything else you want me to take a second pass at?

lib/dash/content_protection.js

TheModMaker · 2018-11-30T00:09:05Z

test/dash/dash_parser_content_protection_unit.js

+        keyId: null,
+        schemeUri: '',
+        node: strToXml([
+          '<test xmlns:ms="urn:microsoft">',


I don't think this isn't the correct namespace URI.

Couldn't find much on this but I did find this Azure Media sample page that links to this manifest which uses the same namespaces so I think it's correct:

<MPD xmlns="urn:mpeg:dash:schema:mpd:2011" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" profiles="urn:mpeg:dash:profile:isoff-live:2011" type="static" xmlns:cenc="urn:mpeg:cenc:2013" xmlns:mspr="urn:microsoft:playready" xmlns:ms="urn:microsoft" mediaPresentationDuration="PT52.208S" minBufferTime="PT3S"> <Period> <AdaptationSet ...snip...> <ContentProtection schemeIdUri="urn:mpeg:dash:mp4protection:2011" value="cenc" cenc:default_KID="096D74B9-362D-4110-949B-EC0F5694F750"/> <ContentProtection schemeIdUri="urn:uuid:9A04F079-9840-4286-AB92-E65BE0885F95" value="2.0" cenc:default_KID="096D74B9-362D-4110-949B-EC0F5694F750"> <mspr:pro>...snip...</mspr:pro> </ContentProtection> <ContentProtection schemeIdUri="urn:uuid:edef8ba9-79d6-4ace-a3c8-27dcd51d21ed" cenc:default_KID="096D74B9-362D-4110-949B-EC0F5694F750"> <cenc:pssh>AAAAMnBzc2gAAAAA7e+LqXnWSs6jyCfc1R0h7QAAABISEAltdLk2LUEQlJvsD1aU91A=</cenc:pssh> <ms:laurl licenseUrl="https://ampvideos.keydelivery.mediaservices.windows.net/Widevine/?kid=096d74b9-362d-4110-949b-ec0f5694f750"/> </ContentProtection> ...snip...

test/dash/dash_parser_content_protection_unit.js

lib/dash/content_protection.js

test/dash/dash_parser_content_protection_unit.js

lib/dash/content_protection.js

…seMsPro_ and parseMsProRecords_() to operate on ArrayBuffers

tylerdaines · 2018-12-06T20:02:31Z

I believe I have addressed all the latest issues. Anything else that needs addressing?

lib/dash/content_protection.js

TheModMaker · 2018-12-06T22:38:25Z

lib/dash/content_protection.js

+
+    records.push({
+      type: type,
+      value: new Uint8Array(recordValue),


Couldn't you just do new Uint8Array(recordData, byteOffset, byteLength)?

This is something I went back and forth on and couldn't find a better way. The issue is in the ArrayBuffer recordData, the data is all 16-bit numbers (i.e. every other byte is 0), so I went with the "cast the Uint16 numbers to Uint8 numbers" approach.

In that case, you should convert to a string here since the data will be UTF-16, not UTF-8 like the later string conversion expects. Skipping every other byte like this only works for ASCII.

You're right. I was too focused on using the existing parseXml which was expecting UTF-8. Fixed.

TheModMaker · 2018-12-06T22:43:38Z

lib/dash/content_protection.js

+  }
+
+  const rootElement = shaka.util.XmlUtils.parseXml(
+    record.value.buffer, 'WRMHEADER');


This won't work since it will parse from the beginning of the buffer, not the beginning of the record. You should edit the parseXml to accept a BufferSource instead and just pass record.value.

No longer using parseXml so I didn't edit it's signature.

tylerdaines · 2018-12-13T18:24:10Z

Think I fixed all the issues in the latest code review. Anything else?

shaka-bot · 2018-12-13T18:51:59Z

All tests passed!

I can't find my pending comments and this has been in review long enough. If we find issues with it later, we can fix it ourselves.

vaage · 2018-12-13T18:54:23Z

The build bot says its good.

@TheModMaker it looks like you had the most substantial concerns. If you are good with this, I think it is safe to accept it and then we can address any minor issues ourselves.

tylerdaines · 2018-12-13T18:59:50Z

Thanks guys!

TheModMaker · 2018-12-13T19:00:10Z

FYI, I will create a follow-up to change how this works with the Player configuration. I will change it to use the manifest value as a default and use the Player configuration as an override. This means the manifest value will only be used if you don't specify a license server in the configuration. This is consistent with our other configuration settings being overrides.

chrisfillmore · 2018-12-13T19:37:48Z

@tylerdaines thanks for your work on this, this will help my team too. :)

tylerdaines · 2018-12-14T05:29:51Z

@TheModMaker I'd be happy to take that on if you haven't already started on it.

TheModMaker · 2018-12-14T16:27:52Z

Thanks for the offer, but I just pushed that change.

If the application developer specifies license servers, only those should be used. Before this, a manifest-specified license server for a certain key system could still be used if the application didn't provide one. Now, if there are _any_ license servers specified by the app, _no_ license servers will be used from the manifest. This is important because it allows the application a clear way to indicate which DRM systems should be used on platforms with multiple DRM systems. The new order of preference for drmInfo: 1. Clear Key config, used for debugging, should override everything else. (The application can still specify a clearkey license server.) 2. Application-configured servers, if any are present, should override anything from the manifest. Nuance: if key system A is in the manifest and key system B is in the player config, only B will be used, not A. 3. Manifest-provided license servers are only used if nothing else is specified. Introduced in #1644 to solve #484 Internal issue b/131264101 Closes #1905 Change-Id: I1a36a70044dc7bcc22681e3e4246d0a43d58e413

tylerdaines added 6 commits October 29, 2018 13:29

Initial commit with changes from #1010

ce9acf3

Addressed most of the issues raised with the code in the original PR.…

a5ccb57

… Tests need to be fixed still.

Got tests working with Chrome. Removed tests of private functions. Ma…

49917cd

…de getWideviceLicenseUrl() and getPlayReadyLicenseServerUrl() public so they could be tested

Added more checks around array indexing and xml parsing

c811ddf

Fixed tests so that they work in IE and Edge

6877824

Changed let to const where appropriate

9ba598b

vaage suggested changes Nov 6, 2018

View reviewed changes

tylerdaines added 2 commits November 6, 2018 11:51

Reverted some formatting changes. Changed filter callback to arrow fu…

bfe8530

…nction

Reverted whitespace changes

85e2b32

vaage suggested changes Nov 15, 2018

View reviewed changes

tylerdaines added 2 commits November 16, 2018 16:45

Created new map of key systems to license url parser functions per co…

0c32745

…de review. Simplified LA_URL xml parsing

More changes to parsing logic as suggested by code review

fa00cc2

vaage previously requested changes Nov 19, 2018

View reviewed changes

tylerdaines added 2 commits November 19, 2018 11:11

Changed let to const in unit tests. Fixed broken test. A few doc stri…

e38b54f

…ng fixes

Simplified parseMsProRecords_. Fixed broken test again. Changed PlayR…

0e8c7fa

…eadyRecord value to be Uint16Array

joeyparrish added the type: enhancement New feature or request label Nov 21, 2018

joeyparrish mentioned this pull request Nov 21, 2018

Add ms:laurl and mspr:pro parsing #1010

Closed

TheModMaker reviewed Nov 30, 2018

View reviewed changes

tylerdaines added 2 commits December 3, 2018 15:27

Added shaka.util.XmlUtils.findChildNS() helper method. Refactored par…

ed7418d

…seMsPro_ and parseMsProRecords_() to operate on ArrayBuffers

Moved shaka.dash.MpdUtils.parseXml() to shaka.util.XmlUtils

8dd163b

theodab reviewed Dec 6, 2018

View reviewed changes

Fixed alignment of JSDoc strings

3caf1ff

TheModMaker reviewed Dec 6, 2018

View reviewed changes

Added shaka.util.XmlUtils.parseXmlString()

3a1cc6b

TheModMaker merged commit 3c8241f into shaka-project:master Dec 13, 2018

tylerdaines deleted the parse-license-url branch December 13, 2018 18:59

joeyparrish mentioned this pull request Apr 29, 2019

Player config should override license servers from the manifest #1905

Closed

github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add license url parsing #1644

Add license url parsing #1644

tylerdaines commented Oct 27, 2018

googlebot commented Oct 27, 2018

tylerdaines commented Oct 29, 2018

googlebot commented Oct 29, 2018

googlebot commented Oct 29, 2018

tylerdaines commented Nov 6, 2018

ismena commented Nov 6, 2018

tylerdaines commented Nov 15, 2018

tdaines commented Nov 29, 2018

TheModMaker Nov 30, 2018

tylerdaines Dec 3, 2018

tylerdaines commented Dec 6, 2018 •

edited

Loading

TheModMaker Dec 6, 2018

tylerdaines Dec 6, 2018 •

edited

Loading

TheModMaker Dec 6, 2018

tylerdaines Dec 7, 2018

TheModMaker Dec 6, 2018

tylerdaines Dec 7, 2018

tylerdaines commented Dec 13, 2018

shaka-bot commented Dec 13, 2018

vaage commented Dec 13, 2018

tylerdaines commented Dec 13, 2018

TheModMaker commented Dec 13, 2018

chrisfillmore commented Dec 13, 2018

tylerdaines commented Dec 14, 2018

TheModMaker commented Dec 14, 2018

Add license url parsing #1644

Add license url parsing #1644

Conversation

tylerdaines commented Oct 27, 2018

googlebot commented Oct 27, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

tylerdaines commented Oct 29, 2018

googlebot commented Oct 29, 2018

googlebot commented Oct 29, 2018

tylerdaines commented Nov 6, 2018

ismena commented Nov 6, 2018

tylerdaines commented Nov 15, 2018

tdaines commented Nov 29, 2018

TheModMaker Nov 30, 2018

Choose a reason for hiding this comment

tylerdaines Dec 3, 2018

Choose a reason for hiding this comment

tylerdaines commented Dec 6, 2018 • edited Loading

TheModMaker Dec 6, 2018

Choose a reason for hiding this comment

tylerdaines Dec 6, 2018 • edited Loading

Choose a reason for hiding this comment

TheModMaker Dec 6, 2018

Choose a reason for hiding this comment

tylerdaines Dec 7, 2018

Choose a reason for hiding this comment

TheModMaker Dec 6, 2018

Choose a reason for hiding this comment

tylerdaines Dec 7, 2018

Choose a reason for hiding this comment

tylerdaines commented Dec 13, 2018

shaka-bot commented Dec 13, 2018

vaage commented Dec 13, 2018

tylerdaines commented Dec 13, 2018

TheModMaker commented Dec 13, 2018

chrisfillmore commented Dec 13, 2018

tylerdaines commented Dec 14, 2018

TheModMaker commented Dec 14, 2018

tylerdaines commented Dec 6, 2018 •

edited

Loading

tylerdaines Dec 6, 2018 •

edited

Loading