Allow hw secured codecs on chromecast

Allow hw secured codecs on chromecast

The CL will allow chromecast see robustness configs in browser process.
Audio devices will reject robustness with hardware features.

1. Add feature flag enable_hw_secure_codec, which is true on android and
chromecast.
2. use_video_overlay_for_embedded_encrypted_video = true for chromecast
video devices.

BUG=470242
TEST=Audio device reject robustness HW_SECURE_*

[yucliu1, 2016-05-23 23:15:06]

Description was changed from

==========
Allow hw secured codecs on chromecast

The CL will allow chromecast see robustness configs in browser process.
Audio devices will reject robustness with hardware features.

1. Add feature flag enable_hw_secure_codec, which is true on android and
chromecast.
2. use_video_overlay_for_embedded_encrypted_video = true for chromecast
video devices.

BUG=470242
TEST=Audio device reject robustness HW_SECURE_*
==========

to

==========
Allow hw secured codecs on chromecast

The CL will allow chromecast see robustness configs in browser process.
Audio devices will reject robustness with hardware features.

1. Add feature flag enable_hw_secure_codec, which is true on android and
chromecast.
2. use_video_overlay_for_embedded_encrypted_video = true for chromecast
video devices.

BUG=470242
TEST=Audio device reject robustness HW_SECURE_*
==========

[yucliu1, 2016-05-23 23:15:06]

yucliu@chromium.org changed reviewers:
+ esprehn@chromium.org, halliwell@chromium.org, jrummell@chromium.org,
xhwang@chromium.org

[yucliu1, 2016-05-23 23:17:39]

The patch would allow chromecast see correct CdmConfig::use_hw_secure_codecs on
browser side and reject robustness >= SW_SECURE_DECODE on audio platforms.

[esprehn, 2016-05-23 23:34:39]

esprehn@chromium.org changed reviewers:
+ jochen@chromium.org

[esprehn, 2016-05-23 23:34:40]

I think jochen@ might be a better reviewer for this?

[jrummell, 2016-05-24 18:27:50]

media/ LGTM

https://codereview.chromium.org/2006113002/diff/1/components/cdm/renderer/wid...
File components/cdm/renderer/widevine_key_system_properties.cc (right):

https://codereview.chromium.org/2006113002/diff/1/components/cdm/renderer/wid...
components/cdm/renderer/widevine_key_system_properties.cc:138: #elif
BUILDFLAG(ENABLE_HW_SECURE_CODEC)
Should this be it's own #if (so #endif // OS_CHROMEOS, #if
ENABLE_HW_SECURE_CODEC)? I don't see any restriction that CrOS can't use secure
codecs in the future.

https://codereview.chromium.org/2006113002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2006113002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:6043: #endif  // defined(OS_ANDROID)
nit: should be BUILDFLAG(ENABLE_HW_SECURE_CODEC)

[yucliu1, 2016-05-24 21:22:36]

https://codereview.chromium.org/2006113002/diff/1/components/cdm/renderer/wid...
File components/cdm/renderer/widevine_key_system_properties.cc (right):

https://codereview.chromium.org/2006113002/diff/1/components/cdm/renderer/wid...
components/cdm/renderer/widevine_key_system_properties.cc:138: #elif
BUILDFLAG(ENABLE_HW_SECURE_CODEC)
On 2016/05/24 18:27:50, jrummell wrote:
> Should this be it's own #if (so #endif // OS_CHROMEOS, #if
> ENABLE_HW_SECURE_CODEC)? I don't see any restriction that CrOS can't use
secure
> codecs in the future. 

Done.

https://codereview.chromium.org/2006113002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2006113002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:6043: #endif  // defined(OS_ANDROID)
On 2016/05/24 18:27:50, jrummell wrote:
> nit: should be BUILDFLAG(ENABLE_HW_SECURE_CODEC)

Done.

[ddorwin, 2016-05-24 22:05:40]

ddorwin@chromium.org changed reviewers:
+ ddorwin@chromium.org

[ddorwin, 2016-05-24 22:05:41]

https://codereview.chromium.org/2006113002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_content_window.cc (right):

https://codereview.chromium.org/2006113002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_content_window.cc:121:
prefs->use_video_overlay_for_embedded_encrypted_video = true;
This is currently related directly to VIDEO_HOLE (and only used on Android via
AwSettings and Cast on Android). If Cast started using WMPI, this might create
unintended consequences (vs. the Ozone-based solution).

