Allow hw secured codecs on chromecast (issue 2006113002 by yucliu@chromium.org)

79 views
Skip to first unread message

yuc...@chromium.org

unread,
May 23, 2016, 7:17:40 PM5/23/16
to hall...@chromium.org, esp...@chromium.org, xhw...@chromium.org, jrum...@chromium.org, chromium...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, creis...@chromium.org, droger+w...@chromium.org, eme-r...@chromium.org, blundell+...@chromium.org, lcwu+...@chromium.org, sdefresne...@chromium.org, j...@chromium.org, feature-me...@chromium.org, dari...@chromium.org, halliwe...@chromium.org, mkwst+moarrev...@chromium.org
Reviewers: halliwell, esprehn, xhwang, jrummell
CL: https://codereview.chromium.org/2006113002/

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

Description:
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_*

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+37, -23 lines):
M chromecast/browser/cast_content_window.cc
M chromecast/browser/media/cast_browser_cdm_factory.cc
M chromecast/renderer/key_systems_cast.cc
M components/cdm/renderer/DEPS
M components/cdm/renderer/widevine_key_system_properties.h
M components/cdm/renderer/widevine_key_system_properties.cc
M content/renderer/render_frame_impl.cc
M media/BUILD.gn
M media/base/key_systems.cc
M media/media_options.gni


Index: chromecast/browser/cast_content_window.cc
diff --git a/chromecast/browser/cast_content_window.cc b/chromecast/browser/cast_content_window.cc
index cd7e7e231a4f5497b009ce2c54b86488118c4ba7..cedd78f98d76779c6e22d976d1e038b8a76a0c6a 100644
--- a/chromecast/browser/cast_content_window.cc
+++ b/chromecast/browser/cast_content_window.cc
@@ -9,10 +9,12 @@
#include "base/threading/thread_restrictions.h"
#include "chromecast/base/metrics/cast_metrics_helper.h"
#include "chromecast/browser/cast_browser_process.h"
+#include "chromecast/chromecast_features.h"
#include "content/public/browser/render_view_host.h"
#include "content/public/browser/render_widget_host.h"
#include "content/public/browser/render_widget_host_view.h"
#include "content/public/browser/web_contents.h"
+#include "content/public/common/renderer_preferences.h"
#include "ipc/ipc_message.h"
#include "ui/display/display.h"
#include "ui/display/screen.h"
@@ -113,6 +115,16 @@ std::unique_ptr<content::WebContents> CastContentWindow::CreateWebContents(
content::WebContents* web_contents = content::WebContents::Create(
create_params);
content::WebContentsObserver::Observe(web_contents);
+
+#if !BUILDFLAG(DISABLE_DISPLAY)
+ content::RendererPreferences* prefs = web_contents->GetMutableRendererPrefs();
+ prefs->use_video_overlay_for_embedded_encrypted_video = true;
+
+ content::RenderViewHost* host = web_contents->GetRenderViewHost();
+ if (host)
+ host->SyncRendererPrefs();
+#endif // !BUILDFLAG(DISABLE_DISPLAY)
+
return base::WrapUnique(web_contents);
}

Index: chromecast/browser/media/cast_browser_cdm_factory.cc
diff --git a/chromecast/browser/media/cast_browser_cdm_factory.cc b/chromecast/browser/media/cast_browser_cdm_factory.cc
index 5393e0a8c9e7e3671765c9cfcd41e16c8e879fe5..86ac18f117897fa2f9e6aea8099cad7636e3afdb 100644
--- a/chromecast/browser/media/cast_browser_cdm_factory.cc
+++ b/chromecast/browser/media/cast_browser_cdm_factory.cc
@@ -40,9 +40,6 @@ void CastBrowserCdmFactory::Create(
::media::CdmCreatedCB bound_cdm_created_cb =
::media::BindToCurrentLoop(cdm_created_cb);

- DCHECK(!cdm_config.use_hw_secure_codecs)
- << "Chromecast does not use |use_hw_secure_codecs|";
-
CastKeySystem cast_key_system(GetKeySystemByName(key_system));

scoped_refptr<chromecast::media::BrowserCdmCast> browser_cdm;
Index: chromecast/renderer/key_systems_cast.cc
diff --git a/chromecast/renderer/key_systems_cast.cc b/chromecast/renderer/key_systems_cast.cc
index 76646540e7c1c519ee8c10dfb305c8787feb0ab4..970d4e1e9b757202ec78dca4462829569eec8402 100644
--- a/chromecast/renderer/key_systems_cast.cc
+++ b/chromecast/renderer/key_systems_cast.cc
@@ -87,9 +87,7 @@ void AddChromecastKeySystems(
::media::EME_CODEC_WEBM_VP8 | ::media::EME_CODEC_WEBM_VP9;
key_systems_properties->emplace_back(new cdm::WidevineKeySystemProperties(
codecs, // Regular codecs.
-#if defined(OS_ANDROID)
codecs, // Hardware-secure codecs.
-#endif
EmeRobustness::HW_SECURE_ALL, // Max audio robustness.
EmeRobustness::HW_SECURE_ALL, // Max video robustness.
EmeSessionTypeSupport::NOT_SUPPORTED, // persistent-license.
Index: components/cdm/renderer/DEPS
diff --git a/components/cdm/renderer/DEPS b/components/cdm/renderer/DEPS
index 725e151a7a51599df3da9cefa866a26df47e9ac1..710d939a5134349b90ce7d51c525d43619cf8aad 100644
--- a/components/cdm/renderer/DEPS
+++ b/components/cdm/renderer/DEPS
@@ -1,3 +1,4 @@
include_rules = [
"+content/public/renderer",
+ "+media/media_features.h", # For flag definitions.
]
Index: components/cdm/renderer/widevine_key_system_properties.cc
diff --git a/components/cdm/renderer/widevine_key_system_properties.cc b/components/cdm/renderer/widevine_key_system_properties.cc
index 3a6eab49728cccf389de14289f54018587bc848d..836ccd064626b40e946e6248168a5b6d3ea695fc 100644
--- a/components/cdm/renderer/widevine_key_system_properties.cc
+++ b/components/cdm/renderer/widevine_key_system_properties.cc
@@ -39,9 +39,9 @@ EmeRobustness ConvertRobustness(const std::string& robustness) {

WidevineKeySystemProperties::WidevineKeySystemProperties(
media::SupportedCodecs supported_codecs,
-#if defined(OS_ANDROID)
+#if BUILDFLAG(ENABLE_HW_SECURE_CODEC)
media::SupportedCodecs supported_secure_codecs,
-#endif // defined(OS_ANDROID)
+#endif // BUILDFLAG(ENABLE_HW_SECURE_CODEC)
media::EmeRobustness max_audio_robustness,
media::EmeRobustness max_video_robustness,
media::EmeSessionTypeSupport persistent_license_support,
@@ -49,9 +49,9 @@ WidevineKeySystemProperties::WidevineKeySystemProperties(
media::EmeFeatureSupport persistent_state_support,
media::EmeFeatureSupport distinctive_identifier_support)
: supported_codecs_(supported_codecs),
-#if defined(OS_ANDROID)
+#if BUILDFLAG(ENABLE_HW_SECURE_CODEC)
supported_secure_codecs_(supported_secure_codecs),
-#endif // defined(OS_ANDROID)
+#endif // BUILDFLAG(ENABLE_HW_SECURE_CODEC)
max_audio_robustness_(max_audio_robustness),
max_video_robustness_(max_video_robustness),
persistent_license_support_(persistent_license_support),
@@ -83,11 +83,11 @@ SupportedCodecs WidevineKeySystemProperties::GetSupportedCodecs() const {
return supported_codecs_;
}

-#if defined(OS_ANDROID)
+#if BUILDFLAG(ENABLE_HW_SECURE_CODEC)
SupportedCodecs WidevineKeySystemProperties::GetSupportedSecureCodecs() const {
return supported_secure_codecs_;
}
-#endif
+#endif // BUILDFLAG(ENABLE_HW_SECURE_CODEC)

EmeConfigRule WidevineKeySystemProperties::GetRobustnessConfigRule(
EmeMediaType media_type,
@@ -135,7 +135,7 @@ EmeConfigRule WidevineKeySystemProperties::GetRobustnessConfigRule(
max_robustness == EmeRobustness::HW_SECURE_ALL) {
return EmeConfigRule::IDENTIFIER_RECOMMENDED;
}
-#elif defined(OS_ANDROID)
+#elif BUILDFLAG(ENABLE_HW_SECURE_CODEC)
// Require hardware secure codecs when SW_SECURE_DECODE or above is specified.
if (robustness >= EmeRobustness::SW_SECURE_DECODE) {
return EmeConfigRule::HW_SECURE_CODECS_REQUIRED;
Index: components/cdm/renderer/widevine_key_system_properties.h
diff --git a/components/cdm/renderer/widevine_key_system_properties.h b/components/cdm/renderer/widevine_key_system_properties.h
index c936c62730d09fcb1b28900eb1434ca887534bc5..d096b8645e0c43e7249cbc0080b418fdb8c531f5 100644
--- a/components/cdm/renderer/widevine_key_system_properties.h
+++ b/components/cdm/renderer/widevine_key_system_properties.h
@@ -7,6 +7,7 @@

#include "build/build_config.h"
#include "media/base/key_system_properties.h"
+#include "media/media_features.h"

namespace cdm {

@@ -15,9 +16,9 @@ class WidevineKeySystemProperties : public media::KeySystemProperties {
public:
WidevineKeySystemProperties(
media::SupportedCodecs supported_codecs,
-#if defined(OS_ANDROID)
+#if BUILDFLAG(ENABLE_HW_SECURE_CODEC)
media::SupportedCodecs supported_secure_codecs,
-#endif // defined(OS_ANDROID)
+#endif // BUILDFLAG(ENABLE_HW_SECURE_CODEC)
media::EmeRobustness max_audio_robustness,
media::EmeRobustness max_video_robustness,
media::EmeSessionTypeSupport persistent_license_support,
@@ -30,9 +31,9 @@ class WidevineKeySystemProperties : public media::KeySystemProperties {
media::EmeInitDataType init_data_type) const override;

media::SupportedCodecs GetSupportedCodecs() const override;
-#if defined(OS_ANDROID)
+#if BUILDFLAG(ENABLE_HW_SECURE_CODEC)
media::SupportedCodecs GetSupportedSecureCodecs() const override;
-#endif
+#endif // BUILDFLAG(ENABLE_HW_SECURE_CODEC)

media::EmeConfigRule GetRobustnessConfigRule(
media::EmeMediaType media_type,
@@ -50,9 +51,9 @@ class WidevineKeySystemProperties : public media::KeySystemProperties {

private:
const media::SupportedCodecs supported_codecs_;
-#if defined(OS_ANDROID)
+#if BUILDFLAG(ENABLE_HW_SECURE_CODEC)
const media::SupportedCodecs supported_secure_codecs_;
-#endif // defined(OS_ANDROID)
+#endif // BUILDFLAG(ENABLE_HW_SECURE_CODEC)
const media::EmeRobustness max_audio_robustness_;
const media::EmeRobustness max_video_robustness_;
const media::EmeSessionTypeSupport persistent_license_support_;
Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index ed2b611bed047fcbbfaef807bb92a21c478f094b..724e11e2be3b92e8f830f2d3409d8f6af662c4ff 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -143,6 +143,7 @@
#include "media/blink/url_index.h"
#include "media/blink/webencryptedmediaclient_impl.h"
#include "media/blink/webmediaplayer_impl.h"
+#include "media/media_features.h"
#include "media/renderers/gpu_video_accelerator_factories.h"
#include "mojo/common/url_type_converters.h"
#include "mojo/edk/js/core.h"
@@ -6033,7 +6034,7 @@ shell::mojom::InterfaceProvider* RenderFrameImpl::GetMediaInterfaceProvider() {
#endif // defined(ENABLE_MOJO_MEDIA)

bool RenderFrameImpl::AreSecureCodecsSupported() {
-#if defined(OS_ANDROID)
+#if BUILDFLAG(ENABLE_HW_SECURE_CODEC)
// Hardware-secure codecs are only supported if secure surfaces are enabled.
return render_view_->renderer_preferences_
.use_video_overlay_for_embedded_encrypted_video;
Index: media/BUILD.gn
diff --git a/media/BUILD.gn b/media/BUILD.gn
index ccbb090fc95a32de28cf0a807fd854e73bc3cb41..4df830af4cde99594096612849be6334a8faf0af 100644
--- a/media/BUILD.gn
+++ b/media/BUILD.gn
@@ -20,6 +20,7 @@ buildflag_header("media_features") {
"ENABLE_HEVC_DEMUXING=$enable_hevc_demuxing",
"ENABLE_MSE_MPEG2TS_STREAM_PARSER=$enable_mse_mpeg2ts_stream_parser",
"ENABLE_MP4_VP9_DEMUXING=0",
+ "ENABLE_HW_SECURE_CODEC=$enable_hw_secure_codec",
]
}

Index: media/base/key_systems.cc
diff --git a/media/base/key_systems.cc b/media/base/key_systems.cc
index fab743252d9f227af048d2fae5b6f52c493b4548..e5b4376f2b2b5ea589e76cc3c399e04eafd7c1e1 100644
--- a/media/base/key_systems.cc
+++ b/media/base/key_systems.cc
@@ -588,10 +588,10 @@ EmeConfigRule KeySystemsImpl::GetContentTypeConfigRule(

SupportedCodecs key_system_codec_mask =
key_system_iter->second->GetSupportedCodecs();
-#if defined(OS_ANDROID)
+#if BUILDFLAG(ENABLE_HW_SECURE_CODEC)
SupportedCodecs key_system_secure_codec_mask =
key_system_iter->second->GetSupportedSecureCodecs();
-#endif // defined(OS_ANDROID)
+#endif // BUILDFLAG(ENABLE_HW_SECURE_CODEC)

// Check that the container is supported by the key system. (This check is
// necessary because |codecs| may be empty.)
@@ -606,7 +606,7 @@ EmeConfigRule KeySystemsImpl::GetContentTypeConfigRule(
SupportedCodecs codec = GetCodecForString(codecs[i]);
if ((codec & key_system_codec_mask & mime_type_codec_mask) == 0)
return EmeConfigRule::NOT_SUPPORTED;
-#if defined(OS_ANDROID)
+#if BUILDFLAG(ENABLE_HW_SECURE_CODEC)
// Check whether the codec supports a hardware-secure mode. The goal is to
// prevent mixing of non-hardware-secure codecs with hardware-secure codecs,
// since the mode is fixed at CDM creation.
@@ -617,7 +617,7 @@ EmeConfigRule KeySystemsImpl::GetContentTypeConfigRule(
// that hardware-secure-only codecs actually exist and are useful.
if ((codec & key_system_secure_codec_mask) == 0)
support = EmeConfigRule::HW_SECURE_CODECS_NOT_ALLOWED;
-#endif // defined(OS_ANDROID)
+#endif // BUILDFLAG(ENABLE_HW_SECURE_CODEC)
}

return support;
Index: media/media_options.gni
diff --git a/media/media_options.gni b/media/media_options.gni
index 21d31d0ee947dbf6b8b5d7da1019b1fd8acb5be8..df20ef8da4823abf0a5c44b1e7389c9861ee5057 100644
--- a/media/media_options.gni
+++ b/media/media_options.gni
@@ -42,6 +42,9 @@ declare_args() {
# Enable HEVC/H265 demuxing. Actual decoding must be provided by the
# platform. Enable by default for Chromecast.
enable_hevc_demuxing = proprietary_codecs && is_chromecast
+
+ # Allow supporting of hardware secured codecs.
+ enable_hw_secure_codec = is_android || is_chromecast
}

# Use a second declare_args() to pick up possible overrides of |use_cras|.


esp...@chromium.org

unread,
May 23, 2016, 7:34:40 PM5/23/16
to yuc...@chromium.org, joc...@chromium.org, hall...@chromium.org, jrum...@chromium.org, xhw...@chromium.org, blundell+...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, droger+w...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, halliwe...@chromium.org, j...@chromium.org, lcwu+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, sdefresne...@chromium.org
I think jochen@ might be a better reviewer for this?

https://codereview.chromium.org/2006113002/

jrum...@chromium.org

unread,
May 24, 2016, 2:27:50 PM5/24/16
to yuc...@chromium.org, joc...@chromium.org, hall...@chromium.org, xhw...@chromium.org, esp...@chromium.org, blundell+...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, droger+w...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, halliwe...@chromium.org, j...@chromium.org, lcwu+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, sdefresne...@chromium.org
media/ LGTM


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

https://codereview.chromium.org/2006113002/diff/1/components/cdm/renderer/widevine_key_system_properties.cc#newcode138
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_frame_impl.cc
File content/renderer/render_frame_impl.cc (right):

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

https://codereview.chromium.org/2006113002/

yuc...@chromium.org

unread,
May 24, 2016, 5:22:36 PM5/24/16
to joc...@chromium.org, hall...@chromium.org, jrum...@chromium.org, xhw...@chromium.org, esp...@chromium.org, blundell+...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, droger+w...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, halliwe...@chromium.org, j...@chromium.org, lcwu+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, sdefresne...@chromium.org

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

https://codereview.chromium.org/2006113002/diff/1/components/cdm/renderer/widevine_key_system_properties.cc#newcode138
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.
On 2016/05/24 18:27:50, jrummell wrote:
> nit: should be BUILDFLAG(ENABLE_HW_SECURE_CODEC)

Done.

https://codereview.chromium.org/2006113002/

ddo...@chromium.org

unread,
May 24, 2016, 6:05:42 PM5/24/16
to yuc...@chromium.org, joc...@chromium.org, hall...@chromium.org, jrum...@chromium.org, xhw...@chromium.org, esp...@chromium.org, blundell+...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, droger+w...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, halliwe...@chromium.org, j...@chromium.org, lcwu+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, sdefresne...@chromium.org

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

https://codereview.chromium.org/2006113002/diff/20001/chromecast/browser/cast_content_window.cc#newcode121
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/renderer_preferences.h
File content/public/common/renderer_preferences.h (right):

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

https://codereview.chromium.org/2006113002/

yuc...@chromium.org

unread,
May 24, 2016, 6:26:33 PM5/24/16
to joc...@chromium.org, hall...@chromium.org, jrum...@chromium.org, xhw...@chromium.org, esp...@chromium.org, ddorwin+...@chromium.org, blundell+...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, droger+w...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, halliwe...@chromium.org, j...@chromium.org, lcwu+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, sdefresne...@chromium.org

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

https://codereview.chromium.org/2006113002/diff/20001/chromecast/browser/cast_content_window.cc#newcode121
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?

https://codereview.chromium.org/2006113002/

hall...@chromium.org

unread,
May 24, 2016, 6:51:02 PM5/24/16
to yuc...@chromium.org, joc...@chromium.org, jrum...@chromium.org, xhw...@chromium.org, esp...@chromium.org, ddorwin+...@chromium.org, blundell+...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, droger+w...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, halliwe...@chromium.org, j...@chromium.org, lcwu+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, sdefresne...@chromium.org

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

https://codereview.chromium.org/2006113002/diff/20001/chromecast/browser/cast_content_window.cc#newcode121
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?

https://codereview.chromium.org/2006113002/

ddo...@chromium.org

unread,
May 24, 2016, 7:11:50 PM5/24/16
to yuc...@chromium.org, joc...@chromium.org, hall...@chromium.org, jrum...@chromium.org, xhw...@chromium.org, esp...@chromium.org, blundell+...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, droger+w...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, halliwe...@chromium.org, j...@chromium.org, lcwu+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, sdefresne...@chromium.org

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

https://codereview.chromium.org/2006113002/diff/20001/chromecast/browser/cast_content_window.cc#newcode121
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_systems_cast.cc
File chromecast/renderer/key_systems_cast.cc (right):

https://codereview.chromium.org/2006113002/diff/20001/chromecast/renderer/key_systems_cast.cc#newcode90
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/widevine_key_system_properties.cc
File components/cdm/renderer/widevine_key_system_properties.cc (right):

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer/widevine_key_system_properties.cc#newcode140
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/widevine_key_system_properties.cc#newcode141
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/widevine_key_system_properties.h
File components/cdm/renderer/widevine_key_system_properties.h (right):

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer/widevine_key_system_properties.h#newcode20
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.

https://codereview.chromium.org/2006113002/

yuc...@chromium.org

unread,
May 24, 2016, 8:43:21 PM5/24/16
to joc...@chromium.org, hall...@chromium.org, jrum...@chromium.org, xhw...@chromium.org, esp...@chromium.org, ddorwin+...@chromium.org, blundell+...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, droger+w...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, halliwe...@chromium.org, j...@chromium.org, lcwu+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, sdefresne...@chromium.org

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

https://codereview.chromium.org/2006113002/diff/20001/chromecast/renderer/key_systems_cast.cc#newcode90
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/widevine_key_system_properties.cc
File components/cdm/renderer/widevine_key_system_properties.cc (right):

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer/widevine_key_system_properties.cc#newcode140
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/widevine_key_system_properties.cc#newcode141
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/widevine_key_system_properties.h
File components/cdm/renderer/widevine_key_system_properties.h (right):

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer/widevine_key_system_properties.h#newcode20
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/renderer_preferences.h
File content/public/common/renderer_preferences.h (right):

https://codereview.chromium.org/2006113002/diff/20001/content/public/common/renderer_preferences.h#newcode131
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?

https://codereview.chromium.org/2006113002/

yuc...@chromium.org

unread,
May 26, 2016, 10:18:07 PM5/26/16
to joc...@chromium.org, hall...@chromium.org, jrum...@chromium.org, xhw...@chromium.org, esp...@chromium.org, ddorwin+...@chromium.org, blundell+...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, droger+w...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, halliwe...@chromium.org, j...@chromium.org, lcwu+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, sdefresne...@chromium.org
1. Add new flag hw_secure_codec_allowed.
2. Handle robustness config rule in chromecast's own widevine key system
properties.

https://codereview.chromium.org/2006113002/

ddo...@chromium.org

unread,
May 27, 2016, 5:33:01 PM5/27/16
to yuc...@chromium.org, joc...@chromium.org, hall...@chromium.org, jrum...@chromium.org, xhw...@chromium.org, esp...@chromium.org, blundell+...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, droger+w...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, halliwe...@chromium.org, j...@chromium.org, lcwu+...@chromium.org, mkwst+moarrev...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, sdefresne...@chromium.org
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/widevine_key_system_properties.h
File components/cdm/renderer/widevine_key_system_properties.h (right):

https://codereview.chromium.org/2006113002/diff/20001/components/cdm/renderer/widevine_key_system_properties.h#newcode20
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_systems_cast.cc
File chromecast/renderer/key_systems_cast.cc (right):

https://codereview.chromium.org/2006113002/diff/40001/chromecast/renderer/key_systems_cast.cc#newcode92
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_systems_cast.cc#newcode108
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_systems_cast.cc#newcode133
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/renderer_preferences.h
File content/public/common/renderer_preferences.h (right):

https://codereview.chromium.org/2006113002/diff/40001/content/public/common/renderer_preferences.h#newcode140
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.

https://codereview.chromium.org/2006113002/
Reply all
Reply to author
Forward
0 new messages