[Widevine] Use of EME defined 'persistentState' attribute not supported by the CDM on Firefox beta47

RESOLVED FIXED in Firefox 50

Status

()

P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: karl.gallagher, Assigned: cpearce, NeedInfo)

Tracking

(Blocks: 1 bug)

47 Branch
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(10 attachments)

1.09 KB, text/html
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.79 Safari/537.36

Steps to reproduce:

Use (modified) shaka player to play any widevine encrypted content :
https://github.com/google/shaka-player

Modification  - eme_manager.js - force the use of persistent state via always setting "persistentState: 'required' "


Actual results:

The CDM makes a single license request (per key) to the proxy without any private information embedded about the device.  Therefore the documented widevine feature  - 'Provider Client Token' cannot be generated by the license proxy.


Expected results:

The CDM should make two requests to the proxy for each key it needs

1.)A request for the server certificate (public key) that is used to obfuscate the device private data (normally this is much shorter in length than the regular key request)

2.)The key request itself with additional data.
(Reporter)

Comment 1

3 years ago
Please note : the above expected results can be observed using the same CDM version on Chrome browser

Updated

3 years ago
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
(Assignee)

Updated

3 years ago
Blocks: 1222845
(Assignee)

Updated

3 years ago
Priority: -- → P2
This issue was also reported in the Shaka Player's GitHub repo:

https://github.com/google/shaka-player/issues/401
(Assignee)

Comment 3

3 years ago
Karl: There's no eme_manager.js that I can find in the latest shaka-player source code. Maybe your version is not the latest version?

Should I get the same result if I patch shaka.media.DrmEngine.prototype.prepareMediaKeyConfigs_:

https://github.com/google/shaka-player/blob/master/lib/media/drm_engine.js#L329

I don't seem to; I always see what looks like two license requests going out in both Chrome and Firefox with the above change.

Can you provide a live example which demonstrates the bug?
(Assignee)

Updated

3 years ago
Flags: needinfo?(karl.gallagher)
(Reporter)

Comment 4

3 years ago
Hi,

I have made the required modifications  -https://dl.dropboxusercontent.com/u/25125951/shaka-player-1.6.5.tar.gz

The latest stable version is 1.6.5 which does have eme_manager.js  - I assume you tried the v2 beta for which significant refactoring was done AFAIK.

The best way to see the issue is to compare with Chrome, however you must host the player on a webserver exposed via a Private IP address (For some reason Chrome won't exercise extended EME functionality on an insecure external origin, and I don't have the ability to host this on a secure origin).

If you attempt playback of the 'oops' mutliDRM stream you will see 4 proxy POSTs from Chrome (the first two are two bytes in length). However with Firefox 47 I only see the 2 license requests for each key.

The first two requests are for the Widevine server certificate which is used to obfuscate the server generated data that the CDM will (eventually) persist.

Note: It is expected that the player will eventually fail once the clear-lead fragments are consumed  - this is due to the fact that it has not been developed to handle response for the the server certificate. request
Flags: needinfo?(karl.gallagher)
(Reporter)

Comment 5

3 years ago
Also note that I can still reproduce this issue on Firefox nightly (v50).
(Reporter)

Comment 6

3 years ago
Hello,

Any update on this?   Has this been acknowledged as a bug ?
(Reporter)

Comment 8

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #7)
> Karl: I extracted your zip file here:
> http://people.mozilla.org/~cpearce/shaka/
> 
> However, when I play oops multi-DRM in chrome, I only see two requests that
> look like license requests, these are to these URLs:
> 
> http://dash-mse-test.appspot.com/api/drm/
> widevine?drm_system=widevine&source=YOUTUBE&video_id=03681262dc412c06&ip=0.0.
> 0.0&ipbits=0&expire=19000000000&sparams=ip,ipbits,expire,source,video_id,
> drm_system&signature=289105AFC9747471DB0D2A998544CC1DAF75B8F9.
> 18DE89BB7C1CE9B68533315D0F84DF86387C6BB3&key=test_key1
> 
> http://dash-mse-test.appspot.com/api/drm/
> widevine?drm_system=widevine&source=YOUTUBE&video_id=03681262dc412c06&ip=0.0.
> 0.0&ipbits=0&expire=19000000000&sparams=ip,ipbits,expire,source,video_id,
> drm_system&signature=289105AFC9747471DB0D2A998544CC1DAF75B8F9.
> 18DE89BB7C1CE9B68533315D0F84DF86387C6BB3&key=test_key1
> 
> Is that what you see?

Hi,

Please note the caveat in my bug report WRT where the app is hosted  -  it either needs to be on an internal host (Private IP range) or on an SSL/TLS enabled host  (with proxy both app and proxy URLs the https variant).

With that setup you will see 4 Widevine proxy POSTs on Chrome and 2 Widevine proxy POSTs on Firefox - Chrome behaviour complies with what is expected based on the EME options enabled in the app.

Thanks,
Karl.
(Reporter)

Comment 9

3 years ago
Hi,

Do you require any further information here?   Have you been able to see the differences in behaviour I have described?

Thanks,
Karl.
(Assignee)

Comment 10

3 years ago
I cannot reproduce the expected results if I run the shaka player locally.

I'll have a go at implementing persistentState for EME. Karl, seeing as you can repro the problem, can you help us test this?
Assignee: nobody → cpearce
(Reporter)

Comment 11

3 years ago
Hi,

Yes of course.  Just let me know when you have a build ready.

Karl.
Flags: needinfo?(karl.gallagher)
Mass change P2 -> P3
Priority: P2 → P3
(Assignee)

Updated

3 years ago
Blocks: 1180482
(Assignee)

Updated

3 years ago
Blocks: 1274338
(Assignee)

Comment 13

3 years ago
Created attachment 8768931 [details]
Testcase

Simple testcase for navigator.requestMediaKeySystemAccess().
(Assignee)

Updated

3 years ago
Blocks: 1282142
(Assignee)

Comment 18

2 years ago
Created attachment 8771815 [details]
Bug 1278198 - Update MediaKeySystemConfiguration and MediaKeys to match draft EME spec.

The only thing we're now not up to date on (in terms of WebIDL) is the
"persistent-usage-record" MediaKeySessionType.

Review commit: https://reviewboard.mozilla.org/r/64788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64788/
Attachment #8771815 - Flags: review?(bugs)
Attachment #8771816 - Flags: review?(gsquelart)
Attachment #8771817 - Flags: review?(gsquelart)
Attachment #8771818 - Flags: review?(gsquelart)
Attachment #8771819 - Flags: review?(gsquelart)
Attachment #8771820 - Flags: review?(gsquelart)
Attachment #8771821 - Flags: review?(gsquelart)
Attachment #8771822 - Flags: review?(gsquelart)
(Assignee)

Comment 19

2 years ago
Created attachment 8771816 [details]
Bug 1278198 -  Update EME code to reflect new WebIDL name changes.

Review commit: https://reviewboard.mozilla.org/r/64790/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64790/
(Assignee)

Comment 20

2 years ago
Created attachment 8771817 [details]
Bug 1278198 - Update navigator.requestMediaKeySystemAccess() MOZ_LOG to log new EME WebIDL attributes.

Review commit: https://reviewboard.mozilla.org/r/64792/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64792/
(Assignee)

Comment 21

2 years ago
Created attachment 8771818 [details]
Bug 1278198 - Implement "Get Supported Configuration" algorithm in MediaKeySystemAccess.

Review commit: https://reviewboard.mozilla.org/r/64794/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64794/
(Assignee)

Comment 23

2 years ago
Created attachment 8771820 [details]
Bug 1278198 - Pipe through distinctive identifier and persistent state allowed.

Review commit: https://reviewboard.mozilla.org/r/64798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64798/
(Assignee)

Comment 25

2 years ago
Created attachment 8771822 [details]
Bug 1278198 - Adapt Adobe GMP's obsolete GMPDecryptor interface to new interface.

The Adobe GMP only supports up to GMPDecryptor version 7. We're now up to
version 9.  So we need to provide an adaptor to convert the old version to run
with the new interface.

Review commit: https://reviewboard.mozilla.org/r/64802/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64802/
Attachment #8771816 - Flags: review?(gsquelart) → review+
Comment on attachment 8771816 [details]
Bug 1278198 -  Update EME code to reflect new WebIDL name changes.

https://reviewboard.mozilla.org/r/64790/#review61822
Comment on attachment 8771817 [details]
Bug 1278198 - Update navigator.requestMediaKeySystemAccess() MOZ_LOG to log new EME WebIDL attributes.

https://reviewboard.mozilla.org/r/64792/#review61824

r+ with nit:

::: dom/base/Navigator.cpp:2599
(Diff revision 1)
> +
> +static nsCString
> +ToCString(const MediaKeysRequirement aValue)
> +{
> +  nsCString str("'");
> +  str.Append(nsDependentCString(MediaKeysRequirementValues::strings[(uint32_t)aValue].value));

Please use C++ cast (construction or static_cast).
Attachment #8771817 - Flags: review?(gsquelart) → review+
https://reviewboard.mozilla.org/r/64794/#review61826

Looks fairly good so far, but as some of my comments may involve non-trivial changes, I'd prefer to review this file again after these changes.

::: dom/media/VideoUtils.cpp:450
(Diff revision 1)
> +  if (!ParseCodecsString(codecsStr, aOutCodecs)) {
> +    return false;
> +  }
> +
> +  return true;

You could just 'return ParseCodecsString(codecsStr, aOutCodecs);' here.

::: dom/media/VideoUtils.cpp:484
(Diff revision 1)
> +bool
>  IsH264ContentType(const nsAString& aContentType)

AFAICS IsH264ContentType is not used anymore (in MediaKeySystemAccess.cpp), so you could just get rid of it.
Same with:
- IsAACContentType
- IsVorbisContentType
- IsVP8ContentType
- IsVP9ContentType
If these are all gone, you can probably get rid of CheckContentType as well.

::: dom/media/eme/MediaKeySystemAccess.cpp:355
(Diff revision 1)
> +struct KeySystemContainerSupport {
> +  bool IsSupported() const {

Style: '{' on seperate line.
Same with other methods, enums, and structs below.

::: dom/media/eme/MediaKeySystemAccess.cpp:402
(Diff revision 1)
> +      clearkey.mPersistentState = KeySystemFeatureSupport::Requestable;
> +      clearkey.mDistinctiveIdentifier = KeySystemFeatureSupport::Prohibited;

You don't absolutely need the 'KeySystemFeatureSupport::' there, unless you make the enum an 'enum class' (which would be nice, I think).

::: dom/media/eme/MediaKeySystemAccess.cpp:408
(Diff revision 1)
> +      if (WMFDecoderModule::HasAAC()) {
> +        clearkey.mMP4.mCodecsDecoded.AppendElement(GMP_CODEC_AAC);
> +        clearkey.mMP4.mCodecsDecoded.AppendElement(GMP_CODEC_H264);
> +      } else {
> +        clearkey.mMP4.mCodecsDecrypted.AppendElement(GMP_CODEC_AAC);
> +        clearkey.mMP4.mCodecsDecrypted.AppendElement(GMP_CODEC_H264);
> +      }

Based on this code logic, we can decode XOR decrypt AAC&H264, is that what you really mean? (Or does decoding imply decrypting?)

::: dom/media/eme/MediaKeySystemAccess.cpp:468
(Diff revision 1)
> +      primetime.mMP4.mCodecsDecoded.AppendElement(GMP_CODEC_AAC);
> +      primetime.mMP4.mCodecsDecoded.AppendElement(GMP_CODEC_H264);
> +      sKeySystemConfigs->AppendElement(Move(primetime));
> +    }
> +  }
> +  return *(sKeySystemConfigs.get());

'return \*sKeySystemConfigs;' should Just Work.

::: dom/media/eme/MediaKeySystemAccess.cpp:474
(Diff revision 1)
> +  const nsTArray<KeySystemConfig>& configs = GetSupportedKeySystems();
> +  nsString keySystemString(aKeySystem);
> +  auto itr = std::find_if(configs.begin(),
> +                          configs.end(),
> +                          [keySystemString](const KeySystemConfig& config) {
> +                            return config.mKeySystem.Equals(keySystemString);
> +                          });
> +  if (itr == configs.end()) {
> +    return nullptr;
> +  }
> +  return &(*itr);

I usually applaud the use of <algorithm>, but in this case the code would feel simpler without it:
  for (const KeySystemConfig& config : GetSupportedKeySystems()) \{
    if (config.mKeySystem).Equals(aKeySystem)) \{
      return &config;
    \}
  \}
  return nullptr;

