1189196 - navigator.requestMediaKeySystemAccess() supportedConfigurations should have initDataTypes property, not initDataType

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

The EME Editor's Draft 28 (July 2015) says MediaKeySystemConfiguration has a property called initDataTypes, not initDataType. initDataTypes is a sequence<DOMString>.

Firefox currently expects navigator.requestMediaKeySystemAccess('org.w3.clearkey', [{initDataType: 'cenc'}]). That is, "initDataType: 'cenc'" instead of "initDataTypes: ['cenc']".

https://w3c.github.io/encrypted-media/#idl-def-MediaKeySystemConfiguration

Comment 1

[Chris Peterson [:cpeterson]]

In addition to the Bitdash ClearKey test from bug 1186162:

http://www.dash-player.com/demo/drm-and-protection/?drm=clearkey&mpd=%2F%2Fbitdash-a.akamaihd.net%2Fcontent%2Fsintel-ck%2Fstream.mpd&key=a0a1a2a3a4a5a6a7a8a9aaabacadaeaf

The following ClearKey demo from Sam Dutton's simpl.info tests doesn't work on Firefox because of this initDataType/initDataTypes bug:

http://simpl.info/eme/clearkey/

Comment 2

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

(In reply to Chris Peterson [:cpeterson] from comment #1)
> The following ClearKey demo from Sam Dutton's simpl.info tests doesn't work
> on Firefox because of this initDataType/initDataTypes bug:
> 
> http://simpl.info/eme/clearkey/

That demo is also non-MSE WebM only, and we only support ClearKey in MSE+MP4/H.264/AAC.

Comment 3

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

Attachment: Patch 1: Rename MediaKeySystemOptions to MediaKeySystemConfiguration

* Rename MediaKeySystemOptions to MediaKeySystemConfiguration in WebIDL and C++ code.
* Add initDataTypes, audioCapabilities and videoCapabilities fields to MediaKeySystemConfiguration, as per W3C draft EME spec.
* Don't implement distinctiveIdentifier, persistentState, sessionTypes fields of MediaKeySystemConfiguration, and don't implement MediaKeySystemMediaCapability robustness field. Will come back to these at a later date.
* Retain existing initDataType, audioType, and videoType fields in MediaKeySystemConfiguration so that existing sites don't break. I'll remove them in a few releases, once sites using them have had time to transition.
* Add MediaKeySystemAccess.getConfiguration(), though it returns an empty dict until a later patch.
* Remove the logging of the MediaKeySystemConfigurations in navigator.requestMediaKeySystemAccess(). I re-add it cleaner in a later patch.

Comment 4

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

Attachment: Patch 2: Process MediaKeySystemConfiguration in navigator.requestMediaKeySystemAccess

Update MediaKeySystemAccessManager::Request() to process new MediaKeySystemConfiguration dictionaries and determine the configuration to use.

Comment 5

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

Attachment: Patch 3: Clean up navigator.requestMediaKeySystemAccess config logging

I removed the logging of MediaKeySystemConfiguration/Options from navigator.requestMediaKeySystemAccess in patch 1. Now re-add it, but cleaner, and with the log string entirely calculated inside the LOG call, so that it's not constructed unless logging is enabled.

Comment 6

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

Attachment: Patch 4: Update tests to new paradigm

Update the EME mochitests to use new request dict, and validate that we're constructing a configuration and returning it.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8677816 [details] [diff] [review]
Patch 3: Clean up navigator.requestMediaKeySystemAccess config logging

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

::: dom/base/Navigator.cpp
@@ +2803,5 @@
> +{
> +  nsCString str;
> +  str.AppendLiteral("{contentType='");
> +  if (!aValue.mContentType.IsEmpty()) {
> +    str.Append(NS_ConvertUTF16toUTF8(aValue.mContentType));

Call ToCString() above.

@@ +2815,5 @@
> +ToCString(const Sequence<Type>& aSequence)
> +{
> +  nsCString s;
> +  s.AppendLiteral("[");
> +  bool first = true;

'first' not used.

@@ +2882,5 @@
>  Navigator::RequestMediaKeySystemAccess(const nsAString& aKeySystem,
>                                         const Sequence<MediaKeySystemConfiguration>& aConfigs,
>                                         ErrorResult& aRv)
>  {
> +  EME_LOG(RequestKeySystemAccessLogString(aKeySystem, aConfigs).get());

It will be safer to say:
EME_LOG("%s", str) instead of EME_LOG(str) to avoid format string attach.

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8677817 [details] [diff] [review]
Patch 4: Update tests to new paradigm

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

::: dom/media/test/test_eme_request_notifications.html
@@ +35,5 @@
> +  observedStatus = "nothing";
> +  var name = "'" + test.keySystem + "'";
> +  return p.then(function() {
> +    return new Promise(function(resolve, reject) {
> +      navigator.requestMediaKeySystemAccess(test.keySystem, [{initDataTypes:["cenc"]}])

This should be the only change. Not sure why the whole file appears to be different.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8677813 [details] [diff] [review]
Patch 1: Rename MediaKeySystemOptions to MediaKeySystemConfiguration

So callers of requestMediaKeySystemAccess that only pass one argument will start throwing.  I assume that's OK, right?

>@@ -169,20 +167,21 @@ MediaKeySystemAccessManager::Request(Det

I don't understand the change here.  Why do we pass a default-initialized MediaKeySystemConfiguration to the MediaKeySystemAccess ctor instead of something based on aConfigs?  If this code is in fact correct, it could use at least some comments explaining what it's after.

r=me on the rest.

Comment 10

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

(In reply to JW Wang [:jwwang] from comment #8)
> Comment on attachment 8677817 [details] [diff] [review]
> Patch 4: Update tests to new paradigm
> 
> Review of attachment 8677817 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/test/test_eme_request_notifications.html
> @@ +35,5 @@
> > +  observedStatus = "nothing";
> > +  var name = "'" + test.keySystem + "'";
> > +  return p.then(function() {
> > +    return new Promise(function(resolve, reject) {
> > +      navigator.requestMediaKeySystemAccess(test.keySystem, [{initDataTypes:["cenc"]}])
> 
> This should be the only change. Not sure why the whole file appears to be
> different.

Line endings changed from Windows to UNIX.

Comment 11

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

(In reply to Boris Zbarsky [:bz] from comment #9)
> Comment on attachment 8677813 [details] [diff] [review]
> Patch 1: Rename MediaKeySystemOptions to MediaKeySystemConfiguration
> 
> So callers of requestMediaKeySystemAccess that only pass one argument will
> start throwing.  I assume that's OK, right?

None of the callers that I'm aware of pass only one argument, so I think it's OK.

> >@@ -169,20 +167,21 @@ MediaKeySystemAccessManager::Request(Det
> 
> I don't understand the change here.  Why do we pass a default-initialized
> MediaKeySystemConfiguration to the MediaKeySystemAccess ctor instead of
> something based on aConfigs?  If this code is in fact correct, it could use
> at least some comments explaining what it's after.

I fill out the config based on aConfigs in a later patch in the series.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4894942822a
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ed3ed1e847
https://hg.mozilla.org/integration/mozilla-inbound/rev/f781ec150019
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2e1b5e5d96

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b4894942822a
https://hg.mozilla.org/mozilla-central/rev/d4ed3ed1e847
https://hg.mozilla.org/mozilla-central/rev/f781ec150019
https://hg.mozilla.org/mozilla-central/rev/7f2e1b5e5d96

Comment 14

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

Comment on attachment 8677813 [details] [diff] [review]
Patch 1: Rename MediaKeySystemOptions to MediaKeySystemConfiguration

Requesting approval for uplift to beta 43 for all patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: EME

[User impact if declined]: Websites that are being written to target EME in Firefox today will have to use a non-standard API to initialize EME in Firefox, and cross browser examples won't work in Firefox but will in all other browsers that support EME.

This patch udpates our implementation to match the spec and what other browsers are doing, while keeping the old way of starting up EME working so that existing sites (i.e. Netflix!) don't break while they transition to use the standardized API for Firefox.

This means new EME users won't have to use the obsolete initialization method for Firefox EME sooner.

[Describe test coverage new/current, TreeHerder]: I update our mochitests to ensure the new standardize way to initialize EME works, and to ensure that the old way works. We will remove the old way some time in the future.
[Risks and why]: Low; we have tests, and we maintain backwards compatibility with the old non-standard way to initialize EME.
[String/UUID change made/needed]: None.

Comment 15

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

In order to cleanly uplift this bug, we also need to uplift bug 1207019.

Comment 16

[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 17

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

Tracking since we're aiming to uplift this to 43.

Comment 18

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

Comment on attachment 8677813 [details] [diff] [review]
Patch 1: Rename MediaKeySystemOptions to MediaKeySystemConfiguration

Standardizing API for EME, ok to uplift to beta.

Comment 19

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

https://hg.mozilla.org/releases/mozilla-beta/rev/0851006030fe
https://hg.mozilla.org/releases/mozilla-beta/rev/0c3338032dc3
https://hg.mozilla.org/releases/mozilla-beta/rev/83b2962d6861
https://hg.mozilla.org/releases/mozilla-beta/rev/ba6b7797e925