Only use selected key-system in EME "mediaencrypted" event handler #6948
Conversation
| const psshInfo = psshResults.filter( | ||
| (pssh): pssh is PsshData => | ||
| !!pssh.systemId && | ||
| keySystemIdToKeySystemDomain(pssh.systemId) === keySystem, | ||
| )[0]; |
There was a problem hiding this comment.
If you wanted to iterate through these to find the first key that matched any in mediaKeySessions that would work too. I don't expect we would have multiple pssh with the same key system.
There was a problem hiding this comment.
This is probably fine. Small improvement might be to save off the filter, always use the 0th psshInfo, but log/warn if psshInfos.length > 1
| if (!this.keyFormatPromise) { | ||
| let keySystems = Object.keys( | ||
| this.keySystemAccessPromises, | ||
| ) as KeySystems[]; | ||
| if (!keySystems.length) { | ||
| keySystems = getKeySystemsForConfig(this.config); | ||
| } | ||
| const keyFormats = keySystems | ||
| .map(keySystemToKeySystemFormat) | ||
| .filter((k) => !!k) as KeySystemFormats[]; | ||
| this.keyFormatPromise = this.getKeyFormatPromise(keyFormats); | ||
| } | ||
|
|
||
| const psshInfo = psshResults.filter((pssh): pssh is PsshData => { | ||
| const keySystem = pssh.systemId | ||
| ? keySystemIdToKeySystemDomain(pssh.systemId) | ||
| : null; | ||
| return keySystem ? keySystems.indexOf(keySystem) > -1 : false; | ||
| })[0]; | ||
| this.keyFormatPromise.then((keySystemFormat) => { |
There was a problem hiding this comment.
keyFormatPromise = this.getKeyFormatPromise now could happen in three places:
onManifestLoadedfor EXT-X-SESSION-KEY session keysselectKeySystemFormat(frag: Fragment)for HLS media playlist keys (assigned to a fragment being loaded)- (new with this change)
onMediaEncryptedat this point we would expect an encrypted fragment to signal its key format(s), but if it hasn't we can check the configured key-systems. It may be good to limit this to the intersection of configured key-systems and init data type and formats. IMO it would be incorrect to configure for a key system supported the device but not present in the media so I don't think that is important to cover.
There was a problem hiding this comment.
If the key format promise returned all the results of it's subroutine getKeySystemSelectionPromise (keySystem (format) and mediaKey), the last part pf this method (clear-lead resolution) could be simplified by just returning the results of attemptSetMediaKeys and generateRequestWithPreferredKeySession.
| let keyId: Uint8Array | null | undefined; | ||
| let keySystemDomain: KeySystems | undefined; | ||
|
|
||
| if (initDataType === 'sinf' && keySystem === KeySystems.FAIRPLAY) { |
There was a problem hiding this comment.
It might be better to log when the "encrypted" event should be ignored:
| if (initDataType === 'sinf' && keySystem === KeySystems.FAIRPLAY) { | |
| if (initDataType === 'sinf') { | |
| if (keySystem !== KeySystems.FAIRPLAY) { | |
| this.log(`Ignoring "${event.type}" event with init data type: "${initDataType}" for selected key-system ${keySystem}`); | |
| return; | |
| } |
There was a problem hiding this comment.
If this occurs (aka a mismatch between getting a sinf type and not having a media session that is fairplay), this (and the related one), it sound like warnings to me - aka we only expect to see sinf if/when we're using fairplay. Maybe switch to a warn and add the word "unexpected" ("Ignoring unexpected...")
| } | ||
| } | ||
|
|
||
| if (!keySessionContextPromise && keySystemDomain === keySystem) { |
There was a problem hiding this comment.
| if (!keySessionContextPromise && keySystemDomain === keySystem) { | |
| if (!keySessionContextPromise) { | |
| if (keySystemDomain !== keySystem) { | |
| this.log(`Ignoring "${event.type}" event with ${keySystemDomain} init data for selected key-system ${keySystem}`); | |
| return; | |
| } |
| this.warn(`${logMessage} Failed to parse sinf: ${error}`); | ||
| return; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
we should probably still check if the key system is either widevine or playready here and early bail + log/warn like we do for the fairplay + sinf case, above.
| !frag.decryptdata && | ||
| frag.encrypted && | ||
| this.emeController && | ||
| this.config.emeEnabled |
There was a problem hiding this comment.
@cjpillsbury I remember you mentioning that you had issues with the behavior when loading encrypted content that requires EME with emeEnabled: false. This would be the ideal place to log an error since playlists must include keys for encrypted content. !frag.decryptdata && frag.encrypted means we encountered DRM protected content. Keep in mind, there is no error if the key isn't handled because the applications can implement EME on its own. Either way we shouldn't call out to the emeController here when emeEnabled: false which is the reason for the addition of && this.config.emeEnabled.
There was a problem hiding this comment.
I think we should treat that as a separate PR/effort, but this looks like a good candidate. There were a few hard to identify cases of errors and this is definitely one. Ideally we'd surface the error as early as possible for root cause analysis reasons. E.g. if we encounter a multi-variant playlist or media playlist with (session) keys but don't have sufficient config for said keys, dispatch an error then (near the playlist parsing stage).
The hardest one that I'm not sure belongs in hls.js at all is when folks think some media is DRM protected but actually isn't. We could make assumptions that if we encounter drm config that means folks think the content is DRM protected, but that's a bad assumption for folks who have mixed content with a shared config. Given the cases of clear lead or valid cases where lower quality renditions may be unencrypted, there may be too many permutations to safely assume this in an overly generic way. Food for thought, though.
There was a problem hiding this comment.
Agree this is for another PR.
if we encounter a multi-variant playlist or media playlist with (session) keys
Session keys don't point to variants and may or may not be used. Encrypted fragments (media playlist keys) is the earliest place we can error with certainty.
when folks think some media is DRM protected but actually isn't
Is there an example where that errors? The eme-controller with emeEnabled and drmSystem configs have no effect outside of adding "encrypted" and "waitingforkey" event listeners to the media element (for unprotected content). Key-system access is only attempted when HLS KEY or SESSION-KEY tags are encountered or "encrypted" is emitted.
Resolves #6947 / Suggested changes for #6946
CC @cjpillsbury. Thanks for reporting #6947 and submitting #6946. I was able to reproduce the issue. The only issue with the PR was that adding async/await functions introduces regenerator-runtime boilerplate (about 300 lines) to hls.js. To avoid this I wanted to see if we could simplify the fix. Resolving the selected key-system up front (as it always should be based on playlist keys/config) simplifies the rest.