1306196 - [EME][Fennec] Implement Gecko Java components to bridge Android MediaDrm/MediaCrypto APIs in OOP-Decode mode.

Status: RESOLVED FIXED

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

Description

[Kilik Kuo [:kikuo] (inactive)]

Divide the solution into following steps.

a. Provide aidl for EME APIs [1]
b. Provide remote process stub (RemoteMediaDrmBridgeStub.java) which is responsible to create the bridge to MediaDrm/MediaCrypto. [2]
c. Create a bridge(RemoteMediaDrmBridge.java) in parent side to communicate with remote stub. [3]
d. Provide a uuid (drmStubId) for each created MediaDrmBridge instance. By this uuid, MediaCodec can find corresponding MediaCrypto object to decrypt data. [4][5]

[1] https://github.com/JamesWCCheng/gecko-dev/commit/a874baaf70b5f5cf7db1a66aae435c2c2bc056c5
[2] https://github.com/JamesWCCheng/gecko-dev/commit/fd192b0f1cfa57b7a3b6c6d2e494a14c9e42d3e3
[3] https://github.com/JamesWCCheng/gecko-dev/commit/2841e6f86de63b0dceb05620cc33ccba14dde87e
[4] https://github.com/JamesWCCheng/gecko-dev/commit/dffc11d6816c82f32d52a5fa1645d23539c5bea4
[5] https://github.com/JamesWCCheng/gecko-dev/commit/197928c9091746f1f73d9ea9a07660d6a03957cc

Comment 1

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [Diagram] Remote MediaDrmBridge & Corresponding AIDLs.pdf

Have a overview on ...
1. The prerequisite part. 
    ==> Local bridge to bridge Implementation.
2. Which part is going to be provided IN THIS BUG.
    ==> Remote bridge & AIDL binders 
3. The next part to be provided.
    ==> MediaCodec get correct MediaCrypto for decryption/decode.

Comment 2

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1306196-[P1] Provide AIDL definiion for EME functionality on Fennec MediaManager Service.

Review commit: https://reviewboard.mozilla.org/r/89200/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89200/

Comment 3

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1306196-[P2] Provide RemoteMediaDrmBridge & corresponding stub to underlying GeckoMediaDrmBridge implementation.

Review commit: https://reviewboard.mozilla.org/r/89198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89198/

Comment 4

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8805460 [details]
Bug 1306196-[P1] Provide AIDL definiion for EME functionality on Fennec MediaManager Service.

https://reviewboard.mozilla.org/r/89196/#review88380

As the interface defined in Bug 1306185 Part1 [1] has been r+'d, I'd like to continue the review procedure for remote drm bridge.

P1 is the IPC definition of our Fennec MediaDrmBridge implementation for out-of-process decoding service.
P2 is the proxy-like classes in parent/child side to deliver requests/results.

[1] https://reviewboard.mozilla.org/r/83250/diff/4#index_header

Comment 5

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

Comment on attachment 8805461 [details]
Bug 1306196-[P2] Provide RemoteMediaDrmBridge & corresponding stub to underlying GeckoMediaDrmBridge implementation.

https://reviewboard.mozilla.org/r/89198/#review88780

::: mobile/android/base/java/org/mozilla/gecko/media/RemoteMediaDrmBridge.java:31
(Diff revision 1)
> +                                     int promiseId,
> +                                     byte[] sessionId,
> +                                     byte[] request) {
> +            assertTrue(mProxyCallbacks != null);
> +            mProxyCallbacks.onSessionCreated(createSessionToken,
> +                                        promiseId,

Indentation of here.

::: mobile/android/base/java/org/mozilla/gecko/media/RemoteMediaDrmBridgeStub.java:23
(Diff revision 1)
> +    private volatile IMediaDrmBridgeCallbacks mCallbacks = null;
> +
> +    // Underlying bridge implmenetaion, i.e. GeckoMediaDrmBrdigeV21.
> +    private GeckoMediaDrm mBridge = null;
> +
> +    // mStubId is initialized during stub construction. It should be an unique

s/an unique/a unique/

Comment 6

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

Comment on attachment 8805460 [details]
Bug 1306196-[P1] Provide AIDL definiion for EME functionality on Fennec MediaManager Service.

https://reviewboard.mozilla.org/r/89200/#review88782

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8805460 [details]
Bug 1306196-[P1] Provide AIDL definiion for EME functionality on Fennec MediaManager Service.

https://reviewboard.mozilla.org/r/89200/#review88910

::: mobile/android/base/java/org/mozilla/gecko/media/SessionKeyInfo.java:11
(Diff revision 1)
>  
> +import android.os.Parcel;
> +import android.os.Parcelable;
>  import org.mozilla.gecko.annotation.WrapForJNI;
>  
>  @WrapForJNI

Move `@WrapForJNI` to the members that need JNI access, so we don't generate JNI bindings for other members like `describeContents()` and `writeToParcel()`.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8805461 [details]
Bug 1306196-[P2] Provide RemoteMediaDrmBridge & corresponding stub to underlying GeckoMediaDrmBridge implementation.

https://reviewboard.mozilla.org/r/89198/#review88912

::: mobile/android/base/java/org/mozilla/gecko/media/RemoteManager.java:159
(Diff revision 1)
> +        try {
> +            IMediaDrmBridge remoteBridge =
> +                mRemote.createRemoteMediaDrmBridge(keySystem, stubId);
> +            return remoteBridge;
> +        } catch (RemoteException e) {
> +            e.printStackTrace();

Use `Log.e(LOGTAG, "MESSAGE", e)`;

::: mobile/android/base/java/org/mozilla/gecko/media/RemoteMediaDrmBridge.java:29
(Diff revision 1)
> +        @Override
> +        public void onSessionCreated(int createSessionToken,
> +                                     int promiseId,
> +                                     byte[] sessionId,
> +                                     byte[] request) {
> +            assertTrue(mProxyCallbacks != null);

You don't need these checks because we'll throw `NullPointerException` in the lines below if `mProxyCallbacks` is null.

::: mobile/android/base/java/org/mozilla/gecko/media/RemoteMediaDrmBridge.java:76
(Diff revision 1)
> +            assertTrue(mProxyCallbacks != null);
> +            mProxyCallbacks.onRejectPromise(promiseId, message);
> +        }
> +    } // CallbacksForwarder
> +
> +    private static void assertTrue(boolean condition) {

Change to package-private access because you're calling it from CallbacksForwarder.

::: mobile/android/base/java/org/mozilla/gecko/media/RemoteMediaDrmBridge.java:111
(Diff revision 1)
> +        if (DEBUG) Log.d(LOGTAG, "createSession()");
> +
> +        try {
> +            mRemote.createSession(createSessionToken, promiseId, initDataType, initData);
> +        } catch (Exception e) {
> +            Log.e(LOGTAG, "Got exception while creating remote session.");

Add `, e);` at the end, and elsewhere

::: mobile/android/base/java/org/mozilla/gecko/media/RemoteMediaDrmBridgeStub.java:75
(Diff revision 1)
> +                                                  promiseId,
> +                                                  sessionId,
> +                                                  request);
> +            } catch (RemoteException e) {
> +                // Dead recipient.
> +                e.printStackTrace();

Use `Log.e(LOGTAG, "MESSAGE", e);` and elsewhere

::: mobile/android/base/java/org/mozilla/gecko/media/RemoteMediaDrmBridgeStub.java:149
(Diff revision 1)
> +                e.printStackTrace();
> +            }
> +        }
> +    }
> +
> +    private static void assertTrue(boolean condition) {

Package-private access.

Comment 9

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8805460 [details]
Bug 1306196-[P1] Provide AIDL definiion for EME functionality on Fennec MediaManager Service.

https://reviewboard.mozilla.org/r/89196/#review89302

All issues fixed & patches rebased.
Thanks !

Comment 10

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8805460 [details]
Bug 1306196-[P1] Provide AIDL definiion for EME functionality on Fennec MediaManager Service.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89200/diff/1-2/

Comment 11

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8805461 [details]
Bug 1306196-[P2] Provide RemoteMediaDrmBridge & corresponding stub to underlying GeckoMediaDrmBridge implementation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89198/diff/1-2/

Comment 12

[Kilik Kuo [:kikuo] (inactive)]

try run is ok, https://treeherder.mozilla.org/#/jobs?repo=try&revision=127bcd3f4767

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4eab781604ad
[P1] Provide AIDL definiion for EME functionality on Fennec MediaManager Service. r=cpearce,jchen
https://hg.mozilla.org/integration/autoland/rev/0aa5b42dccea
[P2] Provide RemoteMediaDrmBridge & corresponding stub to underlying GeckoMediaDrmBridge implementation. r=cpearce,jchen

Comment 14

[Carsten Book [:Tomcat]]

backed this out because parts depend on bug 1306185 which needed to be backed out

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b13127fafa7e
Backed out changeset 0aa5b42dccea 
https://hg.mozilla.org/integration/autoland/rev/2a24a2c246a8
Backed out changeset 4eab781604ad

Comment 16

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8805460 [details]
Bug 1306196-[P1] Provide AIDL definiion for EME functionality on Fennec MediaManager Service.

https://reviewboard.mozilla.org/r/89196/#review89764

Rebase again for a new try.

Comment 17

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8805460 [details]
Bug 1306196-[P1] Provide AIDL definiion for EME functionality on Fennec MediaManager Service.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89200/diff/2-3/

Comment 18

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8805461 [details]
Bug 1306196-[P2] Provide RemoteMediaDrmBridge & corresponding stub to underlying GeckoMediaDrmBridge implementation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89198/diff/2-3/

Comment 19

[Kilik Kuo [:kikuo] (inactive)]

Lint error was fixed in bug 1306185.

New try, https://treeherder.mozilla.org/#/jobs?repo=try&revision=54b9034fe1d8, lovely green !

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c5036988239
[P1] Provide AIDL definiion for EME functionality on Fennec MediaManager Service. r=cpearce,jchen
https://hg.mozilla.org/integration/autoland/rev/16309437315f
[P2] Provide RemoteMediaDrmBridge & corresponding stub to underlying GeckoMediaDrmBridge implementation. r=cpearce,jchen

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4c5036988239
https://hg.mozilla.org/mozilla-central/rev/16309437315f