Closed Bug 1307818 Opened 8 years ago Closed 8 years ago

[EME][Fennec] Make MediaCodec capable of decrypting with MediaCrypto.

Tracking

(firefox53 fixed)

Status:

RESOLVED FIXED

Milestone:

Firefox 53

Tracking Flags:

Tracking

Status

firefox53

---

fixed

People

(Reporter: kikuo, Assigned: kikuo)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

Bug 1307818-[P1] Provide drmStubId for CDMProxy and instantiate {Local,Remote}MediaDrmBridge. 8 years ago Kilik Kuo [:kikuo] (inactive) 58 bytes, text/x-review-board-request	cpearce : review+ jchen : review+	Details
Bug 1307818-[P2] Setup MediaCrypto for both in-process and out-of-process decode. 8 years ago Kilik Kuo [:kikuo] (inactive) 58 bytes, text/x-review-board-request	cpearce : review+ jchen : review+	Details
Bug 1307818-[P3] Notify correct batched-keystatuses-changed information in API V23+. 8 years ago Kilik Kuo [:kikuo] (inactive) 58 bytes, text/x-review-board-request	jchen : review+	Details

Kilik Kuo [:kikuo] (inactive)

Assignee

Description

•

8 years ago

MediaCrypto object should be set into MediaCodec when it comes to a DRM content playback.

I'm going to make this happening in both MediaCodecDataDecoder.cpp and JellyBeanAsyncCodec.java

Blake Wu [:bwu][:blakewu]

Updated

•

8 years ago

Priority: -- → P2

Kilik Kuo [:kikuo] (inactive)

Assignee

Updated

•

8 years ago

Depends on: 1306196

Kilik Kuo [:kikuo] (inactive)

Assignee

Updated

•

8 years ago

Depends on: 1306572

James Cheng[:JamesCheng]

Updated

•

8 years ago

Blocks: Widevine_on_Fennec

James Cheng[:JamesCheng]

Updated

•

8 years ago

Depends on: 1314530

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

Comment on attachment 8809268 [details]
Bug 1307818-[P1] Provide drmStubId for CDMProxy and instantiate {Local,Remote}MediaDrmBridge.

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

Kilik Kuo [:kikuo] (inactive)

Assignee

Comment 3

•

8 years ago

mozreview-review

Comment on attachment 8809268 [details]
Bug 1307818-[P1] Provide drmStubId for CDMProxy and instantiate {Local,Remote}MediaDrmBridge.

https://reviewboard.mozilla.org/r/91858/#review91894

With this WIP patch, Fennec is able to play "OOPS-MultiDrm" & "TearsOfSteel(Widevine)" in both in-process-decode / oop-decode modes.

For thoes platforms where Android API version is lower than Mashmallow, a specific seesion Id which is obtained with dummykey is usded to unblock the waiting-for-key samples.

I'll start seperate this patch into parts for review !  Also need to verify the code on Mashmallow. (We were focusing on Lollipop at first)

James Cheng[:JamesCheng]

Comment 4

•

8 years ago

mozreview-review

Comment on attachment 8809268 [details]
Bug 1307818-[P1] Provide drmStubId for CDMProxy and instantiate {Local,Remote}MediaDrmBridge.

https://reviewboard.mozilla.org/r/91858/#review92632

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:254
(Diff revision 2)
>  
>      @WrapForJNI(calledFrom = "gecko")
>      public static MediaDrmProxy create(String keySystem,
>                                         Callbacks nativeCallbacks,
>                                         boolean isRemote) {
> -        // TODO: Will implement {Local,Remote}MediaDrmBridge instantiation by
> +        String drmStubId = UUID.randomUUID().toString();

Can we move the uuid generation logic to line 272 directly? It ssems it is no need to pass this parameter.

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:273
(Diff revision 2)
> +            } else {
> +                mImpl = new LocalMediaDrmBridge(keySystem);
> +            }
> -        mImpl.setCallbacks(new MediaDrmProxyCallbacks(this, nativeCallbacks));
> +            mImpl.setCallbacks(new MediaDrmProxyCallbacks(this, nativeCallbacks));
> +            mDrmStubId = stubId;
> -        mProxyList.add(this);
> +            mProxyList.add(this);

I think we should modify the prefix "m" to "s" for static member[1] and [2] for better readability. Do you think we can do it in this bug?

[1] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java#45
[2] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/mobile/android/base/java/org/mozilla/gecko/media/RemoteMediaDrmBridgeStub.java#31

Kilik Kuo [:kikuo] (inactive)

Assignee

Comment 5

•

8 years ago

mozreview-review-reply

Comment on attachment 8809268 [details]
Bug 1307818-[P1] Provide drmStubId for CDMProxy and instantiate {Local,Remote}MediaDrmBridge.

https://reviewboard.mozilla.org/r/91858/#review92632

> Can we move the uuid generation logic to line 272 directly? It ssems it is no need to pass this parameter.

Sadly we cannot do that. "drmStubId" is delivered to another process while creating IMediaDrmBridge instance @Line257.

> I think we should modify the prefix "m" to "s" for static member[1] and [2] for better readability. Do you think we can do it in this bug?
> 
> [1] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java#45
> [2] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/mobile/android/base/java/org/mozilla/gecko/media/RemoteMediaDrmBridgeStub.java#31

SGTM, though to keep the goal of this bug simple, I'd prefered that being fixed in another bug.
Thanks for the feedback.

Kilik Kuo [:kikuo] (inactive)

Assignee

Updated

•

8 years ago

Blocks: 1317628

Kilik Kuo [:kikuo] (inactive)

Assignee

Updated

•

8 years ago

Assignee: nobody → kikuo

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

Comment on attachment 8809268 [details]
Bug 1307818-[P1] Provide drmStubId for CDMProxy and instantiate {Local,Remote}MediaDrmBridge.

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

Kilik Kuo [:kikuo] (inactive)

Assignee

Updated

•

8 years ago

Attachment #8809268 - Flags: review?(nchen)

Attachment #8809268 - Flags: review?(cpearce)

Attachment #8810783 - Flags: review?(nchen)

Attachment #8810783 - Flags: review?(cpearce)

Attachment #8810784 - Flags: review?(nchen)

Chris Pearce [:cpearce (Not reading bugmail)]

Comment 9

•

8 years ago

mozreview-review

Comment on attachment 8809268 [details]
Bug 1307818-[P1] Provide drmStubId for CDMProxy and instantiate {Local,Remote}MediaDrmBridge.

https://reviewboard.mozilla.org/r/91858/#review93314

::: dom/media/eme/mediadrm/MediaDrmProxySupport.cpp:210
(Diff revision 3)
>                            mJavaCallbacks,
>                            MediaPrefs::PDMAndroidRemoteCodecEnabled());
> +
> +  MOZ_ASSERT(mBridgeProxy, "mBridgeProxy should not be null");
> +  auto drmStubId = mBridgeProxy->GetStubId();
> +  mMediaDrmStubId = drmStubId->ToString();

Presumably this could just be:
mMediaDrmStubId = mBridgeProxy->GetStubId()->ToString()?

Then you don't have an unnecessary temporary.

Attachment #8809268 - Flags: review?(cpearce) → review+

Chris Pearce [:cpearce (Not reading bugmail)]

Comment 10

•

8 years ago

mozreview-review

Comment on attachment 8810783 [details]
Bug 1307818-[P2] Setup MediaCrypto for both in-process and out-of-process decode.

https://reviewboard.mozilla.org/r/93118/#review93344

Attachment #8810783 - Flags: review?(cpearce) → review+

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

Comment 11

•

8 years ago

mozreview-review

Comment on attachment 8809268 [details]
Bug 1307818-[P1] Provide drmStubId for CDMProxy and instantiate {Local,Remote}MediaDrmBridge.

https://reviewboard.mozilla.org/r/91858/#review93372

::: dom/media/eme/mediadrm/MediaDrmProxySupport.h:57
(Diff revision 3)
>    void CloseSession(uint32_t aPromiseId,
>                      const nsCString& aSessionId);
>  
>    void Shutdown();
>  
> +  const nsString& GetMediaDrmStubId() { return mMediaDrmStubId; }

`const nsString& GetMediaDrmStubId() const { return mMediaDrmStubId; }`

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:45
(Diff revision 3)
>      private static final String OPUS = "audio/opus";
>  
>      // A flag to avoid using the native object that has been destroyed.
>      private boolean mDestroyed;
>      private GeckoMediaDrm mImpl;
> -    public static ArrayList<MediaDrmProxy> mProxyList = new ArrayList<MediaDrmProxy>();
> +    public static ArrayList<MediaDrmProxy> sProxyList = new ArrayList<MediaDrmProxy>();

Move this to its own block to separate static members from non-static members

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:300
(Diff revision 3)
>      private void closeSession(int promiseId, String sessionId) {
>          if (DEBUG) Log.d(LOGTAG, "closeSession, primiseId(" + promiseId  + "sessionId(" + sessionId + ")");
>          mImpl.closeSession(promiseId, sessionId);
>      }
>  
> +    @WrapForJNI

`@WrapForJNI(calledFrom = "gecko")`?

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:306
(Diff revision 3)
> +    private String getStubId() {
> +        return mDrmStubId;
> +    }
> +
> +    // Get corresponding MediaCrypto object by a generated UUID for MediaCodec.
> +    @WrapForJNI

Same?

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:309
(Diff revision 3)
> +
> +    // Get corresponding MediaCrypto object by a generated UUID for MediaCodec.
> +    @WrapForJNI
> +    public static MediaCrypto getMediaCrypto(String stubId) {
> +        for (int i = 0; i < sProxyList.size(); i++) {
> +            if (sProxyList.get(i) != null &&

Only call `get(i)` once:

    final MediaDrmProxy proxy = sProxyList.get(i);

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:314
(Diff revision 3)
> +            if (sProxyList.get(i) != null &&
> +                sProxyList.get(i).getStubId().equals(stubId)) {
> +                return sProxyList.get(i).getMediaCryptoFromBridge();
> +            }
> +        }
> +        Log.d(LOGTAG, " NULL crytpo ");

`if (DEBUG)`

Attachment #8809268 - Flags: review?(nchen) → review+

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

Comment 12

•

8 years ago

mozreview-review

Comment on attachment 8810783 [details]
Bug 1307818-[P2] Setup MediaCrypto for both in-process and out-of-process decode.

https://reviewboard.mozilla.org/r/93118/#review93374

::: dom/media/platforms/android/MediaCodecDataDecoder.cpp:383
(Diff revision 1)
>                      MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__));
>      return NS_ERROR_FAILURE;
>    }
>  
> +  MediaCrypto::LocalRef crypto = MediaDrmProxy::GetMediaCrypto(mDrmStubId);
> +  bool hascrypto = crypto != nullptr;

`bool hasCrypto = !!crypto;`

Attachment #8810783 - Flags: review?(nchen) → review+

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

Comment 13

•

8 years ago

mozreview-review

Comment on attachment 8810784 [details]
Bug 1307818-[P3] Notify correct batched-keystatuses-changed information in API V23+.

