Closed Bug 1319159 Opened 8 years ago Closed 7 years ago

Make GMPParent::ParseChromiumManifest() not assume Widevine

Categories

(Core :: Audio/Video: Playback, defect, P3)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: cpearce, Assigned: jay.harris)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1319159 - Updates how 'ParseChromiumManifest' works so that it doesn't assume Widevine (now it assumes Widevine or Clearkey..).
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1319159 - Updates the whitelist of libraries in GMPChild
59 bytes, text/x-review-board-request
Details 
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.
Attachment #8825219 - Attachment is obsolete: true
Attachment #8825219 - Flags: review?(cpearce)
Assignee: nobody → jharris
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-
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
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 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-
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 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+
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)
Backout by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/ac3275723df5
Backed out 2 changesets for frequent media test failures a=backout
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)
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
https://hg.mozilla.org/mozilla-central/rev/bfb900fe8b23
https://hg.mozilla.org/mozilla-central/rev/ef0736c982a2
Status: NEW → RESOLVED
Closed: 7 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: