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

feat: parse key attributes for Widevine HLS #88

Merged

Conversation

alex-barstow
Copy link
Contributor

This adds parsing of EXT-X-KEY attributes for Widevine support, as outlined in this spec

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

One question that is probably not useful. Looks good, otherwise.

@@ -49,6 +50,7 @@ export default class Parser extends Stream {
'CLOSED-CAPTIONS': {},
'SUBTITLES': {}
};
const widevineUuid = 'urn:uuid:edef8ba9-79d6-4ace-a3c8-27dcd51d21ed';
Copy link
Member

@misteroneill misteroneill Jun 20, 2019

Choose a reason for hiding this comment

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

I know nothing about this really, but is this a universally valid UUID?
Answered my own question.

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

I think it would be good to add a test and update the docs as well.

@@ -49,6 +50,7 @@ export default class Parser extends Stream {
'CLOSED-CAPTIONS': {},
'SUBTITLES': {}
};
const widevineUuid = 'urn:uuid:edef8ba9-79d6-4ace-a3c8-27dcd51d21ed';
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 from the spec?

Copy link
Member

@misteroneill misteroneill Jun 20, 2019

Choose a reason for hiding this comment

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

Yeah, I had the same question. It's in the spec doc... 🤷‍♂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, and the same string is used for mpds (see mpd-parser)

Copy link
Member

Choose a reason for hiding this comment

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

Might be useful to add a comment and link to the widevine hls extension spec

return;
}

if (entry.attributes.METHOD === 'SAMPLE-AES-CENC') {
Copy link
Member

Choose a reason for hiding this comment

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

This case is fine just deprecated?

Maybe we should group the ones that exit early?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just logging a warning should be fine. The spec didn't replace 'SAMPLE-AES-CENC' with 'SAMPLE-AES-CTR' all that long ago so there's likely still a good amount of content out there using the former. I could group the early exits together, but that would require separating the two METHOD validation blocks. I don't feel strongly either way.


// if Widevine key attributes are valid, store them as `contentProtection`
// on the manifest to emulate Widevine tag structure in a DASH mpd
this.manifest.contentProtection = {
Copy link
Member

Choose a reason for hiding this comment

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

should we call this out specifically as widevine since it isn't standard hls?

Copy link
Member

Choose a reason for hiding this comment

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

I guess the mpd parser calls it this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean renaming contentProtection to widevine? We could, but that would require an additional change in VHS (and perhaps elsewhere), which already expects contentProtection. Calling it contentProtection maintains greater parity with mpd-parser.

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Changes and test look good. 👍

@misteroneill
Copy link
Member

Verified manually. :shipit:

@misteroneill misteroneill merged commit d835fa8 into videojs:master Jun 25, 2019
miadabdi pushed a commit to miadabdi/m3u8-parser that referenced this pull request Jul 21, 2021
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.

3 participants