https://reviewboard.mozilla.org/r/93120/#review93378

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:31
(Diff revision 1)
>  import android.media.MediaDrm;
>  import android.media.MediaDrmException;
>  import android.util.Log;
>  
>  public class GeckoMediaDrmBridgeV21 implements GeckoMediaDrm {
> -    private static final String LOGTAG = "GeckoMediaDrmBridgeV21";
> +    protected String LOGTAG = "GeckoMediaDrmBridgeV21";

Make `LOGTAG` protected final String, and set it to `getClass().getSimpleName()` in the constructor. This will automatically work for `GeckoMediaDrmBridgeV23` as well

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV23.java:45
(Diff revision 1)
>                                                   keyStatus.getStatusCode());
>              }
>              onSessionBatchedKeyChanged(sessionId, keyInfos);
> +            if (DEBUG) {
> +              String sId = new String(sessionId);
> +              Log.d(LOGTAG, "Key successfully added for session " + sId);

`Log.d(LOGTAG, "Key successfully added for session " + new String(sessionId));`

::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV23.java:77
(Diff revision 1)
> +                    Log.d(LOGTAG, "InfoMap : key(" + strKey + ")/value(" + strValue + ")");
> -        }
> +                }
> -    }
> +            }
> +            onSessionUpdated(promiseId, session.array());
> +            return;
> +        } catch (android.media.NotProvisionedException e) {

1. import `android.media.NotProvisionedException` and `android.media.DeniedByServerException`

2. Catch all three exceptions in one block, e.g.

`} catch (final NotProvisionedException | DeniedByServerException | IllegalStateException e) {`

3. Pass the exception to `Log.d`, and use the same message for all exceptions, e.g.

`Log.d(LOGTAG, "message", e);`

Attachment #8810784 - Flags: review?(nchen) → review+

James Cheng[:JamesCheng]

Comment 14

•

8 years ago

mozreview-review

Comment on attachment 8809268 [details]
Bug 1307818-[P1] Provide drmStubId for CDMProxy and instantiate {Local,Remote}MediaDrmBridge.

https://reviewboard.mozilla.org/r/91858/#review93386

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:309
(Diff revision 3)
> +
> +    // Get corresponding MediaCrypto object by a generated UUID for MediaCodec.
> +    @WrapForJNI
> +    public static MediaCrypto getMediaCrypto(String stubId) {
> +        for (int i = 0; i < sProxyList.size(); i++) {
> +            if (sProxyList.get(i) != null &&

We can used range-based for instead.
```for( MediaDrmProxy proxy : sProxyList)  ```

Comment hidden (mozreview-request)

Comment on attachment 8809268 [details]
Bug 1307818-[P1] Provide drmStubId for CDMProxy and instantiate {Local,Remote}MediaDrmBridge.

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

Comment hidden (mozreview-request)

Comment on attachment 8810783 [details]
Bug 1307818-[P2] Setup MediaCrypto for both in-process and out-of-process decode.

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

Comment hidden (mozreview-request)

Comment on attachment 8810784 [details]
Bug 1307818-[P3] Notify correct batched-keystatuses-changed information in API V23+.

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

Kilik Kuo [:kikuo] (inactive)

Assignee

Comment 18

•

8 years ago

mozreview-review

Comment on attachment 8809268 [details]
Bug 1307818-[P1] Provide drmStubId for CDMProxy and instantiate {Local,Remote}MediaDrmBridge.

https://reviewboard.mozilla.org/r/91856/#review93472

Chris, Jim, thanks for review !

Kilik Kuo [:kikuo] (inactive)

Assignee

Comment 19

•

8 years ago

Try Looks ok !
https://treeherder.mozilla.org/#/jobs?repo=try&revision=67da7164b3ca

Keywords: checkin-needed

Pulsebot

Comment 20

•

8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/45230682e9c8
[P1] Provide drmStubId for CDMProxy and instantiate {Local,Remote}MediaDrmBridge. r=cpearce,jchen
https://hg.mozilla.org/integration/autoland/rev/f563a0c89ac1
[P2] Setup MediaCrypto for both in-process and out-of-process decode. r=cpearce,jchen
https://hg.mozilla.org/integration/autoland/rev/45be5abea7c0
[P3] Notify correct batched-keystatuses-changed information in API V23+. r=jchen

Keywords: checkin-needed

Phil Ringnalda (:philor)

Comment 21

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/45230682e9c8
https://hg.mozilla.org/mozilla-central/rev/f563a0c89ac1
https://hg.mozilla.org/mozilla-central/rev/45be5abea7c0

Status: NEW → RESOLVED

Closed: 8 years ago

status-firefox53: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 53

areya

Comment 22

•

8 years ago

https://hg.mozilla.org/mozilla-central/rev/45230682e9c8
https://hg.mozilla.org/mozilla-central/rev/f563a0c89ac1
https://hg.mozilla.org/mozilla-central/rev/45be5abea7c0

Jean-Yves Avenard [:jya]

Updated

•

6 years ago

Depends on: 1464822

BMO Automation

Updated

•

3 years ago

Product: Firefox for Android → Firefox for Android Graveyard

You need to log in before you can comment on or make changes to this bug.