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.

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

Improve EME Controller: parse PSSH box in manifest #1915

Closed
wants to merge 4 commits into from

Conversation

ssreed
Copy link
Contributor

@ssreed ssreed commented Sep 13, 2018

This PR will...

Add support for parsing PSSH element from manifest rather than relying on embedded in the init segment.

Why is this Pull Request needed?

Background

According to the "cenc" Initialization Data spec, it "...also specifies storage of a 'pssh' box base64-encoded in an XML element of the form <cenc:pssh (base64 'pssh')>." In our case it's in the hls manfiest which according to the Widevine with DRM doc, it should be in the URI like so: URI=”data:text/plain;base64,<base64 encoded PSSH box>”

Current Widevine Sample Stream

In the case of this sample widevine stream: https://storage.googleapis.com/shaka-demo-assets/angel-one-widevine-hls/hls.m3u8, the init.mp4 has the necessary pssh information for the encrypted event to be triggered.

          ---pssh
                  size: 62
                  version: 0
                  flags: 0x000000
                  System ID: 0xedef8ba979d64acea3c827dcd51d21ed
                  Data Size: 30

But that information is also in the manifest:

#EXT-X-KEY:METHOD=SAMPLE-AES-CTR,URI="data:text/plain;base64,AAAAPnBzc2gAAAAA7e+LqXnWSs6jyCfc1R0h7QAAAB4iFnNoYWthX2NlYzJmNjRhYTc4OTBhMTFI49yVmwY=",KEYID=0x800AACAA522958AE888062B5695DB6BF,KEYFORMATVERSIONS="1",KEYFORMAT="urn:uuid:edef8ba9-79d6-4ace-a3c8-27dcd51d21ed"

Are there any points in the code the reviewer needs to double check?

  1. Should base64ToArrayBuffer be in it's own file?
  2. I attached levelKey to the level but we may need to think about situations where there are multi-drm streams with more than one key tag. ( I have a separate WIP branch which I'm looking into this )
  3. Do we have a sample stream where there isn't a PSSH box in the stream itself? The way I tested that scenario was the simply comment out the following line and play the widevine test stream.

https://github.com/video-dev/hls.js/pull/1915/files#diff-66dbb17319c4f0d5f009baca4e3ecf36R484

Resolves issues:

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

@tchakabam tchakabam self-requested a review September 13, 2018 13:05
Copy link
Collaborator

@tchakabam tchakabam left a comment

Choose a reason for hiding this comment

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

Please extend the existing unit tests!

👍

@@ -73,6 +73,18 @@ const getSupportedMediaKeySystemConfigurations = function (keySystem, audioCodec
}
};

function base64ToArrayBuffer (base64String) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please make a util module (seperate file) for this :)

}

// add initData and type if they are not included in playlist
if (this.initData && !this._hasSetMediaKeys) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

first of all, please document member types in the constructor, and make sure to pre-initialize them with a certain value (null in this case, and document that it is an ArrayBuffer type).

please look at how things are already set up when you add things.

for example, private members are prefixed with underscore. that is our coding style-guide, which we are strict about for newly added code.

}
}

onLevelLoaded (data) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add jsdoc on the data-type you are receiving here

@@ -485,6 +498,35 @@ class EMEController extends EventHandler {

this._attemptKeySystemAccess(KeySystems.WIDEVINE, audioCodecs, videoCodecs);
}

onFragLoaded (data) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add jsdoc on function params

@@ -252,7 +252,7 @@ export default class M3U8Parser {
decryptiv = keyAttrs.hexadecimalInteger('IV');
if (decryptmethod) {
levelkey = new LevelKey();
if ((decrypturi) && (['AES-128', 'SAMPLE-AES', 'SAMPLE-AES-CENC'].indexOf(decryptmethod) >= 0)) {
if ((decrypturi) && (['AES-128', 'SAMPLE-AES', 'SAMPLE-AES-CENC', 'SAMPLE-AES-CTR'].indexOf(decryptmethod) >= 0)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

great

@ssreed
Copy link
Contributor Author

ssreed commented Sep 13, 2018

@tchakabam: Updated as per comments, please let me know if it knows further updating.

Thanks. 👍

@ssreed
Copy link
Contributor Author

ssreed commented Sep 17, 2018

Continued in #1918.

@ssreed ssreed closed this Sep 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants