fix: assumptions in multi-drm cases where only playready is available for playback. #6946
Conversation
| const keySystem = pssh.systemId | ||
| ? keySystemIdToKeySystemDomain(pssh.systemId) | ||
| : null; | ||
| return keySystem ? keySystems.indexOf(keySystem) > -1 : false; | ||
| })[0]; |
There was a problem hiding this comment.
NOTE: Here's an example where we're presumptuously assuming that the 0th key system will work in a multi-drm scenario, which may or may not be true.
| } | ||
| } else if (this.getLicenseServerUrl(KeySystems.WIDEVINE)) { |
There was a problem hiding this comment.
here's an example where we're tying ourselves explicitly to the availability of a Widevine DRM server for PSSH cases, when it may be widevine, playready, or both.
| // FairPlay and only FairPlay uses sinf boxes for EME signaling, so | ||
| // if we don't have a FairPlay license server URL available, we can't | ||
| // use the sinf for EME. | ||
| if (!fairplayLicenseServerUrl) return []; |
There was a problem hiding this comment.
since this code is now a function that yields 0+ key id + key system domain pairs/duals as candidates to use for playback, we return an empty array for any case where there are no matches based on init data + config
| break; | ||
| } | ||
| } | ||
| /** @TODO errors? */ |
There was a problem hiding this comment.
NOTE: If we early bail here, this means we didn't find any matches for playback between the intersection of our config + what was signaled from EME in the init data. We plausibly should construct an error here, and potentially earlier in the newly created getKeyIdInfos() function (if we want more context clues in our error as to what specifically failed)
| ); | ||
|
|
||
| try { | ||
| // Use the first successful key session context promise. |
There was a problem hiding this comment.
NOTE: In order to strike a balance between non-blocking async behavior and not requesting keys for multiple key systems in parallel, the balance I struck in this implementation is to keep everything for a single key system roughly as it was in the old implementation, but then block on a 👍 / 👎 for that key system. If that key system fails to be usable, we proceed to the next (in cases where that applies).
There was a problem hiding this comment.
Would you mind using then/catch (as we do in other parts of the code) rather than await keySessionContextPromise?
| // to get any key for playback. In this case, handle these errors | ||
| // NOTE: Currently, we will not signal failed key requests if at least | ||
| // one succeeds, since this *can* happen and be valid in multi-drm scenarios. | ||
| if (errors.length >= keyIdInfos.length) { |
There was a problem hiding this comment.
NOTE: See note. (tl;dr - I didn't see a clean way to signal errors that are actual errors when one but not all key systems fail without a much larger refactor)
cjpillsbury
changed the title
|
After confirming the bug and verifying the fix these changes will go into v1.6.0 (beta.3). If you'd like a patch for v1.5 we can follow up after that by cherry-picking the changes in the patch/v1.5.x branch. Thanks for the detailed report and fix! |
| @@ -525,7 +525,7 @@ class EMEController extends Logger implements ComponentAPI { | |||
| return this.attemptKeySystemAccess(keySystemsToAttempt); | |||
| } | |||
|
|
|||
| private onMediaEncrypted = (event: MediaEncryptedEvent) => { | |||
| private onMediaEncrypted = async (event: MediaEncryptedEvent) => { | |||
There was a problem hiding this comment.
Making this method async for the await at the bottom of the method adds a lot of boiler plate (regenerator runtime) to the UMD build. Would you mind using then/catch (as we do in other parts of the code) rather than await keySessionContextPromise?
There was a problem hiding this comment.
Rather than suggest changes without verifying that they work, I put together this draft:
Besides avoiding the use of async/await which adds regenerator-runtime boilerplate to our es5 output, I wanted to avoid the additional looping through found pssh and config, by resolving the selected key-system first. This is closer to how playlist keys are selected. onMediaEncrypted will always run after a playlist key has been selected unless the encrypted event is for a clear segment that signals upcoming encrypted keys ("clear-lead").
| ); | ||
|
|
||
| try { | ||
| // Use the first successful key session context promise. |
There was a problem hiding this comment.
Would you mind using then/catch (as we do in other parts of the code) rather than await keySessionContextPromise?
This PR will...
Clean up some assumptions on key request logic in multi-drm scenarios, particularly around PlayReady usage.
Why is this Pull Request needed?
There were
Are there any points in the code the reviewer needs to double check?
Everything was localized to
EmeController::onMediaEncrypted()and much of the core logic stayed the same, but definitely want some attention while reviewing the implementation decisions, particularly the callout questions/comments (including in github comments) before 👍.Smoke tested multi-drm case (on demand stream with FairPlay, Widevine, and PlayReady advertised via
EXT-X-KEYtags andsinf/psshmp4 boxes) on:Also tested using both the repo-provided demo application* and local linking to Mux Elements ("Mux Player") using the repo-provided next.js application.
NOTE: For my testing, where my dev machine is macOS but I needed to test on a Windows machine on my LAN, this required updating the
serverscript (http-server) to usehttps(based on security constraints). If needed for review, see https://www.npmjs.com/package/http-server#tlsssl or reach out to me directly.NOTE: I do not have any publicly-shareable test assets + corresponding service requests, however, if needed for review, reach out to me directly and I can provide one out of band.
Resolves issues:
fixes #6947
Checklist