https://codereview.chromium.org/2006113002/diff/20001/content/public/common/r...
File content/public/common/renderer_preferences.h (right):

https://codereview.chromium.org/2006113002/diff/20001/content/public/common/r...
content/public/common/renderer_preferences.h:131: // encrypted video.  Currently
used by Android and Chromecast
ditto

[yucliu1, 2016-05-24 22:26:33]

https://codereview.chromium.org/2006113002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_content_window.cc (right):

https://codereview.chromium.org/2006113002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_content_window.cc:121:
prefs->use_video_overlay_for_embedded_encrypted_video = true;
On 2016/05/24 22:05:41, ddorwin wrote:
> This is currently related directly to VIDEO_HOLE (and only used on Android via
> AwSettings and Cast on Android). If Cast started using WMPI, this might create
> unintended consequences (vs. the Ozone-based solution).

I found almost all the logics related to this boolean is inside android (except
the usage to select media key system config). Will the change break cast on
android or cast on all platforms (if cast use WMPI)?

Btw, what is WMPI, can you give some pointer?

[halliwell, 2016-05-24 22:51:02]

https://codereview.chromium.org/2006113002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_content_window.cc (right):

https://codereview.chromium.org/2006113002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_content_window.cc:121:
prefs->use_video_overlay_for_embedded_encrypted_video = true;
On 2016/05/24 22:26:33, yucliu1 wrote:
> On 2016/05/24 22:05:41, ddorwin wrote:
> > This is currently related directly to VIDEO_HOLE (and only used on Android
via
> > AwSettings and Cast on Android). If Cast started using WMPI, this might
create
> > unintended consequences (vs. the Ozone-based solution).
> 
> I found almost all the logics related to this boolean is inside android
(except
> the usage to select media key system config). Will the change break cast on
> android or cast on all platforms (if cast use WMPI)?
> 
> Btw, what is WMPI, can you give some pointer?

Yuchen: WMPI = WebMediaPlayerImpl
David, could you expand on your thoughts here?

This change doesn't affect Android or Cast on ATV as it stands, I think?  It's
making use of the same flag but on our linux+WMPI+Ozone pipeline.  Are you
talking about a future scenario where Cast on ATV starts using WMPI?

[ddorwin, 2016-05-24 22:53:37]

ddorwin@chromium.org changed required reviewers:
+ xhwang@chromium.org

[ddorwin, 2016-05-24 23:11:50]

https://codereview.chromium.org/2006113002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_content_window.cc (right):

https://codereview.chromium.org/2006113002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_content_window.cc:121:
prefs->use_video_overlay_for_embedded_encrypted_video = true;
On 2016/05/24 22:51:02, halliwell wrote:
> On 2016/05/24 22:26:33, yucliu1 wrote:
> > On 2016/05/24 22:05:41, ddorwin wrote:
> > > This is currently related directly to VIDEO_HOLE (and only used on Android
> via
> > > AwSettings and Cast on Android). If Cast started using WMPI, this might
> create
> > > unintended consequences (vs. the Ozone-based solution).
> > 
> > I found almost all the logics related to this boolean is inside android
> (except
> > the usage to select media key system config). Will the change break cast on
> > android or cast on all platforms (if cast use WMPI)?
> > 
> > Btw, what is WMPI, can you give some pointer?
> 
> Yuchen: WMPI = WebMediaPlayerImpl
> David, could you expand on your thoughts here?
> 
> This change doesn't affect Android or Cast on ATV as it stands, I think?  It's
> making use of the same flag but on our linux+WMPI+Ozone pipeline.  Are you
> talking about a future scenario where Cast on ATV starts using WMPI?

WMPI will soon be used for all EME content on Android (replacing WMPA).
Presumably, we will need to migrate the related WMPA logic there, which could
then have side effects on Chromecast.

At a higher level, though, this flag means "create a hole for EME video." That
is not necessarily the same thing as "use HW secure codecs," which we use as a
synonym for setting the security level in MediaDrmBridge.

https://codereview.chromium.org/2006113002/diff/20001/chromecast/renderer/key...
File chromecast/renderer/key_systems_cast.cc (right):

