1043147 - Update CDMProxy to use GMPDecryptor

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

I need to update CDMProxy to use the GMPDecryptor (being added in Bug 1042373) to use an out of process Gecko Media Plugin to manage decryption key and sessions, and to decrypt encrypted buffers.

This is based on the patch in Bug 1042373, which is under review.

Basically I need to proxy the calls in CDMProxy across to the GMP thread, and then call the corresponding GMPDecryptor API object to cause the GMP to be called, and wait for the async response (which comes in my GMPDecryptorCallbackProxy implementation), and proxy the response back to the appropriate thread and receiver (JS, or the client in the media stack that wanted to decrypt a buffer).

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1

This is the changes in content/media/eme required to get the media stack to use a CDM for decoding.

There's the a few things that aren't fully implemented here. I want to try to get my code landed, so that it's easier for other people to help out.

MediaKeyError is still there. That's still under discussion at the W3C, but Adobe asked for it to help with their testing, so I'd like to leave it in until the spec there stabilizes.

The code to use the decrypt-an-MP4Sample code added here is in the next patch in my queue.

The GMPDecryptor and GMPDecryptorProxy et al are defined in the patch in bug 1042373.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8461316 [details] [diff] [review]
Patch v1

Chris, I'm pretty far behind on reviews right now.  Ehsan said he'd take a look here, but please let me know if you really want me to look at some part of this in particular.

Comment 3

[(no longer active)]

I will try to get to this on Monday.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

OK, thanks guys.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2: Handle GMP termination

Jesup's recent landings in bug 1041232 added the ability to detect when the GMP was terminated. We need to handle that here too.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8461316 [details] [diff] [review]
Patch v1

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

Basic flow is that MediaKeys JS object is created, and that calls through to create the CDMProxy. That proxies events to the "GMP" (Gecko Media Plugin) thread. All interaction with GMPs must be on the GMP thread. Then calls from the MediaKeys/MediaKeySession JS objects are proxied across to the GMP thread to call into the GMP. We're interacting with the GMPDecryptor object that's in the GMP (across process). The GMPDecryptor implemetation in the child process will call back, and it's calls end up calling the CDMCallbackProxy, which then proxies the calls back to the appropriate thread on the CDMProxy. See also bug 1044738 which uses the CMDProxy from the media stack to decrypt samples before decoding. And the interface for GMPDecryptor (implemented in the child plugin) is in content/media/gmp/gmp-api/gmp-decryption.h.

Comment 7

[(no longer active)]

Comment on attachment 8461316 [details] [diff] [review]
Patch v1

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

Looks great!  Note that I had to make a number of assumptions below, please verify them!

