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

fix(DRM): handle all keys statuses #1423

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

Florent-Bouisset
Copy link
Collaborator

The spec https://w3c.github.io/encrypted-media/#dom-mediakeystatus
defines 8 MediaKeyStatuses. In the RxPlayer, we didn't handle every one of them.
This PR add an appropriate behavior for the missing keys.

`A "${keyStatus}" status has been encountered (${bytesToHex(keyId)})`,
{ keyStatuses: [keyStatusObj, ...badKeyStatuses] },
);
throw error;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if "output-downscaled" should be considered an error, for resiliency.

If it is output-downscaled, it's theoretically playing, just at a lower quality, isn't that semi-OK (like a warning-type of scenario?)

Copy link
Contributor

@lfaureyt lfaureyt Apr 3, 2024

Choose a reason for hiding this comment

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

I agree with @peaBerberian : some platforms thoretically support usage rules that require the device to force the downscale of video frames before outputting them to an uncompressed digital video output interface that is insufficiently protected.
Typical use is a usage rule requiring "downscale video output to SD if HDCP 1.4 or higher cannot be established on HDMI output". When processing a key associated with such usage rule and an unprotected HDMI output:

  • a platform that doesn't support downscaling is expected to fail to decrypt content and to issue an OUTPUT_RESTRICTED error.
  • a platform that does support downscaling is expected to decrypt, downscale, output content, and, I guess, to notify an OUTPUT_DOWNSCALED key status

Playready's uncompressed_digital_video_output_protection_level=270 usage rule is the typical example of such usage rule.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks I have updated the behavior to authorized to use a key that is "output-downscaled".

I'm wondering if we should take in consideration that the key status is "output-downscaled" to select the appropriate track.
For example a content with 2 representations:

  • Representation 1: FHD (1080p) with key status usable.
  • Representation 2: UHD (4K) with key status output-downscaled.

Currently the player will select the highest quality (UHD) if the client has the required bandwidth.

But which representation has a better perceived quality between the downscaled UHD and the FHD ?

Could it be possible that the downscaled UHD representation would be at a worse quality than the FHD representation ? In it's case we don't want to select the highest quality because it has been downscaled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

output-downscaled doesn't seem to indicate details about the downscaling it performs, so I'm not sure if we could be able to compare it with other Representations (if there even is other Representations).

Treating "output-downscaled" as a "whitelisted" case seems ok to me in the meantime, as we've never yet even encountered that key status, but we might have to set a more complex logic in the future if we do observe some unwanted situation with it.

@peaBerberian peaBerberian added this to the 4.1.0 milestone Apr 3, 2024
@peaBerberian peaBerberian added the DRM Relative to DRM (EncryptedMediaExtensions) label Apr 3, 2024
@Florent-Bouisset Florent-Bouisset force-pushed the fix/handle-all-key-statuses branch from 32b2e83 to 83a9c77 Compare April 4, 2024 13:35
@peaBerberian
Copy link
Collaborator

peaBerberian commented Apr 10, 2024

As discussed while drafting things after #1427 for what I would call support of "accidental multi-key responses" scenarios where the license server would respond with multiple keys despite a keySystems[].singleLicensePer: "init-data" RxPlayer option, we may have (due to a very subtle fallbacking edge case) to handle the "pending" MediaKeyStatus as neither a blacklisted key status nor as an unknown one (which could in that scenario be wrongly interpreted as unsupported if the license request was for that key id).

So the question is whether we whitelist that key in the meantime (yet that would be kind of weird to consider corresponding Representations "decipherable") or if a special "pending" status should be added, kind of like unknown but for which no fallback would be triggered (at least until we do know the real status).

@peaBerberian
Copy link
Collaborator

I'm writing about a pretty rare scenario but now that I think of it, I don't see how the same cases couldn't arise in the other singleLicensePer scenario. We could be fallbacking due to "pending" key statuses in any case if we treat it as if the key wasn't in the license.

@Florent-Bouisset Florent-Bouisset force-pushed the fix/handle-all-key-statuses branch from 83a9c77 to 8f9c7e4 Compare June 17, 2024 09:34
@Florent-Bouisset Florent-Bouisset changed the title fix: handle all keys statuses fix(DRM): handle all keys statuses Jun 17, 2024
@peaBerberian peaBerberian added the Priority: 2 (Medium) This issue or PR has a medium priority. label Jun 18, 2024
@peaBerberian peaBerberian modified the milestones: 4.1.0, 4.2.0 Jun 27, 2024
@peaBerberian peaBerberian modified the milestones: 4.2.0, 4.3.0 Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DRM Relative to DRM (EncryptedMediaExtensions) Priority: 2 (Medium) This issue or PR has a medium priority.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants