[EME][Fennec] MediaKeySystem Management for Widevine MediaDrm

RESOLVED FIXED in Firefox 52

Status

()

Firefox for Android
Audio/Video
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Tracking

unspecified
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

a year ago
We will implement the logic to check if the Widevine DRM supported on Fennec by using the Android  MediaDrm & MediaCrypto API [1] & [2].

Based on [3]

[1]
https://developer.android.com/reference/android/media/MediaDrm.html#isCryptoSchemeSupported(java.util.UUID)

[2]
https://developer.android.com/reference/android/media/MediaCrypto.html#isCryptoSchemeSupported(java.util.UUID)

[3]
https://github.com/JamesWCCheng/gecko-dev/commit/bd4363b56a2ebf43e7d82e8f65efd0991d9cf10d

Updated

a year ago
Blocks: 1306185
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8796477 - Flags: review?(cpearce)
Attachment #8796478 - Flags: review?(nchen)
Attachment #8796478 - Flags: review?(cpearce)
(Assignee)

Comment 3

a year ago
Hi Chris,

We will start to separate our work on Fennec into several bugs for incrementally landing.

Please help for review the first one which is intended to check if the widevine is supported on the end-user's device by using [1] & [2] API(in the Part2).

Currently, we do not enable EME pref in mobile.js. We plan to enable it once we finished our widevine work entirely.


Thank you very much.


[1]
https://developer.android.com/reference/android/media/MediaDrm.html#isCryptoSchemeSupported(java.util.UUID)

[2]
https://developer.android.com/reference/android/media/MediaCrypto.html#isCryptoSchemeSupported(java.util.UUID)
(Assignee)

Comment 4

a year ago
Hi Jim,

We would like your help to review the JNI part. We want to expose the managed code APIs for native code usage.

Thanks.
(Assignee)

Updated

a year ago
Blocks: 1306572
(Assignee)

Comment 5

a year ago
mozreview-review
Comment on attachment 8796477 [details]
Bug 1306219-Part1 Rename GMP_* into EME_*

https://reviewboard.mozilla.org/r/82324/#review81138

::: dom/media/eme/MediaKeySystemAccess.cpp:483
(Diff revision 1)
>        widevine.mPersistentState = KeySystemFeatureSupport::Requestable;
>        widevine.mDistinctiveIdentifier = KeySystemFeatureSupport::Prohibited;
>        widevine.mSessionTypes.AppendElement(MediaKeySessionType::Temporary);
> +      // [TODO] Check if any other attributes on Fennec are required
> +#ifdef MOZ_WIDGET_ANDROID
> +      widevine.mSessionTypes.AppendElement(MediaKeySessionType::Persistent_license);

I will get the error when entering shaka demo website with [1]. So I add this line for pass the requirement check.

I'm curious why Desktop widevine did not encounter these kind of error...


[1]
http://searchfox.org/mozilla-central/rev/d1276b5b84e6cf7991c8e640b5e0ffffd54575a6/dom/media/eme/MediaKeySystemAccess.cpp#983

Comment 6

a year ago
mozreview-review
Comment on attachment 8796477 [details]
Bug 1306219-Part1 Rename GMP_* into EME_*

https://reviewboard.mozilla.org/r/82324/#review81186

I think we can make this more platform independent and cleaner.

::: dom/media/eme/EMEUtils.cpp:160
(Diff revision 1)
>  }
>  
> +bool
> +IsUsingMediaDrm(const nsAString& aKeySystem)
> +{
> +  return !CompareUTF8toUTF16(kEMEKeySystemWidevine, aKeySystem) ?

If you need to keep this, we can simplify it:

#ifndef MOZ_WIDGET_ANDROID
  return false;
#else
  !CompareUTF8toUTF16(kEMEKeySystemWidevine, aKeySystem);
#endif

::: dom/media/eme/MediaKeySystemAccess.cpp:483
(Diff revision 1)
>        widevine.mPersistentState = KeySystemFeatureSupport::Requestable;
>        widevine.mDistinctiveIdentifier = KeySystemFeatureSupport::Prohibited;
>        widevine.mSessionTypes.AppendElement(MediaKeySessionType::Temporary);
> +      // [TODO] Check if any other attributes on Fennec are required
> +#ifdef MOZ_WIDGET_ANDROID
> +      widevine.mSessionTypes.AppendElement(MediaKeySessionType::Persistent_license);

The desktop CDM doesn't support persistent licenses apparently.

::: dom/media/eme/MediaKeySystemAccess.cpp:1200
(Diff revision 1)
>    }
>    const KeySystemConfig* implementation = nullptr;
> -  if (!HaveGMPFor(mps,
> +
> +  if (!(implementation = GetKeySystemConfig(aKeySystem)) ||
> +      (!IsUsingMediaDrm(aKeySystem) &&
> +       !HaveGMPFor(mps,

I'd like to make the logic here as independent of the platform we're running on as possible.

So I'm not keen on your changes, as we have to add a bunch of if(gmp) {...} else if (MediaDRM){...} type blocks.

I think we can make things a lot simpler if we recalculate the list of keysystems returned by GetKeySystemConfig() every time its called (i.e. don't store the result in a static, recalculate a new one every time and return that).

We need to recalculate it, as it can change at runtime if a CDM downloads during runtime.

If make GetKeySystemConfig() recalculate the list of KeySystemConfig every time, we can do the HaveGMPFor() and MediaDrmProxy::IsSchemeMIMESupported() checks inside GetKeySystemConfig(), and only add the keysystem for codecs/contains for which it's supported. Then we don't need to post-validate the KeySystemConfig with extra HaveGMPFor()/MediaDrmProxy::IsSchemeMIMESupported() checks. This should make the logic that follows much simpler.

Then I think you can eliminate the existing HaveGMPFor() checks through the code.
Attachment #8796477 - Flags: review?(cpearce) → review-

Comment 7

a year ago
mozreview-review
Comment on attachment 8796477 [details]
Bug 1306219-Part1 Rename GMP_* into EME_*

https://reviewboard.mozilla.org/r/82324/#review81188

::: dom/media/eme/MediaKeySystemAccess.cpp:494
(Diff revision 1)
>        // decode AAC, and a codec wasn't specified, be conservative
>        // and reject the MediaKeys request, since our policy is to prevent
>        //  the Adobe GMP's unencrypted AAC decoding path being used to
>        // decode content decrypted by the Widevine CDM.
>        if (WMFDecoderModule::HasAAC()) {
>          widevine.mMP4.SetCanDecrypt(GMP_CODEC_AAC);

You should only add the GMP_CODEC_AAC if MediaDrmProxy::IsSchemeMIMESupported() returns true for the appropriate mime type string. And the same for the other codecs, including the WebM codecs.

That is, you should be checking that the MediaDRM framework supports the codecs you're saying the CDM supports here.

You may want to rename GMP_CODEC_* to EME_CODEC_*, so that it doesn't seem GMP specific.

Comment 8

a year ago
mozreview-review
Comment on attachment 8796478 [details]
Bug 1306219-Part2 Add utility function to invoke MediaDrm and MediaCrypto checking API.

https://reviewboard.mozilla.org/r/82326/#review81190

The high level interface looks fine, but I don't know much about how the JNI is supposed to work, so I'll rely on jchen or someone else to be reviewing that aspect.
Attachment #8796478 - Flags: review?(cpearce) → review+
(Assignee)

Updated

a year ago
No longer blocks: 1306572
(Assignee)

Updated

a year ago
Blocks: 1306572
(Assignee)

Comment 9

a year ago
Thanks to Chris for the information you provided, I need to some time to absorb your word.

I will provide the renaming GMP* to EME* patch first.

Thank you!

Comment 10

a year ago
mozreview-review
Comment on attachment 8796478 [details]
Bug 1306219-Part2 Add utility function to invoke MediaDrm and MediaCrypto checking API.

https://reviewboard.mozilla.org/r/82326/#review81432

r+ after fixing these nits.

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:12
(Diff revision 1)
> +
> +import java.util.UUID;
> +
> +import org.mozilla.gecko.annotation.RobocopTarget;
> +import org.mozilla.gecko.annotation.WrapForJNI;
> +import org.mozilla.gecko.GeckoAppShell;

nit: unused imports RobocopTarget and GeckoAppShell

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:14
(Diff revision 1)
> +import static android.os.Build.VERSION.SDK_INT;
> +import static android.os.Build.VERSION_CODES.JELLY_BEAN_MR2;
> +import static android.os.Build.VERSION_CODES.LOLLIPOP;
> +import static android.os.Build.VERSION_CODES.M;

Instead of these constants, import `org.mozilla.gecko.AppConstants`, and use `AppConstants.Versions`, e.g. `AppConstants.Versions.preLollipop`

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:24
(Diff revision 1)
> +import android.media.MediaCrypto;
> +import android.media.MediaDrm;
> +import android.util.Log;
> +
> +public final class MediaDrmProxy {
> +    private static final String LOGTAG = "MediaDrmProxy";

nit: "GeckoMediaDrmProxy"

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:31
(Diff revision 1)
> +    private static final UUID WIDEVINE_SCHEME_UUID =
> +            new UUID(0xedef8ba979d64aceL, 0xa3c827dcd51d21edL);
> +
> +    private static final String WIDEVINE_KEY_SYSTEM = "com.widevine.alpha";
> +
> +    static public boolean passMiniumSupportVersionCheck() {

Change it to `private static boolean isSystemSupported()`

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:41
(Diff revision 1)
> +        }
> +        return true;
> +    }
> +
> +    @WrapForJNI
> +    static public boolean isSchemeSupported(String keySystem) {

nit: put `public` before `static`
Attachment #8796478 - Flags: review?(nchen) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

a year ago
Thanks for Jim's reviewed, addressed the nits.

Hi Chris,

Please help to review the Part 1 patch which only rename the GMP_* into EME_* and I will provide part 3  soon.

Thank you very much.

Comment 15

a year ago
mozreview-review
Comment on attachment 8796477 [details]
Bug 1306219-Part1 Rename GMP_* into EME_*

https://reviewboard.mozilla.org/r/82324/#review81598
Attachment #8796477 - Flags: review?(cpearce) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

a year ago
Comment on attachment 8796478 [details]
Bug 1306219-Part2 Add utility function to invoke MediaDrm and MediaCrypto checking API.

Hi Chris,

Please review this patch again since I modified the Api. It is the way to check "can decrypt and decode" on Android.

Thank you.
Attachment #8796478 - Flags: review+ → review?(cpearce)
(Assignee)

Updated

a year ago
Attachment #8797346 - Flags: review?(cpearce)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 23

a year ago
mozreview-review
Comment on attachment 8797346 [details]
Bug 1306219-Part3 Add logic to determine if Widevine keysystem is supported on Fennec.

https://reviewboard.mozilla.org/r/82944/#review86340

Sorry for the slow turn-around. Have been busy getting Web Platform Tests to pass. Just a few simple changes needed.

::: dom/media/eme/MediaKeySystemAccess.cpp:321
(Diff revision 3)
> +#ifdef MOZ_WIDGET_ANDROID
> +    } else if (Preferences::GetBool("media.mediadrm-widevinecdm.visible", false)) {
> +        nsCString keySystem = NS_ConvertUTF16toUTF8(aKeySystem);
> +        bool supported = mozilla::java::MediaDrmProxy::IsSchemeSupported(keySystem);
> +        if (!supported) {
> +          aOutMessage = NS_LITERAL_CSTRING("CDM is not existing");

"Widevine CDM is not available"

(It's grammatically incorrect to say "is not existing").

::: dom/media/eme/MediaKeySystemAccess.cpp:440
(Diff revision 3)
>    if (!sKeySystemConfigs) {
>      sKeySystemConfigs = new nsTArray<KeySystemConfig>();
>      ClearOnShutdown(&sKeySystemConfigs);
> +  }
> +  // Recalculate the list.
> +  sKeySystemConfigs->Clear();

This is a bad API design. You're changing the static list of KeySystemConfigs; that means something else could have a reference to this list, but it would change unexpectedly to that caller.

Please remove sKeySystemConfigs, and create a new instance of that list and populate that and return that every time this function is called.

::: dom/media/eme/MediaKeySystemAccess.cpp:498
(Diff revision 3)
> -      // decode content decrypted by the Widevine CDM.
> +    // decode content decrypted by the Widevine CDM.
> -      if (WMFDecoderModule::HasAAC()) {
> +    if (WMFDecoderModule::HasAAC()) {
> -        widevine.mMP4.SetCanDecrypt(EME_CODEC_AAC);
> +      widevine.mMP4.SetCanDecrypt(EME_CODEC_AAC);
> -      }
> +    }
>  #else
> -      widevine.mMP4.SetCanDecrypt(EME_CODEC_AAC);
> +    widevine.mMP4.SetCanDecrypt(EME_CODEC_AAC);

We'll be adding EME_CODEC_AAC here on android even if MediaDrmProxy::CanDecrypt() reports that we can't play MP4. And if MediaDrmProxy::CanDecrypt() reports that we can play AAC, you'll end up adding it twice.

::: dom/media/eme/MediaKeySystemAccess.cpp:507
(Diff revision 3)
> +    using namespace mozilla::java;
> +    // MediaDrm.isCryptoSchemeSupported only allows passing
> +    // "video/mp4" or "video/webm" for mimetype string.
> +    // See https://developer.android.com/reference/android/media/MediaDrm.html#isCryptoSchemeSupported(java.util.UUID, java.lang.String)
> +    // for more detail.
> +    if (MediaDrmProxy::CanDecrypt(kEMEKeySystemWidevine,

Optional: you might be able to create a static array of:
struct {
  mimeType, MediaDRMProxy::CodecType, EME_CODEC_TYPE
}

and then write all these lines as a loop.

That would likely result in easier to maintain and understand code. Because you'd be writing the loop once, it is less likely that the cases would have errors in them.

::: dom/media/eme/MediaKeySystemAccess.cpp:586
(Diff revision 3)
> +  bool havePlugin = false;
> +  nsCOMPtr<mozIGeckoMediaPluginService> mps =
> +    do_GetService("@mozilla.org/gecko-media-plugin-service;1");
> +  if (mps) {
> +    havePlugin = HaveGMPFor(mps,
> +                    NS_ConvertUTF16toUTF8(aKeySystem),

Indentation doesn't line up with '('.

::: dom/media/eme/MediaKeySystemAccess.cpp:602
(Diff revision 3)
> +}
> +
>  static const KeySystemConfig*
>  GetKeySystemConfig(const nsAString& aKeySystem)
>  {
>    for (const KeySystemConfig& config : GetSupportedKeySystems()) {

Once you make GetSupportedKeySystems() return a non-static, you should cache its return value here.

i.e.

nsTArray<KeySystemConfigs> configs = GetSupportedKeySystems();
for (const KeySystemConfig& config : configs) {
  // ..
}

::: dom/media/eme/MediaKeySystemAccess.cpp:604
(Diff revision 3)
>  static const KeySystemConfig*
>  GetKeySystemConfig(const nsAString& aKeySystem)
>  {
>    for (const KeySystemConfig& config : GetSupportedKeySystems()) {
>      if (config.mKeySystem.Equals(aKeySystem)) {
> +      if (!HavePluginForKeySystem(aKeySystem)) {

Check HavePluginForKeySystem() in GetSupportedKeySystems() so you don't have to do it again here.

That is, make GetSupportedKeySystems() return an array which contains only KeySystemConfigs that have already passed the HavePluginForKeySystem() check.
Attachment #8797346 - Flags: review?(cpearce) → review-

Comment 24

a year ago
mozreview-review
Comment on attachment 8796478 [details]
Bug 1306219-Part2 Add utility function to invoke MediaDrm and MediaCrypto checking API.

https://reviewboard.mozilla.org/r/82326/#review86398

::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:63
(Diff revisions 2 - 4)
> -        log("isSchemeSupported key sytem = " + keySystem);
> +        if (DEBUG) Log.d(LOGTAG, "isSchemeSupported key sytem = " + keySystem);
>          return false;
>      }
>  
>      @WrapForJNI
> -    public static boolean isSchemeMIMESupported(String keySystem, String mimeType) {
> +    public static boolean CanDecrypt(String keySystem, String container) {

I think it's more accurate to call this IsCryptoSchemeSupported(), as what this is actually testing is whether the device can negotiate licenses for CENC/WebM initData, not whether it can decrypt MP4/WebM streams. It doesn't touch the streams; our demuxer does that.
Attachment #8796478 - Flags: review?(cpearce) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 28

a year ago
Hi Chris,

Thanks for your reviewing.

I tried to addressed comment 23 and comment 24,

Please review it again.

Thank you very much.
(Assignee)

Comment 29

a year ago
Hi Chris,

Thanks for your reviewing.

I tried to address comment 23 and comment 24,

Please review it again.

Thank you very much.

Comment 30

a year ago
mozreview-review
Comment on attachment 8797346 [details]
Bug 1306219-Part3 Add logic to determine if Widevine keysystem is supported on Fennec.

https://reviewboard.mozilla.org/r/82944/#review87238

::: dom/media/eme/MediaKeySystemAccess.cpp:458
(Diff revision 4)
> -    sKeySystemConfigs = new nsTArray<KeySystemConfig>();
> -    ClearOnShutdown(&sKeySystemConfigs);
>  
> -    {
> +  {
> +    auto keySystem = NS_ConvertUTF8toUTF16(kEMEKeySystemClearkey);
> +    if (HavePluginForKeySystem(keySystem)) {

Every time you call HavePluginForKeySystem() you convert a UTF8 string to UTF16. Then HavePluginForKeySystem() converts that string back to UTF8. Why don't you just pass a UTF8 string in the first place?

::: dom/media/eme/MediaKeySystemAccess.cpp:532
(Diff revision 4)
> +        const char* const mMimeType;
> +        const nsCString& mEMECodecType;
> +        const char16_t* mCodecType;
> +        KeySystemContainerSupport* mSupportType;
> +      } DataForValidation;
> +      static DataForValidation ValidationList[] = {

Why can't you initialize DataForValidation::mSupportType in the here too? Because ValidationList is static?

::: dom/media/eme/MediaKeySystemAccess.cpp:549
(Diff revision 4)
> +      ValidationList[4].mSupportType = &widevine.mWebM;
> +      ValidationList[5].mSupportType = &widevine.mWebM;
> +
> +      for (const auto& data: ValidationList) {
> +        if (MediaDrmProxy::IsCryptoSchemeSupported(kEMEKeySystemWidevine,
> +                                                   nsCString(data.mMimeType))) {

Maybe DataForValidation::mMimeType can be an nsCString, so you don't need to construct one here? You might need to make ValidationList non-static for that, else the nsCString may be reported as leaking.
Attachment #8797346 - Flags: review?(cpearce) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 34

a year ago
Addressed comment 30 and rebase the patch,
Attach Try link and hope it works fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6b2d759f12eb07d8fad071ae0cfdbedb0dc76c6
(Assignee)

Comment 35

a year ago
Try WPT Test:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f511f52f2ae8&selectedJob=29916118

Seems Try result is OK, set it to check-in-needed
Keywords: checkin-needed
Please mark the pending issues as resolved in MozReview so that this can autoland.
Keywords: checkin-needed
(Assignee)

Comment 37

a year ago
Done. Thank you
Keywords: checkin-needed

Comment 38

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/89cf1a59692a
Part2 Add utility function to invoke MediaDrm and MediaCrypto checking API. r=cpearce,jchen
https://hg.mozilla.org/integration/autoland/rev/9ae64306a553
Part1 Rename GMP_* into EME_* r=cpearce
https://hg.mozilla.org/integration/autoland/rev/660be9ffa179
Part3 Add logic to determine if Widevine keysystem is supported on Fennec. r=cpearce
Keywords: checkin-needed

Updated

a year ago
No longer blocks: 1306185

Comment 39

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/89cf1a59692a
https://hg.mozilla.org/mozilla-central/rev/9ae64306a553
https://hg.mozilla.org/mozilla-central/rev/660be9ffa179
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.