https://codereview.chromium.org/2006113002/diff/20001/chromecast/renderer/key...
chromecast/renderer/key_systems_cast.cc:90: codecs,  // Hardware-secure codecs.
Do you mean to provide the same set of codecs on all devices? That was probably
an assumption about ATV devices.

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer...
File components/cdm/renderer/widevine_key_system_properties.cc (right):

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer...
components/cdm/renderer/widevine_key_system_properties.cc:140: #if
BUILDFLAG(ENABLE_HW_SECURE_CODEC)
This name could be misleading. The parameter name was simply the most generic
term we could think of to pass for Android. It's possible to support higher
robustness levels without needing this logic.

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer...
components/cdm/renderer/widevine_key_system_properties.cc:141: // Require
hardware secure codecs when SW_SECURE_DECODE or above is specified.
This logic may not apply to all platforms.

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer...
File components/cdm/renderer/widevine_key_system_properties.h (right):

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer...
components/cdm/renderer/widevine_key_system_properties.h:20:
media::SupportedCodecs supported_secure_codecs,
As noted elsewhere, this was an Android specific thing. We don't use it on all
platforms that have secure codecs. Now that we have moved away from the Info
object, we might want to move to a different mechanism of providing
codec-robustness pairs.

For now, though, the build flag naming is misleading.

[yucliu1, 2016-05-25 00:43:21]

https://codereview.chromium.org/2006113002/diff/20001/chromecast/renderer/key...
File chromecast/renderer/key_systems_cast.cc (right):

https://codereview.chromium.org/2006113002/diff/20001/chromecast/renderer/key...
chromecast/renderer/key_systems_cast.cc:90: codecs,  // Hardware-secure codecs.
On 2016/05/24 23:11:50, ddorwin wrote:
> Do you mean to provide the same set of codecs on all devices? That was
probably
> an assumption about ATV devices.

Audio devices should use EME_CODEC_NONE. But the concept for secure codecs
should be valid for platform supporting hw secured codecs.

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer...
File components/cdm/renderer/widevine_key_system_properties.cc (right):

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer...
components/cdm/renderer/widevine_key_system_properties.cc:140: #if
BUILDFLAG(ENABLE_HW_SECURE_CODEC)
On 2016/05/24 23:11:50, ddorwin wrote:
> This name could be misleading. The parameter name was simply the most generic
> term we could think of to pass for Android. It's possible to support higher
> robustness levels without needing this logic.

The name ENABLE_HW_SECURE_CODEC or some other name here should imply the
platform is able to handle hardware secured codec if requires.

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer...
components/cdm/renderer/widevine_key_system_properties.cc:141: // Require
hardware secure codecs when SW_SECURE_DECODE or above is specified.
On 2016/05/24 23:11:50, ddorwin wrote:
> This logic may not apply to all platforms.

Agree. This part should be Android specific. And I can implement cast specific
logic by overriding this method.

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer...
File components/cdm/renderer/widevine_key_system_properties.h (right):

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer...
components/cdm/renderer/widevine_key_system_properties.h:20:
media::SupportedCodecs supported_secure_codecs,
On 2016/05/24 23:11:50, ddorwin wrote:
> As noted elsewhere, this was an Android specific thing. We don't use it on all
> platforms that have secure codecs. Now that we have moved away from the Info
> object, we might want to move to a different mechanism of providing
> codec-robustness pairs.
> 
> For now, though, the build flag naming is misleading.

Is the *secure_codecs* here the same as *hw_secure_codecs*? If so, I think the
concept should be valid for all platforms supporting hw secured codecs.

https://codereview.chromium.org/2006113002/diff/20001/content/public/common/r...
File content/public/common/renderer_preferences.h (right):

https://codereview.chromium.org/2006113002/diff/20001/content/public/common/r...
content/public/common/renderer_preferences.h:131: // encrypted video.  Currently
used by Android and Chromecast
On 2016/05/24 22:05:41, ddorwin wrote:
> ditto

How about adding a new flag?

[yucliu1, 2016-05-27 02:18:07]

1. Add new flag hw_secure_codec_allowed.
2. Handle robustness config rule in chromecast's own widevine key system
properties.

[ddorwin, 2016-05-27 21:33:01]

