Closed Bug 1306185 Opened 8 years ago Closed 8 years ago

[EME][Fennec] Implement Gecko Java components to bridge Android MediaDrm/MediaCrypto APIs in non-oop-decode mode.

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: kikuo, Assigned: kikuo)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 1306185-[Part1] Provide Java interface to bridge Gecko's gmp-decryption-like API and MediaDrm API
58 bytes, text/x-review-board-request
cpearce
: review+
jchen
: review+
Details
Bug 1306185-[Part2] Provide GeckoMediaDrmBridge implementation for Lollipop(V21)/Mashmallow(V23).
58 bytes, text/x-review-board-request
cpearce
: review+
jchen
: review+
Details
[Architecture Overview] New components are overlayed in red.
60.66 KB, application/pdf
Details
Bug 1306185-[Part3] Provide LocalMediaDrmBrdige as an intermediate which is the entry to actual GeckoMediaDrmBridge implementation.
58 bytes, text/x-review-board-request
cpearce
: review+
jchen
: review+
Details 
This solution will be provided in separated parts.

a. Provide Java implementation which is a bridge to Android MediaDrm/MediaCrypto API on Lollipop/Marshmallow [1]
b. Provide MediaDrmProxy.java which is the entry from Gecko native code to Java env to control above bridge. [2]
c. Provide the glue implementation from Gecko native code to MediaDrmProxy.java [3]
d. Get MediaCrypto object and configure it into MediaCodec in AndriodDecodeModule [4]
e. Modify moz.build

[1] https://github.com/JamesWCCheng/gecko-dev/commit/fd192b0f1cfa57b7a3b6c6d2e494a14c9e42d3e3
[2] https://github.com/JamesWCCheng/gecko-dev/commit/2841e6f86de63b0dceb05620cc33ccba14dde87e
[3] https://github.com/JamesWCCheng/gecko-dev/commit/7714d6204a3eaa67363c04e23c0f7f74680cdbb5
[4] https://github.com/JamesWCCheng/gecko-dev/commit/197928c9091746f1f73d9ea9a07660d6a03957cc
[5] https://github.com/JamesWCCheng/gecko-dev/commit/4748208fe3b8351d44adeaeddbfa5f85f2aa4d21
No longer blocks: Widevine_on_Fennec
Blocks: 1306196
Depends on: 1306219
Depends on: 1306572
Blocks: 1307818
Summary: [EME][Fennec] Support Widevine video playback in non-out-of-process decode mode. → [EME][Fennec] Implement Gecko Java components to bridge Android MediaDrm/MediaCrypto APIs in non-oop-decode mode.
The design aimed to make it capable of switching between NON-OOP-Decode mode and OOP-Decode mode easily, because it was implemented during the time while OOP-Decode was implementing.

The native implementation (MediaDrmCDMProxy.cpp & JNI glue codes) will be provide in Bug 1306572.
Attachment #8796914 - Attachment is obsolete: true
Comment on attachment 8797579 [details]
Bug 1306185-[Part1] Provide Java interface to bridge Gecko's gmp-decryption-like API and MediaDrm API

https://reviewboard.mozilla.org/r/83248/#review82240

Hello Chris & Jim,

Since there're many patches upcoming related to EME on Fennec, and I'd like to make the review procedure in parallel, so
I'm providing these Java components which are mainly bridging Gecko's CDM decryption API and Andriod MediaDrm/MediaCrypto APIs.  And James will provide patches related to EME decryption capability check and the glue code in Gecko.
Meanwhile, an architecture overview(pdf) is attached for you to have a quick catch on what's going on.  Hope it can help.
Attachment #8797579 - Flags: review?(nchen)
Attachment #8797579 - Flags: review?(cpearce)
Attachment #8797893 - Flags: review?(nchen)
Attachment #8797893 - Flags: review?(cpearce)
Attachment #8798119 - Flags: review?(nchen)
Attachment #8798119 - Flags: review?(cpearce)
Comment on attachment 8797893 [details]
Bug 1306185-[Part2] Provide GeckoMediaDrmBridge implementation for Lollipop(V21)/Mashmallow(V23).

https://reviewboard.mozilla.org/r/83490/#review82336

Lots of progress here. Most of my comments are around tidying the permutations of failure paths. In general, we need to promises to be rejected on all failure paths to that the JavaScript application isn't left waiting around for a promise to resolve or reject on failure.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:115
(Diff revision 1)
> +            catch (Exception e) {
> +                log("Failed to setPropertyString: " + e.getMessage());
> +            }
> +            mDrm.setOnEventListener(new MediaDrmListener());
> +        } catch (MediaDrmException e) {
> +            log("Failed to create MediaDrm: " + e.getMessage());

This failure propogates out to the owner via subsequent calls null checking mDrm?

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:134
(Diff revision 1)
> +                              int promiseId,
> +                              String initDataType,
> +                              byte[] initData) {
> +        log("createSession");
> +        if (mDrm == null) {
> +            onError("mDrm does NOT exist");

You need to reject the promise on all failure paths here. The same comment will apply elsewhere.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:148
(Diff revision 1)
> +        }
> +
> +        ByteBuffer sessionId = null;
> +        String strSessionId = null;
> +        try {
> +            ensureMediaCryptoCreated();

ensureMediaCryptoCreated() calls openSession(), so why do you need to call it again on line 150?

Doesn't that mean you'll have two sessions open instead of 1?

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:152
(Diff revision 1)
> +        try {
> +            ensureMediaCryptoCreated();
> +
> +            sessionId = openSession();
> +            if (sessionId == null) {
> +                onError("Cannot get a session id from MediaDrm !");

You should be rejecting the promise argument on all failure paths here.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:176
(Diff revision 1)
> +            onSessionMessage(sessionId.array(),
> +                             0 /*MediaKeyMessageType::License_request = 0*/,

Recommend you use some kind of constant for MediaKeyMessageType::Licence_request.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:178
(Diff revision 1)
> +                             promiseId,
> +                             sessionId.array(),
> +                             request.getData());
> +            onSessionMessage(sessionId.array(),
> +                             0 /*MediaKeyMessageType::License_request = 0*/,
> +                            request.getData());

Indentation in request.getData() off by 1.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:188
(Diff revision 1)
> +            if (sessionId != null) {
> +                closeSession(0, strSessionId);
> +            }
> +            savePendingCreateSessionData(createSessionToken, promiseId,
> +                                         initData, initDataType);
> +            startProvisioning();

If we throw inside of startProvisioning(), the try/catch inside of startProvisioning() suppresses the exception, so the caller won't know that it's failed, right? So we won't reject the createSession() promise? Perhaps startProvisioning() should either throw or return false on failure, so callers can fail.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:188
(Diff revision 1)
> +            if (sessionId != null) {
> +                closeSession(0, strSessionId);
> +            }
> +            savePendingCreateSessionData(createSessionToken, promiseId,
> +                                         initData, initDataType);
> +            startProvisioning();

What if startProvisioning() fails (silently!)?

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:198
(Diff revision 1)
> +    public void updateSession(int promiseId,
> +                              String sessionId,
> +                              byte[] response) {
> +        log("updateSession, sessionId = " + sessionId);
> +        if (mDrm == null) {
> +            onError("mDrm does NOT exist");

All failure cases here need to reject the promise.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:198
(Diff revision 1)
> +    public void updateSession(int promiseId,
> +                              String sessionId,
> +                              byte[] response) {
> +        log("updateSession, sessionId = " + sessionId);
> +        if (mDrm == null) {
> +            onError("mDrm does NOT exist");

Reject promise on all failure paths.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:214
(Diff revision 1)
> +        try {
> +            final byte [] keySetId = mDrm.provideKeyResponse(session.array(), response);
> +            HashMap<String, String> infoMap = mDrm.queryKeyStatus(session.array());
> +            for (String strKey : infoMap.keySet()) {
> +                String strValue = infoMap.get(strKey);
> +                log("InfoMap : key(" + strKey + ")/value(" + strValue + ")");

Don't you need to call back to Gecko so it knows the keys' status?

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:236
(Diff revision 1)
> +    }
> +
> +    @Override
> +    public void closeSession(int promiseId, String sessionId) {
> +        log("closeSession");
> +        if (mDrm == null) {

You should do something to the promise here... Reject it maybe if you think this situation should never happen.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:250
(Diff revision 1)
> +        }
> +    }
> +
> +    @Override
> +    public void release() {
> +        mPendingCreateSessionDataQueue.clear();

You should reject the promises waiting in pending provisioing.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:251
(Diff revision 1)
> +    }
> +
> +    @Override
> +    public void release() {
> +        mPendingCreateSessionDataQueue.clear();
> +        mPendingCreateSessionDataQueue = null;

You should reject all promises for pending createSessions here.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:284
(Diff revision 1)
> +
> +    protected void onSessionCreated(int createSessionToken,
> +                                    int promiseId,
> +                                    byte[] sessionId,
> +                                    byte[] request) {
> +        mCallbacks.onSessionCreated(createSessionToken, promiseId, sessionId, request);

Should you be null checking mCallbacks before using it? Seems like a good idea to me...

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:311
(Diff revision 1)
> +                                               SessionKeyInfo[] keyInfos) {
> +        mCallbacks.onSessionBatchedKeyChanged(sessionId, keyInfos);
> +    }
> +
> +    protected void onError(String message) {
> +        mCallbacks.onError(message);

Perhaps you should error all operation pending on provisioning here?

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:374
(Diff revision 1)
> +                    MediaDrm.KeyRequest request = null;
> +                    try {
> +                        request = getKeyRequest(session, data, initDataType);
> +                    } catch (android.media.NotProvisionedException e) {
> +                        log("Device not provisioned:" + e.getMessage());
> +                        startProvisioning();

What if this fails?

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:482
(Diff revision 1)
> +            urlConnection.setRequestMethod("POST");
> +            log("Provisioning : postRequest=" + finalURL.toString());
> +            try {
> +                // Add data
> +                urlConnection.setRequestProperty("Accept", "*/*");
> +                urlConnection.setRequestProperty("User-Agent", "Widevine CDM v1.0");

Is it always v1.0?

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:604
(Diff revision 1)
> +
> +        boolean success = provideProvisionResponse(response);
> +        if (success) {
> +            resumePendingOperations();
> +        } else {
> +            log("onProvisionResponse() Failed.");

Can you 'error' the pending operations here, so that the JavaScript app will not end up waiting forever?

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:622
(Diff revision 1)
> +            }
> +
> +            if (MediaCrypto.isCryptoSchemeSupported(mSchemeUUID)) {
> +                final byte [] cryptoSessionId = mCryptoSessionId.array();
> +                mCrypto = new MediaCrypto(mSchemeUUID, cryptoSessionId);
> +                String strCrypteSessionId = new String(mCryptoSessionId.array());

s/strCrypteSessionId/strCryptoSessionId/

??

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:632
(Diff revision 1)
> +                log("Cannot create MediaCrypto for unsupported scheme.");
> +                return false;
> +            }
> +        } catch (android.media.MediaCryptoException e) {
> +            log("Cannot create MediaCrypto:" + e.getMessage());
> +            release();

Why do you call release() on this failure path but not the others?
Attachment #8797893 - Flags: review?(cpearce) → review-
Comment on attachment 8797579 [details]
Bug 1306185-[Part1] Provide Java interface to bridge Gecko's gmp-decryption-like API and MediaDrm API

https://reviewboard.mozilla.org/r/83250/#review82334

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrm.java:23
(Diff revision 1)
> +                              int sessionMessageType,
> +                              byte[] request);
> +        void onSessionError(int promiseId, byte[] sessionId);
> +        void onSessionBatchedKeyChanged(byte[] sessionId,
> +                                        SessionKeyInfo[] keyInfos);
> +        void onError(String message);

Is there a meaningful error code that you can pass out if an error is raised? It may help debugging.
Attachment #8797579 - Flags: review?(cpearce) → review+
Comment on attachment 8798119 [details]
Bug 1306185-[Part3] Provide LocalMediaDrmBrdige as an intermediate which is the entry to actual GeckoMediaDrmBridge implementation.

https://reviewboard.mozilla.org/r/83664/#review82350

Again, lots of good work here, but we need to be careful about rejecting promises when things fail.

::: mobile/android/base/java/org/mozilla/gecko/media/LocalMediaDrmBridge.java:110
(Diff revision 1)
> +    public synchronized void createSession(int createSessionToken,
> +                                           int promiseId,
> +                                           String initDataType,
> +                                           byte[] initData) {
> +        log("createSession");
> +        if (mBridge == null) {

In the case where mBridge is null, you need to reject the promise. This comment applies elsewhere too.

::: mobile/android/base/java/org/mozilla/gecko/media/LocalMediaDrmBridge.java:118
(Diff revision 1)
> +        }
> +        try {
> +            mBridge.createSession(createSessionToken, promiseId, initDataType, initData);
> +        } catch (Exception e) {
> +            log("Fail to createSession");
> +            reportError(e);

Again, can you ensure the promise is rejected on this and all other error paths? You should have a contract that either the caller or the callee is responsible for the promise being rejected.

The promise must be rejected somewhere, otherwise the JS app will be left hanging.

::: mobile/android/base/java/org/mozilla/gecko/media/LocalMediaDrmBridge.java:160
(Diff revision 1)
> +        if (mBridge == null) {
> +            mCallbacks.onError("DrmBridge implementation doesn't exist.");
> +            return;
> +        }
> +        try {
> +            mBridge.release();

If mBridge.release() throws, shouldn't you still set mBridge to null?

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:220
(Diff revision 1)
> +        mImpl.closeSession(promiseId, sessionId);
> +    }
> +
> +    @WrapForJNI // Called when natvie object is destroyed.
> +    private void destroy() {
> +        log("destroy!! Natvie object is destroyed.");

s/Natvie/Native/

::: widget/android/fennec/FennecJNIWrappers.h:874
(Diff revision 1)
>              mozilla::jni::CallingThread::ANY;
>  
>      template<class Impl> class Natives;
>  };
>  
> +class MediaDrmProxy : public mozilla::jni::ObjectBase<MediaDrmProxy>

I assumed this is generated code, and did not review it.
Attachment #8798119 - Flags: review?(cpearce) → review-
Priority: -- → P2
Comment on attachment 8797893 [details]
Bug 1306185-[Part2] Provide GeckoMediaDrmBridge implementation for Lollipop(V21)/Mashmallow(V23).

https://reviewboard.mozilla.org/r/83490/#review82336

> You need to reject the promise on all failure paths here. The same comment will apply elsewhere.

Thanks for the review, I'll provide a new API in interface (Part1) to reject the promise on all failure paths, and also handle all error/release cases in Java more carefully : )

> ensureMediaCryptoCreated() calls openSession(), so why do you need to call it again on line 150?
> 
> Doesn't that mean you'll have two sessions open instead of 1?

This is a special solution for Key Rotation on Android with MediaCodec/MediaDrm/MediaCrypto.
In fact, MediaCodec instance is tied to a single MediaDrm session, which is also used for the creation of MediaCrypto instance.  Each MediaDrm session can only send one initial license request in its lifetime, so it's impossible to make successive key requests without tearing down MediaCodec instance, and that will cause discontinuities in streaming playback.
Widevie plugin on Android provides a session sharing mechanism.  It allows all sessions created by one MediaDrm instance to share a pool of keyids/keys, and the first session should be registerd into MediaCrypto instance, and cannot be closed until playback is complete.

The calling sequence may like the following
1. Create MediaDrm (enable sessionSharing mode)
2. MediaDrm.opensession -> sessionId-1
3. MediaCodec.configure( MediaCrypto(sessionId1) )
4. MediaDrm.opensession -> sessionId-2~N
    => getKeyRequest / provideKeyResponse (with sessionId-2~N)
6. MediaDrm.closesession -> sessionId-2~N
7. MediaDrm.closesession -> sessionId-1

> Is it always v1.0?

I'm not sure if it may change in the futrue, but we found this is hard-coded in Chrome on Android starting from L to N. So I assume it's fixed.
Comment on attachment 8797579 [details]
Bug 1306185-[Part1] Provide Java interface to bridge Gecko's gmp-decryption-like API and MediaDrm API

https://reviewboard.mozilla.org/r/83250/#review82562
Attachment #8797579 - Flags: review?(nchen) → review+
Comment on attachment 8797893 [details]
Bug 1306185-[Part2] Provide GeckoMediaDrmBridge implementation for Lollipop(V21)/Mashmallow(V23).

https://reviewboard.mozilla.org/r/83490/#review82558

I only checked Java usage, not overall logic.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:26
(Diff revision 1)
> +import android.media.MediaCryptoException;
> +import android.media.MediaDrm;
> +import android.media.MediaDrmException;
> +import android.util.Log;
> +
> +public class LollipopGeckoMediaDrmBridge implements GeckoMediaDrm {

Rename it `GeckoMediaDrmBridgeV21`

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:59
(Diff revision 1)
> +        private int token() { return mToken; }
> +        private int promiseId() { return mPromiseId; }
> +        private byte[] initData() { return mInitData; }
> +        private String mimeType() { return mMimeType; }

These methods are redundant. Change the fields to be public or package-private, and use the fields directly.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:72
(Diff revision 1)
> +            return mCrypto.requiresSecureDecoderComponent(mimeType);
> +        }
> +        return false;
> +      }
> +
> +    private void log(String msg) {

Just use `Log.d` directly in your code, and put the logs behind a `private static final boolean DEBUG` flag.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:78
(Diff revision 1)
> +        Log.d(LOGTAG, msg);
> +    }
> +
> +    private void assertMediaCrypto() throws Exception {
> +        if (mCrypto == null) {
> +            throw new Exception(" mCrypto does not exist ! ");

Throw `IllegalStateException` so you can remove the `throws Exception` declaration and `try` statements.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:92
(Diff revision 1)
> +        mSessionMIMETypes = new HashMap<ByteBuffer, String>();
> +        mPendingCreateSessionDataQueue = new ArrayDeque<PendingCreateSessionData>();
> +
> +        Looper myLoop = Looper.myLooper();
> +        if (myLoop == null) {
> +            Looper.prepare();

You should not call `Looper.prepare` in here. If you start a new thread, you should call `Looper.prepare` in the thread Runnable.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:93
(Diff revision 1)
> +        mPendingCreateSessionDataQueue = new ArrayDeque<PendingCreateSessionData>();
> +
> +        Looper myLoop = Looper.myLooper();
> +        if (myLoop == null) {
> +            Looper.prepare();
> +            mHandler = new Handler();

`mHandler` will be null if `myLoop != null`?

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:210
(Diff revision 1)
> +            onSessionError(promiseId, session.array());
> +            return;
> +        }
> +
> +        try {
> +            final byte [] keySetId = mDrm.provideKeyResponse(session.array(), response);

`keySetId` is not used?

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:211
(Diff revision 1)
> +            HashMap<String, String> infoMap = mDrm.queryKeyStatus(session.array());
> +            for (String strKey : infoMap.keySet()) {
> +                String strValue = infoMap.get(strKey);
> +                log("InfoMap : key(" + strKey + ")/value(" + strValue + ")");
> +            }

Put this whole block inside `if (DEBUG)`

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:325
(Diff revision 1)
> +        }
> +
> +        MediaDrm.KeyRequest request = null;
> +        try {
> +            HashMap<String, String> optionalParameters = new HashMap<String, String>();
> +            request = mDrm.getKeyRequest(aSession.array(),

`return mDrm.getKeyRequest(...)` directly

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:331
(Diff revision 1)
> +                                         data,
> +                                         mimeType,
> +                                         MediaDrm.KEY_TYPE_STREAMING,
> +                                         optionalParameters);
> +        } catch (Exception e) {
> +            e.printStackTrace();

`Log.e(LOGTAG, ..., e)`

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:469
(Diff revision 1)
> +                mResponseBody = postRequest(urls[0], mDrmRequest);
> +                if (mResponseBody != null) {
> +                    log("response length=" + mResponseBody.length);
> +                }
> +            } catch (IOException e) {
> +                e.printStackTrace();

`Log.e(LOGTAG, ..., e);`

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:475
(Diff revision 1)
> +            }
> +            return null;
> +        }
> +
> +        private byte[] postRequest(String url, byte[] drmRequest) throws IOException {
> +            URL finalURL = new URL(url + "&signedRequest=" + new String(drmRequest));

Use `URLEncoder` to encode the `drmRequest` string.

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:622
(Diff revision 1)
> +            }
> +
> +            if (MediaCrypto.isCryptoSchemeSupported(mSchemeUUID)) {
> +                final byte [] cryptoSessionId = mCryptoSessionId.array();
> +                mCrypto = new MediaCrypto(mSchemeUUID, cryptoSessionId);
> +                String strCrypteSessionId = new String(mCryptoSessionId.array());

`if (DEBUG)`

::: mobile/android/base/java/org/mozilla/gecko/media/LollipopGeckoMediaDrmBridge.java:641
(Diff revision 1)
> +            throw e;
> +        }
> +    }
> +
> +    private UUID convertKeySystemToSchemeUUID(String keySystem) {
> +      if (keySystem.equals(WIDEVINE_KEY_SYSTEM)) {

`if (WIDEVINE_KEY_SYSTEM.equals(keySystem))`

::: mobile/android/base/java/org/mozilla/gecko/media/MashmallowGeckoMediaDrmBridge.java:13
(Diff revision 1)
> +import static android.os.Build.VERSION_CODES.M;
> +import android.media.MediaDrm;
> +import android.util.Log;
> +import java.util.List;
> +
> +public class MashmallowGeckoMediaDrmBridge extends LollipopGeckoMediaDrmBridge {

Rename it `GeckoMediaDrmBridgeV23`

::: mobile/android/base/java/org/mozilla/gecko/media/MashmallowGeckoMediaDrmBridge.java:17
(Diff revision 1)
> +
> +public class MashmallowGeckoMediaDrmBridge extends LollipopGeckoMediaDrmBridge {
> +
> +    private static final String LOGTAG = "MashmallowGeckoMediaDrmBridge";
> +
> +    private void log(String msg) {

Use `Log.d` directly, within `if (DEBUG)` checks.
Attachment #8797893 - Flags: review?(nchen)
Comment on attachment 8798119 [details]
Bug 1306185-[Part3] Provide LocalMediaDrmBrdige as an intermediate which is the entry to actual GeckoMediaDrmBridge implementation.

https://reviewboard.mozilla.org/r/83664/#review82568

Looks okay but I want to look at the native code too before I give r+.

::: mobile/android/base/java/org/mozilla/gecko/media/LocalMediaDrmBridge.java:80
(Diff revision 1)
> +            e.printStackTrace();
> +        }
> +        mCallbacks.onError(e.getMessage());
> +    }
> +
> +    private void log(String msg) {

Just inline this code.
Attachment #8798119 - Flags: review?(nchen)
Comment on attachment 8797893 [details]
Bug 1306185-[Part2] Provide GeckoMediaDrmBridge implementation for Lollipop(V21)/Mashmallow(V23).

https://reviewboard.mozilla.org/r/83490/#review82558

Thanks for the suggestions, I'll address these issues.

> `keySetId` is not used?

For the case, according to document, the return value will be null if the session type is streaming.
Should be ok to remove it.
Comment on attachment 8797893 [details]
Bug 1306185-[Part2] Provide GeckoMediaDrmBridge implementation for Lollipop(V21)/Mashmallow(V23).

https://reviewboard.mozilla.org/r/83490/#review82336

> Don't you need to call back to Gecko so it knows the keys' status?

The results returned by |mDrm.queryKeyStatus| are not like the information on Desktop.

Here's an output example,
InfoMap : key(PersistAllowed)/value(False)
InfoMap : key(RenewAllowed)/value(True)
InfoMap : key(LicenseType)/value(Streaming)
InfoMap : key(PlaybackDurationRemaining)/value(2592000) // <=== in seconds.
InfoMap : key(PlayAllowed)/value(True)
InfoMap : key(RenewalServerUrl)/value()
InfoMap : key(LicenseDurationRemaining)/value(2591998) // <=== in seconds.

Accroding to Android API document, the 'keySetId' returned by |mDrm.provideKeyResponse| is null when licenseType is Streaming, so it's not possible to update specific keyId statuschange back to gecko.
Above Mashmallow(API 23+), there's a class |MediaDrm.OnKeyStatusChangeListener| for the purpose to fulfill EME usage. But In Lollipop(API 21~22), Chrome just notifies status:usable with a dummy keyId in Lollipop, and implement their own wait-for-key logic.

I think we can also do like Chrome for Lollipop, and should implement the wait-for-key stuff in another bug.
Attachment #8797893 - Flags: review?(nchen)
Attachment #8797893 - Flags: review?(cpearce)
Attachment #8798119 - Flags: review?(nchen)
Attachment #8798119 - Flags: review?(cpearce)
Comment on attachment 8798119 [details]
Bug 1306185-[Part3] Provide LocalMediaDrmBrdige as an intermediate which is the entry to actual GeckoMediaDrmBridge implementation.

https://reviewboard.mozilla.org/r/83664/#review82350

Thanks for review, I revised the code and addressed promise-handling issues here.

> I assumed this is generated code, and did not review it.

Will be handled in Bug 1306572
Comment on attachment 8798119 [details]
Bug 1306185-[Part3] Provide LocalMediaDrmBrdige as an intermediate which is the entry to actual GeckoMediaDrmBridge implementation.

https://reviewboard.mozilla.org/r/83664/#review82568

Thanks for suggestions. I discussed with James, LocalMediaDrmBridge will be created in MediaDrmProxy.java(which is the entry point from Gecko), and all usage of @WrapForJNI and corresponding native implmentaion will be handled in Bug 1306572.
So only java components are left in this bug.  It should be easier for review.
Does that make sense to you ?
Comment on attachment 8797893 [details]
Bug 1306185-[Part2] Provide GeckoMediaDrmBridge implementation for Lollipop(V21)/Mashmallow(V23).

https://reviewboard.mozilla.org/r/83490/#review82336

Thanks for the review and suggestions, I've addressed the promising handling when failure happens, and also simplified some logic.

> This failure propogates out to the owner via subsequent calls null checking mDrm?

Not used, so removed.

> If we throw inside of startProvisioning(), the try/catch inside of startProvisioning() suppresses the exception, so the caller won't know that it's failed, right? So we won't reject the createSession() promise? Perhaps startProvisioning() should either throw or return false on failure, so callers can fail.

I'll store the promiseId for this createSession(), and reject it asynchronously on any failure during startProvisioning().

> What if startProvisioning() fails (silently!)?

I'll reject the promise on all failure paths, this shouldn't happen.

> Perhaps you should error all operation pending on provisioning here?

Thanks for the suggestions, I looked through the provisioning code path and handled on all operations to avoid promise-pending cases.

> Why do you call release() on this failure path but not the others?

I thought if a MediaCrypto exception happens, there's nothing we can do for the following procedure. The resource should be released.
Comment on attachment 8797579 [details]
Bug 1306185-[Part1] Provide Java interface to bridge Gecko's gmp-decryption-like API and MediaDrm API

https://reviewboard.mozilla.org/r/83248/#review84430

Hi Chris and Jim, thanks for your time and suggestions, I've addressed those issues reported since last review.
Also I've decided to leave the managed part of coded(which is the entry from native gecko) in Bug 1306572 for easier review.
There should be only java components in this bug.
Comment on attachment 8797579 [details]
Bug 1306185-[Part1] Provide Java interface to bridge Gecko's gmp-decryption-like API and MediaDrm API

https://reviewboard.mozilla.org/r/83250/#review86176

Sliglty changes in part1. 
1. Add a new method "onRejectPromise" to reject the promise to gekco on all failure happens.
2. Remove the argument "promiseId" from onSessionEror. (This method should just logs when error happens in a session, and also for the consistency with gmp-decryption API.)
Comment on attachment 8797893 [details]
Bug 1306185-[Part2] Provide GeckoMediaDrmBridge implementation for Lollipop(V21)/Mashmallow(V23).

https://reviewboard.mozilla.org/r/83490/#review86182

Hi Chris, Jim, 

All issues were addressed since last time, might need your review again. thanks.
Take your time, since it's not in priority. :)

Part2
 - Fix issues.
Part3
 - Remove MediaDrmProxy.java, because it's related to generated codes, and will be addressed in Bug 1306572.
Attachment #8797893 - Flags: review?(nchen)
Attachment #8797893 - Flags: review?(cpearce)
Attachment #8798119 - Flags: review?(nchen)
Attachment #8798119 - Flags: review?(cpearce)
Comment on attachment 8797893 [details]
Bug 1306185-[Part2] Provide GeckoMediaDrmBridge implementation for Lollipop(V21)/Mashmallow(V23).

https://reviewboard.mozilla.org/r/83490/#review86658

r+ on Java usage after comments are addressed.

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:81
(Diff revision 3)
> +        }
> +        return false;
> +      }
> +
> +    private static void assertTrue(boolean condition) {
> +      if (!condition) {

if (DEBUG && !condition)

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:549
(Diff revision 3)
> +        }
> +    }
> +
> +    private void resumePendingOperations() {
> +        if (mHandlerThread == null) {
> +            mHandlerThread = new HandlerThread("HandlerThread");

The thread name should indicate the purpose of the thread, so use a different name than "HandlerThread"
Attachment #8797893 - Flags: review?(nchen) → review+
Comment on attachment 8798119 [details]
Bug 1306185-[Part3] Provide LocalMediaDrmBrdige as an intermediate which is the entry to actual GeckoMediaDrmBridge implementation.

https://reviewboard.mozilla.org/r/83664/#review86666

::: mobile/android/base/java/org/mozilla/gecko/media/LocalMediaDrmBridge.java:82
(Diff revision 3)
> +            mProxyCallbacks.onRejectPromise(promiseId, message);
> +        }
> +    } // CallbacksForwarder
> +
> +    private static void assertTrue(boolean condition) {
> +        if (!condition) {

if (DEBUG && !condition)

::: widget/android/fennec/FennecJNIWrappers.h:874
(Diff revision 3)
>              mozilla::jni::CallingThread::ANY;
>  
>      template<class Impl> class Natives;
>  };
>  
> +class SessionKeyInfo : public mozilla::jni::ObjectBase<SessionKeyInfo>

You should move the generated portion to part 1 because SessionKeyInfo.java was added in part 1.
Attachment #8798119 - Flags: review?(nchen) → review+
Comment on attachment 8797893 [details]
Bug 1306185-[Part2] Provide GeckoMediaDrmBridge implementation for Lollipop(V21)/Mashmallow(V23).

https://reviewboard.mozilla.org/r/83490/#review86970

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:212
(Diff revisions 3 - 4)
> +            if (DEBUG) Log.d(LOGTAG, "Key successfully added for session " + sessionId);
> +            onSessionUpdated(promiseId, session.array());
> +            return;

Found an incorrect handling in this try/catch block while fixing open issues, get it fixed.
Jim, thanks for your time & review.
No longer depends on: 1306572
Depends on: 1306572
No longer depends on: 1306219
No longer depends on: 1306572
Blocks: 1306572
Assignee: nobody → kikuo
No longer blocks: 1306572
Blocks: 1306572
Comment on attachment 8797893 [details]
Bug 1306185-[Part2] Provide GeckoMediaDrmBridge implementation for Lollipop(V21)/Mashmallow(V23).

https://reviewboard.mozilla.org/r/83490/#review88762

Looks good, only minor issues. Sorry for slow review!

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:90
(Diff revision 4)
> +
> +    GeckoMediaDrmBridgeV21(String keySystem) throws Exception {
> +        if (DEBUG) Log.d(LOGTAG, "GeckoMediaDrmBridgeV21()");
> +
> +        mProvisioning = false;
> +        mProvisioningPromiseId = 0;

mProvisioning is only true when mProvisioningPromiseId is non-zero. So why not just rely on mProvisioningPromiseId being non-zero to tell you whether you're provisioning?

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:151
(Diff revision 4)
> +            if (sessionId == null) {
> +                onRejectPromise(promiseId, "Cannot get a session id from MediaDrm !");
> +                return;
> +            }
> +            strSessionId = new String(sessionId.array());
> +            mSessionIds.put(sessionId, strSessionId);

Here you store (sessionId, new String(sessionId)). Why don't you just reconstruct String(sessonId) when you need it? i.e. if you have the String version, you should be able to get the bytes by calling String.getBytes(), right?

Why do you need to store the String -> bytes mapping? The overhead of maintaining this table is probably more than that of re-creating the strings.

Why don't you just store the sessionIds as a Set<ByteBuffer> of somekind?

The general principle here is to eliminate redundancy, as if you keep multiple/redundant copies of state you need to worry about keeping them in sync.

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:172
(Diff revision 4)
> +            mSessionMIMETypes.put(sessionId, initDataType);
> +            if (DEBUG) Log.d(LOGTAG, " StringID : " + strSessionId + " is put into mSessionIds ");
> +        } catch (android.media.NotProvisionedException e) {
> +            if (DEBUG) Log.d(LOGTAG, "Device not provisioned:" + e.getMessage());
> +            if (sessionId != null) {
> +                // The promise of this createSession will be eiather resolved

s/eiather/either/

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:251
(Diff revision 4)
> +    public void release() {
> +        if (DEBUG) Log.d(LOGTAG, "release()");
> +        if (mProvisioningPromiseId > 0) {
> +            onRejectPromise(mProvisioningPromiseId, "Releasing ... reject provisioning session.");
> +        }
> +        while (!mPendingCreateSessionDataQueue.isEmpty()) {

Could you write that as:


while ((PendingCreateSessionData pendingData = mPendingCreateSessionDataQueue.poll()))\{
  // ...
\}

? My Java is a little rusty... A few other places too.

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:257
(Diff revision 4)
> +            PendingCreateSessionData pendingData = mPendingCreateSessionDataQueue.poll();
> +            onRejectPromise(pendingData.mPromiseId, "Releasing ... reject all pending sessions.");
> +        }
> +        mPendingCreateSessionDataQueue = null;
> +
> +        for (ByteBuffer session : mSessionIds.keySet()) {

You can do the mDrm null check outside the loop, right? And then you won't go through the loop and re-do it unnecessarily.

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:415
(Diff revision 4)
> +            return null;
> +        }
> +    }
> +
> +    private ByteBuffer getSession(String sessionId) {
> +        for (ByteBuffer session : mSessionIds.keySet()) {

return ByteBuffer.wrap(sessionId.getBytes());

??

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:424
(Diff revision 4)
> +        }
> +        return null;
> +    }
> +
> +    private ByteBuffer removeSession(String sessionId) {
> +        for (ByteBuffer session : mSessionIds.keySet()) {

mSessionID.remove(ByteBuffer.wrap(sessionId.getBytes()));

??

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:608
(Diff revision 4)
> +                if (DEBUG) Log.d(LOGTAG, "Cannot open session for MediaCrypto");
> +                return false;
> +            }
> +
> +            if (MediaCrypto.isCryptoSchemeSupported(mSchemeUUID)) {
> +                final byte [] cryptoSessionId = mCryptoSessionId.array();

You're caching mCryptoSessionId.array() in the cryptoSessionId variable.

Then you pass cryptoSessionId in and mCryptoSessionId.array() into functions here. Why not only pass mCryptoSessionId.array() into the functions here? Or only cryptoSessionId?
Attachment #8797893 - Flags: review?(cpearce) → review+
Comment on attachment 8798119 [details]
Bug 1306185-[Part3] Provide LocalMediaDrmBrdige as an intermediate which is the entry to actual GeckoMediaDrmBridge implementation.

https://reviewboard.mozilla.org/r/83664/#review88768

r+ with missing sychronized explained or fixed.

::: mobile/android/base/java/org/mozilla/gecko/media/LocalMediaDrmBridge.java:158
(Diff revision 4)
> +            Log.e(LOGTAG, "Failed to release", e);
> +        }
> +    }
> +
> +    @Override
> +    public MediaCrypto getMediaCrypto() {

Why is everything *except* this function synchronized? You're touching mBridge here, just like in the synchronized functions.
Attachment #8798119 - Flags: review?(cpearce) → review+
Comment on attachment 8797579 [details]
Bug 1306185-[Part1] Provide Java interface to bridge Gecko's gmp-decryption-like API and MediaDrm API

https://reviewboard.mozilla.org/r/83248/#review89176

Thanks for review, all issues were addressed.
Comment on attachment 8797579 [details]
Bug 1306185-[Part1] Provide Java interface to bridge Gecko's gmp-decryption-like API and MediaDrm API

https://reviewboard.mozilla.org/r/83248/#review89220

Rebase patches.
Blocks: 1314530
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/12d6d9c37b2d
[Part1] Provide Java interface to bridge Gecko's gmp-decryption-like API and MediaDrm API r=cpearce,jchen
https://hg.mozilla.org/integration/autoland/rev/a40cb9ae8d07
[Part2] Provide GeckoMediaDrmBridge implementation for Lollipop(V21)/Mashmallow(V23). r=cpearce,jchen
https://hg.mozilla.org/integration/autoland/rev/2d8d0579cb0d
[Part3] Provide LocalMediaDrmBrdige as an intermediate which is the entry to actual GeckoMediaDrmBridge implementation. r=cpearce,jchen
Keywords: checkin-needed
backed this out for causing https://treeherder.mozilla.org/logviewer.html#?job_id=6007451&repo=autoland which turned into a perma failure
Flags: needinfo?(kikuo)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42e6e9452daa
Backed out changeset 2d8d0579cb0d 
https://hg.mozilla.org/integration/autoland/rev/ecc75858b17b
Backed out changeset a40cb9ae8d07 
https://hg.mozilla.org/integration/autoland/rev/ff8d24f06a97
Backed out changeset 12d6d9c37b2d causing lint perma failures
Comment on attachment 8797893 [details]
Bug 1306185-[Part2] Provide GeckoMediaDrmBridge implementation for Lollipop(V21)/Mashmallow(V23).

https://reviewboard.mozilla.org/r/83490/#review89750

"securityLevel"/"sessionSharing" are vendor specific properties and not defined in MediaDrm API.
Fix the Android Lint error "WrongConstant" by using annotation @SuppressLint.

Try run all green ! https://treeherder.mozilla.org/#/jobs?repo=try&revision=c19998d4dfac
Sorry for the bother, lint error was fixed and new try results was in Comment 48.
Flags: needinfo?(kikuo)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8e4733d6c48
[Part1] Provide Java interface to bridge Gecko's gmp-decryption-like API and MediaDrm API r=cpearce,jchen
https://hg.mozilla.org/integration/autoland/rev/c5c4a1219f26
[Part2] Provide GeckoMediaDrmBridge implementation for Lollipop(V21)/Mashmallow(V23). r=cpearce,jchen
https://hg.mozilla.org/integration/autoland/rev/e45f0d1c605e
[Part3] Provide LocalMediaDrmBrdige as an intermediate which is the entry to actual GeckoMediaDrmBridge implementation. r=cpearce,jchen
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: