Closed Bug 1189196 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox39 --- affected
firefox40 --- affected
firefox41 --- affected
firefox42 --- affected
firefox43 + fixed
firefox44 + fixed

People

(Reporter: cpeterson, Assigned: cpearce)

References

Details

Attachments

(4 files)

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
Blocks: 1032660
Priority: -- → P2
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/
(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.
* 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.
Attachment #8677813 - Flags: review?(bzbarsky)
Update MediaKeySystemAccessManager::Request() to process new MediaKeySystemConfiguration dictionaries and determine the configuration to use.
Attachment #8677814 - Flags: review?(jwwang)
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.
Attachment #8677816 - Flags: review?(jwwang)
Update the EME mochitests to use new request dict, and validate that we're constructing a configuration and returning it.
Attachment #8677817 - Flags: review?(jwwang)
Attachment #8677814 - Flags: review?(jwwang) → review+
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.
Attachment #8677816 - Flags: review?(jwwang) → review+
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.
Attachment #8677817 - Flags: review?(jwwang) → review+
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.
Attachment #8677813 - Flags: review?(bzbarsky) → review+
(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.
(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 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.
Attachment #8677813 - Flags: approval-mozilla-beta?
In order to cleanly uplift this bug, we also need to uplift bug 1207019.
Beta try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f5c7a269c0d

The rebased patches are in the parents of this push.
Flags: needinfo?(cpearce)
Tracking since we're aiming to uplift this to 43.
Comment on attachment 8677813 [details] [diff] [review]
Patch 1: Rename MediaKeySystemOptions to MediaKeySystemConfiguration

Standardizing API for EME, ok to uplift to beta.
Attachment #8677813 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1224074
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.