::: dom/media/eme/MediaKeySystemAccess.cpp:551
(Diff revision 1)
> -                     NS_LITERAL_CSTRING("vorbis")) &&
> -         WebMDecoder::CanHandleMediaType(aContentType);
>  }
>  
>  static bool
> -GMPDecryptsAndGeckoDecodesVP8(mozIGeckoMediaPluginService* aGMPService,
> +ToSessionType(const nsAString& aSessionType, MediaKeySessionType* aOutType)

In other functions, out-parameters are passed by reference. For consistency I think you should change this lonely out-pointer to a reference (and get rid of the null-pointer assertion).

::: dom/media/eme/MediaKeySystemAccess.cpp:554
(Diff revision 1)
> -                             const nsAString& aKeySystem,
> -                             const nsAString& aContentType,
> -                             DecoderDoctorDiagnostics* aDiagnostics)
>  {
> -  MOZ_ASSERT(HaveGMPFor(aGMPService,
> -             NS_ConvertUTF16toUTF8(aKeySystem),
> +  MOZ_ASSERT(aOutType);
> +  if (aSessionType.EqualsASCII(MediaKeySessionTypeValues::strings[(uint32_t)MediaKeySessionType::Temporary].value)) {

Style: Way over the 80-column limit.
Style: Please use C++ cast.

Same issues 4 lines below.

::: dom/media/eme/MediaKeySystemAccess.cpp:595
(Diff revision 1)
> -                 DecoderDoctorDiagnostics* aDiagnostics)
> +                         DecoderDoctorDiagnostics* aDiagnostics)
>  {
> -  if (IsH264ContentType(aVideoType)) {
> -    return GMPDecryptsAndDecodesH264(aGMPService, aKeySystem, aDiagnostics) ||
> -           GMPDecryptsAndGeckoDecodesH264(aGMPService, aKeySystem, aVideoType, aDiagnostics);
> +  // Let local accumulated configuration be a local copy of partial configuration.
> +  // (Note: It's not necessary for us to maintain a local copy, as we don't need
> +  // to test whether capabilites from previous calls to this algorithm work with
> +  // the capcbilities currently being considered in this call. )

'capcbilities' -> 'capabilities'

::: dom/media/eme/MediaKeySystemAccess.cpp:602
(Diff revision 1)
> +  // Let supported media capabilities be an empty sequence of
> +  // MediaKeySystemMediaCapability dictionaries.
> +  Sequence<MediaKeySystemMediaCapability> supportedCapabilities;
> +
> +  // For each requested media capability in requested media capabilities:
> +  for (const MediaKeySystemMediaCapability& capbilities : aRequestedCapabilities) {

'capbilities' -> 'capabilities'
(This word doesn't like you!)

::: dom/media/eme/MediaKeySystemAccess.cpp:652
(Diff revision 1)
> +    if (invalid) {
> +      continue;
> +    }
> +
> +    // If the user agent does not support container, continue to the next iteration.
> +    // The case-sensitivity of string comparisons is determined by the appropriate RFC.

This case-sensitivity statement is a bit obscure, and it's not obvious what you're doing about it.
I see there's a note in the specs: 'Per RFC 6838 [RFC6838], "Both top-level type and subtype names are case-insensitive."' Maybe you could include it, and explain that we are following it elsewhere (because we are, right?!)
(Note: I'm reading https://www.w3.org/TR/2016/CR-encrypted-media-20160705/ )

::: dom/media/eme/MediaKeySystemAccess.cpp:695
(Diff revision 1)
> +    if (codecs.IsEmpty()) {
> +      // If container normatively implies a specific set of codecs and codec constraints :
> +      // Let parameters be that set.

What about "Otherwise: Continue to the next iteration" from the specs?
I'm guessing our supported containers MP4 and WebM cannot fall into that case.
I'd suggest still including that line somewhere, so we know it was considered (and it will need to be considered if we add more containers).

::: dom/media/eme/MediaKeySystemAccess.cpp:726
(Diff revision 1)
> +      continue;
> +    }
> +
> +    // If robustness is not the empty string and contains an unrecognized
> +    // value or a value not supported by implementation, continue to the
> +    // next iteration.String comparison is case-sensitive.

Need space between 'iteration.' and 'String'.

::: dom/media/eme/MediaKeySystemAccess.cpp:802
(Diff revision 1)
> +    requirement = MediaKeysRequirement::Not_allowed;
> +  }
> +
> +  // Follow the steps for requirement from the following list:
> +  switch (requirement) {
> +    case MediaKeysRequirement::Required: {

'{}' blocks are not strictly needed in any of these case statements.

::: dom/media/eme/MediaKeySystemAccess.cpp:893
(Diff revision 1)
> +  // Follow the steps for the first matching condition from the following list:
> +  // If the sessionTypes member is present in candidate configuration.
> +  if (aCandidate.mSessionTypes.WasPassed()) {
> +    // Let session types be candidate configuration's sessionTypes member.
> +    // For each value in session types:
> +    for (const auto& sessionTypeString : aCandidate.mSessionTypes.Value()) {

You don't mention the 2nd item in "the following list": 'Otherwise Let session types be [ "temporary" ].'
Is that somehow handled implicitly?
If yes, please add a comment.
If not, you'll probably need to deal with this here, e.g.:
  nsTArray<MediaKeySessionType>& sessionTypes =
    aCandidate.mSessionTypes.WasPassed()
    ? aCandidate.mSessionTypes.Value()
    : something that returns an nsTArray with MediaKeySessionType::Temporary
And then run steps 13-14 on that.

::: dom/media/eme/MediaKeySystemAccess.cpp:939
(Diff revision 1)
> +          IsPersistentSessionType(sessionType)) {
> +        config.mPersistentState = MediaKeysRequirement::Required;
> +      }
> +    }
> +    // Set the sessionTypes member of accumulated configuration to session types.
> +	  config.mSessionTypes.Construct(aCandidate.mSessionTypes.Value());

tab -> 2 spaces
Where does that tab come from? (Guessing: from the copy&pasted specs page?) You may want to do a quick search for other tabs in this file, just in case.

::: dom/media/eme/MediaKeySystemAccess.cpp:942
(Diff revision 1)
> +  // If the videoCapabilities and audioCapabilities members in candidate
> +  // configuration are both empty, return NotSupported.
> +  // TODO: Most sites using EME still don't pass capabilities, so we
> +  // can't reject on it yet without breaking them. So add this later.

Could you easily output some browser/console logs? It would exercise this code path (if you choose to add it), and hopefully some webdevs would notice and start doing the right thing.

::: dom/media/eme/MediaKeySystemAccess.cpp:950
(Diff revision 1)
> +  // can't reject on it yet without breaking them. So add this later.
> +
> +  // If the videoCapabilities member in candidate configuration is non-empty:
> +  if (!aCandidate.mVideoCapabilities.IsEmpty()) {
> +    // Let video capabilities be the result of executing the Get Supported
> +    // Capabilities for Audio / Video Type algorithm on Video, candidate

Remove spaces around slash.

::: dom/media/eme/MediaKeySystemAccess.cpp:973
(Diff revision 1)
> +  } else {
> +    // Otherwise:
> +    // Set the videoCapabilities member of accumulated configuration to an empty sequence.
> +  }
> +
> +  // If the audioCapabilities member in candidate configuration is non - empty:

Remove spaces in 'non - empty'

::: dom/media/eme/MediaKeySystemAccess.cpp:976
(Diff revision 1)
> +  }
> +
> +  // If the audioCapabilities member in candidate configuration is non - empty:
> +  if (!aCandidate.mAudioCapabilities.IsEmpty()) {
> +    // Let audio capabilities be the result of executing the Get Supported Capabilities
> +    // for Audio / Video Type algorithm on Audio, candidate configuration's audioCapabilities

Remove spaces around slash.

::: dom/media/eme/MediaKeySystemAccess.cpp:1049
(Diff revision 1)
>    aOutConfig = config;
>  
>    return true;

To be complete, you might as well show step 23: 'Return accumulated configuration.'
Comment on attachment 8771815 [details]
Bug 1278198 - Update MediaKeySystemConfiguration and MediaKeys to match draft EME spec.

https://reviewboard.mozilla.org/r/64788/#review61842

::: dom/webidl/MediaKeys.webidl:13
(Diff revision 1)
>   *
>   * Copyright © 2014 W3C® (MIT, ERCIM, Keio, Beihang), All Rights Reserved.
>   * W3C liability, trademark and document use rules apply.
>   */
>  
> -enum SessionType { "temporary", "persistent" };
> +enum MediaKeySessionType {

Could you add some comment here that persistent-usage-record isn't supported, and perhaps even add it as
// "persistent-usage-record",
Attachment #8771815 - Flags: review?(bugs) → review+
Comment on attachment 8771819 [details]
Bug 1278198 - Add Widevine FileIO.

https://reviewboard.mozilla.org/r/64796/#review61848

r+ with nits:

::: dom/media/gmp/widevine-adapter/WidevineFileIO.h:9
(Diff revision 1)
> +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef WidevineFileIO_h_
> +#define WidevineFileIO_h_
> +
> +#include <stddef.h>

Do you actually need to #include <stddef.h>?

::: dom/media/gmp/widevine-adapter/WidevineFileIO.cpp:5
(Diff revision 1)
> +#include "WidevineFileIO.h"
> +#include "WidevineUtils.h"
> +#include "WidevineAdapter.h"
> +
> +#include <string>

Already included in the header file.
Attachment #8771819 - Flags: review?(gsquelart) → review+
Comment on attachment 8771820 [details]
Bug 1278198 - Pipe through distinctive identifier and persistent state allowed.

https://reviewboard.mozilla.org/r/64798/#review61852
Attachment #8771820 - Flags: review?(gsquelart) → review+
Comment on attachment 8771821 [details]
Bug 1278198 - Fix tests to work with new EME API.

https://reviewboard.mozilla.org/r/64800/#review61854

r+ assuming no major changes due to my comments in part 4 (e.g., maybe 'sessionTypes' should default to ["temporary"] when not provided?)
Attachment #8771821 - Flags: review?(gsquelart) → review+
Comment on attachment 8771822 [details]
Bug 1278198 - Adapt Adobe GMP's obsolete GMPDecryptor interface to new interface.

https://reviewboard.mozilla.org/r/64802/#review61856
Attachment #8771822 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 35

2 years ago
https://reviewboard.mozilla.org/r/64794/#review61826

> You could just 'return ParseCodecsString(codecsStr, aOutCodecs);' here.

Done.

> You don't absolutely need the 'KeySystemFeatureSupport::' there, unless you make the enum an 'enum class' (which would be nice, I think).

Made it an enum class.

> Based on this code logic, we can decode XOR decrypt AAC&H264, is that what you really mean? (Or does decoding imply decrypting?)

The CDM either decrypts and Gecko decodes, or the CDM both decrypts and decodes, and passes decoded samples back to Gecko for rendering. It can do one of those, but not both. That is, either the decode is considered "DRM robust" (i.e. is inside the CDM) or it isn't.

I've tried to make that clearer by changing KeySystemContainerSupport.

I'll also check that WMF has H264 here, rather than assume we have H264 if we have AAC (though I think it's unlikely we'd have AAC without H264).

> 'return \*sKeySystemConfigs;' should Just Work.

Done.

> I usually applaud the use of <algorithm>, but in this case the code would feel simpler without it:
>   for (const KeySystemConfig& config : GetSupportedKeySystems()) \{
>     if (config.mKeySystem).Equals(aKeySystem)) \{
>       return &config;
>     \}
>   \}
>   return nullptr;

Done.

> In other functions, out-parameters are passed by reference. For consistency I think you should change this lonely out-pointer to a reference (and get rid of the null-pointer assertion).

Done.

> This case-sensitivity statement is a bit obscure, and it's not obvious what you're doing about it.
> I see there's a note in the specs: 'Per RFC 6838 [RFC6838], "Both top-level type and subtype names are case-insensitive."' Maybe you could include it, and explain that we are following it elsewhere (because we are, right?!)
> (Note: I'm reading https://www.w3.org/TR/2016/CR-encrypted-media-20160705/ )

I'll add a note. We're using nsContentTypeParser, and that's case insensitive, and converts all the parameter outputs to lower case.

> What about "Otherwise: Continue to the next iteration" from the specs?
> I'm guessing our supported containers MP4 and WebM cannot fall into that case.
> I'd suggest still including that line somewhere, so we know it was considered (and it will need to be considered if we add more containers).

Added that line.

This section will be fun when we start adding VP9/Opus to MP4.

> '{}' blocks are not strictly needed in any of these case statements.

Indeed, strictly {} are not requried for case statements, but I think it's a good habit to be in.

> tab -> 2 spaces
> Where does that tab come from? (Guessing: from the copy&pasted specs page?) You may want to do a quick search for other tabs in this file, just in case.

Visual Studio spontaneously reset my tab/spaces settings, and I missed cleaning up some of the damage. I suspect it due to one of my plugins updating.

> Could you easily output some browser/console logs? It would exercise this code path (if you choose to add it), and hopefully some webdevs would notice and start doing the right thing.

That is a good idea; we can do that in a follow up.

> To be complete, you might as well show step 23: 'Return accumulated configuration.'

Done.
(Assignee)

Comment 36

2 years ago
https://reviewboard.mozilla.org/r/64800/#review61854

Thanks, no changes are be necessary.
(Assignee)

Comment 37

2 years ago
Comment on attachment 8771815 [details]
Bug 1278198 - Update MediaKeySystemConfiguration and MediaKeys to match draft EME spec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64788/diff/1-2/
(Assignee)

Comment 38

2 years ago
Comment on attachment 8771816 [details]
Bug 1278198 -  Update EME code to reflect new WebIDL name changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64790/diff/1-2/
(Assignee)

Comment 39

2 years ago
Comment on attachment 8771817 [details]
Bug 1278198 - Update navigator.requestMediaKeySystemAccess() MOZ_LOG to log new EME WebIDL attributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64792/diff/1-2/
(Assignee)

Comment 40

2 years ago
Comment on attachment 8771818 [details]
Bug 1278198 - Implement "Get Supported Configuration" algorithm in MediaKeySystemAccess.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64794/diff/1-2/
(Assignee)

Comment 41

2 years ago
Comment on attachment 8771819 [details]
Bug 1278198 - Add Widevine FileIO.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64796/diff/1-2/
(Assignee)

Comment 42

2 years ago
Comment on attachment 8771820 [details]
Bug 1278198 - Pipe through distinctive identifier and persistent state allowed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64798/diff/1-2/
(Assignee)

Comment 43

2 years ago
Comment on attachment 8771821 [details]
Bug 1278198 - Fix tests to work with new EME API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64800/diff/1-2/
(Assignee)

Comment 44

2 years ago
Comment on attachment 8771822 [details]
Bug 1278198 - Adapt Adobe GMP's obsolete GMPDecryptor interface to new interface.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64802/diff/1-2/
Comment on attachment 8771818 [details]
Bug 1278198 - Implement "Get Supported Configuration" algorithm in MediaKeySystemAccess.

https://reviewboard.mozilla.org/r/64794/#review62758

r+ pending fixes:

::: dom/media/eme/MediaKeySystemAccess.cpp:381
(Diff revisions 1 - 2)
> +  {
> +    // Can't both decrypt and decrypt-and-decode a codec.
> +    MOZ_ASSERT(!Decrypts(aCodec));
> +    // Prevent duplicates.
> +    MOZ_ASSERT(!DecryptsAndDecodes(aCodec));
> +    SetCanDecryptAndDecode(aCodec);

Recursion, see next comment for solution.

::: dom/media/eme/MediaKeySystemAccess.cpp:390
(Diff revisions 1 - 2)
> +  {
> +    // Prevent duplicates.
> +    MOZ_ASSERT(!Decrypts(aCodec));
> +    // Can't both decrypt and decrypt-and-decode a codec.
> +    MOZ_ASSERT(!DecryptsAndDecodes(aCodec));
> +    SetCanDecrypt(aCodec);

Recursion, see previous comment for solution.

::: dom/media/eme/MediaKeySystemAccess.cpp:877
(Diff revisions 1 - 2)
>  
> +// 3.1.2.2, step 12
> +// Follow the steps for the first matching condition from the following list:
> +// If the sessionTypes member is present in candidate configuration.
> +// Let session types be candidate configuration's sessionTypes member.
> +// Otherwise let session types be["temporary"].

Add space between 'be' and '['.

::: dom/media/eme/MediaKeySystemAccess.cpp:891
(Diff revisions 1 - 2)
> +    using MediaKeySessionTypeValues::strings;
> +    const char* temporary = strings[static_cast<uint32_t>(MediaKeySessionType::Temporary)].value;
> +    // Note: fallible. Results in an empty array.
> +    sessionTypes.AppendElement(NS_ConvertUTF8toUTF16(nsDependentCString(temporary)), mozilla::fallible);
> +  }
> +  return Move(sessionTypes);

This is the only 'return' in this function, and it is always returning the same local variable of the correct type, so you should remove the 'Move()' to guarantee RVO.

(Note: cpearce found the recursion issues himself, I failed the test)
Attachment #8771818 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 46

2 years ago
Comment on attachment 8771818 [details]
Bug 1278198 - Implement "Get Supported Configuration" algorithm in MediaKeySystemAccess.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64794/diff/2-3/
(Assignee)

Comment 47

2 years ago
Comment on attachment 8771819 [details]
Bug 1278198 - Add Widevine FileIO.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64796/diff/2-3/
(Assignee)

Comment 48

2 years ago
Comment on attachment 8771820 [details]
Bug 1278198 - Pipe through distinctive identifier and persistent state allowed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64798/diff/2-3/
(Assignee)

Comment 49

2 years ago
Comment on attachment 8771821 [details]
Bug 1278198 - Fix tests to work with new EME API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64800/diff/2-3/
(Assignee)

Comment 50

2 years ago
Comment on attachment 8771822 [details]
Bug 1278198 - Adapt Adobe GMP's obsolete GMPDecryptor interface to new interface.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64802/diff/2-3/
(Assignee)

Comment 51

2 years ago
Comment on attachment 8771818 [details]
Bug 1278198 - Implement "Get Supported Configuration" algorithm in MediaKeySystemAccess.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64794/diff/2-3/
(Assignee)

Comment 52

2 years ago
Comment on attachment 8771819 [details]
Bug 1278198 - Add Widevine FileIO.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64796/diff/2-3/
(Assignee)

Comment 53

2 years ago
Comment on attachment 8771820 [details]
Bug 1278198 - Pipe through distinctive identifier and persistent state allowed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64798/diff/2-3/
(Assignee)

Comment 54

2 years ago
Comment on attachment 8771821 [details]
Bug 1278198 - Fix tests to work with new EME API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64800/diff/2-3/
(Assignee)

Comment 55

2 years ago
Comment on attachment 8771822 [details]
Bug 1278198 - Adapt Adobe GMP's obsolete GMPDecryptor interface to new interface.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64802/diff/2-3/
(Assignee)

Comment 56

2 years ago
Comment on attachment 8771822 [details]
Bug 1278198 - Adapt Adobe GMP's obsolete GMPDecryptor interface to new interface.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64802/diff/3-4/
(Assignee)

Comment 60

2 years ago
Created attachment 8773187 [details]
Bug 1278198 - Enforce codecs match the capability and content type in GetSupportedCapabilities().

We're supposed to reject MediaKeySystemCapabilities which have a contentType
that has codecs which don't match their major type, i.e. audio codecs in a
video container type.

I missed that, and it's causing us to fail the
test_eme_requestMediaKeySystemAccess case "MP4 video container with both audio
and video codec type in videoType".

Review commit: https://reviewboard.mozilla.org/r/65914/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65914/
Attachment #8773187 - Flags: review?(gsquelart)
Attachment #8773187 - Flags: review?(gsquelart) → review+
Comment on attachment 8773187 [details]
Bug 1278198 - Enforce codecs match the capability and content type in GetSupportedCapabilities().

https://reviewboard.mozilla.org/r/65914/#review62838
(Assignee)

Comment 62

2 years ago
Comment on attachment 8771815 [details]
Bug 1278198 - Update MediaKeySystemConfiguration and MediaKeys to match draft EME spec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64788/diff/2-3/
(Assignee)

Comment 63

2 years ago
Comment on attachment 8771816 [details]
Bug 1278198 -  Update EME code to reflect new WebIDL name changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64790/diff/2-3/
(Assignee)

Comment 64

2 years ago
Comment on attachment 8771817 [details]
Bug 1278198 - Update navigator.requestMediaKeySystemAccess() MOZ_LOG to log new EME WebIDL attributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64792/diff/2-3/
(Assignee)

Comment 65

2 years ago
Comment on attachment 8771818 [details]
Bug 1278198 - Implement "Get Supported Configuration" algorithm in MediaKeySystemAccess.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64794/diff/3-4/
(Assignee)

Comment 66

2 years ago
Comment on attachment 8771819 [details]
Bug 1278198 - Add Widevine FileIO.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64796/diff/3-4/
(Assignee)

Comment 67

2 years ago
Comment on attachment 8771820 [details]
Bug 1278198 - Pipe through distinctive identifier and persistent state allowed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64798/diff/3-4/
(Assignee)

Comment 68

2 years ago
Comment on attachment 8771821 [details]
Bug 1278198 - Fix tests to work with new EME API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64800/diff/3-4/
(Assignee)

Comment 69

2 years ago
Comment on attachment 8771822 [details]
Bug 1278198 - Adapt Adobe GMP's obsolete GMPDecryptor interface to new interface.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64802/diff/4-5/
(Assignee)

Comment 70

2 years ago
Comment on attachment 8773187 [details]
Bug 1278198 - Enforce codecs match the capability and content type in GetSupportedCapabilities().

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

Comment 72

2 years ago
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92a49962cec3
Update MediaKeySystemConfiguration and MediaKeys to match draft EME spec. r=smaug
https://hg.mozilla.org/integration/autoland/rev/656b778e0f15
Update EME code to reflect new WebIDL name changes. r=gerald
https://hg.mozilla.org/integration/autoland/rev/ca66f75b7fb0
Update navigator.requestMediaKeySystemAccess() MOZ_LOG to log new EME WebIDL attributes. r=gerald
https://hg.mozilla.org/integration/autoland/rev/9c3ef7ef33d9
Implement "Get Supported Configuration" algorithm in MediaKeySystemAccess. r=gerald
https://hg.mozilla.org/integration/autoland/rev/1894b74b8931
Add Widevine FileIO. r=gerald
https://hg.mozilla.org/integration/autoland/rev/af577b7547c4
Pipe through distinctive identifier and persistent state allowed. r=gerald
https://hg.mozilla.org/integration/autoland/rev/1a73ee138e23
Fix tests to work with new EME API. r=gerald
https://hg.mozilla.org/integration/autoland/rev/c9e56c91112e
Adapt Adobe GMP's obsolete GMPDecryptor interface to new interface. r=gerald
https://hg.mozilla.org/integration/autoland/rev/3ab20077e16d
Enforce codecs match the capability and content type in GetSupportedCapabilities(). r=gerald
Hi Chris, have back this out due to GTest crash, https://treeherder.mozilla.org/logviewer.html#?job_id=870700&repo=autoland#L11024, please help to check it. Thanks.
Flags: needinfo?(cpearce)

Comment 78

2 years ago
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f302d665680d
Update MediaKeySystemConfiguration and MediaKeys to match draft EME spec. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/943ead383727
Update EME code to reflect new WebIDL name changes. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/2866a18663fc
Update navigator.requestMediaKeySystemAccess() MOZ_LOG to log new EME WebIDL attributes. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3cb023ed5a8
Implement "Get Supported Configuration" algorithm in MediaKeySystemAccess. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a94e3e85347
Add Widevine FileIO. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/5325ebce5681
Pipe through distinctive identifier and persistent state allowed. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6744f21215
Fix tests to work with new EME API. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/aec9905b06fe
Adapt Adobe GMP's obsolete GMPDecryptor interface to new interface. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad262be679de
Enforce codecs match the capability and content type in GetSupportedCapabilities(). r=gerald
(Assignee)

Comment 79

2 years ago
(In reply to Iris Hsiao [:ihsiao] from comment #73)
> Hi Chris, have back this out due to GTest crash,
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=870700&repo=autoland#L11024, please help to check it. Thanks.

Relanded.
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.