Closed Bug 1306572 Opened 8 years ago Closed 8 years ago

[EME][Fennec] Implement MediaDrm CDMProxy

Categories

(Firefox for Android Graveyard :: Audio/Video, defect, P2)

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Type:
defect

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

Details

Attachments

(3 files)

Bug 1306572 - Part1 - Implement GeckoMediaDrm API as a glue for native CDMProxy and java impl MediaDrmBridge.
58 bytes, text/x-review-board-request
cpearce
: review+
jchen
: review+
Details
Bug 1306572 - Part2 - The native implementation of MediaDrmProxy by MediaDrmProxySupport
58 bytes, text/x-review-board-request
cpearce
: review+
jchen
: review+
Details
Bug 1306572 - Part3 - Implement MediaCDMProxy and the callback proxy.
58 bytes, text/x-review-board-request
cpearce
: review+
Details 
We would like to implement the non-GMP CDMProxy on Fennec.

These native codes will be a glue to use MediaDrmProxy.java

Basically this bug will be based on [1]

[1]
https://github.com/JamesWCCheng/gecko-dev/commit/7714d6204a3eaa67363c04e23c0f7f74680cdbb5#diff-50d13d117480e65c454115f1b3803a5f
depend on bug 1303922, since it will change the CDM interface.
Depends on: 1303922
No longer depends on: 1306219
Depends on: 1306219
Priority: -- → P2
Hi Chris, 

This bug is based on Bug 1306219 which has not landed yet.

I briefly describe the picture of those two patches.

I just implement a CDM for Fennec named MediaDrmCDMProxy

The working flow is described as below,

When MediaKeys calls CDM APIs

MediaDrmCDMProxy.cpp -> MediaDrmProxySupport.cpp -> MediaDrmProxy.java -> MediaDrmProxyImpl.java(bug 1306185)

APIs are all asynchronous. So the callback path is

MediaDrmProxyImpl.java -> MediaDrmProxy.java -> MediaDrmProxySupport.cpp -> MediaDrmCDMCallbackProxy.cpp -> MediaDrmCDMProxy.cpp

MediaDrmProxyImpl.java should be responsible to handle all the async call which could be resolved or rejected.

This MediaDrmCDMProxy has not been instantiated yet in current patch, we planned to create it once we are capable of enabling EME on Fennec.

There are some TODO tag that we need to implement in other following bugs or currently we have no idea how to implement.

Please review these patches

Thank you.


Hi Jim,

Would you please review the java and native implementation parts(Part1 and Part2)

Thank you.
Attachment #8802845 - Flags: review?(nchen)
Attachment #8802845 - Flags: review?(cpearce)
Attachment #8802846 - Flags: review?(nchen)
Attachment #8802846 - Flags: review?(cpearce)
Attachment #8802856 - Flags: review?(cpearce)
For these two SessionError and RejectPromise API, we found that in MediaDrmBrige implementation, we are lack of information that can tell the CDM the system error code and the only situation we will encounter in the MediaDrmBrige implementation is invalid state error.

So we did not provide java API to convey these information to natvie CDM but hard code the error code instead.

Maybe Kilik has further explanation on this, ni him for more details.
Flags: needinfo?(kikuo)
Comment on attachment 8802845 [details]
Bug 1306572 - Part1 - Implement GeckoMediaDrm API as a glue for native CDMProxy and java impl MediaDrmBridge.

https://reviewboard.mozilla.org/r/87152/#review86650

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:22
(Diff revision 1)
>  import android.media.MediaCrypto;
>  import android.media.MediaDrm;
>  import android.util.Log;
>  import android.os.Build;
>  
> -public final class MediaDrmProxy {
> +public final class MediaDrmProxy extends JNIObject {

Why does `MediaDrmProxy` extend `JNIObject`? You are not using native methods in `MediaDrmProxy` itself.

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:127
(Diff revision 1)
> +
> +        void onRejectPromise(int promiseId,
> +                             String message);
> +    } // Callbacks
> +
> +    @WrapForJNI

Add `(dispatchTo = "...")` if you want these callbacks to run on a different thread than the thread that's making these calls.

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:237
(Diff revision 1)
> +
> +    public boolean isDestroyed() {
> +        return mDestroyed;
> +    }
> +
> +    @WrapForJNI

Please add `(calledFrom = "gecko")` if these methods are only called from the Gecko thread.
Attachment #8802845 - Flags: review?(nchen)
Comment on attachment 8802846 [details]
Bug 1306572 - Part2 - The native implementation of MediaDrmProxy by MediaDrmProxySupport

https://reviewboard.mozilla.org/r/87154/#review86648

::: dom/media/eme/mediadrm/MediaDrmProxySupport.h:10
(Diff revision 2)
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef MediaDrmProxySupport_H
> +#define MediaDrmProxySupport_H
> +
> +#include <jni.h>

Don't need this.

::: dom/media/eme/mediadrm/MediaDrmProxySupport.h:12
(Diff revision 2)
> +#ifndef MediaDrmProxySupport_H
> +#define MediaDrmProxySupport_H
> +
> +#include <jni.h>
> +#include "mozilla/DecryptorProxyCallback.h"
> +#include "mozilla/jni/Types.h"

Don't need this.

::: dom/media/eme/mediadrm/MediaDrmProxySupport.h:14
(Diff revision 2)
> +
> +#include <jni.h>
> +#include "mozilla/DecryptorProxyCallback.h"
> +#include "mozilla/jni/Types.h"
> +#include "mozilla/Logging.h"
> +#include "FennecJNINatives.h"

Include this in the .cpp file.

::: dom/media/eme/mediadrm/MediaDrmProxySupport.cpp:164
(Diff revision 2)
> +    nsTArray<uint8_t> keyId;
> +    keyId.AppendElements(reinterpret_cast<uint8_t*>(keyIdInt8Array.Elements()),
> +                         keyIdInt8Array.Length());

You can reinterpret_cast `nsTArray<int8_t>*` to `nsTArray<uint8_t>*` to avoid an additional copy

::: dom/media/eme/mediadrm/MediaDrmProxySupport.cpp:215
(Diff revision 2)
> +  MediaDrmJavaCallbacksSupport::AttachNative(mJavaCallbacks,
> +                                             mozilla::MakeUnique<MediaDrmJavaCallbacksSupport>(mCallback));

You must call `DisposeNative` somewhere else to free up the memory. Otherwise you will have a memory leak.
Attachment #8802846 - Flags: review?(nchen)
Hi Jim,

Thanks for pointing out the defect that can be refined.

One question below,

> ::: dom/media/eme/mediadrm/MediaDrmProxySupport.cpp:164
> (Diff revision 2)
> > +    nsTArray<uint8_t> keyId;
> > +    keyId.AppendElements(reinterpret_cast<uint8_t*>(keyIdInt8Array.Elements()),
> > +                         keyIdInt8Array.Length());
> 
> You can reinterpret_cast `nsTArray<int8_t>*` to `nsTArray<uint8_t>*` to
> avoid an additional copy
> 
My Code:
auto keyIdInt8Array = keyIdByteArray->GetElements(); // nsTArray<int8_t>
nsTArray<uint8_t> keyId;
keyId.AppendElements(reinterpret_cast<uint8_t*>(keyIdInt8Array.Elements()),
                     keyIdInt8Array.Length());   // a copy occurred

then pass keyId into other API. // a copy or move occurred

Your suggestion:
nsTArray<uint8_t>* uarray = reinterpret_cast<nsTArray<uint8_t>*>(keyIdByteArray);

then pass *uarray into other API.(a copy or move occurred)

Is it safe to cast the nsTArray here since I don't know the implementation at all(no template specialization)?
Any existing code on gecko used this casting?

Thank you
Flags: needinfo?(nchen)
(In reply to James Cheng[:JamesCheng] from comment #7)
> For these two SessionError and RejectPromise API, we found that in
> MediaDrmBrige implementation, we are lack of information that can tell the
> CDM the system error code and the only situation we will encounter in the
> MediaDrmBrige implementation is invalid state error.
> 
> So we did not provide java API to convey these information to natvie CDM but
> hard code the error code instead.
> 
> Maybe Kilik has further explanation on this, ni him for more details.

According to "GenerateRequest"[1], "update"[2], "close"[3],... algorithms in EME spec, user agent should be responsible of verifying & sanitizing input arguments and should reject the promise if there's something wrong.  

CDM only rejects promise in a few situations, i.e.
for "GenerateRequest",
 - if the sanitized init data is not supported by the cdm, reject promise with a NotSupportedError.

for "update",
 - if the format of sanitized response is invalid in any way, reject promise with a newly created TypeError.

For now, I'm inclined not to define DOM exceptions in the managed code.  I'd expect that if anything wrong happens in managed code (MediaDrmProxy/LocalMediaDrmBridge/GeckoMediaDrmBridge implementation), these components should be released as we can do nothing about it.  For me, that's an invalid state.

If we have to reject corresponding error in managed part, I'd suggest adding new API, e.g. "onRejectPromiseTypeError", "onRejectPromiseNotSupportedError" instead of rejecting with an error code, because we may reject the promise in many cases without error code...

[1] https://w3c.github.io/encrypted-media/#dom-mediakeysession-generaterequest
[2] https://w3c.github.io/encrypted-media/#dom-mediakeysession-update
[3] https://w3c.github.io/encrypted-media/#dom-mediakeysession-close
Flags: needinfo?(kikuo)
(In reply to James Cheng[:JamesCheng] from comment #10)
> 
> Is it safe to cast the nsTArray here since I don't know the implementation
> at all(no template specialization)?
> Any existing code on gecko used this casting?

Yes, we do it at [1], for example. I think it's safe to assume nsTArray<int8_t> and nsTArray<uint8_t> have the same structure. If you're really worried, you can add a constructor to CDMKeyInfo that have `uint8_t* aKeyId` and `size_t aLength` parameters, and pass in the raw data.

[1] https://dxr.mozilla.org/mozilla-central/rev/215f9686117673a2c914ed207bc7da9bb8d741ad/dom/media/AudioSegment.h#227
Flags: needinfo?(nchen)
Cool, Thanks for your advise, I will refine the patch.
Comment on attachment 8802845 [details]
Bug 1306572 - Part1 - Implement GeckoMediaDrm API as a glue for native CDMProxy and java impl MediaDrmBridge.

https://reviewboard.mozilla.org/r/87152/#review87242
Attachment #8802845 - Flags: review?(cpearce) → review+
Comment on attachment 8802846 [details]
Bug 1306572 - Part2 - The native implementation of MediaDrmProxy by MediaDrmProxySupport

https://reviewboard.mozilla.org/r/87154/#review87244

r+ with comments addressed.

::: dom/media/eme/mediadrm/MediaDrmProxySupport.h:57
(Diff revision 2)
> +                     const nsCString& aSessionId,
> +                     const nsTArray<uint8_t>& aResponse);
> +
> +  void CloseSession(uint32_t aPromiseId,
> +                    const nsCString& aSessionId);
> +

Do you need implementations of LoadSession() and/or RemoveSession()? If you're supporting persistent-license sessions, you'll need them I expect.

If you agree, you could do that in a follow up patch.

::: dom/media/eme/mediadrm/MediaDrmProxySupport.cpp:69
(Diff revision 2)
> +                                               int aPromiseId,
> +                                               jni::ByteArray::Param aSessionId,
> +                                               jni::ByteArray::Param aRequest)
> +{
> +  auto reqDataArray = aRequest->GetElements();
> +  nsCString sessionId(reinterpret_cast<char*>(aSessionId->GetElements().Elements()),

You can use nsDependentCSubstring() here, and avoid copying the data. You have to use Substring as nsDependentCString assumes the data is null terminated.

Same comment applies elsewhere.

::: dom/media/eme/mediadrm/MediaDrmProxySupport.cpp:161
(Diff revision 2)
> +
> +  nsTArray<CDMKeyInfo> keyInfosArray;
> +
> +  for (auto&& keyInfoObject : keyInfosObjectArray) {
> +    java::SessionKeyInfo::LocalRef keyInfo(mozilla::Move(keyInfoObject));
> +    auto keyIdByteArray = keyInfo->KeyId(); // mozilla::jni::ByteArray::LocalRef

Instead of going
auto name = blah; // type

why don't you go:

type name = blah;

??

The fact that you felt compelled to document the type show that using 'auto' here is not making the code easier to understand.

This is why I'm not a huge fan of 'auto' other than for templated iterators; you don't have the type documented in the code, it's harder for the reader to read.

::: dom/media/eme/mediadrm/MediaDrmProxySupport.cpp:228
(Diff revision 2)
> +                                    const nsCString& aInitDataType,
> +                                    const nsTArray<uint8_t>& aInitData,
> +                                    MediaDrmSessionType aSessionType)
> +{
> +  MOZ_ASSERT(mBridgeProxy);
> +  NS_ConvertUTF8toUTF16 initDataType(aInitDataType);

May as well do the UTF8 to UTF16 conversion after the length check so that if you exit on the length check you don't waste time on unnecessary work, and so that the declaration of initDataType is closer to where it's used.

In fact, since initDataType is only used on line 243, you could just pass in  NS_ConvertUTF8toUTF16(aInitDataType) as an argument to the CreateSession() call rather than storing it in a temporary.
Attachment #8802846 - Flags: review?(cpearce) → review+
Comment on attachment 8802856 [details]
Bug 1306572 - Part3 - Implement MediaCDMProxy and the callback proxy.

https://reviewboard.mozilla.org/r/87156/#review87254

I don't understand why you create a thread to run the proxy on. Also I'm unsure what thread the proxy callback is called on. I bet it's not the thread the proxy creates. I suspect you don't need the proxy thread?

Also, I think the code would be clearer if you used NS_NewRunnableFunction() with lambdas instead of lots of bespoke Runnable classes everywhere.

::: dom/media/eme/mediadrm/MediaDrmCDMCallbackProxy.cpp:1
(Diff revision 1)
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

If you copy a file, use \`hg cp $src $dst\` so that the diff shows the file as copied; the diff is smaller, and easier to check for mistakes then.

::: dom/media/eme/mediadrm/MediaDrmCDMCallbackProxy.cpp:51
(Diff revision 1)
> +
> +void
> +MediaDrmCDMCallbackProxy::SetSessionId(uint32_t aToken,
> +                                       const nsCString& aSessionId)
> +{
> +  nsCOMPtr<nsIRunnable> task(new SetSessionIdTask(mProxy,

What thread are these callbacks called on? Is it the main thread? If so, why bother dispatching to the main thread here?

::: dom/media/eme/mediadrm/MediaDrmCDMCallbackProxy.cpp:82
(Diff revision 1)
> +
> +void
> +MediaDrmCDMCallbackProxy::ResolveLoadSessionPromise(uint32_t aPromiseId,
> +                                                    bool aSuccess)
> +{
> +  nsCOMPtr<nsIRunnable> task(new LoadSessionTask(mProxy,

You could write this as:

RefPtr<CDMProxy> proxy = mProxy;
nsCOMPtr<nsIRunnable> task = NS_NewRunnableFunction([proxy, aPromiseId, aSuccess] ()
{
  proxy->OnResolveLoadSessionPromise(aPromiseId, aSuccess);
});


And then you'd save on class definitions. Here and for other tasks.

The reason the original CDMProxy doesn't do this is because I wrote the code before we were allowed to use lambdas, and I never got around to cleaning it up.

::: dom/media/eme/mediadrm/MediaDrmCDMProxy.cpp:61
(Diff revision 1)
> +          NS_ConvertUTF16toUTF8(aOrigin).get(),
> +          NS_ConvertUTF16toUTF8(aTopLevelOrigin).get(),
> +          (aInPrivateBrowsing ? "PrivateBrowsing" : "NonPrivateBrowsing"));
> +
> +  // Create a thread to work with cdm.
> +  if (!mOwnerThread) {

Why do you need to create a thread? Is the message passing to the CDM doing remote procedure call, i.e. is it blocking and slow?

::: dom/media/eme/mediadrm/MediaDrmCDMProxy.cpp:402
(Diff revision 1)
> +  MOZ_ASSERT(mCDM);
> +
> +  mCallback = new MediaDrmCDMCallbackProxy(this);
> +  mCDM->Init(mCallback);
> +  nsCOMPtr<nsIRunnable> task(
> +    NewRunnableMethod<uint32_t>(this,

Can use a RunnableFunction and lambda here and elsewhere too.

::: dom/media/eme/mediadrm/MediaDrmCDMProxy.cpp:463
(Diff revision 1)
> +  MOZ_ASSERT(mCDM);
> +  if (mShutdownCalled) {
> +    return;
> +  }
> +  mShutdownCalled = true;
> +  mCDM->Shutdown();

You should probably null out mCDM here.
Attachment #8802856 - Flags: review?(cpearce) → review-
Comment on attachment 8802846 [details]
Bug 1306572 - Part2 - The native implementation of MediaDrmProxy by MediaDrmProxySupport

https://reviewboard.mozilla.org/r/87154/#review87244

> Do you need implementations of LoadSession() and/or RemoveSession()? If you're supporting persistent-license sessions, you'll need them I expect.
> 
> If you agree, you could do that in a follow up patch.

We plan to implement that once the browser really needs these functionality and do it in a follow up bug.
We found chromium did not support it either [1][2]

[1]
https://cs.chromium.org/chromium/src/media/base/android/media_drm_bridge.cc?q=media_drm_bridge.cc&sq=package:chromium&dr&l=425
[2]
https://cs.chromium.org/chromium/src/media/base/android/media_drm_bridge.cc?q=media_drm_bridge.cc&sq=package:chromium&dr&l=467

> You can use nsDependentCSubstring() here, and avoid copying the data. You have to use Substring as nsDependentCString assumes the data is null terminated.
> 
> Same comment applies elsewhere.

I tried to use it but I found that the nsDependentCSubstring is a subclass of nsACString. That means I can not pass it to callbackproxy API which accepted nsCString.

> Instead of going
> auto name = blah; // type
> 
> why don't you go:
> 
> type name = blah;
> 
> ??
> 
> The fact that you felt compelled to document the type show that using 'auto' here is not making the code easier to understand.
> 
> This is why I'm not a huge fan of 'auto' other than for templated iterators; you don't have the type documented in the code, it's harder for the reader to read.

I agree.

> May as well do the UTF8 to UTF16 conversion after the length check so that if you exit on the length check you don't waste time on unnecessary work, and so that the declaration of initDataType is closer to where it's used.
> 
> In fact, since initDataType is only used on line 243, you could just pass in  NS_ConvertUTF8toUTF16(aInitDataType) as an argument to the CreateSession() call rather than storing it in a temporary.

Thanks for reminding. 

I removed the length check due to 
http://searchfox.org/mozilla-central/rev/8cf1367dd89cc36ef8f025dfc6af6d5c086838a7/dom/media/eme/MediaKeySession.cpp#274

has already checked.
Comment on attachment 8802856 [details]
Bug 1306572 - Part3 - Implement MediaCDMProxy and the callback proxy.

Cancel r? since I haven't addressed all the review feedback(I delete the reviewer name on review board, but no effect...).

Briefly summarized,

1. According to Jim Chen's suggestion, I've added @WrapForJNI(calledFrom = "gecko") on the callback that means it will automatically dispatch to "gecko main thread".

Therefore, I don't need to write any lambda or task for dispatching to main thread.

2. Creating a MDCDMThread is for easing the overhead of main thread since we don't know how long the MediaDrm API performs even for the out of process RPC.

I will revised the patch soon.

Thank you
Attachment #8802856 - Flags: review?(cpearce)
No longer blocks: 1306185
No longer blocks: 1306185
Depends on: 1306185
Hi Chris, 

Based on comment 21,

I don't need to manually dispatch to main thread.

All the callbacks has been called by main thread already. 

For creating a dedicated thread, it may be slow since we use JNI to invoke JAVA API even for oop RPC case(much slower).

Could you please review this patch again?

Thank you very much.
No longer depends on: 1306185
Depends on: 1306185
Attachment #8802856 - Flags: review?(cpearce)
Comment on attachment 8802856 [details]
Bug 1306572 - Part3 - Implement MediaCDMProxy and the callback proxy.

https://reviewboard.mozilla.org/r/87156/#review88760

r+ provided you remove the const_cast.

::: dom/media/eme/mediadrm/MediaDrmCDMCallbackProxy.cpp:30
(Diff revisions 1 - 2)
> -
>  void
>  MediaDrmCDMCallbackProxy::SetSessionId(uint32_t aToken,
>                                         const nsCString& aSessionId)
>  {
> -  nsCOMPtr<nsIRunnable> task(new SetSessionIdTask(mProxy,
> +  mProxy->OnSetSessionId(aToken, NS_ConvertUTF8toUTF16(aSessionId));

I think it would be best to assert that you're on the main thread here, and in the other callbacks that are called on the main thread.

::: dom/media/eme/mediadrm/MediaDrmCDMCallbackProxy.cpp:60
(Diff revisions 1 - 2)
>  void
>  MediaDrmCDMCallbackProxy::SessionMessage(const nsCString& aSessionId,
>                                           dom::MediaKeyMessageType aMessageType,
>                                           const nsTArray<uint8_t>& aMessage)
>  {
> -  nsCOMPtr<nsIRunnable> task;
> +  nsTArray<uint8_t>& message = const_cast<nsTArray<uint8_t>&>(aMessage);

Don't cast away const-ness, except in extreme circumstances. Make a copy of this if you can't pass a const reference in.

If you start changing the contents of const references passed into functions, no-one can assume that the things they thought weren't changed actually weren't changed. It makes it hard to reason about code.
Attachment #8802856 - Flags: review?(cpearce) → review+
Comment on attachment 8802845 [details]
Bug 1306572 - Part1 - Implement GeckoMediaDrm API as a glue for native CDMProxy and java impl MediaDrmBridge.

https://reviewboard.mozilla.org/r/87152/#review88902

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:133
(Diff revision 2)
> +        @WrapForJNI(calledFrom = "gecko")
> +        NativeMediaDrmProxyCallbacks() {}
> +
> +        @Override
> +        @WrapForJNI(calledFrom = "gecko")
> +        public native void onSessionCreated(int createSessionToken,

Are these native methods really called from the Gecko main thread? Which code will be calling these methods?

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:168
(Diff revision 2)
> +        @WrapForJNI(calledFrom = "gecko")
> +        public native void onRejectPromise(int promiseId,
> +                                           String message);
> +
> +        @Override // JNIObject
> +        protected native void disposeNative();

Make `disposeNative` throw an Exception because you're not calling it from Java, e.g.

    @Override // JNIObject
    protected void disposeNative() {
        throw new UnsupportedOperationException();
    }
Attachment #8802845 - Flags: review?(nchen) → review+
Comment on attachment 8802846 [details]
Bug 1306572 - Part2 - The native implementation of MediaDrmProxy by MediaDrmProxySupport

https://reviewboard.mozilla.org/r/87154/#review88908
Attachment #8802846 - Flags: review?(nchen) → review+
Addressed the review comments and rebased, wait for bug 1306185 part1 patch landing.
Once bug 1306185 is landed, I will enable mImpl = new LocalMediaDrmBridge(keySystem); on this bug.
Blocks: 1314530
rebased.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d85acf31e216
Part1 - Implement GeckoMediaDrm API as a glue for native CDMProxy and java impl MediaDrmBridge. r=cpearce,jchen
https://hg.mozilla.org/integration/autoland/rev/f6e81941c537
Part2 - The native implementation of MediaDrmProxy by MediaDrmProxySupport r=cpearce,jchen
https://hg.mozilla.org/integration/autoland/rev/3d1ccb0ce457
Part3 - Implement MediaCDMProxy and the callback proxy. r=cpearce
Keywords: checkin-needed
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: