1207019 - [EME] Remove WMF availability check in MediaKeySystemAccess requests

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

We don't need to include the "does WMF work" checks in the navigator.requestMediaKeySystemAccess() code path, since Adobe are now bundling the decoder inside their CDM.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: Remove WMF availability check in MediaKeySystemAccess requests.

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8664039 [details] [diff] [review]
Patch: Remove WMF availability check in MediaKeySystemAccess requests.

Review of attachment 8664039 [details] [diff] [review]:
-----------------------------------------------------------------

Don't we still have to check for MP4 support on those pesky non-Windows platforms?

::: dom/media/eme/MediaKeySystemAccess.cpp
@@ +341,4 @@
>    return (!hasAAC ||
>            !HaveGMPFor(aGMPS,
>                        NS_ConvertUTF16toUTF8(aKeySystem),
>                        NS_LITERAL_CSTRING(GMP_API_DECRYPTOR),

Should we be checking the GMP_API_{AUDIO,VIDEO}_DECODER strings as well?

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2: Remove WMF availability check in MediaKeySystemAccess requests.

Explicitly check whether the GMP expects to decode or not, special casing ClearKey on Windows < Vista.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8667115 [details] [diff] [review]
Patch v2: Remove WMF availability check in MediaKeySystemAccess requests.

Review of attachment 8667115 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review. This patch is failing tests because it doesn't explicitly exclude MP3.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1 v3: Remove WMF check from MediaKeySystemAccess requests.

We now only query whether the platform can decode if we need the platform to decode, and the CDM doesn't specify that it decodes (except ClearKey on Win < Vista).

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2: Fixup EME mochitests

With Patch 1 above, we now expect the MediaKeySystemAccess request to specify an audio type in the audioType field of the options passed, and a video type in the videoType. The mochitests don't always do that. This patch fixes up the tests to do that, and to match the new pedantic behaviour.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd86cda2ed75

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8668722 [details] [diff] [review]
Patch 2: Fixup EME mochitests

Review of attachment 8668722 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits and an old blooper.

::: dom/media/test/eme.js
@@ +330,5 @@
>        }
>        processInitDataQueue();
>      });
>    }
> +  

Whitespaces in empty lines 334 and 339.

@@ +332,5 @@
>      });
>    }
> +  
> +  function streamType(type) {
> +    var x = test.tracks.find((o) => {return o.name == type;});

No need for parens for one arg, and no need for braces nor 'return' for a single return expression:
  find(o => o.name == type)

::: dom/media/test/test_eme_non_mse_fails.html
@@ +16,5 @@
>  {
>    var options = [{
>      initDataType: "cenc",
> +    audioType: test.audioType,
> +    videoType: test.videoType,

Just noticed that 'test' was/is not passed from the caller 'TestSetSrc', not sure how this ever worked!
Please add 'test' as a function argument.

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Chris Pearce (:cpearce) from comment #7)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd86cda2ed75

I don't see any 'test_eme_*' in there (but not all tests have completed yet as of this writing, they may be lurking in unfinished M2 tests).

In case you don't know about it:
There is now a directory-based "try" command line, which should always contain the tests you want, even if these tests float between different "mochitest-N" groupings, e.g. I'm using:
> try: -b do -p all -u crashtest,crashtest-e10s,mochitest-1,mochitest-e10s-1,mochitest-o -t none
>      --try-test-paths chrome:dom/media/test mochitest:dom/media/mediasource/test
>      mochitest:dom/media/test mochitest:dom/media/tests
Here's a try with this line: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc3294931144

You may use "./mach try --no-push -b do -p all <list directories with your tests here>" to cook up a try line, or even "./mach try ..." to directly push to try (but I haven't got that one working yet myself).

Comment 10

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8668719 [details] [diff] [review]
Patch 1 v3: Remove WMF check from MediaKeySystemAccess requests.

Review of attachment 8668719 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/eme/MediaKeySystemAccess.cpp
@@ +329,5 @@
> +// for decoding. Note we special case Clearkey on Windows, where we need
> +// to check for whether WMF is usable because the CDM uses that
> +// to decode.
> +static bool
> +GMPDecrytsAndGeckoDecodesH264(mozIGeckoMediaPluginService* aGMPService,

"Decryts"

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b6bf2244d7b

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f027322b5073
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcafe73deb41

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f027322b5073
https://hg.mozilla.org/mozilla-central/rev/fcafe73deb41

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8668719 [details] [diff] [review]
Patch 1 v3: Remove WMF check from MediaKeySystemAccess requests.

Requesting uplift for all patches in this bug.

The patches in this bug are required for uplifting bug 1189196 cleanly to beta.

Approval Request Comment
[Feature/regressing bug #]: This patch changes the API that initializes EME so that we don't require Windows system audio/video decoders to be installed on the user's system before we allow EME to work. This is no longer needed, because the Adobe EME plugin provides its own decoders now, and if Netflix are going to use our decoders, they check to see if they're usable first.
[User impact if declined]: Can't uplift bug 1189196, developers targeting EME will be sad because they have to write Firefox specific code to make EME work in Firefox.
[Describe test coverage new/current, TreeHerder]: We have lots of media and EME mochitests.
[Risks and why]: Low; this makes our EME implementation better reflect reality.
[String/UUID change made/needed]: None.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Beta try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f5c7a269c0d

The rebased patches are in the parents of this push.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8668719 [details] [diff] [review]
Patch 1 v3: Remove WMF check from MediaKeySystemAccess requests.

API changes for EME, ok to uplift to beta.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking to keep an eye on this and make sure it lands. cpearce will back out if we run into trouble.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-beta/rev/45f9754edccf
https://hg.mozilla.org/releases/mozilla-beta/rev/d7438e734352