Closed Bug 1101328 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla37
Tracking Flags:
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 5 obsolete files)

Patch 1: Use AsyncEventDispatcher to dispatch keyschange
2.46 KB, patch
peterv
: review+
Details | Diff | Splinter Review
Patch 2: Rely on CDM to mark keyIds as unusable when closing/removing sessions
4.61 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
Patch 3: Hook up CDM session close notification to MediaKeySession.
991 bytes, patch
jwwang
: review+
Details | Diff | Splinter Review 
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.
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.
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)
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)
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)
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?
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?
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.
Right, I see your point, we need to chain these two promises.
Attachment #8528795 - Flags: review?(peterv)
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)
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)
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?
(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!
Attachment #8529374 - Flags: review?(peterv)
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)
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)
Yes please! :) I wasn't abt to get any work done last week for some reason...
Flags: needinfo?(cpearce)
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.
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)
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)
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+
Depends on: 1109457
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+
Filed Bug 1109457 to track implementing persistent sessions for gmp-clearkey.
(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?
Not yet. We could change the "ended" handler in test_eme_playback to cover it pretty easily.
Attachment #8528792 - Flags: review?(peterv) → review+
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.

Attachment

General

Creator:
Created:
Updated:
Size: