1095257 - [EME] Implement Navigator.requestMediaKeySystemAccess()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

The way to create a MediaKeys object has changed recently; now we're supposed to create it using these:

partial interface Navigator {
    Promise<MediaKeySystemAccess> requestMediaKeySystemAccess (DOMString keySystem, optional sequence<MediaKeySystemOptions> supportedConfigurations);
};

interface MediaKeySystemAccess {
    readonly    attribute DOMString keySystem;
    Promise<MediaKeys> createMediaKeys ();
};

dictionary MediaKeySystemOptions {
    DOMString            initDataType = "";
    DOMString            audioType = "";
    DOMString            audioCapability = "";
    DOMString            videoType = "";
    DOMString            videoCapability = "";
    MediaKeysRequirement uniqueidentifier = "optional";
    MediaKeysRequirement stateful = "optional";
};

enum MediaKeysRequirement {
    "required",
    "optional",
    "disallowed"
};

Comment 1

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

Attachment: Patch

Remove MediaKeys.create and MediaKeys.isTypeSupported, and add navigator.requestMediaKeySystemAccess() as per current draft spec.

The capability checks are only wired up for clearkey, since we haven't discussed with Adobe what capability strings they want to expose.

Note: the members of MediaKeySystemOptions are still in flux, so there will likely be some changes to the members of MediaKeySystemOptions in future.

Comment 2

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

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=06fd931f2d06

Comment 3

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

Comment on attachment 8521013 [details] [diff] [review]
Patch

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

Sweet. Couple small nits.

::: dom/media/fmp4/MP4Decoder.cpp
@@ +105,5 @@
>  MP4Decoder::CanHandleMediaType(const nsACString& aType,
> +                               const nsAString& aCodecs,
> +                               bool& aOutContainsAAC,
> +                               bool& aOutContainsH264,
> +                               bool& aOutContainsMP3)

:(

I have no suggestion here; just... :(

::: dom/media/test/eme.js
@@ +199,5 @@
> +      }
> +    ];
> +    navigator.requestMediaKeySystemAccess(KEYSYSTEM_TYPE, options).then(
> +      function(keySystemAccess) {
> +        keySystemAccess.createMediaKeys().then(function(mediaKeys) {

nit: |return keySystemAccess.createMediaKeys();| and move this .then chain to the end of requestMediaKeySystemAccess.

::: dom/media/test/test_eme_requestKeySystemAccess.html
@@ +34,5 @@
> +      });
> +  });
> +}
> +
> +const clearkey = 'org.w3.clearkey';

nit: CLEARKEY_ID or something like that. Something uppercase.

Comment 4

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

Attachment: Patch v2

With Edwin's review comments addressed... and with `hg add dom/webidl/MediaKeySystemAccess.webidl`...

Comment 5

[Chris Peterson [:cpeterson]]

This bug is necessary for basic playback of EME video on Windows.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8521729 [details] [diff] [review]
Patch v2

>+++ b/dom/media/eme/MediaKeySystemAccess.cpp
>+MediaKeySystemAccess::IsSupported(const nsAString& aKeySystem,

Why does this not depend on initDataType, uniqueidentifier, stateful?  At leat document why those don't matter here, please.

>+++ b/dom/media/eme/MediaKeySystemAccess.h
>+struct JSContext;

Including js/TypeDecls.h might be better.

I didn't review the tests very carefully.

r=me.  Sorry for the lag.  :(

Comment 7

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

(In reply to Please do not ask for reviews for a bit [:bz] from comment #6)
> Comment on attachment 8521729 [details] [diff] [review]
> Patch v2
> 
> >+++ b/dom/media/eme/MediaKeySystemAccess.cpp
> >+MediaKeySystemAccess::IsSupported(const nsAString& aKeySystem,
> 
> Why does this not depend on initDataType, uniqueidentifier, stateful?  At
> leat document why those don't matter here, please.

MediaKeySystemAccess::IsSupported() checks options.mInitDataType. 

Our sandbox provides an origin specific uniqueidentifier, and the ability to persist data. We don't yet have a way to turn those off and on for specific GMPs/CDMs, and I don't know if we want to, so I just left that unimplemented.

I'll add a comment.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/54f79521758b

Comment 9

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

Backed out again due to m3 failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee39194ac67

4175 INFO TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface MediaKeySystemAccess to all webpages as a property on the window (XBL: false)? Do not make a change to this file without a review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals) - expected PASS

I bet this is because I forgot the pref=media.eme.enabled on MediaKeySystemAccess.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Er, yes, exactly.  Sorry, I should have caught that.  :(

Comment 11

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

But that adding that pref annotation to MediaKeySystemAccess didn't fix the error, I still see the error:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=61d2569643a4

Comment 12

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

Because navigator.requestMediaKeySystemAccess is still visible somehow...

Comment 13

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

Ah, I need to add MediaKeySystemAccess to interfaceNamesInGlobalScope in dom/tests/mochitest/general/test_interfaces.html for the test to pass... And I'm told repeatedly that I need DOM Peer review to change that.

Comment 14

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

Attachment: Patch v3

Added Pref=media.eme.enabled to MediaKeySystemAccess.

Comment 15

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

Attachment: Patch: Add MediaKeySystemAccess to DOM interfaceNamesInGlobalScope test

MediaKeySystemAccess needs to be in interfaceNamesInGlobalScope for dom/tests/mochitest/general/test_interfaces.html to pass.

It says repeatedly that I need DOM peer review to change that list... So I guess I'd better do that...

Comment 16

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

Thanks for the review peterv!

I rolled the test_interfaces.html fix into the main patch and re-landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa6291d952d

Comment 17

[Carsten Book [:Tomcat]]

sory chris had to back this out since we are seeing test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3984801&repo=mozilla-inbound starting with this push and  releng investigations found no change in the infrastructure that could explain that failures for a infrastructure reason.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Chris, is the pref set during the test run or something?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Ah, yes, it is.  See testing/profiles/prefs_general.js

Comment 20

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

(In reply to Carsten Book [:Tomcat] from comment #17)
> sory chris had to back this out since we are seeing test failures like
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=3984801&repo=mozilla-inbound starting with this push and 
> releng investigations found no change in the infrastructure that could
> explain that failures for a infrastructure reason.

I still see this test failure happening in revisions after my push, and I don't see how my patch can have caused it.

Comment 21

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

Discussed with KWierso on IRC, and he OK'd me to reland:

https://hg.mozilla.org/integration/mozilla-inbound/rev/40711e8b703d

We're hitting bug 1101133.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40711e8b703d

Comment 23

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

Mass update firefox-status to track EME uplift.