I think there might be a much simpler solution. See the comment in
/renderer_preferences.h.

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer...
File components/cdm/renderer/widevine_key_system_properties.h (right):

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer...
components/cdm/renderer/widevine_key_system_properties.h:20:
media::SupportedCodecs supported_secure_codecs,
On 2016/05/25 00:43:21, yucliu1 wrote:
> On 2016/05/24 23:11:50, ddorwin wrote:
> > As noted elsewhere, this was an Android specific thing. We don't use it on
all
> > platforms that have secure codecs. Now that we have moved away from the Info
> > object, we might want to move to a different mechanism of providing
> > codec-robustness pairs.
> > 
> > For now, though, the build flag naming is misleading.
> 
> Is the *secure_codecs* here the same as *hw_secure_codecs*? If so, I think the
> concept should be valid for all platforms supporting hw secured codecs.

I believe (on Android) `supported_secure_codecs` is the set of video codecs that
will be available if you set `use_hw_secure_codecs`. The fact that the
robustness affects the available codecs is currently unique to Android and
something we ignore on all other platforms (e.g. Chrome OS). If we want to
generalize this, we should do that as a separate CL and figure out what that
means.

xhwang and I will discuss offline next week.

https://codereview.chromium.org/2006113002/diff/40001/chromecast/renderer/key...
File chromecast/renderer/key_systems_cast.cc (right):

https://codereview.chromium.org/2006113002/diff/40001/chromecast/renderer/key...
chromecast/renderer/key_systems_cast.cc:92: // Note: On Chromecast, all CDMs may
have persistent state.
Even the audio devices?

https://codereview.chromium.org/2006113002/diff/40001/chromecast/renderer/key...
chromecast/renderer/key_systems_cast.cc:108: if (robustness >=
EmeRobustness::SW_SECURE_DECODE)
It seems that this object and method only exist to duplicate code that is
currently in a #elif defined(OS_ANDROID) block in the parent class's
implementation.

We should either extract all platform-specific logic or find a way to trigger
this check. For example, by appling "secure codecs" to all platforms and define
a min_robustness_requiring_secure_codecs_ member.

https://codereview.chromium.org/2006113002/diff/40001/chromecast/renderer/key...
chromecast/renderer/key_systems_cast.cc:133: EmeRobustness max_video_robustness
= EmeRobustness::INVALID;
This will cause all configurations to fail because EMPTY > INVALID.

We might want to DCHECK in the WidevineKeySystemProperties that the values are >
INVALID. It should only be returned when the string conversion fails.

https://codereview.chromium.org/2006113002/diff/40001/content/public/common/r...
File content/public/common/renderer_preferences.h (right):

https://codereview.chromium.org/2006113002/diff/40001/content/public/common/r...
content/public/common/renderer_preferences.h:140: bool hw_secure_codec_allowed;
I don't think we should be adding members to this, especially for something that
is not runtime-configurable as is the case for Cast.

Do you even need this now that you have different max robustness levels?

AFAICT, all this does is cause HW_SECURE_CODECS_NOT_ALLOWED to not be added as a
rule. But that only matters if a HW_SECURE_CODECS_REQUIRED rule is added.

So, while not strictly correct (as is the case on other non-Android platforms),
there might be a much simpler way to address this that doesn't involve
addressing the general problem.

[esprehn, 2016-06-01 05:07:33]

esprehn@chromium.org changed reviewers:
- esprehn@chromium.org

[yucliu1, 2016-06-14 00:17:46]

Sorry for the delay.

The simplest way to satisfy the requirement to reject hardware secured codec for
audio devices is to just change the widevine cdm configs. The drawback is that
browser side won't be able to know anything about user specified robustness
string through CdmConfig::use_hw_secure_codec. Given we don't rely on that flag
now, I'll go with the simple method (change widevine config).

I'll submit a new CL later.

https://codereview.chromium.org/2006113002/diff/40001/chromecast/renderer/key...
File chromecast/renderer/key_systems_cast.cc (right):

https://codereview.chromium.org/2006113002/diff/40001/chromecast/renderer/key...
chromecast/renderer/key_systems_cast.cc:133: EmeRobustness max_video_robustness
= EmeRobustness::INVALID;
On 2016/05/27 21:33:00, ddorwin wrote:
> This will cause all configurations to fail because EMPTY > INVALID.
> 
> We might want to DCHECK in the WidevineKeySystemProperties that the values are
>
> INVALID. It should only be returned when the string conversion fails.

This is guarded under DISABLE_DISPLAY. All video related config should be
rejected.