Closed Bug 1306219 Opened 8 years ago Closed 8 years ago

[EME][Fennec] MediaKeySystem Management for Widevine MediaDrm

Categories

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

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Type:
defect
Priority:
Not set
Severity:
normal

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 1306219-Part1 Rename GMP_* into EME_*
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1306219-Part2 Add utility function to invoke MediaDrm and MediaCrypto checking API.
58 bytes, text/x-review-board-request
cpearce
: review+
jchen
: review+
Details
Bug 1306219-Part3 Add logic to determine if Widevine keysystem is supported on Fennec.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Attachment #8796477 - Flags: review?(cpearce)
Attachment #8796478 - Flags: review?(nchen)
Attachment #8796478 - Flags: review?(cpearce)
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)
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.
Blocks: 1306572
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 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 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 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+
No longer blocks: 1306572
Blocks: 1306572
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 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+
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 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 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)
Attachment #8797346 - Flags: review?(cpearce)
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 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+
Hi Chris,

Thanks for your reviewing.

I tried to addressed comment 23 and comment 24,

Please review it again.

Thank you very much.
Hi Chris,

Thanks for your reviewing.

I tried to address comment 23 and comment 24,

Please review it again.

Thank you very much.
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+
Addressed comment 30 and rebase the patch,
Attach Try link and hope it works fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6b2d759f12eb07d8fad071ae0cfdbedb0dc76c6
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
Done. Thank you
Keywords: checkin-needed
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
No longer blocks: 1306185
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: