Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
923283002:
Implement checks for distinctiveIdentifier and persistentState in requestMediaKeySystemAccess(). (Closed)
|
Created:
5 years, 10 months ago by sandersd (OOO until July 31) Modified:
5 years, 10 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, eme-reviews_chromium.org, gunsch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement checks for distinctiveIdentifier and persistentState in requestMediaKeySystemAccess().
Also includes additional metadata in KeySystemInfo to support those checks.
BUG=4484922
Committed: https://crrev.com/f629e579c00a93751c75883d48c600d3df4d1ed6
Cr-Commit-Position: refs/heads/master@{#317257}
Patch Set 1 #
Total comments: 63
Patch Set 2 : #Patch Set 3 : Rebase #Patch Set 4 : Match rebased method names. #Patch Set 5 : Cast Widevine config #
Total comments: 82
Patch Set 6 : Fix namespacing. #Patch Set 7 : #Patch Set 8 : Fix compile. #
Total comments: 22
Patch Set 9 : Nits. #
Total comments: 8
Patch Set 10 : Corrected permissions. #
Total comments: 4
Patch Set 11 : Whitelist. #Patch Set 12 : Comment for ECK #
Total comments: 4
Patch Set 13 : Typo. #Patch Set 14 : Remove dependency on Blink roll. #Patch Set 15 : 0ul #Patch Set 16 : Hacks to remove Blink dependency. #Patch Set 17 : Fix media_unittests. #Patch Set 18 : Remove Blink dependency again. #
Messages
Total messages: 30 (7 generated)
sandersd@chromium.org changed reviewers: + jrummell@chromium.org
LG. https://codereview.chromium.org/923283002/diff/1/components/cdm/renderer/andr... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/923283002/diff/1/components/cdm/renderer/andr... components/cdm/renderer/android_key_systems.cc:78: // Prevent creation thorugh the requestMediaKeySystemAccess() API. s/thorugh/through/ https://codereview.chromium.org/923283002/diff/1/media/base/key_system_info.cc File media/base/key_system_info.cc (right): https://codereview.chromium.org/923283002/diff/1/media/base/key_system_info.c... media/base/key_system_info.cc:9: KeySystemInfo::KeySystemInfo() Is this required? I don't see any user of it (although maybe hash_map requires it). https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:74: // 2. Let media type capabilities be empty. (Done by caller.) Can you add an ASSERT(media_type_capabilities.size() == 0); ? https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:230: accumulated_configuration->videoCapabilities = audio_capabilities; video = audio? https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:398: // 7.3. For each value in supportedConfigurations: Two 7.3 comments, remove one.
sandersd@chromium.org changed reviewers: + ddorwin@chromium.org
I still need to review the last three files. https://codereview.chromium.org/923283002/diff/1/components/cdm/renderer/andr... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/923283002/diff/1/components/cdm/renderer/andr... components/cdm/renderer/android_key_systems.cc:78: // Prevent creation thorugh the requestMediaKeySystemAccess() API. Until https://github.com/w3c/encrypted-media/issues/29 is implemented, this does not actually affect requestMediaKeySystemAccess(), right? Is the goal to prevent use of platform key systems with unprefixed EME? If so, maybe we should be clearer about this or handle it differently. Let's discuss. https://codereview.chromium.org/923283002/diff/1/components/cdm/renderer/wide... File components/cdm/renderer/widevine_key_systems.cc (right): https://codereview.chromium.org/923283002/diff/1/components/cdm/renderer/wide... components/cdm/renderer/widevine_key_systems.cc:70: // TODO(sandersd): Change to optional once CDM8 adds the ability to block Add a bug reference. Also, it may be helpful to explain that the goal/result in the meantime is to support "optional" and "required" but not "not-allowed" https://codereview.chromium.org/923283002/diff/1/components/cdm/renderer/wide... components/cdm/renderer/widevine_key_systems.cc:75: // TODO(sandersd): Add PERSISTENT_LICENSE once there is logic to predict if Would it help to have PERSISTENT_LICENSE_WITH_AUTHORIZATION or something so that you can make the decision in the core? https://codereview.chromium.org/923283002/diff/1/components/cdm/renderer/wide... components/cdm/renderer/widevine_key_systems.cc:78: #else This is Windows, Mac, Linux desktop, Cast, and every Linux-based device. We can't answer these questions here. I think we will need to pass them to this function. Note that there are platform-specific calls to AddWidevineWithCodecs, which should solve this. Also, this file/function is intended to contain platform-independent information about the Widevine key system (as much as possible), so this code probably shouldn't be here. https://codereview.chromium.org/923283002/diff/1/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/923283002/diff/1/media/base/eme_constants.h#n... media/base/eme_constants.h:61: EME_REQUIREMENT_NOT_ALLOWED = 1 << 0, Would it make sense (and help with the code) to have a different set of values when specifying key system capabilities? For example, SUPPORTS_NOT_ALLOWED, REQUIRES_ALWAYS, WILL_USE_IF_ALLOWED. https://codereview.chromium.org/923283002/diff/1/media/base/eme_constants.h#n... media/base/eme_constants.h:63: EME_REQUIREMENT_OPTIONAL = Embedders should never use this, right? Maybe it shouldn't be in this file. I'm not sure "OPTIONAL" here has the same meaning as the spec - it's really just "supports both", which we probably don't need. Also, see above. https://codereview.chromium.org/923283002/diff/1/media/base/key_system_info.cc File media/base/key_system_info.cc (right): https://codereview.chromium.org/923283002/diff/1/media/base/key_system_info.c... media/base/key_system_info.cc:9: KeySystemInfo::KeySystemInfo() On 2015/02/14 01:28:35, jrummell wrote: > Is this required? I don't see any user of it (although maybe hash_map requires > it). If so, we should consider eliminating the other one. https://codereview.chromium.org/923283002/diff/1/media/base/key_system_info.c... media/base/key_system_info.cc:22: persistent_state_requirement(EME_REQUIREMENT_REQUIRED), Default to the worst case: For this one, not_allowed (no support). For the identifier, the worst case is it's required. https://codereview.chromium.org/923283002/diff/1/media/base/key_system_info.h File media/base/key_system_info.h (right): https://codereview.chromium.org/923283002/diff/1/media/base/key_system_info.h... media/base/key_system_info.h:33: KeySystemInfo(); Why do we have a default constructor and no longer a destructor defined? https://codereview.chromium.org/923283002/diff/1/media/base/key_system_info.h... media/base/key_system_info.h:45: EmeRequirement persistent_state_requirement; Related to earlier: Is it requirement or support?
https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc#ne... media/base/key_systems.cc:334: DCHECK(!info.key_system.empty()) << "Missing key system name"; nit: The << part is probably unnecessary for this one. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc#ne... media/base/key_systems.cc:335: DCHECK(!IsConcreteSupportedKeySystem(info.key_system)) I believe this needs to change after https://codereview.chromium.org/912233004/. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc#ne... media/base/key_systems.cc:344: NOTREACHED() << "Key system '" << info.key_system << "' does not support" nit: The issue is really that no session types were specified for the system. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc#ne... media/base/key_systems.cc:348: We can DCHECK that the persistent session types are not specified unless persistent state is supported. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc#ne... media/base/key_systems.cc:546: EmeRequirement requirement_mask = An enum cannot be a mask. You must use an int. If the member below cannot be a mask, you can just rename this. If it can, you need to change both types. Also, it is really the support rather than the requirement. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc#ne... media/base/key_systems.cc:561: EmeRequirement requirement_mask = ditto https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc#ne... media/base/key_systems.cc:576: SupportedSessionTypes session_type_mask = This one is okay. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.h File media/base/key_systems.h (right): https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.h#new... media/base/key_systems.h:84: MEDIA_EXPORT bool IsSupportedDistinctiveIdentifierRequirement( We may need to pass more information (is authorized), track this in an object, or return more states (i.e. yes, now, if authorized). https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.h#new... media/base/key_systems.h:86: EmeRequirement requirement); In this case, we are passing the requirement from the app. We could do the conversion inside these functions. https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:121: NOTREACHED(); Can we get away with no code in this "branch" since all enum values are covered? https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:173: requirement = ConvertToEmeRequirement(candidate.distinctiveIdentifier); collapse to one statement. Alternatively, can this be made to fit in 174? https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:219: // Capabilities for Media Type algorithm on Video, candidate Audio https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:229: // 8.3. Add video capabilities to accumulated configuration. audio https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:241: // (Without robustness support, capabilities do not affect this.) TODO with bug? https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:287: if (IsSupportedSessionType(key_system, EME_SESSION_TYPE_TEMPORARY)) { Per the spec, this can be a DCHECK https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:412: // 7.3.3. If supported configuration is not null, [initialize and return nit: This should be above 409, right?
https://codereview.chromium.org/923283002/diff/1/components/cdm/renderer/andr... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/923283002/diff/1/components/cdm/renderer/andr... components/cdm/renderer/android_key_systems.cc:78: // Prevent creation thorugh the requestMediaKeySystemAccess() API. On 2015/02/14 01:28:35, jrummell wrote: > s/thorugh/through/ Done. https://codereview.chromium.org/923283002/diff/1/components/cdm/renderer/andr... components/cdm/renderer/android_key_systems.cc:78: // Prevent creation thorugh the requestMediaKeySystemAccess() API. On 2015/02/17 20:56:46, ddorwin wrote: > Until https://github.com/w3c/encrypted-media/issues/29 is implemented, this does > not actually affect requestMediaKeySystemAccess(), right? > > Is the goal to prevent use of platform key systems with unprefixed EME? If so, > maybe we should be clearer about this or handle it differently. Let's discuss. Previously there was a specific check for this, but it's been removed in this patch set. Is there something more we should be doing? (I'm assuming it's not necessary since you are disabling unprefixed on the relevant platforms.) https://codereview.chromium.org/923283002/diff/1/components/cdm/renderer/wide... File components/cdm/renderer/widevine_key_systems.cc (right): https://codereview.chromium.org/923283002/diff/1/components/cdm/renderer/wide... components/cdm/renderer/widevine_key_systems.cc:70: // TODO(sandersd): Change to optional once CDM8 adds the ability to block On 2015/02/17 20:56:47, ddorwin wrote: > Add a bug reference. > > Also, it may be helpful to explain that the goal/result in the meantime is to > support "optional" and "required" but not "not-allowed" Done. https://codereview.chromium.org/923283002/diff/1/components/cdm/renderer/wide... components/cdm/renderer/widevine_key_systems.cc:75: // TODO(sandersd): Add PERSISTENT_LICENSE once there is logic to predict if On 2015/02/17 20:56:47, ddorwin wrote: > Would it help to have PERSISTENT_LICENSE_WITH_AUTHORIZATION or something so that > you can make the decision in the core? Done. This isn't a perfect definition, since what we really want is more like 'WITH_RA_PREDICTED', but it's close. https://codereview.chromium.org/923283002/diff/1/components/cdm/renderer/wide... components/cdm/renderer/widevine_key_systems.cc:78: #else On 2015/02/17 20:56:46, ddorwin wrote: > This is Windows, Mac, Linux desktop, Cast, and every Linux-based device. We > can't answer these questions here. I think we will need to pass them to this > function. Note that there are platform-specific calls to AddWidevineWithCodecs, > which should solve this. > > Also, this file/function is intended to contain platform-independent information > about the Widevine key system (as much as possible), so this code probably > shouldn't be here. Done. https://codereview.chromium.org/923283002/diff/1/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/923283002/diff/1/media/base/eme_constants.h#n... media/base/eme_constants.h:61: EME_REQUIREMENT_NOT_ALLOWED = 1 << 0, On 2015/02/17 20:56:47, ddorwin wrote: > Would it make sense (and help with the code) to have a different set of values > when specifying key system capabilities? For example, SUPPORTS_NOT_ALLOWED, > REQUIRES_ALWAYS, WILL_USE_IF_ALLOWED. Done. https://codereview.chromium.org/923283002/diff/1/media/base/eme_constants.h#n... media/base/eme_constants.h:63: EME_REQUIREMENT_OPTIONAL = On 2015/02/17 20:56:47, ddorwin wrote: > Embedders should never use this, right? Maybe it shouldn't be in this file. I'm > not sure "OPTIONAL" here has the same meaning as the spec - it's really just > "supports both", which we probably don't need. Also, see above. Acknowledged. https://codereview.chromium.org/923283002/diff/1/media/base/key_system_info.cc File media/base/key_system_info.cc (right): https://codereview.chromium.org/923283002/diff/1/media/base/key_system_info.c... media/base/key_system_info.cc:9: KeySystemInfo::KeySystemInfo() On 2015/02/14 01:28:35, jrummell wrote: > Is this required? I don't see any user of it (although maybe hash_map requires > it). Yup, it's for hash_map. https://codereview.chromium.org/923283002/diff/1/media/base/key_system_info.c... media/base/key_system_info.cc:9: KeySystemInfo::KeySystemInfo() On 2015/02/17 20:56:47, ddorwin wrote: > On 2015/02/14 01:28:35, jrummell wrote: > > Is this required? I don't see any user of it (although maybe hash_map requires > > it). > > If so, we should consider eliminating the other one. Done. https://codereview.chromium.org/923283002/diff/1/media/base/key_system_info.c... media/base/key_system_info.cc:22: persistent_state_requirement(EME_REQUIREMENT_REQUIRED), On 2015/02/17 20:56:47, ddorwin wrote: > Default to the worst case: For this one, not_allowed (no support). For the > identifier, the worst case is it's required. Changed to an illegal value with DCHECKS. (For future reference though, this is the correct worst case.) https://codereview.chromium.org/923283002/diff/1/media/base/key_system_info.h File media/base/key_system_info.h (right): https://codereview.chromium.org/923283002/diff/1/media/base/key_system_info.h... media/base/key_system_info.h:33: KeySystemInfo(); On 2015/02/17 20:56:47, ddorwin wrote: > Why do we have a default constructor and no longer a destructor defined? The constructor was added so that KeySystemInfo structs can go in containers (hash_maps in particular). The destructor was removed because it didn't do anything. https://codereview.chromium.org/923283002/diff/1/media/base/key_system_info.h... media/base/key_system_info.h:45: EmeRequirement persistent_state_requirement; On 2015/02/17 20:56:48, ddorwin wrote: > Related to earlier: Is it requirement or support? Acknowledged. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc#ne... media/base/key_systems.cc:334: DCHECK(!info.key_system.empty()) << "Missing key system name"; On 2015/02/17 22:34:58, ddorwin wrote: > nit: The << part is probably unnecessary for this one. Done. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc#ne... media/base/key_systems.cc:334: DCHECK(!info.key_system.empty()) << "Missing key system name"; On 2015/02/17 22:34:58, ddorwin wrote: > nit: The << part is probably unnecessary for this one. Done. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc#ne... media/base/key_systems.cc:335: DCHECK(!IsConcreteSupportedKeySystem(info.key_system)) On 2015/02/17 22:34:58, ddorwin wrote: > I believe this needs to change after https://codereview.chromium.org/912233004/. Done. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc#ne... media/base/key_systems.cc:344: NOTREACHED() << "Key system '" << info.key_system << "' does not support" On 2015/02/17 22:34:58, ddorwin wrote: > nit: The issue is really that no session types were specified for the system. Done. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc#ne... media/base/key_systems.cc:348: On 2015/02/17 22:34:58, ddorwin wrote: > We can DCHECK that the persistent session types are not specified unless > persistent state is supported. Done. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc#ne... media/base/key_systems.cc:546: EmeRequirement requirement_mask = On 2015/02/17 22:34:58, ddorwin wrote: > An enum cannot be a mask. You must use an int. If the member below cannot be a > mask, you can just rename this. If it can, you need to change both types. > > Also, it is really the support rather than the requirement. Acknowledged. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc#ne... media/base/key_systems.cc:561: EmeRequirement requirement_mask = On 2015/02/17 22:34:57, ddorwin wrote: > ditto Acknowledged. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.cc#ne... media/base/key_systems.cc:576: SupportedSessionTypes session_type_mask = On 2015/02/17 22:34:58, ddorwin wrote: > This one is okay. Acknowledged. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.h File media/base/key_systems.h (right): https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.h#new... media/base/key_systems.h:84: MEDIA_EXPORT bool IsSupportedDistinctiveIdentifierRequirement( On 2015/02/17 22:34:58, ddorwin wrote: > We may need to pass more information (is authorized), track this in an object, > or return more states (i.e. yes, now, if authorized). That's true. I was hoping to skip that for this CL, as it doesn't affect the logic we are implementing quite yet. https://codereview.chromium.org/923283002/diff/1/media/base/key_systems.h#new... media/base/key_systems.h:86: EmeRequirement requirement); On 2015/02/17 22:34:58, ddorwin wrote: > In this case, we are passing the requirement from the app. We could do the > conversion inside these functions. I've left the conversion outside to avoid infecting KeySystems with knowledge of Blink. https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:74: // 2. Let media type capabilities be empty. (Done by caller.) On 2015/02/14 01:28:35, jrummell wrote: > Can you add an ASSERT(media_type_capabilities.size() == 0); ? Done. https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:121: NOTREACHED(); On 2015/02/17 22:34:58, ddorwin wrote: > Can we get away with no code in this "branch" since all enum values are covered? MSVC has complained at me before with other similar code (we do this kind of conversion a lot). https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:173: requirement = ConvertToEmeRequirement(candidate.distinctiveIdentifier); On 2015/02/17 22:34:58, ddorwin wrote: > collapse to one statement. > Alternatively, can this be made to fit in 174? This seems to be the cleanest way, everything else has exceptionally ugly wrapping. https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:219: // Capabilities for Media Type algorithm on Video, candidate On 2015/02/17 22:34:58, ddorwin wrote: > Audio Done. https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:229: // 8.3. Add video capabilities to accumulated configuration. On 2015/02/17 22:34:58, ddorwin wrote: > audio Done. https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:230: accumulated_configuration->videoCapabilities = audio_capabilities; On 2015/02/14 01:28:35, jrummell wrote: > video = audio? Done. https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:241: // (Without robustness support, capabilities do not affect this.) On 2015/02/17 22:34:58, ddorwin wrote: > TODO with bug? Done. https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:287: if (IsSupportedSessionType(key_system, EME_SESSION_TYPE_TEMPORARY)) { On 2015/02/17 22:34:58, ddorwin wrote: > Per the spec, this can be a DCHECK Acknowledged. https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:398: // 7.3. For each value in supportedConfigurations: On 2015/02/14 01:28:35, jrummell wrote: > Two 7.3 comments, remove one. Done. https://codereview.chromium.org/923283002/diff/1/media/blink/webencryptedmedi... media/blink/webencryptedmediaclient_impl.cc:412: // 7.3.3. If supported configuration is not null, [initialize and return On 2015/02/17 22:34:58, ddorwin wrote: > nit: This should be above 409, right? Done.
Not quite finished reviewing, but I wanted to get the comments out. I'll finish soon. LG overall. https://codereview.chromium.org/923283002/diff/80001/chrome/renderer/media/ch... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/923283002/diff/80001/chrome/renderer/media/ch... chrome/renderer/media/chrome_key_systems.cc:193: // Persistent licenses are supported if remote attestatin succeeds. attestation https://codereview.chromium.org/923283002/diff/80001/chromecast/renderer/key_... File chromecast/renderer/key_systems_cast.cc (right): https://codereview.chromium.org/923283002/diff/80001/chromecast/renderer/key_... chromecast/renderer/key_systems_cast.cc:20: void AddKeySystemWithCodecs( This function is insufficient. I wonder if it is even necessary or we should just inline it at line 43. The Cast team will need to provide the new enum values. I assume they are the same as for Widevine. https://codereview.chromium.org/923283002/diff/80001/chromecast/renderer/key_... chromecast/renderer/key_systems_cast.cc:22: std::vector< ::media::KeySystemInfo>* concrete_key_systems) { can we remove the space before ::? https://codereview.chromium.org/923283002/diff/80001/chromecast/renderer/key_... chromecast/renderer/key_systems_cast.cc:25: info.supported_codecs = ::media::EME_CODEC_MP4_ALL; same as below https://codereview.chromium.org/923283002/diff/80001/chromecast/renderer/key_... chromecast/renderer/key_systems_cast.cc:34: ::media::EME_CODEC_MP4_ALL, Let's not use *_ALL (I know it was there before but let's fix it). https://codereview.chromium.org/923283002/diff/80001/components/cdm/renderer/... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/923283002/diff/80001/components/cdm/renderer/... components/cdm/renderer/android_key_systems.cc:54: AddWidevineWithCodecs( TODO: Remove with unprefixed. crbug.com/249976 https://codereview.chromium.org/923283002/diff/80001/components/cdm/renderer/... components/cdm/renderer/android_key_systems.cc:55: WIDEVINE_HR_NON_COMPOSITING, We need to not support this in unprefixed. We might need to explicitly filter somewhere. Alternatively, this might be some property that we set. You can reference 394926. https://codereview.chromium.org/923283002/diff/80001/components/cdm/renderer/... components/cdm/renderer/android_key_systems.cc:91: info.persistent_state_support = media::EME_FEATURE_ALWAYS_AVAILABLE; You might mention that this is the worst case for the user (even thought it might be a false positive for the app). https://codereview.chromium.org/923283002/diff/80001/content/renderer/media/r... File content/renderer/media/render_media_client_unittest.cc (right): https://codereview.chromium.org/923283002/diff/80001/content/renderer/media/r... content/renderer/media/render_media_client_unittest.cc:35: key_systems_info->push_back(wv_key_system_info); The lack of valid enum values doesn't cause DCHECKs somewhere? https://codereview.chromium.org/923283002/diff/80001/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/923283002/diff/80001/media/base/eme_constants... media/base/eme_constants.h:54: EME_SESSION_TYPE_SUPPORT_NOT_DECLARED, nit: This name makes it seem valid. INVALID or something like that would be clearer. UNINITIALIZED? https://codereview.chromium.org/923283002/diff/80001/media/base/eme_constants... media/base/eme_constants.h:55: // The session type is not available. s/available/supported/? and line 57 https://codereview.chromium.org/923283002/diff/80001/media/base/eme_constants... media/base/eme_constants.h:74: EME_FEATURE_ALWAYS_AVAILABLE, s/AVAILABLE/USED/ ? https://codereview.chromium.org/923283002/diff/80001/media/base/key_system_in... File media/base/key_system_info.h (right): https://codereview.chromium.org/923283002/diff/80001/media/base/key_system_in... media/base/key_system_info.h:33: KeySystemInfo(); We need to declare the destructor so that it is not implicitly inlined. http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in.... doesn't explicitly address this, but the spec says "An implicitly-declared destructor is an inline public member of its class." https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:140: bool IsPersistentLicenseSupported( ditto from the .h file. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:352: EME_FEATURE_SUPPORTED); also != EME_FEATURE_ALWAYS_AVAILABLE. That would not be spec compliant. (That would mean changing AddAndroidPlatformKeySystems() and the temporary CrOS value.) Perhaps: == EME_FEATURE_NOT_SUPPORTED || == EME_FEATURE_SUPPORTED_WITH_PERMISSION https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:359: DCHECK(!IsConcreteSupportedKeySystem(info.key_system)) IsSupportedKeySystem https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:563: if (key_system_iter == concrete_key_system_map_.end()) Should this be a DCHECK instead? Should these be called for an unsupported key system? Note that the function name is not IsKeySystemSupported... https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:571: return support >= requirement; It's not obvious from the enum definitions that the values are in order of increasing requirement. We should probably explicitly check (switch statement?) or specify that requirement in the consts header. A switch statement might also make the logic clearer like the following: case EME_SESSION_TYPE_SUPPORTED_WITH_PERMISSION: return permission_granted; https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:610: return support < EME_FEATURE_ALWAYS_AVAILABLE; USED makes more sense here (see earlier comment) https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.h File media/base/key_systems.h (right): https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.h... media/base/key_systems.h:89: MEDIA_EXPORT bool IsPersistentLicenseSupported( ...SessionSupported https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.h... media/base/key_systems.h:91: bool permission_granted); nit: is_... https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.h... media/base/key_systems.h:99: MEDIA_EXPORT bool IsPersistentStateSupported( IsCompatibleWithPersistentStateRequirement? (The comment probably needs to be updated too.)
Thanks. https://codereview.chromium.org/923283002/diff/80001/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/923283002/diff/80001/media/base/eme_constants... media/base/eme_constants.h:70: EME_FEATURE_SUPPORTED_WITH_PERMISSION, s/SUPPORTED/REQUESTABLE/ or something like that to differentiate from ALWAYS (without having to understand the intricacies). Ditto below. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:604: EmeFeatureSupport permission_requirement = This should only be in the specific case at line 613 (or just replace this with a switch statement per previous comment). https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:610: return support < EME_FEATURE_ALWAYS_AVAILABLE; This logic is only valid if we can enforce that CDMS that EME_FEATURE_SUPPORTED* don't use it. Perhaps we need a comment, bug reference, and note that we prevent "not-allowed" from getting here. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:614: return support >= permission_requirement; ditto with line 571, though this case would end up with nested switch statements. I suppose comments in the consts header and some thorough unit tests could work. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:635: permission_granted ? EME_FEATURE_SUPPORTED_WITH_PERMISSION See below. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:640: return support < EME_FEATURE_ALWAYS_AVAILABLE; ditto with line 610 (and especially important here) https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:644: return support >= permission_requirement; Per the comment at line 352, some of the values are not valid. Can we make a more direct check here? You may just be able to check (support==EME_FEATURE_SUPPORTED_WITH_PERMISSION && permission_granted). https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:74: // 2. Let media type capabilities be empty. (Done by caller.) nit: You can remove the () part now that you have a check. That's the pattern we've used. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:96: continue; Can we log a console info message here to say it's not yet supported? https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:171: // - "not-allowed": If the implementation requires a Distinctive Since we can't enforce this, I think we need to always reject this in M42, right? https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:175: requirement = ConvertRequirement(candidate.distinctiveIdentifier); EmeFeatureRequirement requirement = ConvertRequirement(candidate.distinctiveIdentifier); doesn't seem too ugly. And then we avoid the uninitialized variable. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:176: if (!IsDistinctiveIdentifierSupported(key_system, requirement, true)) There should be a comment for this "true". For example, state that we assume the best case (that permission will be granted) here and do a final check below. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:189: // - "not-allowed": If the implementation requires persisting state in ditto about rejecting. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:192: if (!IsPersistentStateSupported(key_system, requirement, true)) ditto https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:248: true)) { Explain "true" here too. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:267: if (IsPersistentStateSupported(key_system, EME_FEATURE_NOT_ALLOWED, true)) { ditto https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:279: // TODO(sandersd): Implement prompting. http://crbug.com/446263 // For now, pass "false" for |is_permission_granted|.
https://codereview.chromium.org/923283002/diff/80001/chrome/renderer/media/ch... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/923283002/diff/80001/chrome/renderer/media/ch... chrome/renderer/media/chrome_key_systems.cc:193: // Persistent licenses are supported if remote attestatin succeeds. On 2015/02/19 01:41:33, ddorwin wrote: > attestation Done. https://codereview.chromium.org/923283002/diff/80001/chromecast/renderer/key_... File chromecast/renderer/key_systems_cast.cc (right): https://codereview.chromium.org/923283002/diff/80001/chromecast/renderer/key_... chromecast/renderer/key_systems_cast.cc:20: void AddKeySystemWithCodecs( On 2015/02/19 01:41:33, ddorwin wrote: > This function is insufficient. I wonder if it is even necessary or we should > just inline it at line 43. > The Cast team will need to provide the new enum values. I assume they are the > same as for Widevine. Done. https://codereview.chromium.org/923283002/diff/80001/chromecast/renderer/key_... chromecast/renderer/key_systems_cast.cc:22: std::vector< ::media::KeySystemInfo>* concrete_key_systems) { On 2015/02/19 01:41:33, ddorwin wrote: > can we remove the space before ::? Done. https://codereview.chromium.org/923283002/diff/80001/chromecast/renderer/key_... chromecast/renderer/key_systems_cast.cc:25: info.supported_codecs = ::media::EME_CODEC_MP4_ALL; On 2015/02/19 01:41:33, ddorwin wrote: > same as below Done. https://codereview.chromium.org/923283002/diff/80001/chromecast/renderer/key_... chromecast/renderer/key_systems_cast.cc:34: ::media::EME_CODEC_MP4_ALL, On 2015/02/19 01:41:33, ddorwin wrote: > Let's not use *_ALL (I know it was there before but let's fix it). Done. https://codereview.chromium.org/923283002/diff/80001/chromecast/renderer/key_... chromecast/renderer/key_systems_cast.cc:34: ::media::EME_CODEC_MP4_ALL, On 2015/02/19 01:41:33, ddorwin wrote: > Let's not use *_ALL (I know it was there before but let's fix it). Done. https://codereview.chromium.org/923283002/diff/80001/components/cdm/renderer/... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/923283002/diff/80001/components/cdm/renderer/... components/cdm/renderer/android_key_systems.cc:54: AddWidevineWithCodecs( On 2015/02/19 01:41:33, ddorwin wrote: > TODO: Remove with unprefixed. crbug.com/249976 Done. https://codereview.chromium.org/923283002/diff/80001/components/cdm/renderer/... components/cdm/renderer/android_key_systems.cc:55: WIDEVINE_HR_NON_COMPOSITING, On 2015/02/19 01:41:33, ddorwin wrote: > We need to not support this in unprefixed. We might need to explicitly filter > somewhere. > Alternatively, this might be some property that we set. You can reference > 394926. Isn't this what IsKnownKeySystem() does? https://codereview.chromium.org/923283002/diff/80001/components/cdm/renderer/... components/cdm/renderer/android_key_systems.cc:91: info.persistent_state_support = media::EME_FEATURE_ALWAYS_AVAILABLE; On 2015/02/19 01:41:33, ddorwin wrote: > You might mention that this is the worst case for the user (even thought it > might be a false positive for the app). Done. https://codereview.chromium.org/923283002/diff/80001/content/renderer/media/r... File content/renderer/media/render_media_client_unittest.cc (right): https://codereview.chromium.org/923283002/diff/80001/content/renderer/media/r... content/renderer/media/render_media_client_unittest.cc:35: key_systems_info->push_back(wv_key_system_info); On 2015/02/19 01:41:33, ddorwin wrote: > The lack of valid enum values doesn't cause DCHECKs somewhere? Done. https://codereview.chromium.org/923283002/diff/80001/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/923283002/diff/80001/media/base/eme_constants... media/base/eme_constants.h:54: EME_SESSION_TYPE_SUPPORT_NOT_DECLARED, On 2015/02/19 01:41:33, ddorwin wrote: > nit: This name makes it seem valid. INVALID or something like that would be > clearer. UNINITIALIZED? Done. https://codereview.chromium.org/923283002/diff/80001/media/base/eme_constants... media/base/eme_constants.h:55: // The session type is not available. On 2015/02/19 01:41:33, ddorwin wrote: > s/available/supported/? > > and line 57 Done. https://codereview.chromium.org/923283002/diff/80001/media/base/eme_constants... media/base/eme_constants.h:74: EME_FEATURE_ALWAYS_AVAILABLE, On 2015/02/19 01:41:33, ddorwin wrote: > s/AVAILABLE/USED/ ? Done. https://codereview.chromium.org/923283002/diff/80001/media/base/key_system_in... File media/base/key_system_info.h (right): https://codereview.chromium.org/923283002/diff/80001/media/base/key_system_in... media/base/key_system_info.h:33: KeySystemInfo(); On 2015/02/19 01:41:33, ddorwin wrote: > We need to declare the destructor so that it is not implicitly inlined. > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in.... > doesn't explicitly address this, but the spec says "An implicitly-declared > destructor is an inline public member of its class." Done. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:140: bool IsPersistentLicenseSupported( On 2015/02/19 01:41:34, ddorwin wrote: > ditto from the .h file. Done. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:352: EME_FEATURE_SUPPORTED); On 2015/02/19 01:41:33, ddorwin wrote: > also != EME_FEATURE_ALWAYS_AVAILABLE. That would not be spec compliant. > > (That would mean changing AddAndroidPlatformKeySystems() and the temporary CrOS > value.) > > Perhaps: == EME_FEATURE_NOT_SUPPORTED || == > EME_FEATURE_SUPPORTED_WITH_PERMISSION I've just added a comment for now. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:359: DCHECK(!IsConcreteSupportedKeySystem(info.key_system)) On 2015/02/19 01:41:34, ddorwin wrote: > IsSupportedKeySystem Why do we have both? They are identical. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:563: if (key_system_iter == concrete_key_system_map_.end()) On 2015/02/19 01:41:33, ddorwin wrote: > Should this be a DCHECK instead? Should these be called for an unsupported key > system? Note that the function name is not IsKeySystemSupported... Done. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:571: return support >= requirement; On 2015/02/19 01:41:33, ddorwin wrote: > It's not obvious from the enum definitions that the values are in order of > increasing requirement. We should probably explicitly check (switch statement?) > or specify that requirement in the consts header. > > A switch statement might also make the logic clearer like the following: > > case EME_SESSION_TYPE_SUPPORTED_WITH_PERMISSION: > return permission_granted; Done. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:604: EmeFeatureSupport permission_requirement = On 2015/02/19 04:23:41, ddorwin wrote: > This should only be in the specific case at line 613 (or just replace this with > a switch statement per previous comment). Done. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:610: return support < EME_FEATURE_ALWAYS_AVAILABLE; On 2015/02/19 01:41:34, ddorwin wrote: > USED makes more sense here (see earlier comment) Done. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:610: return support < EME_FEATURE_ALWAYS_AVAILABLE; On 2015/02/19 04:23:41, ddorwin wrote: > This logic is only valid if we can enforce that CDMS that EME_FEATURE_SUPPORTED* > don't use it. Perhaps we need a comment, bug reference, and note that we prevent > "not-allowed" from getting here. Done. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:614: return support >= permission_requirement; On 2015/02/19 04:23:41, ddorwin wrote: > ditto with line 571, though this case would end up with nested switch > statements. > > I suppose comments in the consts header and some thorough unit tests could work. Done. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:635: permission_granted ? EME_FEATURE_SUPPORTED_WITH_PERMISSION On 2015/02/19 04:23:41, ddorwin wrote: > See below. Done. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:640: return support < EME_FEATURE_ALWAYS_AVAILABLE; On 2015/02/19 04:23:41, ddorwin wrote: > ditto with line 610 (and especially important here) Done. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:644: return support >= permission_requirement; On 2015/02/19 04:23:42, ddorwin wrote: > Per the comment at line 352, some of the values are not valid. Can we make a > more direct check here? > > You may just be able to check (support==EME_FEATURE_SUPPORTED_WITH_PERMISSION && > permission_granted). Done. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.h File media/base/key_systems.h (right): https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.h... media/base/key_systems.h:89: MEDIA_EXPORT bool IsPersistentLicenseSupported( On 2015/02/19 01:41:34, ddorwin wrote: > ...SessionSupported Done. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.h... media/base/key_systems.h:91: bool permission_granted); On 2015/02/19 01:41:34, ddorwin wrote: > nit: is_... Done. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.h... media/base/key_systems.h:99: MEDIA_EXPORT bool IsPersistentStateSupported( On 2015/02/19 01:41:34, ddorwin wrote: > IsCompatibleWithPersistentStateRequirement? > > (The comment probably needs to be updated too.) Done. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:74: // 2. Let media type capabilities be empty. (Done by caller.) On 2015/02/19 04:23:42, ddorwin wrote: > nit: You can remove the () part now that you have a check. That's the pattern > we've used. Done. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:96: continue; On 2015/02/19 04:23:42, ddorwin wrote: > Can we log a console info message here to say it's not yet supported? Done. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:171: // - "not-allowed": If the implementation requires a Distinctive On 2015/02/19 04:23:42, ddorwin wrote: > Since we can't enforce this, I think we need to always reject this in M42, > right? That is done implicitly when CDMs declare ALWAYS_ENABLED. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:175: requirement = ConvertRequirement(candidate.distinctiveIdentifier); On 2015/02/19 04:23:42, ddorwin wrote: > EmeFeatureRequirement requirement = > ConvertRequirement(candidate.distinctiveIdentifier); > > doesn't seem too ugly. And then we avoid the uninitialized variable. Done. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:176: if (!IsDistinctiveIdentifierSupported(key_system, requirement, true)) On 2015/02/19 04:23:42, ddorwin wrote: > There should be a comment for this "true". For example, state that we assume the > best case (that permission will be granted) here and do a final check below. Done. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:189: // - "not-allowed": If the implementation requires persisting state in On 2015/02/19 04:23:42, ddorwin wrote: > ditto about rejecting. Acknowledged. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:192: if (!IsPersistentStateSupported(key_system, requirement, true)) On 2015/02/19 04:23:42, ddorwin wrote: > ditto Done. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:248: true)) { On 2015/02/19 04:23:42, ddorwin wrote: > Explain "true" here too. It doesn't actually make any difference, so it's hard to write a cogent explanation. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:267: if (IsPersistentStateSupported(key_system, EME_FEATURE_NOT_ALLOWED, true)) { On 2015/02/19 04:23:42, ddorwin wrote: > ditto Acknowledged. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:279: // TODO(sandersd): Implement prompting. http://crbug.com/446263 On 2015/02/19 04:23:42, ddorwin wrote: > // For now, pass "false" for |is_permission_granted|. Done.
Patchset #8 (id:140001) has been deleted
sandersd@chromium.org changed reviewers: + lcwu@chromium.org
lcwu@chromium.org: Please review changes to chromecast/renderer/key_systems_cast.*
LG. Might be some corner cases to clean up. 2 comments are in PS5. https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:359: DCHECK(!IsConcreteSupportedKeySystem(info.key_system)) On 2015/02/19 21:08:34, sandersd wrote: > On 2015/02/19 01:41:34, ddorwin wrote: > > IsSupportedKeySystem > > Why do we have both? They are identical. Once prefixed is removed, "concrete" won't be a thing and we can remove it. You don't need to differentiate about concrete here. Not really a big deal. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:171: // - "not-allowed": If the implementation requires a Distinctive On 2015/02/19 21:08:35, sandersd wrote: > On 2015/02/19 04:23:42, ddorwin wrote: > > Since we can't enforce this, I think we need to always reject this in M42, > > right? > > That is done implicitly when CDMs declare ALWAYS_ENABLED. We don't seem to be enforcing this assumption in key_systems.cc. https://codereview.chromium.org/923283002/diff/160001/chromecast/renderer/key... File chromecast/renderer/key_systems_cast.cc (right): https://codereview.chromium.org/923283002/diff/160001/chromecast/renderer/key... chromecast/renderer/key_systems_cast.cc:38: info.supported_init_data_types = ::media::EME_INIT_DATA_TYPE_NONE; CENC https://codereview.chromium.org/923283002/diff/160001/chromecast/renderer/key... chromecast/renderer/key_systems_cast.cc:42: info.persistent_state_support = ::media::EME_FEATURE_NOT_SUPPORTED; I don't know what the correct answer. We should probably use "worst case" like we do for Android platform. Thus, ALWAYS_ENABLED, right? https://codereview.chromium.org/923283002/diff/160001/components/cdm/renderer... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/923283002/diff/160001/components/cdm/renderer... components/cdm/renderer/android_key_systems.cc:88: // Assume the worst case for requiring user permission. // Assume the worst case (from a user point of view). https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.... media/base/key_systems.cc:350: // http://crbug.com/457482 For now, I think we need to assert NOT_SUPPORTED or ALWAYS_ENABLED. Your comment in webencryptedmediaclient_impl.cc (PS5) appears to assume this is the case. https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.... media/base/key_systems.cc:353: EME_FEATURE_REQUESTABLE); << "Use REQUESTABLE_WITH_PERMISSION" https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.... media/base/key_systems.cc:564: if (key_system_iter == concrete_key_system_map_.end()) { nit: You _can_ collapse this to one line. We will crash at 569 in an opt build, but that should never happen since we do not expect to get here. And, we're suppressing crash reports for such bugs. https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.... media/base/key_systems.cc:633: return (requirement != EME_FEATURE_REQUIRED) || is_permission_granted; Until we can enforce that SUPPORTED* CDMs do not use, we should probably fail for requirement==EME_FEATURE_NOT_ALLOWED. Ditto below. WDYT? https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.... media/base/key_systems.cc:664: return (requirement != EME_FEATURE_REQUIRED) || is_permission_granted; ditto https://codereview.chromium.org/923283002/diff/160001/media/blink/webencrypte... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/923283002/diff/160001/media/blink/webencrypte... media/blink/webencryptedmediaclient_impl.cc:137: bool is_permission_possible = true; const bool kIsP....
https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/923283002/diff/80001/media/base/key_systems.c... media/base/key_systems.cc:359: DCHECK(!IsConcreteSupportedKeySystem(info.key_system)) On 2015/02/19 23:03:57, ddorwin wrote: > On 2015/02/19 21:08:34, sandersd wrote: > > On 2015/02/19 01:41:34, ddorwin wrote: > > > IsSupportedKeySystem > > > > Why do we have both? They are identical. > > Once prefixed is removed, "concrete" won't be a thing and we can remove it. You > don't need to differentiate about concrete here. Not really a big deal. Acknowledged. https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/923283002/diff/80001/media/blink/webencrypted... media/blink/webencryptedmediaclient_impl.cc:171: // - "not-allowed": If the implementation requires a Distinctive On 2015/02/19 23:03:57, ddorwin wrote: > On 2015/02/19 21:08:35, sandersd wrote: > > On 2015/02/19 04:23:42, ddorwin wrote: > > > Since we can't enforce this, I think we need to always reject this in M42, > > > right? > > > > That is done implicitly when CDMs declare ALWAYS_ENABLED. > > We don't seem to be enforcing this assumption in key_systems.cc. KeySystems does this explicitly: case EME_FEATURE_ALWAYS_ENABLED: // TODO(sandersd): This should be NOTREACHED(). http://crbug.com/457482 return requirement != EME_FEATURE_NOT_ALLOWED; https://codereview.chromium.org/923283002/diff/160001/chromecast/renderer/key... File chromecast/renderer/key_systems_cast.cc (right): https://codereview.chromium.org/923283002/diff/160001/chromecast/renderer/key... chromecast/renderer/key_systems_cast.cc:38: info.supported_init_data_types = ::media::EME_INIT_DATA_TYPE_NONE; On 2015/02/19 23:03:57, ddorwin wrote: > CENC Done. https://codereview.chromium.org/923283002/diff/160001/chromecast/renderer/key... chromecast/renderer/key_systems_cast.cc:42: info.persistent_state_support = ::media::EME_FEATURE_NOT_SUPPORTED; On 2015/02/19 23:03:57, ddorwin wrote: > I don't know what the correct answer. We should probably use "worst case" like > we do for Android platform. Thus, ALWAYS_ENABLED, right? Done. I was hoping lcwu@ would comment on this. https://codereview.chromium.org/923283002/diff/160001/components/cdm/renderer... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/923283002/diff/160001/components/cdm/renderer... components/cdm/renderer/android_key_systems.cc:88: // Assume the worst case for requiring user permission. On 2015/02/19 23:03:57, ddorwin wrote: > // Assume the worst case (from a user point of view). Done. https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.... media/base/key_systems.cc:350: // http://crbug.com/457482 On 2015/02/19 23:03:57, ddorwin wrote: > For now, I think we need to assert NOT_SUPPORTED or ALWAYS_ENABLED. > Your comment in webencryptedmediaclient_impl.cc (PS5) appears to assume this is > the case. WebEncryptedMediaClient doesn't concern itself with these distinctions, it just asks KeySystems if a configuration is supported or not (and KeySystems is pretty explicit about how to handle this). https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.... media/base/key_systems.cc:353: EME_FEATURE_REQUESTABLE); On 2015/02/19 23:03:57, ddorwin wrote: > << "Use REQUESTABLE_WITH_PERMISSION" Done. https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.... media/base/key_systems.cc:564: if (key_system_iter == concrete_key_system_map_.end()) { On 2015/02/19 23:03:57, ddorwin wrote: > nit: You _can_ collapse this to one line. We will crash at 569 in an opt build, > but that should never happen since we do not expect to get here. And, we're > suppressing crash reports for such bugs. I'd rather leave this explicit. My proposed refactor to create keysystem specific classes solves this much more directly. https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.... media/base/key_systems.cc:633: return (requirement != EME_FEATURE_REQUIRED) || is_permission_granted; On 2015/02/19 23:03:57, ddorwin wrote: > Until we can enforce that SUPPORTED* CDMs do not use, we should probably fail > for requirement==EME_FEATURE_NOT_ALLOWED. > Ditto below. > WDYT? Currently we use ALWAYS_ENABLED, which has exactly the intended effect. We can't force non-widevine implementations to do this correctly, so I think it's fine to have correct logic here. https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.... media/base/key_systems.cc:664: return (requirement != EME_FEATURE_REQUIRED) || is_permission_granted; On 2015/02/19 23:03:57, ddorwin wrote: > ditto Acknowledged. https://codereview.chromium.org/923283002/diff/160001/media/blink/webencrypte... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/923283002/diff/160001/media/blink/webencrypte... media/blink/webencryptedmediaclient_impl.cc:137: bool is_permission_possible = true; On 2015/02/19 23:03:57, ddorwin wrote: > const bool kIsP.... See my prompting CL for the direction with this.
https://codereview.chromium.org/923283002/diff/160001/chromecast/renderer/key... File chromecast/renderer/key_systems_cast.cc (right): https://codereview.chromium.org/923283002/diff/160001/chromecast/renderer/key... chromecast/renderer/key_systems_cast.cc:42: info.persistent_state_support = ::media::EME_FEATURE_NOT_SUPPORTED; On 2015/02/20 00:17:12, sandersd wrote: > On 2015/02/19 23:03:57, ddorwin wrote: > > I don't know what the correct answer. We should probably use "worst case" like > > we do for Android platform. Thus, ALWAYS_ENABLED, right? > > Done. > > I was hoping lcwu@ would comment on this. Since we don't control the implementation, I don't think we'll ever have confidence. Perhaps we need a "can't be sure" value that causes us to reject anything other than "optional". Perhaps in a follow-on CL. https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.... media/base/key_systems.cc:350: // http://crbug.com/457482 On 2015/02/20 00:17:12, sandersd wrote: > On 2015/02/19 23:03:57, ddorwin wrote: > > For now, I think we need to assert NOT_SUPPORTED or ALWAYS_ENABLED. > > Your comment in webencryptedmediaclient_impl.cc (PS5) appears to assume this > is > > the case. > > WebEncryptedMediaClient doesn't concern itself with these distinctions, it just > asks KeySystems if a configuration is supported or not (and KeySystems is pretty > explicit about how to handle this). See comment in latest PS. https://codereview.chromium.org/923283002/diff/180001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/923283002/diff/180001/media/base/key_systems.... media/base/key_systems.cc:349: // there are no longer any CDMs declaring ALWAYS_ENABLED. As discussed, ALWAYS_ENABLED is correct for Android, etc. We will prevent these from being used if there is no permission. The REQUESTABLE values simply mean that it can work either way (and we should enforce it - see below). https://codereview.chromium.org/923283002/diff/180001/media/base/key_systems.... media/base/key_systems.cc:350: // http://crbug.com/457482 As discussed, we currently assume all CDMs are either NOT_SUPPORTED or ALWAYS_ENABLED. We should assert this. Perhaps a better way to implement these checks is to do NOT_SUPPORTED || ALWAYS_ENABLED. Later, we will add || REQUESTABLE_WITH_PERMISSION https://codereview.chromium.org/923283002/diff/180001/media/base/key_systems.... media/base/key_systems.cc:633: return (requirement != EME_FEATURE_REQUIRED) || is_permission_granted; On 2015/02/20 00:17:12, sandersd wrote: > On 2015/02/19 23:03:57, ddorwin wrote: > > Until we can enforce that SUPPORTED* CDMs do not use, we should probably fail > > for requirement==EME_FEATURE_NOT_ALLOWED. > > Ditto below. > > WDYT? > > Currently we use ALWAYS_ENABLED, which has exactly the intended effect. We can't > force non-widevine implementations to do this correctly, so I think it's fine to > have correct logic here. That's true for the ID, but desktop/CrOS (chrome_key_systems.cc) use REQUESTABLE. Thus, we need a check or to change those to use ALWAYS and enforce this in the registration like we are going to for the ID. https://codereview.chromium.org/923283002/diff/180001/media/base/key_systems.... media/base/key_systems.cc:664: return (requirement != EME_FEATURE_REQUIRED) || is_permission_granted; This one is okay I guess if we make the registration DCHECK.
https://codereview.chromium.org/923283002/diff/160001/chromecast/renderer/key... File chromecast/renderer/key_systems_cast.cc (right): https://codereview.chromium.org/923283002/diff/160001/chromecast/renderer/key... chromecast/renderer/key_systems_cast.cc:42: info.persistent_state_support = ::media::EME_FEATURE_NOT_SUPPORTED; On 2015/02/20 00:49:07, ddorwin wrote: > On 2015/02/20 00:17:12, sandersd wrote: > > On 2015/02/19 23:03:57, ddorwin wrote: > > > I don't know what the correct answer. We should probably use "worst case" > like > > > we do for Android platform. Thus, ALWAYS_ENABLED, right? > > > > Done. > > > > I was hoping lcwu@ would comment on this. > > Since we don't control the implementation, I don't think we'll ever have > confidence. Perhaps we need a "can't be sure" value that causes us to reject > anything other than "optional". Perhaps in a follow-on CL. Acknowledged. https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/923283002/diff/160001/media/base/key_systems.... media/base/key_systems.cc:350: // http://crbug.com/457482 On 2015/02/20 00:49:07, ddorwin wrote: > On 2015/02/20 00:17:12, sandersd wrote: > > On 2015/02/19 23:03:57, ddorwin wrote: > > > For now, I think we need to assert NOT_SUPPORTED or ALWAYS_ENABLED. > > > Your comment in webencryptedmediaclient_impl.cc (PS5) appears to assume this > > is > > > the case. > > > > WebEncryptedMediaClient doesn't concern itself with these distinctions, it > just > > asks KeySystems if a configuration is supported or not (and KeySystems is > pretty > > explicit about how to handle this). > > See comment in latest PS. Done. https://codereview.chromium.org/923283002/diff/180001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/923283002/diff/180001/media/base/key_systems.... media/base/key_systems.cc:349: // there are no longer any CDMs declaring ALWAYS_ENABLED. On 2015/02/20 00:49:08, ddorwin wrote: > As discussed, ALWAYS_ENABLED is correct for Android, etc. We will prevent these > from being used if there is no permission. > > The REQUESTABLE values simply mean that it can work either way (and we should > enforce it - see below). Done. https://codereview.chromium.org/923283002/diff/180001/media/base/key_systems.... media/base/key_systems.cc:350: // http://crbug.com/457482 On 2015/02/20 00:49:08, ddorwin wrote: > As discussed, we currently assume all CDMs are either NOT_SUPPORTED or > ALWAYS_ENABLED. We should assert this. > > Perhaps a better way to implement these checks is to do NOT_SUPPORTED || > ALWAYS_ENABLED. Later, we will add || REQUESTABLE_WITH_PERMISSION Done. https://codereview.chromium.org/923283002/diff/180001/media/base/key_systems.... media/base/key_systems.cc:633: return (requirement != EME_FEATURE_REQUIRED) || is_permission_granted; On 2015/02/20 00:49:08, ddorwin wrote: > On 2015/02/20 00:17:12, sandersd wrote: > > On 2015/02/19 23:03:57, ddorwin wrote: > > > Until we can enforce that SUPPORTED* CDMs do not use, we should probably > fail > > > for requirement==EME_FEATURE_NOT_ALLOWED. > > > Ditto below. > > > WDYT? > > > > Currently we use ALWAYS_ENABLED, which has exactly the intended effect. We > can't > > force non-widevine implementations to do this correctly, so I think it's fine > to > > have correct logic here. > > That's true for the ID, but desktop/CrOS (chrome_key_systems.cc) use > REQUESTABLE. Thus, we need a check or to change those to use ALWAYS and enforce > this in the registration like we are going to for the ID. Done. https://codereview.chromium.org/923283002/diff/180001/media/base/key_systems.... media/base/key_systems.cc:664: return (requirement != EME_FEATURE_REQUIRED) || is_permission_granted; On 2015/02/20 00:49:08, ddorwin wrote: > This one is okay I guess if we make the registration DCHECK. Done.
I think we're missing some checks. Other than that, LG. https://codereview.chromium.org/923283002/diff/200001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/923283002/diff/200001/media/base/key_systems.... media/base/key_systems.cc:346: DCHECK_NE(info.persistent_state_support, EME_FEATURE_INVALID); We need the same checks for persistent state as we have for the identifier. (Consider doing a whitelist approach.) https://codereview.chromium.org/923283002/diff/200001/media/base/key_systems.... media/base/key_systems.cc:352: // But actually for now we only support NOT_SUPPORTED or ALWAYS_ENABLED, With all the complexity, a whitelist instead of a blacklist would be nicer. Perhaps we can clean that up when all the exceptions are gone.
https://codereview.chromium.org/923283002/diff/200001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/923283002/diff/200001/media/base/key_systems.... media/base/key_systems.cc:346: DCHECK_NE(info.persistent_state_support, EME_FEATURE_INVALID); On 2015/02/20 01:39:49, ddorwin wrote: > We need the same checks for persistent state as we have for the identifier. > (Consider doing a whitelist approach.) Done. https://codereview.chromium.org/923283002/diff/200001/media/base/key_systems.... media/base/key_systems.cc:352: // But actually for now we only support NOT_SUPPORTED or ALWAYS_ENABLED, On 2015/02/20 01:39:49, ddorwin wrote: > With all the complexity, a whitelist instead of a blacklist would be nicer. > Perhaps we can clean that up when all the exceptions are gone. Done.
lgtm Thank you!
https://codereview.chromium.org/923283002/diff/240001/chromecast/renderer/key... File chromecast/renderer/key_systems_cast.cc (right): https://codereview.chromium.org/923283002/diff/240001/chromecast/renderer/key... chromecast/renderer/key_systems_cast.cc:25: ::media::EME_CODEC_MP4_AAC | ::media::EME_CODEC_MP4_AVC1, Why not just use ::media::EME_CODEC_MP4_ALL as was done before? https://codereview.chromium.org/923283002/diff/240001/chromecast/renderer/key... chromecast/renderer/key_systems_cast.cc:37: ::media::EME_CODEC_MP4_AAC | ::media::EME_CODEC_MP4_AVC1; Ditto.
https://codereview.chromium.org/923283002/diff/240001/chromecast/renderer/key... File chromecast/renderer/key_systems_cast.cc (right): https://codereview.chromium.org/923283002/diff/240001/chromecast/renderer/key... chromecast/renderer/key_systems_cast.cc:25: ::media::EME_CODEC_MP4_AAC | ::media::EME_CODEC_MP4_AVC1, On 2015/02/20 02:06:28, lcwu1 wrote: > Why not just use ::media::EME_CODEC_MP4_ALL as was done before? As per eme_constants.h: // *_ALL values should only be used for masking, do not use them to specify // codec support because they may be extended to include more codecs. https://codereview.chromium.org/923283002/diff/240001/chromecast/renderer/key... chromecast/renderer/key_systems_cast.cc:37: ::media::EME_CODEC_MP4_AAC | ::media::EME_CODEC_MP4_AVC1; On 2015/02/20 02:06:28, lcwu1 wrote: > Ditto. Acknowledged.
New patchsets have been uploaded after l-g-t-m from ddorwin@chromium.org
chromecast/ lgtm
New patchsets have been uploaded after l-g-t-m from lcwu@chromium.org
The CQ bit was checked by sandersd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/923283002/350001
Message was sent while issue was closed.
Committed patchset #18 (id:350001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/f629e579c00a93751c75883d48c600d3df4d1ed6 Cr-Commit-Position: refs/heads/master@{#317257} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
