[EME] Can't fire "keyschange" at a MediaKeySession if we've not associated with an HTMLMediaElement

RESOLVED FIXED in Firefox 37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks: 1 bug)

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed, firefox39 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

4 years ago
We are unable to fire a "keyschange" at a MediaKeySession if we've not associated with an HTMLMediaElement, because we're using nsContentUtils::DispatchTrustedEvent to dispatch the "keyschange" event, and it requires an nsIDocument pointer to work, and we get that from associated with an HTMLMediaElement.

There are use cases for EME that where they want to receive a "keyschange" without having to associate with an HTMLMediaElement yet.

We could just use AsyncEventDispatcher instead, and then we'd not have this limitation.
(Assignee)

Comment 1

4 years ago
We also need to ensure that when we call MediaKeySession::Remove() that a "keyschange" is dispatched to signal that sessions are removed.

Call flow would be:
var m = MediaKeys.create(<hand rolled initData>);
s = m.createSession("persistent");
s.loadSession(<some session id that wasn't closed last time>);
// CDM's loadSession implementation should mark any non-closed keys from loaded session as usable, resulting in keyschange event(s).
s.remove();
// CDM's remove implementation should mark all non-closed keys as unusable, resulting in a keyschange.
(Assignee)

Comment 2

4 years ago
Created attachment 8528792 [details] [diff] [review]
Patch 1: Use AsyncEventDispatcher to dispatch keyschange

peterv: can you review some EME patches? bz suggested that it would be good to spread the review load and expertise around.

This patch changes MediaKeySession to dispatch the "keyschange" event using the same mechanism that we dispatch the "keymessage" event; AsyncEventDispatcher. This means we don't need to have a reference to the owning document, so we can dispatch the event if the MediaKeys object that owns the MediaKeySession has not yet been associated with a HTMLMediaElement (which is required in some use cases).
Attachment #8528792 - Flags: review?(peterv)
(Assignee)

Comment 3

4 years ago
Created attachment 8528794 [details] [diff] [review]
Patch 2: Rely on the CDM to clear the usable keys when removing sessions

We should be letting the CDM mark what keyIds are not usable when we instruct it to remove a session, rather than clearing the usable keyIds.

If we don't do this, when we re-load a MediaKeySession that has been persisted, an then we call remove() on it, we won't receive a "keyschange" event when the keys in the session are marked as unusable by the CDM.

It's important that the CDM be able to determine that keys in a re-loaded persisted session that we call remove() on are no longer usable.
Attachment #8528794 - Flags: review?(peterv)
(Assignee)

Comment 4

4 years ago
Created attachment 8528795 [details] [diff] [review]
Patch 3: the MediaKeySession.closed promise to the CDM to resolve when it's told to close the MediaKeySession's session

I realised while testing that when a MediaKeySession is closed, we don't actually resolve the MediaKeySession.closed() promise because we don't pass that promise's ID over to the CDM for it to resolve when it's finished closing the session. d'oh.
Attachment #8528795 - Flags: review?(peterv)
(Assignee)

Comment 5

4 years ago
Created attachment 8528796 [details] [diff] [review]
WIP Patch 4: Implement persistent sessions for ClearKey and tests

This patch is not polished yet, but the tests pass. I will clean it up tomorrow and get Edwin to review it.

I'm putting it up here to prove that I'm not a cowboy who hasn't tested the previous patches...
Comment on attachment 8528795 [details] [diff] [review]
Patch 3: the MediaKeySession.closed promise to the CDM to resolve when it's told to close the MediaKeySession's session

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

::: dom/media/eme/MediaKeySession.cpp
@@ +183,5 @@
>  
>  already_AddRefed<Promise>
>  MediaKeySession::Close(ErrorResult& aRv)
>  {
> +  nsRefPtr<Promise> promise(mClosed);

I intended to use this approach to fix bug 1082203. But since the spec. says it has to return a new promise, I take another route. Is it ok to reuse mClosed here?
(Assignee)

Comment 7

4 years ago
Once closed, the MediaKeySession object cannot be re-initialized. If the same EME key session is desired to be re-opened a new MediaKeySession object must be constructed and the session loaded using the sessionId.

Promises also have the behaviour that if they are fulfilled before their .then member is set, when the .then member is set on the promise, the .then listener will immediately be called. In order to achieve this, we need to be storing the promise that JS receives here.

(In reply to JW Wang [:jwwang] from comment #6)
> Comment on attachment 8528795 [details] [diff] [review]
> But since the spec. says
> it has to return a new promise, I take another route. Is it ok to reuse
> mClosed here?

I didn't see the spec say that the closed attribute should return a new object every time... Normally that behaviour is applies to methods. Can you point out where it says that?
https://w3c.github.io/encrypted-media/#widl-MediaKeySession-close-Promise-void
3. Let promise be a new promise.
5. Return promise.
(Assignee)

Comment 9

4 years ago
And right beneath that is: "Note

The returned promise is resolved when the request has been processed, and the closed attribute promise is resolved when the session is closed."

The promise returned by close() is not the same as the promise which is the closed attribute.
(Assignee)

Comment 10

4 years ago
Right, I see your point, we need to chain these two promises.
(Assignee)

Updated

4 years ago
Attachment #8528795 - Flags: review?(peterv)
(Assignee)

Comment 11

4 years ago
Created attachment 8529374 [details] [diff] [review]
Patch 3: When we resolve the promise returned by MediaKeySession.close(), also close the MediaKeySession.

Keep the existing behaviour of returning a new promise from MediaKeySession.close(), but attach a native handler to it, so that when the CDM resolves the promise, we'll get a notification and we can close the session too.

Thanks for pointing out the problem JW!
Attachment #8528795 - Attachment is obsolete: true
Attachment #8529374 - Flags: review?(peterv)
(Assignee)

Comment 12

4 years ago
Created attachment 8529445 [details] [diff] [review]
Patch 4: Implement persistent sessions for ClearKey and tests

Patch 4 cleaned up, adds persistent sessions to ClearKey, with test.

I imported gmp-task-utils.h from OpenH264 so that there's something like NS_NewRunnableMethod() that we can use.
Attachment #8528796 - Attachment is obsolete: true
Attachment #8529445 - Flags: review?(edwin)
(Assignee)

Comment 13

4 years ago
Comment on attachment 8529445 [details] [diff] [review]
Patch 4: Implement persistent sessions for ClearKey and tests

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

::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp
@@ +435,5 @@
>    CK_LOGD("ClearKeyDecryptionManager::UpdateSession");
>    string sessionId(aSessionId, aSessionId + aSessionIdLength);
>  
> +  auto itr = mSessions.find(sessionId);
> +  if (itr == mSessions.end() || !itr->second) {

Note about this change: mSessions[sessionId] actually *inserts* a zero value for key sessionId in the map, which probably isn't what you waned here (and below).
Comment on attachment 8529374 [details] [diff] [review]
Patch 3: When we resolve the promise returned by MediaKeySession.close(), also close the MediaKeySession.

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

GMPDecryptorCallback::SessionClosed is not used at all. I think ClearKeyDecryptionManager::CloseSession could call mCallback->SessionClosed() which will go all the way to CDMProxy::OnSessionClosed() which should do something like CDMProxy::OnSessionMessage but is somehow not implemented. I am having a hard time in deciding whether MediaKeys::OnSessionClosed should call into MediaKeySession::OnClosed or the other way around. For now MediaKeys::OnSessionLoaded and MediaKeys::OnSessionClosed have similar names, but one is called from CDMProxy and the other from MediaKeySession. I hope one day we can clean up the object relationship and reduce bi-directional calls as much as possible. It will also make shutdown management easier by providing a single entrance for the shutdown sequence.
Comment on attachment 8529445 [details] [diff] [review]
Patch 4: Implement persistent sessions for ClearKey and tests

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

::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp
@@ +550,5 @@
> +                             aPromiseId,
> +                             kGMPInvalidAccessError,
> +                             message,
> +                             strlen(message));
> +  StoreData(sessionId, keydata, resolve, reject);

Ain't we going to remove data instead of storing data?

::: media/gmp-clearkey/0.1/ClearKeySession.h
@@ +13,5 @@
>  class GMPDecryptorHost;
>  class GMPEncryptedBufferMetadata;
>  
>  /**
>   * Currently useless; will be fleshed out later with support for persistent

Update the comment since persistent key is implemented.

@@ +40,5 @@
>    std::string mSessionId;
>    std::vector<KeyId> mKeyIds;
>  
>    GMPDecryptorCallback* mCallback;
> +  GMPSessionType mSessionType;

Add const modifier.

::: media/gmp-clearkey/0.1/ClearKeyStorage.cpp
@@ +15,5 @@
> +
> +GMPErr
> +GMPRunOnMainThread(GMPTask* aTask)
> +{
> +  return GetPlatform()->runonmainthread(aTask);

Do you mean "RunOnMainThread"?

@@ +74,5 @@
> +              uint32_t aNameLength,
> +              GMPRecord** aOutRecord,
> +              GMPRecordClient* aClient)
> +{
> +  return GetPlatform()->createrecord(aName, aNameLength, aOutRecord, aClient);

"CreateRecord"

@@ +119,5 @@
> +
> +  virtual void ReadComplete(GMPErr aStatus,
> +                            const uint8_t* aData,
> +                            uint32_t aDataSize) MOZ_OVERRIDE {
> +    std::vector<uint8_t> data(aData, aData + aDataSize);

Can we pass aData to ReadComplete without copying the data to a vector?
(Assignee)

Comment 17

4 years ago
(In reply to JW Wang [:jwwang] from comment #15)
> Comment on attachment 8529374 [details] [diff] [review]
> Patch 3: When we resolve the promise returned by MediaKeySession.close(),
> also close the MediaKeySession.
> 
> Review of attachment 8529374 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> GMPDecryptorCallback::SessionClosed is not used at all. I think
> ClearKeyDecryptionManager::CloseSession could call
> mCallback->SessionClosed() which will go all the way to
> CDMProxy::OnSessionClosed() which should do something like
> CDMProxy::OnSessionMessage but is somehow not implemented.

Oh snap! Yes!
(Assignee)

Updated

4 years ago
Attachment #8529374 - Flags: review?(peterv)
(Assignee)

Comment 18

4 years ago
Comment on attachment 8528794 [details] [diff] [review]
Patch 2: Rely on the CDM to clear the usable keys when removing sessions

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

Test failures are caused by this patch... Cancelling review...
Attachment #8528794 - Flags: review?(peterv)
(Assignee)

Updated

4 years ago
Attachment #8529445 - Flags: review?(edwin)
Chris, I was holding off on reviewing since unmarked some of the patches. Should I already start with attachment 8528792 [details] [diff] [review] anyway?
Flags: needinfo?(cpearce)
(Assignee)

Comment 20

4 years ago
Yes please! :) I wasn't abt to get any work done last week for some reason...
Flags: needinfo?(cpearce)
(Assignee)

Comment 21

4 years ago
Adobe are blocked by this bug. I've been trying to add persistent session (which exercises this code here) but I still need to make that reliable before I can land, so I will upload and land the patches related to the DOM behaviour here, so that I unblock Adobe.

I will follow up in another bug with implementing the persistent sessions in our ClearKey plugins, which will have tests that exercise the code here.
(Assignee)

Comment 22

4 years ago
Created attachment 8534164 [details] [diff] [review]
Patch 2: Rely on CDM to mark keyIds as unusable when closing/removing sessions

Don't clear the keyIds when we remove or close a session in the Gecko process. We'll rely on the CDM to mark its keyIds as non usable, so that we correctly dispatch the "keyschange" event when it removes a closed session.
Attachment #8528794 - Attachment is obsolete: true
Attachment #8529374 - Attachment is obsolete: true
Attachment #8529445 - Attachment is obsolete: true
Attachment #8534164 - Flags: review?(jwwang)
(Assignee)

Comment 23

4 years ago
Created attachment 8534166 [details] [diff] [review]
Patch 3: Hook up CDM session close notification to MediaKeySession.

Connect the GMPDecryptorCallback::SessionClosed IPC message end point in the parent process to the MediaKeySession, so that we resolve the MediaKeySession.closed promise promise when the GMP calls GMPDecryptorCallback::SessionClosed in the child process.
Attachment #8534166 - Flags: review?(jwwang)
(Assignee)

Updated

4 years ago
Attachment #8534164 - Attachment description: 1101328-rely-on-cdm-to-mark-keys-unusable.patch → Patch 2: Rely on CDM to mark keyIds as unusable when closing/removing sessions
Comment on attachment 8534164 [details] [diff] [review]
Patch 2: Rely on CDM to mark keyIds as unusable when closing/removing sessions

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

Looks good to me. But where is the job done that was previously done in CDMCaps::AutoLock::DropKeysForSession? In next patch I guess...
Attachment #8534164 - Flags: review?(jwwang) → review+
(Assignee)

Updated

4 years ago
Depends on: 1109457
(Assignee)

Comment 25

4 years ago
Filed Bug 1109457 to track implementing persistent sessions for gmp-clearkey.
Comment on attachment 8534166 [details] [diff] [review]
Patch 3: Hook up CDM session close notification to MediaKeySession.

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

We still need to call mCallback->SessionClosed in ClearKeyDecryptionManager::CloseSession in the next patch.
Attachment #8534166 - Flags: review?(jwwang) → review+
(Assignee)

Comment 27

4 years ago
Filed Bug 1109457 to track implementing persistent sessions for gmp-clearkey.
(Assignee)

Comment 28

4 years ago
(In reply to JW Wang [:jwwang] from comment #24)
> Comment on attachment 8534164 [details] [diff] [review]
> Patch 2: Rely on CDM to mark keyIds as unusable when closing/removing
> sessions
> 
> Review of attachment 8534164 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. But where is the job done that was previously done in
> CDMCaps::AutoLock::DropKeysForSession? In next patch I guess...

Basically, the CDM needs to call GMPDecryptorCallback::KeyIdNotUsable to mark the keys of a session unusable when it's closing/removing sessions. We'll then end up firing a "keyschange" event and the CDM listens for that to confirm that the session was closed. So yeah, in the next patch where I add persistent session to gmp-clearkey, this happens.
Btw, do we have a test case to test MediaKeySession.close which is not related to persistent session?
(Assignee)

Comment 31

4 years ago
Not yet. We could change the "ended" handler in test_eme_playback to cover it pretty easily.
Attachment #8528792 - Flags: review?(peterv) → review+
https://hg.mozilla.org/mozilla-central/rev/af3dbca127d1
https://hg.mozilla.org/mozilla-central/rev/1a2cec4f5833
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
(Assignee)

Comment 34

4 years ago
Mass update firefox-status to track EME uplift.
status-firefox37: --- → fixed
status-firefox38: --- → fixed
status-firefox39: --- → fixed
You need to log in before you can comment on or make changes to this bug.