Make GMPParent::ParseChromiumManifest() not assume Widevine

RESOLVED FIXED in Firefox 53

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: cpearce, Assigned: jay.harris)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 months ago
Currently GMPParent::ParseChromiumManifest() assumes that it's only called for the Widevine CDM. But once the ClearKey CDM switches to using the Chromium CDM API, I think it also makes sense for us to have ClearKey using the Chromium manifest.json format for ClearKey so that we have test coverage manifest.json of the json parser.

So we need to ensure that GMPParent::ParseChromiumManifest() doesn't assert mName.EqualsLiteral("widevinecdm"), and we need to parse the x-cdm-codecs field and assign the appropriate GMPCapabilities for the codec support listed in the x-cdm-codecs field.

This is because our ClearKey CDM supports decoding of H.264 only on Windows, and we don't yet support decoding with VP8 or VP9 in ClearKey yet.

We also need to ensure we don't assume that the decryption always uses the Widevine keysystem string the (GMP_API_DECRYPTOR capability).

Note a typical manifest.json for Widevine looks like this:

{
    "arch": "ia32",
    "description": "Widevine Content Decryption Module",
    "manifest_version": 2,
    "name": "WidevineCdm",
    "offline_enabled": false,
    "os": "win",
    "version": "1.4.8.903",
    "x-cdm-codecs": "vp8,vp9.0,avc1",
    "x-cdm-host-versions": "8",
    "x-cdm-interface-versions": "8",
    "x-cdm-module-versions": "4"
}

The goal here to make sure that we can have a similar manifest.json for ClearKey that works.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8825219 - Attachment is obsolete: true
Attachment #8825219 - Flags: review?(cpearce)
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Assignee: nobody → jharris
(Reporter)

Comment 4

6 months ago
mozreview-review
Comment on attachment 8825235 [details]
Bug 1319159 - Updates how 'ParseChromiumManifest' works so that it doesn't assume Widevine (now it assumes Widevine or Clearkey..).

https://reviewboard.mozilla.org/r/103412/#review104058

::: dom/media/gmp/GMPParent.cpp:963
(Diff revision 1)
>    mDescription = NS_ConvertUTF16toUTF8(m.mDescription);
>    mVersion = NS_ConvertUTF16toUTF8(m.mVersion);
>  
> +  // We have two booleans, because it could concievably be a third, unsupported
> +  // CDM.
> +  bool isClearkey = mDisplayName.EqualsASCII("clearkey");

I wouldn't bother storing these in booleans. I would just go:

if (mDisplayName.EqualsASCII("clearkey")) {
  kEMEKeySystem = ...
#ifdef XP_WIN
  mLibs = ...
#endif
} else if (mDisplayName.EqualsASCII("WidevineCdm")) {
  kEMEKeySystem = ...
  mLibs = ...
}

That should make the code simpler, and means you don't need to store thing unnecessarily.

In general, it's better to not store what you can calculate, as then you need to worry about the calculated value expiring.

::: dom/media/gmp/GMPParent.cpp:967
(Diff revision 1)
> +  // CDM.
> +  bool isClearkey = mDisplayName.EqualsASCII("clearkey");
> +  bool isWidevine = mDisplayName.EqualsASCII("WidevineCdm");
> +
> +  // Assert that it is one, so it breaks here if we're adding more CDMs.
> +  MOZ_ASSERT(isClearkey || isWidevine);

We may as well reject the promise if !isClearKey && !isWidevine. That way we only let through what we want.

::: dom/media/gmp/GMPParent.cpp:976
(Diff revision 1)
> +
> +  // We hard code a few of the settings because they can't be stored in the
> +  // widevine manifest without making our API different to widevine's.
> +  if (isClearkey) {
> +    kEMEKeySystem = kEMEKeySystemClearkey;
> +    libs = NS_LITERAL_CSTRING(

The code to initialize libs should be inside #ifdef XP_WIN blocks.

In fact, I would just assign to mLibs directly here, and thus save storing a copy until assigning later on.

::: dom/media/gmp/GMPParent.cpp:977
(Diff revision 1)
> +  // We hard code a few of the settings because they can't be stored in the
> +  // widevine manifest without making our API different to widevine's.
> +  if (isClearkey) {
> +    kEMEKeySystem = kEMEKeySystemClearkey;
> +    libs = NS_LITERAL_CSTRING(
> +         "dxva2.dll, d3d9.dll, msmpeg2vdec.dll, msmpeg2adec.dll, MSAudDecMFT.dll, evr.dll, mfheaacdec.dll, mfh264dec.dll, mfplat.dll");

You don't need the audio decoding DLLs anymore, since we've removed that.

And I don't think we need d3d9.dll anymore either. 

So I think this can be:

dxva2.dll, msmpeg2vdec.dll, evr.dll, mfh264dec.dll, mfplat.dll

Can you also (in a separate changeset) update the whitelist in GMPChild::RecvPreloadLibs() to match this list.

::: dom/media/gmp/GMPParent.cpp:987
(Diff revision 1)
> +    libs = NS_LITERAL_CSTRING("dxva2.dll");
> +  }
> +
>    GMPCapability video(NS_LITERAL_CSTRING(GMP_API_VIDEO_DECODER));
> -  video.mAPITags.AppendElement(NS_LITERAL_CSTRING("h264"));
> -  video.mAPITags.AppendElement(NS_LITERAL_CSTRING("vp8"));
> +
> +  nsCString codecsString = NS_ConvertUTF16toUTF8(m.mX_cdm_codecs);

This is not going to work. In fact, I bet you broke Netflix with this change.

The x-cdm-codecs has a format like:

"x-cdm-codecs": "vp8,vp9.0,avc1"

Whereas the GMPCapabilities expect the strings to be in the set {"vp8","vp9","h264"}. So you need to convert "vp9.0" to "vp9", and "avc1" to "h264". And you need to exclude anything not in the expected set. I think we should reject if a manifest is loaded with a codec string that's not in the expected set. This will mean that nothing unexpected happens without us noticing.

::: dom/media/gmp/GMPParent.cpp:991
(Diff revision 1)
> -  video.mAPITags.AppendElement(NS_LITERAL_CSTRING("h264"));
> -  video.mAPITags.AppendElement(NS_LITERAL_CSTRING("vp8"));
> -  video.mAPITags.AppendElement(NS_LITERAL_CSTRING("vp9"));
> -  video.mAPITags.AppendElement(kEMEKeySystemWidevine);
> +
> +  nsCString codecsString = NS_ConvertUTF16toUTF8(m.mX_cdm_codecs);
> +  nsTArray<nsCString> codecs;
> +  SplitAt(",", codecsString, codecs);
> +
> +  for (uint32_t i = 0; i < codecs.Length(); ++i) {

Where possible you should avoid loops with raw array indices, as they're a source of errors. Prefer range-based for loops.

So make this:

for (const nsCString& codec : codecs) \{
  ... 
\}

::: media/gmp-clearkey/0.1/manifest.json.in:9
(Diff revision 1)
> +    "version": "1",
> +    "x-cdm-module-versions": "4",
> +    "x-cdm-interface-versions": "8",
> +    "x-cdm-host-versions": "8",
> +#ifdef ENABLE_WMF
> +    "x-cdm-codecs": "h264"

This should be avc1 to match what the Widevine CDM uses.
Attachment #8825235 - Flags: review?(cpearce) → review-
(Assignee)

Comment 5

6 months ago
mozreview-review-reply
Comment on attachment 8825235 [details]
Bug 1319159 - Updates how 'ParseChromiumManifest' works so that it doesn't assume Widevine (now it assumes Widevine or Clearkey..).

https://reviewboard.mozilla.org/r/103412/#review104058

> The code to initialize libs should be inside #ifdef XP_WIN blocks.
> 
> In fact, I would just assign to mLibs directly here, and thus save storing a copy until assigning later on.

If I make this change we'll need two #ifdef XP_WIN blocks, one for Clearkey and one for Widevine? I haven't made the change yet, I'd just like to confirm that's actually what you intended
(Assignee)

Comment 6

6 months ago
mozreview-review-reply
Comment on attachment 8825235 [details]
Bug 1319159 - Updates how 'ParseChromiumManifest' works so that it doesn't assume Widevine (now it assumes Widevine or Clearkey..).

https://reviewboard.mozilla.org/r/103412/#review104058

> I wouldn't bother storing these in booleans. I would just go:
> 
> if (mDisplayName.EqualsASCII("clearkey")) {
>   kEMEKeySystem = ...
> #ifdef XP_WIN
>   mLibs = ...
> #endif
> } else if (mDisplayName.EqualsASCII("WidevineCdm")) {
>   kEMEKeySystem = ...
>   mLibs = ...
> }
> 
> That should make the code simpler, and means you don't need to store thing unnecessarily.
> 
> In general, it's better to not store what you can calculate, as then you need to worry about the calculated value expiring.

This guard only needs to be here if we assign directly to mLibs. If we do that we'll need to #ifdef guard both of the CDMs
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 11

6 months ago
mozreview-review
Comment on attachment 8825235 [details]
Bug 1319159 - Updates how 'ParseChromiumManifest' works so that it doesn't assume Widevine (now it assumes Widevine or Clearkey..).

https://reviewboard.mozilla.org/r/103412/#review104776

::: dom/media/gmp/GMPParent.cpp:970
(Diff revision 3)
> +  if (mDisplayName.EqualsASCII("clearkey")) {
> +    kEMEKeySystem = kEMEKeySystemClearkey;
> +#if XP_WIN
> +    mLibs = NS_LITERAL_CSTRING("dxva2.dll, msmpeg2vdec.dll, evr.dll, mfh264dec.dll, mfplat.dll");
> +#endif
> +  }

} else {

You can configure Visual Studio to not add new lines before 'else' statements.

::: dom/media/gmp/GMPParent.cpp:976
(Diff revision 3)
> +  else if (mDisplayName.EqualsASCII("WidevineCdm")) {
> +    kEMEKeySystem = kEMEKeySystemWidevine;
> +#if XP_WIN
> +    mLibs = NS_LITERAL_CSTRING("dxva2.dll");
> +#endif
> +  }

} else {

::: dom/media/gmp/GMPParent.cpp:987
(Diff revision 3)
> -  video.mAPITags.AppendElement(NS_LITERAL_CSTRING("h264"));
> -  video.mAPITags.AppendElement(NS_LITERAL_CSTRING("vp8"));
> -  video.mAPITags.AppendElement(NS_LITERAL_CSTRING("vp9"));
> -  video.mAPITags.AppendElement(kEMEKeySystemWidevine);
> +
> +  nsCString codecsString = NS_ConvertUTF16toUTF8(m.mX_cdm_codecs);
> +  nsTArray<nsCString> codecs;
> +  SplitAt(",", codecsString, codecs);
> +
> +  for (const nsCString& wideVineCodec : codecs) {

It's acutally the Chromium CDM API, not the Widevine API. So it's more accurate to call it chromiumCodec than widevineCodec.

And, irrespective of that, the 'v' in Widevine is lowercase.
Attachment #8825235 - Flags: review?(cpearce) → review-
(Reporter)

Comment 12

6 months ago
mozreview-review
Comment on attachment 8825597 [details]
Bug 1319159 - Updates the whitelist of libraries in GMPChild

https://reviewboard.mozilla.org/r/103712/#review104778
Attachment #8825597 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 15

6 months ago
mozreview-review
Comment on attachment 8825235 [details]
Bug 1319159 - Updates how 'ParseChromiumManifest' works so that it doesn't assume Widevine (now it assumes Widevine or Clearkey..).

https://reviewboard.mozilla.org/r/103412/#review104802
Attachment #8825235 - Flags: review?(cpearce) → review+

Comment 16

6 months ago
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a14cade8b962
Updates how 'ParseChromiumManifest' works so that it doesn't assume Widevine (now it assumes Widevine or Clearkey..). r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/afb84605c3c4
Updates the whitelist of libraries in GMPChild, r=cpearce
I had to back this and bug 1318965 out for frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=68903985&repo=mozilla-inbound

You'd probably need to retrigger the jobs on your try runs several times to catch it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/68c824ad72b8
Flags: needinfo?(jharris)

Comment 18

6 months ago
Backout by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/ac3275723df5
Backed out 2 changesets for frequent media test failures a=backout
(Assignee)

Comment 19

5 months ago
It looks like this issue is caused by the WidevineAdapter. We have a race condition there where the decryptor is being destroyed before we create a decoder, causing the CDM to crash. Hopefully I've got a fix on the way.
Flags: needinfo?(jharris)
(Reporter)

Comment 20

5 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfb900fe8b2378f00487515ac40e9908bf35becf
Bug 1319159 - Updates how 'ParseChromiumManifest' works so that it doesn't assume Widevine (now it assumes Widevine or Clearkey..). r=cpearce

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef0736c982a270d8af599251063df1d104503912
Bug 1319159 - Updates the whitelist of libraries in GMPChild. r=cpearce

Comment 21

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bfb900fe8b23
https://hg.mozilla.org/mozilla-central/rev/ef0736c982a2
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.