Closed Bug 1278198 Opened 8 years ago Closed 8 years ago

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

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
47 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: karl.gallagher, Assigned: cpearce)

References

Details

Attachments

(10 files)

Testcase
1.09 KB, text/html
Details
Bug 1278198 - Update MediaKeySystemConfiguration and MediaKeys to match draft EME spec.
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1278198 - Update EME code to reflect new WebIDL name changes.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1278198 - Update navigator.requestMediaKeySystemAccess() MOZ_LOG to log new EME WebIDL attributes.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1278198 - Implement "Get Supported Configuration" algorithm in MediaKeySystemAccess.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1278198 - Add Widevine FileIO.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1278198 - Pipe through distinctive identifier and persistent state allowed.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1278198 - Fix tests to work with new EME API.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1278198 - Adapt Adobe GMP's obsolete GMPDecryptor interface to new interface.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1278198 - Enforce codecs match the capability and content type in GetSupportedCapabilities().
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
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.
Please note : the above expected results can be observed using the same CDM version on Chrome browser
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
Priority: -- → P2
This issue was also reported in the Shaka Player's GitHub repo: https://github.com/google/shaka-player/issues/401
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?
Flags: needinfo?(karl.gallagher)
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)
Also note that I can still reproduce this issue on Firefox nightly (v50).
Hello, Any update on this? Has this been acknowledged as a bug ?
(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.
Hi, Do you require any further information here? Have you been able to see the differences in behaviour I have described? Thanks, Karl.
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
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
Attached file Testcase —
Simple testcase for navigator.requestMediaKeySystemAccess().
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)
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 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+
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.
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/
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/
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/
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/
Comment on attachment 8771819 [details] Bug 1278198 - Add Widevine FileIO. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64796/diff/1-2/
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/
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/
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+
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/
Comment on attachment 8771819 [details] Bug 1278198 - Add Widevine FileIO. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64796/diff/2-3/
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/
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/
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/
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/
Comment on attachment 8771819 [details] Bug 1278198 - Add Widevine FileIO. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64796/diff/2-3/
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/
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/
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/
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/
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
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/
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/
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/
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/
Comment on attachment 8771819 [details] Bug 1278198 - Add Widevine FileIO. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64796/diff/3-4/
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/
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/
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/
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/
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)
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
(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)
Flags: needinfo?(karl.gallagher)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: