1368583 - Add deprecation warning for MediaKeySystemConfigurations without MediaKeySystemCapabilities, or with MediaKeySystemCapabilities with no codecs specified

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Description

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

As per Bug 1355252, the EME spec requires our navigator.requestMediaKeySystemAccess() function to reject request for configurations which don't contain at least one MediaKeySystemCapabilities (audioCapabilities or videoCapabilities).

That's step 15 of this algorithm:
https://w3c.github.io/encrypted-media/#get-supported-configuration-and-consent

We also shouldn't be assuming that WebM and MP4 normatively implies a specific set of codecs and codec constraints, as that violates step 10 of the "Get Supported Capabilities for Audio/Video Type" algorithm.

https://w3c.github.io/encrypted-media/#get-supported-capabilities-for-audio-video-type

Making this change has the effect of causing us to reject configurations with MediaKeySystemCapabilities where those capabilities don't specify a codec string.

We should add a deprecation warning to encourage authors to make their sites spec compliant. We should also have telemetry to measure how much of the internet we'll break if we start enforcing this.

Note, Chrome shipped these changes in M58.

Comment 1

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

Attachment: Bug 1368583 - Add deprecation warning for MediaKeySystemConfiguration properties no longer supported by the EME spec.

As per Bug 1355252, the EME spec requires our
navigator.requestMediaKeySystemAccess() function to reject request for
configurations which don't contain at least one MediaKeySystemCapabilities
(audioCapabilities or videoCapabilities).

That's step 15 of this algorithm:
https://w3c.github.io/encrypted-media/#get-supported-configuration-and-consent

We also shouldn't be assuming that WebM and MP4 normatively implies a specific
set of codecs and codec constraints, as that violates step 10 of the "Get
Supported Capabilities for Audio/Video Type" algorithm.

https://w3c.github.io/encrypted-media/#get-supported-capabilities-for-audio-video-type

Making this change has the effect of causing us to reject configurations with
MediaKeySystemCapabilities where those capabilities don't specify a codec
string.

This patch adds a deprecation warning to encourage authors to make their sites
spec compliant.

Review commit: https://reviewboard.mozilla.org/r/144006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144006/

Comment 2

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

Comment on attachment 8872482 [details]
Bug 1368583 - Add deprecation warning for MediaKeySystemConfiguration properties no longer supported by the EME spec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144006/diff/1-2/

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8872482 [details]
Bug 1368583 - Add deprecation warning for MediaKeySystemConfiguration properties no longer supported by the EME spec.

https://reviewboard.mozilla.org/r/144006/#review147720

r+ with suggestion & typo:

::: commit-message-8f1c4:9
(Diff revision 2)
> +navigator.requestMediaKeySystemAccess() function to reject request for
> +configurations which don't contain at least one MediaKeySystemCapabilities
> +(audioCapabilities or videoCapabilities).
> +
> +That's step 15 of this algorithm:
> +https://w3c.github.io/encrypted-media/#get-supported-configuration-and-consent

Would it be possible to link to the exact document version you're working with? -- Probably not that important; archeologists could trace the doc version based on this patch's date.
(And another one below)

::: commit-message-8f1c4:11
(Diff revision 2)
> +(audioCapabilities or videoCapabilities).
> +
> +That's step 15 of this algorithm:
> +https://w3c.github.io/encrypted-media/#get-supported-configuration-and-consent
> +
> +We also shouldn't be assuming that WebM and MP4 normatively implies a specific

'implies' -> 'imply'

Comment 4

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

Comment on attachment 8872482 [details]
Bug 1368583 - Add deprecation warning for MediaKeySystemConfiguration properties no longer supported by the EME spec.

https://reviewboard.mozilla.org/r/144006/#review147720

> Would it be possible to link to the exact document version you're working with? -- Probably not that important; archeologists could trace the doc version based on this patch's date.
> (And another one below)

I don't think this is possible, as the draft spec is on a github-pages site, which by default serves the tip of master branch.

Comment 5

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

Attachment: Bug 1368583 - Add deprecation warning for MediaKeySystemConfiguration properties no longer supported by the EME spec.

As per Bug 1355252, the EME spec requires our
navigator.requestMediaKeySystemAccess() function to reject request for
configurations which don't contain at least one MediaKeySystemCapabilities
(audioCapabilities or videoCapabilities).

That's step 15 of this algorithm:
https://w3c.github.io/encrypted-media/#get-supported-configuration-and-consent

We also shouldn't be assuming that WebM and MP4 normatively imply a specific
set of codecs and codec constraints, as that violates step 10 of the "Get
Supported Capabilities for Audio/Video Type" algorithm.

https://w3c.github.io/encrypted-media/#get-supported-capabilities-for-audio-video-type

Making this change has the effect of causing us to reject configurations with
MediaKeySystemCapabilities where those capabilities don't specify a codec
string.

This patch adds a deprecation warning to encourage authors to make their sites
spec compliant.

Review commit: https://reviewboard.mozilla.org/r/144284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144284/

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2dfb7338a49e5e0c0b19be457a040d9288d1d4
Bug 1368583 - Add deprecation warning for MediaKeySystemConfiguration properties no longer supported by the EME spec. r=gerald

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1368583 - Implement SourceBufferResource::GetCachedDataEnd -

MediaResourceIndex caching requires GetCachedDataEnd and ReadFromCache.
Implementing SourceBufferResource::GetCachedDataEnd is trivial, as it's just a
buffer from 0 to GetLength(), so if the requested cached-data offset is inside
the buffer, we can just return the total length as known cached data.

Review commit: https://reviewboard.mozilla.org/r/144298/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144298/

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1368583 - MockMediaResource::GetCachedDataEnd should return the offset if out of range -

If the requested cached-data offset is out of range, we should just return the
same offset, as it signals that the actual cached range is 0 bytes, without
having to deal with -1.

Review commit: https://reviewboard.mozilla.org/r/144300/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144300/

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1368583 - MockMediaResource should fread individual bytes -

`fread(buf, count, 1, f)` meant that exactly one object of size `count` could
be read.
Changing that to `fread(buf, 1, count, f)` now reads up to `count` size-1
objects (aka bytes) up to `count` and returns that number of bytes read, which
is usually what we want from a read, i.e. as much as possible even if it's less
than requested.

Review commit: https://reviewboard.mozilla.org/r/144302/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144302/

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1368583 - WaveTrackDemuxer should copy the MediaResource* instead of a whole MediaResourceIndex -

When the WaveTrackDemuxer is given a MediaResourceIndex, it's only really
interested in the MediaResource pointer, so we should just pass that, and
WaveTrackDemuxer can construct its own MediaResourceIndex from it.

Also, MediaResourceIndex will become non-copyable soon.

Review commit: https://reviewboard.mozilla.org/r/144304/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144304/

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1368583 - MediaResourceIndex::ReadAt tries to cache last-read block -

This is the core of this bug:
- We try to read past the end of the requested range, and save a block-full of
cached data. ("Block" is a memory range, with an alignment and size being a
power of two, to match existing caching happening in MediaCache and
FileBlockCache, and to play nice with the memory allocator.)
- If part of a requested read touches the existing cache, we can just read from
the cache, which means we don't involve any of the locking and IOs that normal
reads use.

The small extra work needed to cache more data in some reads is largely offset
by all the lock&IO-heavy reads that we can subsequently avoid.

UncachedReadAt, which is used internally by CachedReadAt, is left public
because it could be useful if the caller knows for sure that a particular read
is isolated.

(Note: The printfs, and comparison code in ReadAt, will be removed in later
patches. Also the block size will be later controlled by a pref.)

Review commit: https://reviewboard.mozilla.org/r/144306/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144306/

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1368583 - media.cache.resource-index controls the MediaResourceIndex cache size -

8KB by default, otherwise using the next power of two from the given
media.cache.resource-index (but staying within 32B-128KB).
'0' means we don't want to use caching.

Review commit: https://reviewboard.mozilla.org/r/144308/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144308/

Comment 13

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1368583 - Replace debugging ReadAt with CachedReadAt code -

Review commit: https://reviewboard.mozilla.org/r/144310/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144310/

Comment 14

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1368583 - Convert printfs into MOZ_LOG(MediaResourceIndex) -

Review commit: https://reviewboard.mozilla.org/r/144312/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144312/

Comment 15

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8872803 [details]
Bug 1368583 - Implement SourceBufferResource::GetCachedDataEnd -

Sorry, wrong bug

Comment 16

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8872788 [details]
Bug 1368583 - Add deprecation warning for MediaKeySystemConfiguration properties no longer supported by the EME spec.

(was wrongly obsoleted by incorrect push)

Comment 17

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8872788 [details]
Bug 1368583 - Add deprecation warning for MediaKeySystemConfiguration properties no longer supported by the EME spec.

https://reviewboard.mozilla.org/r/144284/#review148110

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/062af1485e68
Use proper Unicode double-quotes in the MediaEMENoCodecsDeprecatedWarning key.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe2dfb7338a4
https://hg.mozilla.org/mozilla-central/rev/062af1485e68

Comment 20

[Francesco Lodolo [:flod]]

Is this string what you actually wanted to land?

MediaEMENoCodecsDeprecatedWarning = Calling navigator.requestMediaKeySystemAccess() (at %S) without passing a candidate MediaKeySystemConfiguration containing audioCapabilities or videoCapabilities without a contentType with a "codecs" string is deprecated and will soon become unsupported.

It feels like a bad copy and paste from 'MediaEMENoCapabilitiesDeprecatedWarning', and 'without passing a candidate MediaKeySystemConfiguration containing audioCapabilities or videoCapabilities' should not be in this string.

I don't want to ask for this to be backed out, but I also want to fix it ASAP if it's an error (hence the double NI). Original commit was also amended by :ryanvm to pass tests.
https://hg.mozilla.org/mozilla-central/rev/062af1485e68

Comment 21

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

(In reply to Francesco Lodolo [:flod] from comment #20)
> Is this string what you actually wanted to land?

Yes, but I'm open to improving the strings.
 
> MediaEMENoCodecsDeprecatedWarning = Calling
> navigator.requestMediaKeySystemAccess() (at %S) without passing a candidate
> MediaKeySystemConfiguration containing audioCapabilities or
> videoCapabilities without a contentType with a "codecs" string is deprecated
> and will soon become unsupported.
> 
> It feels like a bad copy and paste from
> 'MediaEMENoCapabilitiesDeprecatedWarning', and 'without passing a candidate
> MediaKeySystemConfiguration containing audioCapabilities or
> videoCapabilities' should not be in this string.

These strings are deliberately similar, so that that UI is consistent.

Why do you think that 'without passing a candidate MediaKeySystemConfiguration containing audioCapabilities or videoCapabilities' should not be in this string? Could you elaborate?

I am happy to improve these strings. How can we make them better?

> I don't want to ask for this to be backed out, but I also want to fix it
> ASAP if it's an error (hence the double NI). Original commit was also
> amended by :ryanvm to pass tests.
> https://hg.mozilla.org/mozilla-central/rev/062af1485e68

Let me tell describe the context of where the string is, to help us understand how we can improve it.

The target audience of the string is web developers.

Firefox has a JS API navigator.requestMediaKeySystemAccess() which has two features that we're deprecating. We show the string in the Developer Console when JavaScript calls these deprecated APIs. So I don't expect people other than web developers to see it.

Here's an example showing how the function is used, and the two cases we're deprecating

var setOfInputs = [
  // Case 1. Deprecated; no audioCapabilities or videoCapabilities supplied
  {
    initDataTypes: [ 'cenc' ],
  },
  // Case 2. Deprecated; no codec specified in the contentType member.
  {
    initDataTypes: [ 'cenc' ],
    videoCapabilities: [ {
      contentType: 'video/mp4'
    } ]
  },
  // Case 3. Correct, at least one *Capabilities supplied with codecs specified. 
  {
    initDataTypes: [ 'cenc' ],
    videoCapabilities: [ {
      contentType: 'video/mp4; codecs="avc1.640028"'
    } ]
  }
];
navigator.requestMediaKeySystemAccess(
    'com.widevine.alpha', setOfInputs)
  .then(function(keySystemAccess) { 
...
 } );

The function (navigator.requestMediaKeySystemAccess) takes a set of inputs (setOfInputsin this example), and we're deprecating inputs without "audioCapabilities or videoCapabilities" supplied (case 1 above), and we're also deprecating cases which do supply audioCapabilities and videoCapabilities but which don't have a "contentType" which specifies a "codecs" value (case 2 above). The correct usage is 3. above.

navigator.requestMediaKeySystemAccess checks every member in the set of inputs and will log the string to the console when the deprecated cases are encountered.

Given that context, how can we improve the strings?

Comment 22

[Francesco Lodolo [:flod]]

Thanks for the detailed explanation.

> The target audience of the string is web developers.

True, but localizers need to translate this, and they need to understand the sense of the sentence to do it properly.

>   // Case 2. Deprecated; no codec specified in the contentType member.
>   {
>     initDataTypes: [ 'cenc' ],
>     videoCapabilities: [ {
>       contentType: 'video/mp4'
>     } ]
>   },
> The function (navigator.requestMediaKeySystemAccess) takes a set of inputs
> (setOfInputsin this example), and we're deprecating inputs without
> "audioCapabilities or videoCapabilities" supplied (case 1 above), and we're
> also deprecating cases which do supply audioCapabilities and
> videoCapabilities but which don't have a "contentType" which specifies a
> "codecs" value (case 2 above). The correct usage is 3. above.

Current string: "Calling navigator.requestMediaKeySystemAccess() (at %S) without passing a candidate MediaKeySystemConfiguration containing audioCapabilities or videoCapabilities without a contentType with a "codecs" string is deprecated and will soon become unsupported."

Let's try to strip it down to the minimum: "Calling X without passing a MediaKeySystemConfiguration containing audioCapabilities or videoCapabilities without a contentType with a "codecs" string is deprecated".

It looks like two sentences glued together, but that's also not case 2 as you described it here, is it? That would be: "Calling navigator.requestMediaKeySystemAccess() (at %S) passing a candidate MediaKeySystemConfiguration containing audioCapabilities or videoCapabilities without a contentType with a "codecs" string is deprecated and will soon become unsupported."

The first "without" shouldn't be there.

Comment 23

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

Attachment: Bug 1368583 - Adjust MediaEMENoCodecsDeprecatedWarning string to make it clearer.

Review commit: https://reviewboard.mozilla.org/r/144636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144636/

Comment 24

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

Thanks Francesco, I've added a patch to make the changes you've suggested.

Comment 25

[Francesco Lodolo [:flod]]

Comment on attachment 8873168 [details]
Bug 1368583 - Adjust MediaEMENoCodecsDeprecatedWarning string to make it clearer.

https://reviewboard.mozilla.org/r/144636/#review148668

Thanks. Note that normally you would need a new string ID, but in this case it's OK to use the existing one, since I was waiting for a fix to expose new strings to tools.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/582b3347c32b
Adjust MediaEMENoCodecsDeprecatedWarning string to make it clearer. r=flod

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/582b3347c32b

Comment 28

[Kohei Yoshino]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/navigator-requestmediakeysystemaccess-will-soon-require-mediakeysystemcapabilities-as-configurations/

Comment 29

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

(In reply to Kohei Yoshino [:kohei] from comment #28)
> Posted the site compatibility note:
> https://www.fxsitecompat.com/en-CA/docs/2017/navigator-
> requestmediakeysystemaccess-will-soon-require-mediakeysystemcapabilities-as-
> configurations/

Thank you!

Comment 30

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en-US/docs/Web/API/Navigator/requestMediaKeySystemAccess

While I was at it, I cleaned things up a bit, standardized the layout better, and added the missing section on exceptions.

Firefox 55 for developers updated as well.