-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(hls): Support AES-128 in HLS TS segments. #3880
Conversation
lib/hls/hls_parser.js
Outdated
throw new shaka.util.Error( | ||
shaka.util.Error.Severity.CRITICAL, | ||
shaka.util.Error.Category.MANIFEST, | ||
shaka.util.Error.Code.HLS_KEYFORMATS_NOT_SUPPORTED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to add a new error code for this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Done.
lib/hls/hls_parser.js
Outdated
throw new shaka.util.Error( | ||
shaka.util.Error.Severity.CRITICAL, | ||
shaka.util.Error.Category.MANIFEST, | ||
shaka.util.Error.Code.HLS_KEYFORMATS_NOT_SUPPORTED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to add a new error code for this case. Because then there is a better explanation of what is happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/hls/hls_parser.js
Outdated
throw new shaka.util.Error( | ||
shaka.util.Error.Severity.CRITICAL, | ||
shaka.util.Error.Category.MANIFEST, | ||
shaka.util.Error.Code.HLS_KEYFORMATS_NOT_SUPPORTED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to add a new error code for this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/hls/hls_parser.js
Outdated
throw new shaka.util.Error( | ||
shaka.util.Error.Severity.CRITICAL, | ||
shaka.util.Error.Category.MANIFEST, | ||
shaka.util.Error.Code.HLS_AES_128_ENCRYPTION_NOT_SUPPORTED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it would be good to add an extra parameter that says the mimetype is not supported.
throw new shaka.util.Error(
shaka.util.Error.Severity.CRITICAL,
shaka.util.Error.Category.MANIFEST,
shaka.util.Error.Code.HLS_AES_128_ENCRYPTION_NOT_SUPPORTED,
mimeType);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed to the same error handling as above for raw and video/webm.
lib/hls/hls_parser.js
Outdated
@@ -2145,6 +2197,8 @@ shaka.hls.HlsParser = class { | |||
}, | |||
); | |||
|
|||
// If AES-128 is used, this option should be enabled to decrypt the full | |||
// segment. | |||
if (this.config_.hls.useFullSegmentsForStartTime) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that instead of doing this configuration manually, if it is detected that the stream is AES-128, the entire segment should always be requested automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@wjywbs What would be missing for MP4 support? |
Actually I don't have a MP4 AES test stream, and my use case doesn't need it yet. |
From what I know, whether the mp4 init segment is encrypted depends on the order of EXT-X-MAP and EXT-X-KEY. "If the #EXT-X-MAP tag follows #EXT-X-KEY tag, the initialization segment must also be encrypted." |
https://datatracker.ietf.org/doc/html/draft-pantos-hls-rfc8216bis-10#section-4.4.4.4
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for your contribution!
externs/shaka/manifest.js
Outdated
* | ||
* @exportDoc | ||
*/ | ||
shaka.extern.ClearKey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the biggest issue I have with this PR is this name. ClearKey is an open, EME-compatible key system. AES-128 in HLS is not the same thing at all. ClearKey and other EME key systems are sample-encrypted, and AES-128 in HLS is whole-segment encryption.
If these were compatible, we could implement HLS's AES-128 encryption with a ClearKey CDM, but instead we have to use WebCrypto.
Similarly, to implement ClearKey DRM with WebCrypto, we would have to parse the segments to determine which parts to decrypt.
I know this is a confusing area of the media ecosystem, but I think it would be best to find another name for this type. How about something like shaka.extern.HlsAes128Key
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Changed.
externs/shaka/manifest.js
Outdated
@@ -332,6 +360,9 @@ shaka.extern.CreateSegmentIndexFunction; | |||
* The stream's key IDs as lowercase hex strings. These key IDs identify the | |||
* encryption keys that the browser (key system) can use to decrypt the | |||
* stream. | |||
* @property {(shaka.extern.ClearKey|undefined)} clearKey | |||
* <i>Defaults to undefined (i.e., no AES-128 encryption).</i> <br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since EME-compatible DRM also uses AES-128 (in CTR or CBC mode, encrypted at the sample level), this is ambiguous. We should reference HLS here for clarity, since this is specifically HLS's AES-128-CBC, full-segment encryption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
/** @type {!Array.<shaka.extern.DrmInfo>}*/ | ||
const drmInfos = []; | ||
const keyIds = new Set(); | ||
|
||
// TODO: May still need changes to support key rotation. | ||
/* eslint-disable no-await-in-loop */ | ||
// TODO: May still need changes to support key rotation and discontinuity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the keys can change across a discontinuity, then the key and IV need to be added to the SegmentReference. We can use the same object reference for key data in several SegmentReference objects. This is similar to how we handle init segments today.
If we will need to change the structure in a backward-incompatible way to handle discontinuities, I would prefer to make that change before we merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I see other players store key info per segment, so key rotation can be handled. I'm checking how to add key info to SegmentReference, but likely only for AES-128. DRM is handled outside of segment for a long time, so I don't know how much change is needed to support DRM key rotation, or how unencrypted discontinuity is currently handled in a DRM stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain how unencrypted discontinuity is currently handled in a DRM stream? I could follow the same process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discontinuity results in two side-by-side references having certain parameters change. For example, different init segment references.
When StreamingEngine sees a change in init segment, timestamp offset, etc., it will handle that by adjusting offsets, fetching new init segments, etc.
So if a key can vary from segment to segment, you could put the key structure in SegmentReference instead of Stream. Then StreamingEngine becomes responsible for fetching new keys, just like it would fetch new init segments. (It is also smart enough not to fetch the same init segment over and over, so you should handle that for keys, too.)
lib/hls/hls_parser.js
Outdated
/** @type {!Array.<shaka.extern.DrmInfo>}*/ | ||
const drmInfos = []; | ||
const keyIds = new Set(); | ||
|
||
// TODO: May still need changes to support key rotation. | ||
/* eslint-disable no-await-in-loop */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use eslint-disable-next-line and move this to just above the await statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/hls/hls_parser.js
Outdated
@@ -2080,14 +2133,15 @@ shaka.hls.HlsParser = class { | |||
* @param {string} type | |||
* @param {string} codecs | |||
* @param {number|undefined} bandwidth | |||
* @param {(shaka.extern.ClearKey|undefined)} clearKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JFYI, we're in process of removing the whole system of fetching and parsing segments for timestamps. We're switching to MSE sequence mode for HLS, which will both eliminate this code and add support for containerless formats (like raw AAC). If that change is merged first, you'll need to deal with merge conflicts here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that may simplify the aes-128 process here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That work has now been merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebased the change.
lib/hls/hls_parser.js
Outdated
@@ -2287,6 +2345,14 @@ shaka.hls.HlsParser = class { | |||
} | |||
|
|||
if (mimeType == 'video/mp4' || mimeType == 'audio/mp4') { | |||
if (clearKey) { | |||
shaka.log.alwaysWarn( | |||
'AES-128 in MP4 HLS is not yet supported. Skipping ' + mimeType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to get full support without regard for type. If the concern is that order of tags might change whether a given segment (init segment, etc) is encrypted, then perhaps moving the encryption data to SegmentReference would help. See my comments above regarding discontinuities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be deferred to a future contribution? I don't have an HLS AES MP4 test stream, and it's less popular than AES TS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some tests to the demo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added "Art of Motion".
lib/hls/hls_parser.js
Outdated
const keyUri = shaka.hls.Utils.constructAbsoluteUri( | ||
this.masterPlaylistUri_, drmTag.getRequiredAttrValue('URI')); | ||
// eslint-disable-next-line no-await-in-loop | ||
const keyResponse = await this.requestManifest_(keyUri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not correct, it seems that you are trying to request a manifest, when a license request should be made. Ex: https://github.com/google/shaka-player/blob/master/lib/media/drm_engine.js#L1380
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a DRM license request. Otherwise some existing DRM license wrapping code may break.
https://shaka-player-demo.appspot.com/docs/api/tutorial-fairplay.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it would be best to create a new type, see: https://shaka-player-demo.appspot.com/docs/api/lib_net_networking_engine.js.html#line738
So in the application we can use specific filters for this.
@wjywbs what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope @joeyparrish could provide some input on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think filters need to be aware of what they are playing. License-wrapping filters, for example, shouldn't be applied to content that doesn't use licenses.
That said, it's trivial to add another request type (such as KEY or something) that is distinct from LICENSE, to make the lives of developers somewhat easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added KEY request type.
@joeyparrish can you review it? Thanks! |
@joeyparrish is it possible include it in 4.0? Thanks! |
@joeyparrish up! Thanks! |
@joeyparrish Please don't forget this PR |
Thanks, I won't forget. Though I will miss my own estimates/deadlines. :-( |
Ping @joeyparrish / @theodab |
I hope to see it in 4.0 😄 |
/** @type {!Array.<shaka.extern.DrmInfo>}*/ | ||
const drmInfos = []; | ||
const keyIds = new Set(); | ||
|
||
// TODO: May still need changes to support key rotation. | ||
/* eslint-disable no-await-in-loop */ | ||
// TODO: May still need changes to support key rotation and discontinuity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discontinuity results in two side-by-side references having certain parameters change. For example, different init segment references.
When StreamingEngine sees a change in init segment, timestamp offset, etc., it will handle that by adjusting offsets, fetching new init segments, etc.
So if a key can vary from segment to segment, you could put the key structure in SegmentReference instead of Stream. Then StreamingEngine becomes responsible for fetching new keys, just like it would fetch new init segments. (It is also smart enough not to fetch the same init segment over and over, so you should handle that for keys, too.)
The biggest thing holding me back from merging this is that it seems like this will not work with key rotation or discontinuities. It seems like those things will require the key metadata to be attached to SegmentReferences. I don't want to merge it as-is, only to have to make a breaking change to the structure later to support those things. I would also like to have support for MP4 content, as well, but that could reasonably be done in a follow-up PR. |
If we can at least resolve the discontinuity issue in the next week or so, I could hold v4.0 for this. Otherwise, I think this may slip to v4.1+. Sorry for any inconvenience for those who are waiting for this. |
@wjywbs , can you review the latest comments? (and resolve conflicts). Thanks! |
I constructed a few discontinuity test cases and found that discontinuity in DRM HLS streams didn't work either. Both DRM and HLS AES key infos should be moved to SegmentReferences, but I'm not sure how large the change would be, or how feasible to do these in this change. These 2 examples can play in video.js but can't play in shaka:
Note: HLS AES-128 specific, if EXT-X-KEY does not have IV attribute, the Media Sequence Number (EXT-X-MEDIA-SEQUENCE) is used as IV. Since EXT-X-MEDIA-SEQUENCE can only appear once in the manifest, the packager needs to account for the Media Sequence Number increase of discontinuity segments. |
Hello, will there be support for AES 128 for HLS MP4? |
Ultimately, yes. This PR doesn't provide MP4 support, though. |
@wjywbs I couldn't find links to AES-128 with mp4, but I have seen someone on Exoplayer post examples with SAMPLE-AES. |
Also adds support for key rotation in HLS, but only for AES-128. Based on shaka-project#3880 Issue shaka-project#850
Also adds support for key rotation in HLS, but only for AES-128. Based on shaka-project#3880 Issue shaka-project#850
Also adds support for key rotation in HLS, but only for AES-128. Based on shaka-project#3880 Issue shaka-project#850
Awaiting this feature with breathless anticipation. Any hope of this making it into the next release? |
Also adds support for key rotation in HLS, but only for AES-128. Based on shaka-project#3880 Issue shaka-project#850
Also adds support for key rotation in HLS, but only for AES-128. Based on shaka-project#3880 Issue shaka-project#850
Also adds support for key rotation in HLS, but only for AES-128. Based on shaka-project#3880 Issue shaka-project#850
@tim-edgren We now have an updated version of this PR (#4386) in review. So it's hopefully not going to be very long anymore. |
Expands on the original PR (#3880) by adding support for MP4 and key rotation. Close #850 Co-authored-by: wjywbs <[email protected]>
Expands on the original PR (shaka-project#3880) by adding support for MP4 and key rotation. Close shaka-project#850 Co-authored-by: wjywbs <[email protected]>
Expands on the original PR (shaka-project#3880) by adding support for MP4 and key rotation. Close shaka-project#850 Co-authored-by: wjywbs <[email protected]>
Description
Part of #850. Use with mux.js:
These demo videos can play:
https://playertest.longtailvideo.com/adaptive/oceans_aes/oceans_aes.m3u8 (No IV)
https://bitmovin-a.akamaihd.net/content/art-of-motion_drm/m3u8s/11331.m3u8 (Const IV)
Key rotation (and maybe discontinuity) is not supported. (Existing TODO: May still need changes to support key rotation)
Type of change
not work as expected)
Checklist:
./build/all.py
and the build passes./build/test.py
and all tests pass