::: content/media/eme/CDMCallbackProxy.cpp
@@ +45,5 @@
> +
> +void
> +CDMCallbackProxy::ResolveNewSessionPromise(uint32_t aPromiseId,
> +                                           const nsCString& aSessionId)
> +{

Please add assertions to ensure that all of these functions run on the thread that you expect them to.  If we don't have a way to tell the GMP thread from others (which I think should not be the case, since you need to be able to use CDMProxy::mGMPThread here), MOZ_ASSERT(!NS_IsMainThread()) would be fine too.

::: content/media/eme/CDMCallbackProxy.h
@@ +17,5 @@
> +// object on the main thread.
> +class CDMCallbackProxy : public GMPDecryptorProxyCallback {
> +public:
> +
> +  CDMCallbackProxy(CDMProxy* aProxy);

Nit: explicit.

@@ +58,5 @@
> +  ~CDMCallbackProxy() {}
> +
> +private:
> +  // Warning: Weak ref.
> +  CDMProxy* mProxy;

Hmm, what makes this safe going forwards?  If you want to enforce that this class will always be contained within a CDMProxy, perhaps make the ctor private, convert this and the ctor arg to references and make CDMProxy a friend of this class.  Or something.  :-)

::: content/media/eme/CDMCaps.cpp
@@ +132,5 @@
> +                                     nsIThread* aTarget)
> +{
> +  mData.mMonitor.AssertCurrentThreadOwns();
> +  MOZ_ASSERT(!IsKeyUsable(aKey));
> +  MOZ_ASSERT(aContinuation);

Please MOZ_ASSERT aTarget as well.

@@ +155,5 @@
> +bool
> +CDMCaps::AutoLock::AreCapsKnown()
> +{
> +  mData.mMonitor.AssertCurrentThreadOwns();
> +  return mData.mCaps != 0;

I assume caps can never be 0...

::: content/media/eme/CDMCaps.h
@@ +18,5 @@
> +typedef nsTArray<uint8_t> CencKeyId;
> +
> +// CDM capabilities; what keys a CDMProxy can use, and whether it can decrypt, or
> +// decrypt-and-decode on a per stream basis. Must be locked to access state.
> +class CDMCaps {

It seems very bad if someone copies this object accidentally.  Can you please delete the copy ctor and the assignment operator?

@@ +25,5 @@
> +  ~CDMCaps();
> +
> +  // Locks the CDMCaps. It must be locked to access its shared state.
> +  // Threadsafe when locked.
> +  class AutoLock {

This needs to be MOZ_STACK_CLASS, I think.

@@ +27,5 @@
> +  // Locks the CDMCaps. It must be locked to access its shared state.
> +  // Threadsafe when locked.
> +  class AutoLock {
> +  public:
> +    AutoLock(CDMCaps& aKeyCaps);

Nit: explicit.

::: content/media/eme/CDMProxy.cpp
@@ +95,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +  if (!mKeys.IsNull()) {
> +    mKeys->OnCDMCreated(aPromiseId);
> +  } else {
> +    NS_WARNING("CDMProxy unable to reject promise!");

Hmm, this seems bad, why can't you just call RejectPromise here?

Also, I tried really hard to understand when mKeys can be null, and it seems impossible.  Should you just MOZ_ASSERT(!mKeys.IsNull())?

@@ +117,3 @@
>  
> +  nsRefPtr<nsIRunnable> task(
> +    NS_NewRunnableMethodWithArg<nsAutoPtr<CreateSessionData>>(this, &CDMProxy::gmp_CreateSession, data));

Nit-ish: data.forget()

@@ +153,5 @@
> +  nsAutoPtr<SessionOpData> data(new SessionOpData());
> +  data->mPromiseId = aPromiseId;
> +  data->mSessionId = NS_ConvertUTF16toUTF8(aSessionId);
> +  nsRefPtr<nsIRunnable> task(
> +    NS_NewRunnableMethodWithArg<nsAutoPtr<SessionOpData>>(this, &CDMProxy::gmp_LoadSession, data));

Here too, and further down..

@@ +261,5 @@
>  CDMProxy::RemoveSession(const nsAString& aSessionId,
>                          PromiseId aPromiseId)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  NS_ENSURE_TRUE_VOID(!mKeys.IsNull());

Like I said above, I think this should be an assertion.  The reason why this is important is that we basically never reject/resolve the promise if this error condition can happen at runtime, which is very bad.

@@ +364,5 @@
> +void
> +CDMProxy::OnSessionClosed(const nsAString& aSessionId)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  NS_WARNING("CDMProxy::OnSessionClosed() not implemented");

I'm assuming these are fine...

@@ +370,5 @@
> +
> +static void
> +LogToConsole(const nsAString& aMsg)
> +{
> +  if (!PR_GetEnv("MOZ_GMP_CONSOLE_LOG")) {

Is there a good reason to hide this behind an env var?

@@ +456,5 @@
> +  MOZ_ASSERT(NS_GetCurrentThread() == mGMPThread);
> +  for (size_t i = 0; i < mDecryptionJobs.Length(); i++) {
> +    DecryptJob* job = mDecryptionJobs[i];
> +    if (job->mId == aId) {
> +      memcpy(job->mSample->data,

Please use PodCopy instead.

@@ +458,5 @@
> +    DecryptJob* job = mDecryptionJobs[i];
> +    if (job->mId == aId) {
> +      memcpy(job->mSample->data,
> +             aDecryptedData.Elements(),
> +             std::min<size_t>(aDecryptedData.Length(), job->mSample->size));

I'm assuming the CDM giving us a smaller size of data back is fine...

@@ +464,5 @@
> +      job->mClient->Decrypted(rv, job->mSample.forget());
> +      mDecryptionJobs.RemoveElementAt(i);
> +      return;
> +    }
> +  }

Please log something if the CDM is giving us an unexpected job ID back.

::: content/media/eme/CDMProxy.h
@@ +17,5 @@
> +#include "mozilla/Monitor.h"
> +#include "nsIThread.h"
> +#include "GMPDecryptorProxy.h"
> +#include "mp4_demuxer/DecoderData.h"
> +#include "mozilla/CDMCaps.h"

With some forward-delcaring, you should be able to move most of these to the .cpp files here and elsewhere in the patch.

@@ +285,5 @@
> +  // Decryption jobs sent to CDM, awaiting result.
> +  // GMP thread only.
> +  nsTArray<nsAutoPtr<DecryptJob>> mDecryptionJobs;
> +
> +  // Number of buffer's we've decrypted. Used to uniquely identify

Nit: buffers

@@ +286,5 @@
> +  // GMP thread only.
> +  nsTArray<nsAutoPtr<DecryptJob>> mDecryptionJobs;
> +
> +  // Number of buffer's we've decrypted. Used to uniquely identify
> +  // decryption jobs sent to CMD.

Please extend this comment to mention you couldn't use mDecryptionJobs.Length() instead because that array may shrink.

::: content/media/eme/MediaKeys.cpp
@@ +16,2 @@
>  #include "nsContentUtils.h"
> +#include "mozilla/EMELog.h"

You just included this!

@@ +181,5 @@
> +      NS_WARNING("Received activation for non-existent session!");
> +      promise->MaybeReject(NS_ERROR_DOM_INVALID_ACCESS_ERR);
> +      return;
> +    }
> +    mPendingSessions.Remove(aId);

Shouldn't this happen before the early return above too?

@@ +217,5 @@
>  
>    keys->mProxy = new CDMProxy(keys, aKeySystem);
> +
> +  // The CDMProxy's initialization is asynchronous. The MediaKeys is
> +  // refcounted, and it's instance is returned to JS by promise once

Nit: its.

@@ +226,5 @@
> +  // here, and hold a self-reference until that promise is resolved or
> +  // rejected.
> +  MOZ_ASSERT(!keys->mCreatePromiseId, "Should only be created once!");
> +  keys->mCreatePromiseId = keys->StorePromise(promise);
> +  keys->AddRef();

I don't really like this manual AddRef/Release.  Not sure if I have a better suggestion though.

@@ +370,5 @@
> +  }
> +  NS_ENSURE_TRUE(principal, NS_ERROR_FAILURE);
> +
> +  nsAutoString origin;
> +  nsresult res = nsContentUtils::GetUTFOrigin(principal, origin);

I think you should be able to pass aOutOrigin directly to GetUTFOrigin.

Comment 8

[(no longer active)]

Comment on attachment 8463176 [details] [diff] [review]
Patch 2: Handle GMP termination

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

::: content/media/eme/CDMCallbackProxy.cpp
@@ +252,5 @@
>  
> +void
> +CDMCallbackProxy::Terminated()
> +{
> +  NS_WARNING("CDM terminated.");

Why warn here?  Is this not expected?

::: content/media/eme/CDMProxy.cpp
@@ +472,5 @@
> +CDMProxy::gmp_Terminated()
> +{
> +  MOZ_ASSERT(NS_GetCurrentThread() == mGMPThread);
> +  EME_LOG("CDM terminated");
> +  if (mCDM) {

It might be worth logging something if mCDM is null here.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks for the review!

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7)
> Comment on attachment 8461316 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8461316 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great!  Note that I had to make a number of assumptions below, please
> verify them!
> 
> ::: content/media/eme/CDMCallbackProxy.cpp
> @@ +45,5 @@
> > +
> > +void
> > +CDMCallbackProxy::ResolveNewSessionPromise(uint32_t aPromiseId,
> > +                                           const nsCString& aSessionId)
> > +{
> 
> Please add assertions to ensure that all of these functions run on the
> thread that you expect them to.

I'll add a CDMProxy::IsOnGMPThread(), and call that here.


> @@ +58,5 @@
> > +  ~CDMCallbackProxy() {}
> > +
> > +private:
> > +  // Warning: Weak ref.
> > +  CDMProxy* mProxy;
> 
> Hmm, what makes this safe going forwards?  If you want to enforce that this
> class will always be contained within a CDMProxy, perhaps make the ctor
> private, convert this and the ctor arg to references and make CDMProxy a
> friend of this class.  Or something.  :-)

Done, except I passed a pointer not a reference, since it's more convenient.

> 
> ::: content/media/eme/CDMCaps.cpp
> @@ +132,5 @@
> > +                                     nsIThread* aTarget)
> > +{
> > +  mData.mMonitor.AssertCurrentThreadOwns();
> > +  MOZ_ASSERT(!IsKeyUsable(aKey));
> > +  MOZ_ASSERT(aContinuation);
> 
> Please MOZ_ASSERT aTarget as well.

aTarget=nullptr means "run this on the GMP thread." This is because in a later patch in my queue I need to have an event that puts a task in a MediaTaskQueue; I don't have a thread I can target directly, so I pass a runnable in instead that puts a second task into the MediaTaskQueue.


> 
> @@ +155,5 @@
> > +bool
> > +CDMCaps::AutoLock::AreCapsKnown()
> > +{
> > +  mData.mMonitor.AssertCurrentThreadOwns();
> > +  return mData.mCaps != 0;
> 
> I assume caps can never be 0...

Umm, it would be a very boring EME/GMP/CDM then, as it could not decrypt any content.


> ::: content/media/eme/CDMProxy.cpp
> @@ +95,5 @@
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  if (!mKeys.IsNull()) {
> > +    mKeys->OnCDMCreated(aPromiseId);
> > +  } else {
> > +    NS_WARNING("CDMProxy unable to reject promise!");
> 
> Hmm, this seems bad, why can't you just call RejectPromise here?

Because the promise is stored in a hashmap on the MediaKeys object, indexed by aPromiseId. So if we don't have a reference to mKeys, we can't find the promise to reject.

> Also, I tried really hard to understand when mKeys can be null, and it seems
> impossible.  Should you just MOZ_ASSERT(!mKeys.IsNull())?

mKeys is reset when CDMProxy::Shutdown() is called. It's not currently, but needs to be. I will follow up on that.


> 
> @@ +117,3 @@
> >  
> > +  nsRefPtr<nsIRunnable> task(
> > +    NS_NewRunnableMethodWithArg<nsAutoPtr<CreateSessionData>>(this, &CDMProxy::gmp_CreateSession, data));
> 
> Nit-ish: data.forget()

We don't actually need to do that, as copying an nsAutoPtr to an nsAutoPtr forget()s the one being assigned from. And calling forget() here causes a compile error on Windows...


> @@ +261,5 @@
> >  CDMProxy::RemoveSession(const nsAString& aSessionId,
> >                          PromiseId aPromiseId)
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> > +  NS_ENSURE_TRUE_VOID(!mKeys.IsNull());
> 
> Like I said above, I think this should be an assertion.  The reason why this
> is important is that we basically never reject/resolve the promise if this
> error condition can happen at runtime, which is very bad.

OK, I think what we can do is when I fix the shutdown code so that when we clear CDMProxy's reference to its MediaKeys, we can reject all promises waiting for responses from the CDM. That should satisfy your concern about un-rejected promises.


> 
> @@ +364,5 @@
> > +void
> > +CDMProxy::OnSessionClosed(const nsAString& aSessionId)
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  NS_WARNING("CDMProxy::OnSessionClosed() not implemented");
> 
> I'm assuming these are fine...

Basically, I need to land my code, so I can unblock other people helping out with things like this...

> 
> @@ +370,5 @@
> > +
> > +static void
> > +LogToConsole(const nsAString& aMsg)
> > +{
> > +  if (!PR_GetEnv("MOZ_GMP_CONSOLE_LOG")) {
> 
> Is there a good reason to hide this behind an env var?

No. I'll remove it.



> @@ +458,5 @@
> > +    DecryptJob* job = mDecryptionJobs[i];
> > +    if (job->mId == aId) {
> > +      memcpy(job->mSample->data,
> > +             aDecryptedData.Elements(),
> > +             std::min<size_t>(aDecryptedData.Length(), job->mSample->size));
> 
> I'm assuming the CDM giving us a smaller size of data back is fine...

It shouldn't, but we shouldn't trust the CDM (in case it gets p0wned) either.

I'll warn aDecryptedData.Length()!=job->mSample->size


> 
> @@ +181,5 @@
> > +      NS_WARNING("Received activation for non-existent session!");
> > +      promise->MaybeReject(NS_ERROR_DOM_INVALID_ACCESS_ERR);
> > +      return;
> > +    }
> > +    mPendingSessions.Remove(aId);
> 
> Shouldn't this happen before the early return above too?

Yes! Thanks!

Comment 10

[(no longer active)]

Thanks, Chris!  Everything in comment 9 makes sense.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84f498627e87

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/84f498627e87