Skip to content

Improve EME Controller: parse PSSH box in manifest - #1915

Closed
ssreed wants to merge 4 commits into
video-dev:masterfrom
ssreed:add-pssh-in-manifest-support
Closed

Improve EME Controller: parse PSSH box in manifest#1915
ssreed wants to merge 4 commits into
video-dev:masterfrom
ssreed:add-pssh-in-manifest-support

Conversation

@ssreed

@ssreed ssreed commented Sep 13, 2018

Copy link
Copy Markdown
Contributor

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

@tchakabam tchakabam left a comment

Copy link
Copy Markdown
Collaborator

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!

👍

Comment thread src/controller/eme-controller.js Outdated
}
};

function base64ToArrayBuffer (base64String) {

Copy link
Copy Markdown
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 :)

Comment thread src/controller/eme-controller.js Outdated
}

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

Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread src/controller/eme-controller.js Outdated
this._attemptKeySystemAccess(KeySystems.WIDEVINE, audioCodecs, videoCodecs);
}

onFragLoaded (data) {

Copy link
Copy Markdown
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

Comment thread src/loader/m3u8-parser.js
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
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

great

@ssreed

ssreed commented Sep 13, 2018

Copy link
Copy Markdown
Contributor Author

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

Thanks. 👍

@ssreed

ssreed commented Sep 17, 2018

Copy link
Copy Markdown
Contributor Author

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.

2 participants