EME: Clear Key CDM now generates keystatuseschange event on close() (issue 2108033002 by jrummell@chromium.org)

15 views
Skip to first unread message

jrum...@chromium.org

unread,
Jun 28, 2016, 8:13:57 PM6/28/16
to ddorwin+...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, eme-r...@chromium.org, mlamouri+w...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
Reviewers: ddorwin
CL: https://codereview.chromium.org/2108033002/

Message:
PTAL.

Description:
EME: Clear Key CDM now generates keystatuseschange event on close()

EME spec requires a CDM to generate a keystatuseschange event when
close() is called (as any available keys for that session are now
gone). This updates the Clear Key CDM to generate the required event.

BUG=622956
TEST=updated EME tests passes

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+30, -2 lines):
M media/cdm/aes_decryptor.cc
M third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-keystatuses.html


Index: media/cdm/aes_decryptor.cc
diff --git a/media/cdm/aes_decryptor.cc b/media/cdm/aes_decryptor.cc
index af7182f1097ab3fdbaeebe8180c97bb95d690802..f01fe8b10593645d130095d8ae55757083b7510d 100644
--- a/media/cdm/aes_decryptor.cc
+++ b/media/cdm/aes_decryptor.cc
@@ -411,6 +411,13 @@ void AesDecryptor::CloseSession(const std::string& session_id,
// Close the session.
DeleteKeysForSession(session_id);
promise->resolve();
+
+ // Update key statuses. All keys are gone, so it's an empty set.
+ session_keys_change_cb_.Run(session_id, false, CdmKeysInfo());
+
+ // Update expiration time to NaN. (http://crbug.com/624192)
+
+ // Resolve the closed attribute.
session_closed_cb_.Run(session_id);
}

Index: third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-keystatuses.html
diff --git a/third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-keystatuses.html b/third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-keystatuses.html
index 3654f8c273b373c8a186a655646ba69ce48d7a07..91922bbe08e32e4de75a9b2f1f6b5383214fbad8 100644
--- a/third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-keystatuses.html
+++ b/third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-keystatuses.html
@@ -14,6 +14,7 @@
var mediaKeySession;
var initDataType;
var initData;
+ var closed = false;

// Even though key ids are uint8, using printable values so that
// they can be verified easily.
@@ -39,7 +40,7 @@
});
}

- function processKeyStatusesChange(event)
+ function checkKeyStatusFor2Keys()
{
// Two keys added, so both should show up in |keyStatuses|.
assert_equals(mediaKeySession.keyStatuses.size, 2);
@@ -124,8 +125,28 @@
invalid6.set(key1, 0); // Last element will be 0.
assert_false(mediaKeySession.keyStatuses.has(invalid6));
assert_equals(mediaKeySession.keyStatuses.get(invalid6), undefined);
+ }

- test.done();
+ function processKeyStatusesChange(event)
+ {
+ if ( !closed )
+ {
+ // The first keystatuseschange (caused by update())
+ // should include both keys.
+ checkKeyStatusFor2Keys();
+
+ mediaKeySession.close().catch(function(error) {
+ forceTestFailureFromPromise(test, error);
+ });
+ closed = true;
+ }
+ else
+ {
+ // The second keystatuseschange (caused by close())
+ // should not have any keys.
+ assert_equals(mediaKeySession.keyStatuses.size, 0);
+ test.done();
+ }
}

getSupportedInitDataType().then(function(type) {


ddo...@chromium.org

unread,
Jun 30, 2016, 2:51:24 AM6/30/16
to jrum...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, eme-r...@chromium.org, mlamouri+w...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
LGTM with comments. Please make the order match the spec.


https://codereview.chromium.org/2108033002/diff/1/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/2108033002/diff/1/media/cdm/aes_decryptor.cc#newcode413
media/cdm/aes_decryptor.cc:413: promise->resolve();
This is supposed to be last.
https://w3c.github.io/encrypted-media/#session-closed

Also, everything here and below is supposed to be run in a task to
ensure it happens in the correct order. That's probably a larger issue,
though.

For now, move the resolve to the end.

https://codereview.chromium.org/2108033002/diff/1/media/cdm/aes_decryptor.cc#newcode415
media/cdm/aes_decryptor.cc:415: // Update key statuses. All keys are

gone, so it's an empty set.
nit: "gone" sounds informal. I suggest either copy the spec line or say
all keys have been destroyed, so ...

https://codereview.chromium.org/2108033002/

jrum...@chromium.org

unread,
Jun 30, 2016, 8:01:22 PM6/30/16
to ddorwin+...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, eme-r...@chromium.org, mlamouri+w...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
Updated, and a question on switching to PostTask.
On 2016/06/30 06:51:24, ddorwin wrote:
> This is supposed to be last.
> https://w3c.github.io/encrypted-media/#session-closed
>
> Also, everything here and below is supposed to be run in a task to
ensure it
> happens in the correct order. That's probably a larger issue, though.
>
> For now, move the resolve to the end.

Agreed. However, since we're not currently using a task, calling the
cb's after resolving the promise is the way it's been done in this file
before (see UpdateSession() directly above).

This could be converted to call PostTask (plus the other calls above).
WDYT?


https://codereview.chromium.org/2108033002/diff/1/media/cdm/aes_decryptor.cc#newcode415
media/cdm/aes_decryptor.cc:415: // Update key statuses. All keys are
gone, so it's an empty set.
On 2016/06/30 06:51:24, ddorwin wrote:
> nit: "gone" sounds informal. I suggest either copy the spec line or
say all keys
> have been destroyed, so ...

ddo...@chromium.org

unread,
Jul 1, 2016, 8:24:33 PM7/1/16
to jrum...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, eme-r...@chromium.org, mlamouri+w...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
LGTM.

I'm not sure that Chrome tasks guarantee the same behavior as Blink tasks. We'll
need to address the larger issues separately.

https://codereview.chromium.org/2108033002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 1, 2016, 8:25:03 PM7/1/16
to jrum...@chromium.org, ddorwin+...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, eme-r...@chromium.org, mlamouri+w...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 1, 2016, 10:39:09 PM7/1/16
to jrum...@chromium.org, ddorwin+...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, eme-r...@chromium.org, mlamouri+w...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
Committed patchset #2 (id:20001)

https://codereview.chromium.org/2108033002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 1, 2016, 10:39:13 PM7/1/16
to jrum...@chromium.org, ddorwin+...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, eme-r...@chromium.org, mlamouri+w...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 1, 2016, 10:41:19 PM7/1/16
to jrum...@chromium.org, ddorwin+...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, eme-r...@chromium.org, mlamouri+w...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
Patchset 2 (id:??) landed as
https://crrev.com/7166975fdb70c379be724825394c20c01e606606
Cr-Commit-Position: refs/heads/master@{#403595}

https://codereview.chromium.org/2108033002/
Reply all
Reply to author
Forward
0 new messages