Closed Bug 1095257 Opened 10 years ago Closed 10 years ago

[EME] Implement Navigator.requestMediaKeySystemAccess()

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 2 obsolete files)

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"
};
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8521013 - Flags: review?(edwin)
Attachment #8521013 - Flags: review?(bzbarsky)
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.
Attachment #8521013 - Flags: review?(edwin) → review+
Attached patch Patch v2 (obsolete) — Splinter Review
With Edwin's review comments addressed... and with `hg add dom/webidl/MediaKeySystemAccess.webidl`...
Attachment #8521013 - Attachment is obsolete: true
Attachment #8521013 - Flags: review?(bzbarsky)
Attachment #8521729 - Flags: review?(bzbarsky)
This bug is necessary for basic playback of EME video on Windows.
Blocks: eme-m1
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.  :(
Attachment #8521729 - Flags: review?(bzbarsky) → review+
(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.
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.
Er, yes, exactly.  Sorry, I should have caught that.  :(
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
Because navigator.requestMediaKeySystemAccess is still visible somehow...
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.
Attached patch Patch v3Splinter Review
Added Pref=media.eme.enabled to MediaKeySystemAccess.
Attachment #8521729 - Attachment is obsolete: true
Attachment #8524388 - Flags: review+
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...
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
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.
Flags: needinfo?(cpearce)
Chris, is the pref set during the test run or something?
Ah, yes, it is.  See testing/profiles/prefs_general.js
(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.
Flags: needinfo?(cpearce)
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.
https://hg.mozilla.org/mozilla-central/rev/40711e8b703d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
No longer blocks: eme-m1
Mass update firefox-status to track EME uplift.
You need to log in before you can comment on or make changes to this bug.