Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1106263004:
Support Android secure codecs in requestMediaKeySystemAccess(). (Closed)
|
Created:
5 years, 7 months ago by sandersd (OOO until July 31) Modified:
5 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, android-webview-reviews_chromium.org, eme-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport Android secure codecs in requestMediaKeySystemAccess().
This adds the bits necessary to determine if a configuration requires hardware-secure codecs. It does not add the logic to actually enable secure surfaces based on the requirement, that decision is still based on the renderer preference.
BUG=467779
Committed: https://crrev.com/a6bf8b52a70dafe95e8e2d8827f5e1283ec1bf56
Cr-Commit-Position: refs/heads/master@{#328464}
Patch Set 1 #
Total comments: 32
Patch Set 2 : Fix Android. #Patch Set 3 : Address comments. #
Total comments: 22
Patch Set 4 : Update tests. #Patch Set 5 : Cast key systems. #Patch Set 6 : Comments. #
Total comments: 6
Patch Set 7 : Include build_config.h. #Messages
Total messages: 52 (21 generated)
sandersd@chromium.org changed reviewers: + ddorwin@chromium.org, jrummell@chromium.org
This is not quite done yet, there are two places I need to read the renderer preference to properly implement the logic (one is for prefixed, the other for prefixed). The main body of code will not be changed.
LG. https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/and... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:57: media::EME_SESSION_TYPE_NOT_SUPPORTED, // persistent-license. Shouldn't these be media::EmeSessionTypeSupport::? https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:74: response.non_compositing_codecs), // Regular codecs. Can't you just use codecs here? Surprised the compiler doesn't complain about unused variable. https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:110: // Assume that plaform key systems support no features but can and will s/plaform/platform/ https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/wid... File components/cdm/renderer/widevine_key_systems.cc (right): https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/wid... components/cdm/renderer/widevine_key_systems.cc:64: info.supported_secure_codecs = supported_secure_codecs; Do we need a DCHECK that |supported_secure_codecs| is a subset of |supported_codecs|, since we don't support only hardware-secure codecs? https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/wid... File components/cdm/renderer/widevine_key_systems.h (right): https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/wid... components/cdm/renderer/widevine_key_systems.h:26: media::SupportedCodecs supported_secure_codecs, Will this be needed by other platforms (e.g. CrOS)? Would it make sense to always include it, and all non-Android callers would pass in media::EME_CODEC_NONE to indicate there are no secure codecs? https://codereview.chromium.org/1106263004/diff/1/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1106263004/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:722: support = EmeConfigRule::SECURE_CODECS_NOT_ALLOWED; If |codecs| contains 2 codecs, both of which are in |key_system_codec_mask| but only 1 in |key_system_secure_codec_mask|, is this the correct response? Does it depend on order?
LG. Thanks. https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/and... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:50: if (response.compositing_codecs != media::EME_CODEC_NONE) { Should we DCHECK that this is not NONE if noncomp is not NONE? This would be unexpected and cause unprefixed to never work, right? https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:69: SupportedCodecs codecs = response.compositing_codecs & Probably worth a comment explaining why. https://codereview.chromium.org/1106263004/diff/1/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1106263004/diff/1/media/base/eme_constants.h#... media/base/eme_constants.h:24: const InitDataTypeMask kInitDataTypeMaskNone = 0; We should eventually be consistent in how we define masks (or stop using them). :) https://codereview.chromium.org/1106263004/diff/1/media/base/eme_constants.h#... media/base/eme_constants.h:137: // In this mode only secure surfaces (hole-punching) will be functional. This last sentence only applies to video. https://codereview.chromium.org/1106263004/diff/1/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1106263004/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:719: // We don't support the inverse (only hardware-secure codecs allowed) for The subject of "the inverse" is not clearly specified. (Presumably it's line 712, but that's not directly related.) It seems there should be a comment before this explaining what is happening. Then you can say that the inverse is not supported. https://codereview.chromium.org/1106263004/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:720: // simplicity, but such support could be added here. What do you mean "could be added"? Does this mean, if we need support, it would be added here? https://codereview.chromium.org/1106263004/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:771: if (key_system == kWidevineKeySystem) { We might extract the WV logic into a function for clarity and potential refactoring later. https://codereview.chromium.org/1106263004/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:785: #endif // defined(OS_CHROMEOS) #elif https://codereview.chromium.org/1106263004/diff/1/media/blink/key_system_conf... File media/blink/key_system_config_selector.cc (right): https://codereview.chromium.org/1106263004/diff/1/media/blink/key_system_conf... media/blink/key_system_config_selector.cc:221: are_secure_codecs_not_allowed_ = true; DCHECK(!are_secure_codecs_required_); Ditto below. https://codereview.chromium.org/1106263004/diff/1/media/blink/key_system_conf... media/blink/key_system_config_selector.cc:295: // |config_state| will only be committed by the caller if we return true, so This is a bit scary. Ideally, this function should enforce this (i.e. with a temporary local config_state, as in the function below). At a minimum, the header file should say that |config_state| should be discarded if false is returned (or something like that). I didn't look at where the config_state comes from, though (i.e. whether it's a member of something or a local var created to pass to this function). Ditto for all other functions that take config_state?
https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/and... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:57: media::EME_SESSION_TYPE_NOT_SUPPORTED, // persistent-license. On 2015/04/30 02:08:37, jrummell wrote: > Shouldn't these be media::EmeSessionTypeSupport::? Done. https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:74: response.non_compositing_codecs), // Regular codecs. On 2015/04/30 02:08:37, jrummell wrote: > Can't you just use codecs here? Surprised the compiler doesn't complain about > unused variable. Done. https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:110: // Assume that plaform key systems support no features but can and will On 2015/04/30 02:08:37, jrummell wrote: > s/plaform/platform/ Done. https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/wid... File components/cdm/renderer/widevine_key_systems.cc (right): https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/wid... components/cdm/renderer/widevine_key_systems.cc:64: info.supported_secure_codecs = supported_secure_codecs; On 2015/04/30 02:08:37, jrummell wrote: > Do we need a DCHECK that |supported_secure_codecs| is a subset of > |supported_codecs|, since we don't support only hardware-secure codecs? I had that, but in the end moved the subset logic to KeySystems. This way the KeySystemInfo object is correct, and if we change our minds on how to handle that case only KeySystems needs to change. https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/wid... File components/cdm/renderer/widevine_key_systems.h (right): https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/wid... components/cdm/renderer/widevine_key_systems.h:26: media::SupportedCodecs supported_secure_codecs, On 2015/04/30 02:08:37, jrummell wrote: > Will this be needed by other platforms (e.g. CrOS)? Would it make sense to > always include it, and all non-Android callers would pass in > media::EME_CODEC_NONE to indicate there are no secure codecs? No, this distinction is Android-only, at least as currently designed. https://codereview.chromium.org/1106263004/diff/1/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1106263004/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:722: support = EmeConfigRule::SECURE_CODECS_NOT_ALLOWED; On 2015/04/30 02:08:37, jrummell wrote: > If |codecs| contains 2 codecs, both of which are in |key_system_codec_mask| but > only 1 in |key_system_secure_codec_mask|, is this the correct response? Does it > depend on order? I believe this is correct; and codec that does not support hardware-secure decode, anywhere (that is, as any codec in any capability), prevents use of all secure codecs. This is because we can't switch security modes on the fly, so we have to pick one or the other at the start, but we need to be able to support any/all of the capabilities we accept.
https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/and... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:50: if (response.compositing_codecs != media::EME_CODEC_NONE) { On 2015/04/30 18:18:04, ddorwin wrote: > Should we DCHECK that this is not NONE if noncomp is not NONE? This would be > unexpected and cause unprefixed to never work, right? Done. https://codereview.chromium.org/1106263004/diff/1/components/cdm/renderer/and... components/cdm/renderer/android_key_systems.cc:69: SupportedCodecs codecs = response.compositing_codecs & On 2015/04/30 18:18:04, ddorwin wrote: > Probably worth a comment explaining why. Done. https://codereview.chromium.org/1106263004/diff/1/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1106263004/diff/1/media/base/eme_constants.h#... media/base/eme_constants.h:24: const InitDataTypeMask kInitDataTypeMaskNone = 0; On 2015/04/30 18:18:04, ddorwin wrote: > We should eventually be consistent in how we define masks (or stop using them). > :) Acknowledged. https://codereview.chromium.org/1106263004/diff/1/media/base/eme_constants.h#... media/base/eme_constants.h:137: // In this mode only secure surfaces (hole-punching) will be functional. On 2015/04/30 18:18:04, ddorwin wrote: > This last sentence only applies to video. Done. https://codereview.chromium.org/1106263004/diff/1/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1106263004/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:719: // We don't support the inverse (only hardware-secure codecs allowed) for On 2015/04/30 18:18:04, ddorwin wrote: > The subject of "the inverse" is not clearly specified. (Presumably it's line > 712, but that's not directly related.) It seems there should be a comment before > this explaining what is happening. Then you can say that the inverse is not > supported. Done. https://codereview.chromium.org/1106263004/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:720: // simplicity, but such support could be added here. On 2015/04/30 18:18:04, ddorwin wrote: > What do you mean "could be added"? Does this mean, if we need support, it would > be added here? Done. https://codereview.chromium.org/1106263004/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:771: if (key_system == kWidevineKeySystem) { On 2015/04/30 18:18:04, ddorwin wrote: > We might extract the WV logic into a function for clarity and potential > refactoring later. Acknowledged. https://codereview.chromium.org/1106263004/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:785: #endif // defined(OS_CHROMEOS) On 2015/04/30 18:18:04, ddorwin wrote: > #elif Done. https://codereview.chromium.org/1106263004/diff/1/media/blink/key_system_conf... File media/blink/key_system_config_selector.cc (right): https://codereview.chromium.org/1106263004/diff/1/media/blink/key_system_conf... media/blink/key_system_config_selector.cc:221: are_secure_codecs_not_allowed_ = true; On 2015/04/30 18:18:04, ddorwin wrote: > DCHECK(!are_secure_codecs_required_); > > Ditto below. This is implemented in the IsRuleSupported() check above. https://codereview.chromium.org/1106263004/diff/1/media/blink/key_system_conf... media/blink/key_system_config_selector.cc:295: // |config_state| will only be committed by the caller if we return true, so On 2015/04/30 18:18:04, ddorwin wrote: > This is a bit scary. Ideally, this function should enforce this (i.e. with a > temporary local config_state, as in the function below). > > At a minimum, the header file should say that |config_state| should be discarded > if false is returned (or something like that). I didn't look at where the > config_state comes from, though (i.e. whether it's a member of something or a > local var created to pass to this function). > > Ditto for all other functions that take config_state? I've re-ordered the method to follow the same logic as every other method.
lgtm
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1106263004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jrummell@chromium.org Link to the patchset: https://codereview.chromium.org/1106263004/#ps60001 (title: "Update tests.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1106263004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jrummell@chromium.org Link to the patchset: https://codereview.chromium.org/1106263004/#ps80001 (title: "Cast key systems.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1106263004/80001
LG. Just comments on comments. https://codereview.chromium.org/1106263004/diff/40001/components/cdm/renderer... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/1106263004/diff/40001/components/cdm/renderer... components/cdm/renderer/android_key_systems.cc:48: // Since we do not control the implementation of the MediaDrm API on Android, This seems to be in the wrong place. Or at least there should be an empty line. Perhaps a function-level comment. https://codereview.chromium.org/1106263004/diff/40001/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1106263004/diff/40001/media/base/eme_constant... media/base/eme_constants.h:132: #if defined(OS_ANDROID) I think this comment block is confusing and possibly inaccurate wrt audio now. Suggestions for fixing below. https://codereview.chromium.org/1106263004/diff/40001/media/base/eme_constant... media/base/eme_constants.h:133: // The configuration option is supported if hardware-secure video codecs are remove "video" https://codereview.chromium.org/1106263004/diff/40001/media/base/eme_constant... media/base/eme_constants.h:136: // The configuration option is supported if only hardware-secure video codecs remove "video" https://codereview.chromium.org/1106263004/diff/40001/media/base/eme_constant... media/base/eme_constants.h:138: // functional. ...for video. (or something like that) https://codereview.chromium.org/1106263004/diff/40001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1106263004/diff/40001/media/base/key_systems.... media/base/key_systems.cc:719: // Check whether the codec supports a hardware-secure mode. This nesting Is it really nesting or the fact that we check each iteration? https://codereview.chromium.org/1106263004/diff/40001/media/base/key_systems.... media/base/key_systems.cc:720: // ensures that only codecs that also support the regular mode are allowed; This is a bit confusing to me. Do we mean "ensures that we allow codecs that..."? https://codereview.chromium.org/1106263004/diff/40001/media/base/key_systems.... media/base/key_systems.cc:722: // SECURE_CODECS_REQUIRED when the regular mode is not available, if it Wouldn't we need to then check that all codecs support secure (rather than returning)? https://codereview.chromium.org/1106263004/diff/40001/media/base/key_systems.... media/base/key_systems.cc:723: // turns out that that is a useful feature. Define the second "that". Allowing only hardware-secure codecs? https://codereview.chromium.org/1106263004/diff/40001/media/blink/key_system_... File media/blink/key_system_config_selector.cc (right): https://codereview.chromium.org/1106263004/diff/40001/media/blink/key_system_... media/blink/key_system_config_selector.cc:303: // is stripped. Validation of the correctness of such strings was handled above. https://codereview.chromium.org/1106263004/diff/40001/media/blink/key_system_... media/blink/key_system_config_selector.cc:310: config_state->AddRule(codecs_rule); nit: empty line
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/1106263004/diff/40001/components/cdm/renderer... File components/cdm/renderer/android_key_systems.cc (right): https://codereview.chromium.org/1106263004/diff/40001/components/cdm/renderer... components/cdm/renderer/android_key_systems.cc:48: // Since we do not control the implementation of the MediaDrm API on Android, On 2015/05/01 20:39:11, ddorwin wrote: > This seems to be in the wrong place. Or at least there should be an empty line. > Perhaps a function-level comment. Done. https://codereview.chromium.org/1106263004/diff/40001/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1106263004/diff/40001/media/base/eme_constant... media/base/eme_constants.h:132: #if defined(OS_ANDROID) On 2015/05/01 20:39:11, ddorwin wrote: > I think this comment block is confusing and possibly inaccurate wrt audio now. > Suggestions for fixing below. Acknowledged. https://codereview.chromium.org/1106263004/diff/40001/media/base/eme_constant... media/base/eme_constants.h:133: // The configuration option is supported if hardware-secure video codecs are On 2015/05/01 20:39:11, ddorwin wrote: > remove "video" Done. https://codereview.chromium.org/1106263004/diff/40001/media/base/eme_constant... media/base/eme_constants.h:136: // The configuration option is supported if only hardware-secure video codecs On 2015/05/01 20:39:11, ddorwin wrote: > remove "video" Done. https://codereview.chromium.org/1106263004/diff/40001/media/base/eme_constant... media/base/eme_constants.h:138: // functional. On 2015/05/01 20:39:11, ddorwin wrote: > ...for video. > > (or something like that) Done. https://codereview.chromium.org/1106263004/diff/40001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1106263004/diff/40001/media/base/key_systems.... media/base/key_systems.cc:719: // Check whether the codec supports a hardware-secure mode. This nesting On 2015/05/01 20:39:11, ddorwin wrote: > Is it really nesting or the fact that we check each iteration? Really it's that failing the regular codec test is an early exit. I've attempted to clarify. https://codereview.chromium.org/1106263004/diff/40001/media/base/key_systems.... media/base/key_systems.cc:720: // ensures that only codecs that also support the regular mode are allowed; On 2015/05/01 20:39:12, ddorwin wrote: > This is a bit confusing to me. Do we mean "ensures that we allow codecs > that..."? Acknowledged. https://codereview.chromium.org/1106263004/diff/40001/media/base/key_systems.... media/base/key_systems.cc:722: // SECURE_CODECS_REQUIRED when the regular mode is not available, if it On 2015/05/01 20:39:12, ddorwin wrote: > Wouldn't we need to then check that all codecs support secure (rather than > returning)? Done. https://codereview.chromium.org/1106263004/diff/40001/media/base/key_systems.... media/base/key_systems.cc:723: // turns out that that is a useful feature. On 2015/05/01 20:39:12, ddorwin wrote: > Define the second "that". > Allowing only hardware-secure codecs? Done. https://codereview.chromium.org/1106263004/diff/40001/media/blink/key_system_... File media/blink/key_system_config_selector.cc (right): https://codereview.chromium.org/1106263004/diff/40001/media/blink/key_system_... media/blink/key_system_config_selector.cc:303: // is stripped. On 2015/05/01 20:39:12, ddorwin wrote: > Validation of the correctness of such strings was handled above. Done. https://codereview.chromium.org/1106263004/diff/40001/media/blink/key_system_... media/blink/key_system_config_selector.cc:310: config_state->AddRule(codecs_rule); On 2015/05/01 20:39:12, ddorwin wrote: > nit: empty line Done.
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jrummell@chromium.org Link to the patchset: https://codereview.chromium.org/1106263004/#ps100001 (title: "Comments.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1106263004/100001
LGTM % comments. https://codereview.chromium.org/1106263004/diff/100001/chromecast/renderer/ke... File chromecast/renderer/key_systems_cast.cc (right): https://codereview.chromium.org/1106263004/diff/100001/chromecast/renderer/ke... chromecast/renderer/key_systems_cast.cc:53: codecs, // Hardware-secure codecs. Hmm. We should have someone from Cast on Android look at this. https://codereview.chromium.org/1106263004/diff/100001/media/blink/key_system... File media/blink/key_system_config_selector.cc (right): https://codereview.chromium.org/1106263004/diff/100001/media/blink/key_system... media/blink/key_system_config_selector.cc:310: config_state->AddRule(codecs_rule); nit: I meant before this line. The "success path" should be separated.
https://codereview.chromium.org/1106263004/diff/100001/chromecast/renderer/ke... File chromecast/renderer/key_systems_cast.cc (right): https://codereview.chromium.org/1106263004/diff/100001/chromecast/renderer/ke... chromecast/renderer/key_systems_cast.cc:53: codecs, // Hardware-secure codecs. On 2015/05/01 22:26:13, ddorwin wrote: > Hmm. We should have someone from Cast on Android look at this. Possibly. However, since they are specifically listing codecs here, I assume it applies regardless of the renderer pref, so there should be no behavior change due to this CL. https://codereview.chromium.org/1106263004/diff/100001/media/blink/key_system... File media/blink/key_system_config_selector.cc (right): https://codereview.chromium.org/1106263004/diff/100001/media/blink/key_system... media/blink/key_system_config_selector.cc:310: config_state->AddRule(codecs_rule); On 2015/05/01 22:26:13, ddorwin wrote: > nit: I meant before this line. The "success path" should be separated. This same if-block followed by AddRule() pattern appears repeatedly in KeySystemConfigSelector; the idea is that you want to do as little as possible between the check and adding the rule (in particular, it's wrong to do two checks and then add both rules; you must add a rule before checking the next). We could combine the operations into a single call, the only place that needs the separation is steps 12 & 13 where we pick a preferred supported rule.
lgtm https://codereview.chromium.org/1106263004/diff/100001/chromecast/renderer/ke... File chromecast/renderer/key_systems_cast.cc (right): https://codereview.chromium.org/1106263004/diff/100001/chromecast/renderer/ke... chromecast/renderer/key_systems_cast.cc:53: codecs, // Hardware-secure codecs. On 2015/05/01 22:33:21, sandersd wrote: > On 2015/05/01 22:26:13, ddorwin wrote: > > Hmm. We should have someone from Cast on Android look at this. > > Possibly. However, since they are specifically listing codecs here, I assume it > applies regardless of the renderer pref, so there should be no behavior change > due to this CL. Acknowledged. You'll at least need an OWNER to review this file. :) https://codereview.chromium.org/1106263004/diff/100001/media/blink/key_system... File media/blink/key_system_config_selector.cc (right): https://codereview.chromium.org/1106263004/diff/100001/media/blink/key_system... media/blink/key_system_config_selector.cc:310: config_state->AddRule(codecs_rule); On 2015/05/01 22:33:21, sandersd wrote: > On 2015/05/01 22:26:13, ddorwin wrote: > > nit: I meant before this line. The "success path" should be separated. > > This same if-block followed by AddRule() pattern appears repeatedly in > KeySystemConfigSelector; the idea is that you want to do as little as possible > between the check and adding the rule (in particular, it's wrong to do two > checks and then add both rules; you must add a rule before checking the next). > > We could combine the operations into a single call, the only place that needs > the separation is steps 12 & 13 where we pick a preferred supported rule. Acknowledged.
sandersd@chromium.org changed reviewers: + boliu@chromium.org, gunsch@chromium.org
boliu@chromium.org: Please review changes in android_webview/renderer/aw_key_systems.cc gunsch@chromium.org: Please review changes in chromecast/renderer/key_systems_cast.cc
lgtm for chromecast/ Out of curiosity, what are the future plans for "secure" codec detection? Right now it seems to be set here, not actually hooked up to the platform: https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/bro... In particular, 1) Are there plans to actually query the Android platform for secure audio codecs? (I assume yes) 2) Are there plans to add support for secure audio codecs? (less clear)
rs lgtm
> 1) Are there plans to actually query the Android platform for secure audio > codecs? (I assume yes) No. > 2) Are there plans to add support for secure audio codecs? (less clear) No. My understanding is that there is no secure output path, and thus no benefit to offering secure codecs. The implementation follows this doc: https://docs.google.com/document/d/1sownzH8P12V9pCI1HaUtx66WRPc_wIZc928K2hJNt.... Please do add comments there if you have additional details. As usual Chromecast does not use the detection logic anyway and instead has a hardcoded list.
On 2015/05/04 18:19:53, sandersd wrote: > > 1) Are there plans to actually query the Android platform for secure audio > > codecs? (I assume yes) > > No. > Whoops, my questions are clearly redundant as written. I meant this first question to be, "are there plans to query the Android platform for secure _video_ codecs?" > > 2) Are there plans to add support for secure audio codecs? (less clear) > > No. My understanding is that there is no secure output path, and thus no benefit > to offering secure codecs. I'm not sure that's true, per conversations with Guru and the APE team lately. They've been asking us about what it would take to support secure audio output in Cast on ATV (and thus the Clank audio path). We may want to continue this conversation separately. > > The implementation follows this doc: > https://docs.google.com/document/d/1sownzH8P12V9pCI1HaUtx66WRPc_wIZc928K2hJNt.... > Please do add comments there if you have additional details. SGTM, thanks for the link. > > As usual Chromecast does not use the detection logic anyway and instead has a > hardcoded list.
> > Are there plans to query the Android platform for secure > _video_ codecs? > We do query that, the implementation you linked checks for ".secure" variants of video codecs. It's just not hooked up for Chromecast; the Chromecast team is free to do that. I'm not sure that's true, per conversations with Guru and the APE team > lately. > They've been asking us about what it would take to support secure audio > output > in Cast on ATV (and thus the Clank audio path). We may want to continue > this > conversation separately. I don't believe that Android gives us a way to require secure audio. If there was, we would need a design that goes all the way from the codec API, through Widevine, and finally Chrome would need to be aware that it shouldn't handle the audio. It's a pretty big change, and that last one is problematic on Android for synchronization reasons. Getting all that right probably puts us back to using the built-in player, in full-screen mode. I doubt you can convince Clank to to that, but WebView is a possibility. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #7 (id:120001) has been deleted
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jrummell@chromium.org, ddorwin@chromium.org, boliu@chromium.org, gunsch@chromium.org Link to the patchset: https://codereview.chromium.org/1106263004/#ps140001 (title: "Include build_config.h")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1106263004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jrummell@chromium.org, boliu@chromium.org, ddorwin@chromium.org, gunsch@chromium.org Link to the patchset: https://codereview.chromium.org/1106263004/#ps160001 (title: "Include build_config.h.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1106263004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/1106263004/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a6bf8b52a70dafe95e8e2d8827f5e1283ec1bf56 Cr-Commit-Position: refs/heads/master@{#328464} |
