Closed Bug 1228461 Opened 4 years ago Closed 4 years ago

[EME] Implement "keyids" initData type for ClearKey

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

The EME spec has a "keyids" initData type which is used in ClearKey. We should support that so that we match the spec.
In ~ClearKeySession() we assert that when we're releasing the ClearKeyDecryptionManager's ClearKeyDecryptor's that we have a key for the decryptor's we're releasing. That's not a valid assumption, as a license may not have been sent to the CDM, but the CDM may still have encountered initData causing us to expect the key, and thus create a ClearKeyDecryptor for it.

Without this change, we'll hit the assert in ~ClearKeySession if we create a clearkey MediaKeys, send it initData which contains keyIds, and refresh the page without sending a license to the CDM.
Attachment #8692709 - Flags: review?(gsquelart)
* Plumb up "keyids" initDataType in Gecko and gmp-clearkey.
* Split ClearKeyUtils::ParseInitData() into CENC and KeyIds parts.
* Implement a JSON keyIds parser, similar to the existing JSON based parser for license data.
* Add mochitest, in which all the test cases pass in Chrome too (outside of mochitest).
* Remove spaces from our license-request message, to make the test easier to write.
Attachment #8692711 - Flags: review?(gsquelart)
Attachment #8692709 - Flags: review?(gsquelart) → review+
Comment on attachment 8692711 [details] [diff] [review]
Patch 2: Implement keyids initData type for ClearKey

Review of attachment 8692711 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments:

::: dom/media/eme/MediaKeySystemAccess.cpp
@@ +443,5 @@
>      for (const nsString& candidate : aCandidate.mInitDataTypes.Value()) {
>        if (candidate.EqualsLiteral("cenc")) {
>          initDataTypes.AppendElement(candidate);
>        }
> +      if (candidate.EqualsLiteral("keyids") && aKeySystem.EqualsLiteral("org.w3.clearkey")) {

Nit: I'd put an 'else' there, to show that this test and the previous one can't both be true.

@@ +444,5 @@
>        if (candidate.EqualsLiteral("cenc")) {
>          initDataTypes.AppendElement(candidate);
>        }
> +      if (candidate.EqualsLiteral("keyids") && aKeySystem.EqualsLiteral("org.w3.clearkey")) {
> +        initDataTypes.AppendElement( candidate);

Nit: Remove space before 'candidate'.

::: dom/media/test/test_eme_key_ids_initdata.html
@@ +84,5 @@
> +          session.addEventListener("message", function(event) {
> +            is(event.messageType, "license-request", "'" + test.name + "' MediaKeyMessage type should be license-request.");
> +            var text = new TextDecoder().decode(event.message);
> +            is(text, test.expectedRequest, "'" + test.name + "' got expected response.");
> +            is(text == test.expectedRequest, test.expectPass, "'" + test.name + "' expected to pass");

"expected to pass" is incorrect if test.expectPass is false, you could probably do: |"' expected to " + (test.expectPass ? "pass" : "fail")|.
Attachment #8692711 - Flags: review?(gsquelart) → review+
https://hg.mozilla.org/mozilla-central/rev/05c5396b7230
https://hg.mozilla.org/mozilla-central/rev/ec6611006fd0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.