Closed Bug 1207019 Opened 5 years ago Closed 5 years ago

[EME] Remove WMF availability check in MediaKeySystemAccess requests


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




Tracking Status
firefox43 + fixed
firefox44 --- fixed


(Reporter: cpearce, Assigned: cpearce)


(Blocks 1 open bug)



(2 files, 2 obsolete files)

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 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?
Attachment #8664039 - Flags: review?(edwin)
Explicitly check whether the GMP expects to decode or not, special casing ClearKey on Windows < Vista.
Attachment #8664039 - Attachment is obsolete: true
Attachment #8667115 - Flags: review?(edwin)
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.
Attachment #8667115 - Flags: review?(edwin)
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).
Attachment #8667115 - Attachment is obsolete: true
Attachment #8668719 - Flags: review?(edwin)
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.
Attachment #8668722 - Flags: review?(gsquelart)
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 == type;});

No need for parens for one arg, and no need for braces nor 'return' for a single return expression:
  find(o => == 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.
Attachment #8668722 - Flags: review?(gsquelart) → review+
(In reply to Chris Pearce (:cpearce) from comment #7)

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:

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 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,

Attachment #8668719 - Flags: review?(edwin) → review+
Blocks: 1211335
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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.
Attachment #8668719 - Flags: approval-mozilla-beta?
Beta try push:

The rebased patches are in the parents of this push.
Flags: needinfo?(cpearce)
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.
Attachment #8668719 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Tracking to keep an eye on this and make sure it lands. cpearce will back out if we run into trouble.
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.