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.

Sign up for GitHub

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

Jump to bottom

Some DRM providers require default KID in wrapped license requests #529

Merged
merged 9 commits into from
Sep 26, 2016
Merged

Some DRM providers require default KID in wrapped license requests #529

merged 9 commits into from
Sep 26, 2016

Conversation

birme
Copy link
Contributor

@birme birme commented Sep 20, 2016

Suggested fix for #528

joeyparrish
joeyparrish requested changes Sep 22, 2016
View reviewed changes
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some feedback on your design. Are you using player.drmInfo() to get the new information into your filter?

AUTHORS
@@ -31,3 +31,4 @@ SameGoal Inc. <*@samegoal.com>
Sanborn Hilland <sanbornh@rogers.com>
TalkTalk Plc <*@talktalkplc.com>
uStudio Inc. <*@ustudio.com>
Jonas Birmé <jonas.birme@eyevinn.se>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep this list alphabetized.

CONTRIBUTORS
@@ -48,3 +48,4 @@ Thomas Stephens <thomas@ustudio.com>
Timothy Drews <tdrews@google.com>
Vasanth Polipelli <vasanthap@google.com>
Vignesh Venkatasubramanian <vigneshv@google.com>
Jonas Birmé <jonas.birme@eyevinn.se>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep this list alphabetized.

lib/media/drm_engine.js
@@ -764,6 +778,10 @@ shaka.media.DrmEngine.prototype.processDrmInfos_ =
}
});
}

if (drmInfo.defaultKeyID) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inconsistent with other places where you called it defaultKID, but as mentioned earlier, please rename to keyIds.

externs/shaka/manifest.js
@@ -136,7 +136,8 @@ shakaExtern.InitDataOverride;
* audioRobustness: string,
* videoRobustness: string,
* serverCertificate: Uint8Array,
* initData: Array.<!shakaExtern.InitDataOverride>
* initData: Array.<!shakaExtern.InitDataOverride>,
* defaultKID: ?string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There can be (and best practice for content authoring dictates there should be) multiple key IDs in a single manifest. If you're going to go this route, this should be an array of strings (default empty list, not null), and it should be called keyIds.

@birme
Copy link
Contributor Author

birme commented Sep 22, 2016

Thank you for the feedback. I will correct these things

joeyparrish
joeyparrish requested changes Sep 23, 2016
View reviewed changes
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one more change, please. The rest looks good.

lib/media/drm_engine.js
@@ -723,10 +740,11 @@ shaka.media.DrmEngine.prototype.createCurrentDrmInfo_ = function(
* @param {!Array.<string>} licenseServers
* @param {!Array.<!Uint8Array>} serverCerts
* @param {!Array.<!shakaExtern.InitDataOverride>} initDatas
* @param {!Array.<string>} defaultKeys
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, let's give this the same name as the structure member: keyIds.

lib/media/drm_engine.js
@@ -685,7 +697,11 @@ shaka.media.DrmEngine.prototype.createCurrentDrmInfo_ = function(
/** @type {!Array.<!shakaExtern.InitDataOverride>} */
var initDatas = [];

this.processDrmInfos_(drmInfos, licenseServers, serverCerts, initDatas);
/** @type {!Array.<!string>} */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strings are non-nullable by default, so you can just say !Array.<string>

@birme
Copy link
Contributor Author

birme commented Sep 23, 2016

No probs, fixed that last thing now

joeyparrish
joeyparrish approved these changes Sep 23, 2016
View reviewed changes
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I'll run it through the build bot to test it.

@shaka-bot
Copy link
Collaborator

Testing in progress...

@shaka-bot
Copy link
Collaborator

Failure:

Chrome 53.0.2785 (Linux 0.0.0).DrmEngine getDrmInfo includes correct info FAILED
  Expected Object({ keySystem: 'drm.abc', licenseServerUri: 'http://abc.drm/license', distinctiveIdentifierRequired: true, persistentStateRequired: true, audioRobustness: 'good', videoRobustness: 'really_really_ridiculously_good', serverCertificate: undefined, initData: [  ], keyIds: [  ] }) to equal Object({ keySystem: 'drm.abc', licenseServerUri: 'http://abc.drm/license', distinctiveIdentifierRequired: true, persistentStateRequired: true, audioRobustness: 'good', videoRobustness: 'really_really_ridiculously_good', serverCertificate: undefined, initData: [  ] }).
      at test/media/drm_engine_unit.js:1455:25
      at wrapper (lib/polyfill/promise.js:333:21)
      at Function.shaka.polyfill.Promise.flush (lib/polyfill/promise.js:440:11)
      at test/test/util/util.js:76:32


Failed 1 tests, passed 890, skipped 36

@joeyparrish
Copy link
Member

Looks like you'll need to update the tests to match. You can run the tests for yourself with python build/test.py.

@birme
Copy link
Contributor Author

birme commented Sep 24, 2016

Chrome 53.0.2785 (Mac OS X 10.10.5): Executed 891 of 927 (skipped 36) SUCCESS (6 mins 11.643 secs / 7 mins 27.425 secs)

@shaka-bot
Copy link
Collaborator

Testing in progress...

@shaka-bot
Copy link
Collaborator

Failure:

+ echo START-BUILD
START-BUILD
+ ./build/all.py
----- FILE  :  /var/lib/jenkins/jobs/Manual PR Test/workspace/test/media/drm_engine_unit.js -----
Line 1444, E:0110: Line too long (99 characters).
Found 1 errors, including 0 new errors, in 1 files (161 files OK).
Generating Closure dependencies...
Running Closure linter...
Build step 'Execute shell' marked build as failure

@birme
Copy link
Contributor Author

birme commented Sep 26, 2016

Sorry, thought I did a lint check before pushing. Will fix

@shaka-bot
Copy link
Collaborator

Testing in progress...

@shaka-bot
Copy link
Collaborator

All tests passed!

@joeyparrish joeyparrish merged commit d05b7d5 into shaka-project:master Sep 26, 2016
@joeyparrish joeyparrish mentioned this pull request Sep 26, 2016
Closed
joeyparrish pushed a commit that referenced this pull request Oct 19, 2016
@birme @joeyparrish
Some DRM providers require default KID in wrapped license requests (#529
550a05c 
)

Makes key IDs from the manifest available to the app through DrmInfo.  From there, they can be used in license request filters.
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joeyparrish joeyparrish joeyparrish approved these changes

Assignees
No one assigned
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@birme @shaka-bot @joeyparrish