Adds initial key systems support for Chromecast.

Adds initial key systems support for Chromecast.

R=lcwu@chromium.org,byungchul@chromium.org
BUG=400914
Committed: https://crrev.com/b50a4e3f22e64129176a9dccd4d280b9aba2f700
Cr-Commit-Position: refs/heads/master@{#295226}

[gunsch, 2014-09-13 02:13:31]

gunsch@chromium.org changed reviewers:
+ damienv@chromium.org

[gunsch, 2014-09-13 02:14:15]

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

[gunsch, 2014-09-13 02:15:01]

On 2014/09/13 02:12:35, gunsch wrote:

+ddorwin to review per crbug/393397 : can you comment on if you're okay with our
use of key systems here?

[ddorwin, 2014-09-13 03:27:07]

I don't think I have the full context, but here are some general comments.

https://codereview.chromium.org/568243002/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/chromecast.gyp#ne...
chromecast/chromecast.gyp:208:
'../third_party/widevine/cdm/widevine_cdm.gyp:widevine_cdm_version_h',
Is this not covered by the other .gyp?

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
File chromecast/media/base/key_systems_common.cc (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.cc:13: const char kClearKeySystem[] =
"org.w3.clearkey";
Ideally, these names would only be in one place.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.cc:14: const char
kUnsupportedClearKeySystem[] = "unsupported-org.w3.clearkey";
You don't need this. The intent of this is to cause a not-supported error when
mixing prefixed and unprefixed API and name.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.cc:30: if
(key_system_name.compare(kPlayreadyKeySystems[i]) == 0) {
Use arraysize() instead of NULL marker

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
File chromecast/media/base/key_systems_common.h (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.h:5: #ifndef
CHROMECAST_MEDIA_BASE_KEY_SYSTEMS_COMMON_H_
common? Is this also used in the browser process or something? Is this for IPC
stuff?

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.h:19: KEY_SYSTEM_PLAYREADY,
To avoid confusion, be explicit about CHROMECAST_PLAYREADY. Possibly throughout.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.h:23: CastKeySystem
GetKeySystemByName(const std::string& key_system_name);
What is the enum used for? I don't see it in this patch.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/media.gyp
File chromecast/media/media.gyp (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/media/media.gyp#n...
chromecast/media/media.gyp:12: '../../crypto/crypto.gyp:crypto',
Why?

https://codereview.chromium.org/568243002/diff/1/chromecast/media/media.gyp#n...
chromecast/media/media.gyp:13:
'../../third_party/widevine/cdm/widevine_cdm.gyp:widevine_cdm_version_h',
Do you have a modification to this .gyp?

https://codereview.chromium.org/568243002/diff/1/chromecast/shell/renderer/ke...
File chromecast/shell/renderer/key_systems_cast.cc (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/shell/renderer/ke...
chromecast/shell/renderer/key_systems_cast.cc:46: // Chromecast supports
unprefixed clearkey, which upstream rewrites to
I don't think this comment is correct. Are you saying the
webkitGenerateKeyRequest() expects "org.w3.clearkey" without the "webkit-"?

Hmm, that will be a problem. Maybe you should put a Chromecast-specific fix in
these methods:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

https://codereview.chromium.org/568243002/diff/1/chromecast/shell/renderer/ke...
chromecast/shell/renderer/key_systems_cast.cc:55:
AddKeySystemWithCodecs(kWidevineKeySystem, true, key_systems_info);
It may not be useful, but FYI:
https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren...

[lcwu1, 2014-09-15 18:09:50]

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
File chromecast/media/base/key_systems_common.cc (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.cc:11: namespace media {
s/media/chromecast

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
File chromecast/media/base/key_systems_common.h (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.h:10: namespace media {
s/media/chromecast

[gunsch, 2014-09-15 21:43:38]

David: thanks for the thoughtful feedback. PTAL.

https://codereview.chromium.org/568243002/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/chromecast.gyp#ne...
chromecast/chromecast.gyp:208:
'../third_party/widevine/cdm/widevine_cdm.gyp:widevine_cdm_version_h',
On 2014/09/13 03:27:06, ddorwin wrote:
> Is this not covered by the other .gyp?

Yes, it's "covered" by it, but I'm also declaring this one as an explicit
dependency, since key_systems_cast.cc [in this target] depends on
widevine_cdm_version.h.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
File chromecast/media/base/key_systems_common.cc (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.cc:11: namespace media {
On 2014/09/15 18:09:50, lcwu1 wrote:
> s/media/chromecast

Done.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.cc:13: const char kClearKeySystem[] =
"org.w3.clearkey";
On 2014/09/13 03:27:06, ddorwin wrote:
> Ideally, these names would only be in one place.

This is the only location they're in for chromecast/. Removed duplicate with
media::kClearKey, however.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.cc:14: const char
kUnsupportedClearKeySystem[] = "unsupported-org.w3.clearkey";
On 2014/09/13 03:27:06, ddorwin wrote:
> You don't need this. The intent of this is to cause a not-supported error when
> mixing prefixed and unprefixed API and name.

I think we currently expect those to be mixed (see other comment), partly
because we don't yet support the unprefixed EME API. 

Added a TODO.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.cc:30: if
(key_system_name.compare(kPlayreadyKeySystems[i]) == 0) {
On 2014/09/13 03:27:06, ddorwin wrote:
> Use arraysize() instead of NULL marker

This list is exposed to other files (notably key_systems_cast.cc). Can't use
arraysize macro when included from elsewhere.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
File chromecast/media/base/key_systems_common.h (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.h:5: #ifndef
CHROMECAST_MEDIA_BASE_KEY_SYSTEMS_COMMON_H_
On 2014/09/13 03:27:06, ddorwin wrote:
> common? Is this also used in the browser process or something? Is this for IPC
> stuff?

Yes. We communicate CDM information over IPC, then do browser-process-side
decryption. (For Chromecast, most of our media playback happens browser-side, in
a fairly-similar-to-Android architecture. xhwang is leading an effort to unify
this).

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.h:10: namespace media {
On 2014/09/15 18:09:50, lcwu1 wrote:
> s/media/chromecast

Done.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.h:19: KEY_SYSTEM_PLAYREADY,
On 2014/09/13 03:27:06, ddorwin wrote:
> To avoid confusion, be explicit about CHROMECAST_PLAYREADY. Possibly
throughout.

Would prefer to not. This enum is only for use within the chromecast/ directory,
which makes it redundant. Further, we currently respond to requests for various
PlayReady names (added a TODO for the EMEv2 transition) that are all considered
"KEY_SYSTEM_PLAYREADY".

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.h:23: CastKeySystem
GetKeySystemByName(const std::string& key_system_name);
On 2014/09/13 03:27:06, ddorwin wrote:
> What is the enum used for? I don't see it in this patch.

No, not yet in this patch. This enum is used for our CDM factory (not yet
upstreamed, dependent on xhwang's new CDM code) and for (at the platform level)
switching on how to provide key information to the hardware. It's largely so
that we're not comparing key system name strings everywhere.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/media.gyp
File chromecast/media/media.gyp (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/media/media.gyp#n...
chromecast/media/media.gyp:12: '../../crypto/crypto.gyp:crypto',
On 2014/09/13 03:27:07, ddorwin wrote:
> Why?

Eager copy from downstream, removed for this CL.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/media.gyp#n...
chromecast/media/media.gyp:13:
'../../third_party/widevine/cdm/widevine_cdm.gyp:widevine_cdm_version_h',
On 2014/09/13 03:27:07, ddorwin wrote:
> Do you have a modification to this .gyp?

No, why?

We're adding the dependency because it a) causes a copy and b) IIRC has a
direct_dependent_settings + include_dirs combination so that
"widevine_cdm_version.h" gets included correctly.

https://codereview.chromium.org/568243002/diff/1/chromecast/shell/renderer/ke...
File chromecast/shell/renderer/key_systems_cast.cc (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/shell/renderer/ke...
chromecast/shell/renderer/key_systems_cast.cc:46: // Chromecast supports
unprefixed clearkey, which upstream rewrites to
On 2014/09/13 03:27:07, ddorwin wrote:
> I don't think this comment is correct. Are you saying the
> webkitGenerateKeyRequest() expects "org.w3.clearkey" without the "webkit-"?
> 
> Hmm, that will be a problem. Maybe you should put a Chromecast-specific fix in
> these methods:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

Ah, yes: we're expecting to handle "org.w3.clearkey" used in conjunction with
"webkitGenerateKeyRequest". Rewrote this comment to clarify, and added a more
explicit TODO (also in key_systems_common.cc).

The methods you linked to are exactly what this is trying to work around.
However, we have been explicitly asked to not have any sort of "CHROMECAST"
C-define in the codebase, so we don't have any "Chromecast-specific fixes" in
the rest of Chromium.

https://codereview.chromium.org/568243002/diff/1/chromecast/shell/renderer/ke...
chromecast/shell/renderer/key_systems_cast.cc:55:
AddKeySystemWithCodecs(kWidevineKeySystem, true, key_systems_info);
On 2014/09/13 03:27:07, ddorwin wrote:
> It may not be useful, but FYI:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren...

Thanks! That is useful. Updated.

[ddorwin, 2014-09-15 22:50:54]

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
File chromecast/media/base/key_systems_common.cc (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.cc:30: if
(key_system_name.compare(kPlayreadyKeySystems[i]) == 0) {
On 2014/09/15 21:43:38, gunsch wrote:
> On 2014/09/13 03:27:06, ddorwin wrote:
> > Use arraysize() instead of NULL marker
> 
> This list is exposed to other files (notably key_systems_cast.cc). Can't use
> arraysize macro when included from elsewhere.

You should be able to expose a size constant that is declared in the header and
defined in the .cc file just like the array.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
File chromecast/media/base/key_systems_common.h (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.h:23: CastKeySystem
GetKeySystemByName(const std::string& key_system_name);
On 2014/09/15 21:43:38, gunsch wrote:
> On 2014/09/13 03:27:06, ddorwin wrote:
> > What is the enum used for? I don't see it in this patch.
> 
> No, not yet in this patch. This enum is used for our CDM factory (not yet
> upstreamed, dependent on xhwang's new CDM code) and for (at the platform
level)
> switching on how to provide key information to the hardware. It's largely so
> that we're not comparing key system name strings everywhere.

FYI, such an enum could be generally useful, especially as part of the
unification work. I believe we currently pass strings for Android, which is not
ideal. The AW implementation would need to be made to handle this, though.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/media.gyp
File chromecast/media/media.gyp (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/media/media.gyp#n...
chromecast/media/media.gyp:13:
'../../third_party/widevine/cdm/widevine_cdm.gyp:widevine_cdm_version_h',
On 2014/09/15 21:43:38, gunsch wrote:
> On 2014/09/13 03:27:07, ddorwin wrote:
> > Do you have a modification to this .gyp?
> 
> No, why?
> 
> We're adding the dependency because it a) causes a copy and b) IIRC has a
> direct_dependent_settings + include_dirs combination so that
> "widevine_cdm_version.h" gets included correctly.

This line copies the header file for the platform to the intermediate output
directory to be used by all the C++ code that needs it. Without modifying that
.gyp file, I think it would fail when building Chromecast. Maybe this change is
not yet upstreamed?

https://codereview.chromium.org/568243002/diff/20001/chromecast/media/base/ke...
File chromecast/media/base/key_systems_common.cc (right):

https://codereview.chromium.org/568243002/diff/20001/chromecast/media/base/ke...
chromecast/media/base/key_systems_common.cc:15: // TODO(gunsch): remove this
support when moving to EMEv2.
The correct term is "unprefixed EME". It's not really "v2".

https://codereview.chromium.org/568243002/diff/20001/chromecast/media/base/ke...
chromecast/media/base/key_systems_common.cc:16: // See also: crbug/393397
I don't think this bug addresses the clear key naming issue.

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

https://codereview.chromium.org/568243002/diff/20001/chromecast/shell/rendere...
chromecast/shell/renderer/key_systems_cast.cc:37: // TODO(gunsch): remove this
after moving to EMEv2.
ditto on v2

https://codereview.chromium.org/568243002/diff/20001/chromecast/shell/rendere...
chromecast/shell/renderer/key_systems_cast.cc:38: // See: crbug/393397
I don't think this bug addresses the clear key naming issue.

[gunsch, 2014-09-16 16:58:00]

Pulled out all of (AFAICT) the objectionable parts and delegated to
platform-specific internal implementations. I think this should address the
concerns raised. PTAL!

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
File chromecast/media/base/key_systems_common.cc (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.cc:30: if
(key_system_name.compare(kPlayreadyKeySystems[i]) == 0) {
On 2014/09/15 22:50:53, ddorwin wrote:
> On 2014/09/15 21:43:38, gunsch wrote:
> > On 2014/09/13 03:27:06, ddorwin wrote:
> > > Use arraysize() instead of NULL marker
> > 
> > This list is exposed to other files (notably key_systems_cast.cc). Can't use
> > arraysize macro when included from elsewhere.
> 
> You should be able to expose a size constant that is declared in the header
and
> defined in the .cc file just like the array.

Done.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
File chromecast/media/base/key_systems_common.h (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/media/base/key_sy...
chromecast/media/base/key_systems_common.h:23: CastKeySystem
GetKeySystemByName(const std::string& key_system_name);
On 2014/09/15 22:50:53, ddorwin wrote:
> On 2014/09/15 21:43:38, gunsch wrote:
> > On 2014/09/13 03:27:06, ddorwin wrote:
> > > What is the enum used for? I don't see it in this patch.
> > 
> > No, not yet in this patch. This enum is used for our CDM factory (not yet
> > upstreamed, dependent on xhwang's new CDM code) and for (at the platform
> level)
> > switching on how to provide key information to the hardware. It's largely so
> > that we're not comparing key system name strings everywhere.
> 
> FYI, such an enum could be generally useful, especially as part of the
> unification work. I believe we currently pass strings for Android, which is
not
> ideal. The AW implementation would need to be made to handle this, though.

Acknowledged.

https://codereview.chromium.org/568243002/diff/1/chromecast/media/media.gyp
File chromecast/media/media.gyp (right):

https://codereview.chromium.org/568243002/diff/1/chromecast/media/media.gyp#n...
chromecast/media/media.gyp:13:
'../../third_party/widevine/cdm/widevine_cdm.gyp:widevine_cdm_version_h',
On 2014/09/15 22:50:53, ddorwin wrote:
> On 2014/09/15 21:43:38, gunsch wrote:
> > On 2014/09/13 03:27:07, ddorwin wrote:
> > > Do you have a modification to this .gyp?
> > 
> > No, why?
> > 
> > We're adding the dependency because it a) causes a copy and b) IIRC has a
> > direct_dependent_settings + include_dirs combination so that
> > "widevine_cdm_version.h" gets included correctly.
> 
> This line copies the header file for the platform to the intermediate output
> directory to be used by all the C++ code that needs it. Without modifying that
> .gyp file, I think it would fail when building Chromecast. Maybe this change
is
> not yet upstreamed?

It does not fail. It uses a default Linux implementation where Widevine is
disabled.

However, widevine_cdm.gyp exposes a "widevine_cdm_version_h_file" variable,
which we override conditionally based on if internal CDM code is available.

https://codereview.chromium.org/568243002/diff/20001/chromecast/media/base/ke...
File chromecast/media/base/key_systems_common.cc (right):

https://codereview.chromium.org/568243002/diff/20001/chromecast/media/base/ke...
chromecast/media/base/key_systems_common.cc:15: // TODO(gunsch): remove this
support when moving to EMEv2.
On 2014/09/15 22:50:53, ddorwin wrote:
> The correct term is "unprefixed EME". It's not really "v2".

Done.

https://codereview.chromium.org/568243002/diff/20001/chromecast/media/base/ke...
chromecast/media/base/key_systems_common.cc:16: // See also: crbug/393397
On 2014/09/15 22:50:53, ddorwin wrote:
> I don't think this bug addresses the clear key naming issue.

Done.

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

https://codereview.chromium.org/568243002/diff/20001/chromecast/shell/rendere...
chromecast/shell/renderer/key_systems_cast.cc:37: // TODO(gunsch): remove this
after moving to EMEv2.
On 2014/09/15 22:50:53, ddorwin wrote:
> ditto on v2

Done.

https://codereview.chromium.org/568243002/diff/20001/chromecast/shell/rendere...
chromecast/shell/renderer/key_systems_cast.cc:38: // See: crbug/393397
On 2014/09/15 22:50:53, ddorwin wrote:
> I don't think this bug addresses the clear key naming issue.

Done.

[ddorwin, 2014-09-16 18:15:00]

LGTM % comments. Thanks!

https://codereview.chromium.org/568243002/diff/40001/chromecast/media/base/ke...
File chromecast/media/base/key_systems_common.h (right):

https://codereview.chromium.org/568243002/diff/40001/chromecast/media/base/ke...
chromecast/media/base/key_systems_common.h:23: // platform for known key systems
if needed.
I think the text after the comma is incorrect based on the implementation and
function below.

https://codereview.chromium.org/568243002/diff/40001/chromecast/media/base/ke...
chromecast/media/base/key_systems_common.h:26: // Translates a platform-specific
key system string into a CastKeySystem.
TODO: Remove when prefixed EME is removed?

https://codereview.chromium.org/568243002/diff/40001/chromecast/shell/rendere...
File chromecast/shell/renderer/key_systems_cast.h (right):

https://codereview.chromium.org/568243002/diff/40001/chromecast/shell/rendere...
chromecast/shell/renderer/key_systems_cast.h:18: void
AddChromecastPlatformKeySystems(
TODO: Remove when prefixed EME is removed?

[gunsch, 2014-09-16 18:55:34]

https://codereview.chromium.org/568243002/diff/40001/chromecast/media/base/ke...
File chromecast/media/base/key_systems_common.h (right):

https://codereview.chromium.org/568243002/diff/40001/chromecast/media/base/ke...
chromecast/media/base/key_systems_common.h:23: // platform for known key systems
if needed.
On 2014/09/16 18:15:00, ddorwin wrote:
> I think the text after the comma is incorrect based on the implementation and
> function below.

Comment was correct, fixed the implementation :)

https://codereview.chromium.org/568243002/diff/40001/chromecast/media/base/ke...
chromecast/media/base/key_systems_common.h:26: // Translates a platform-specific
key system string into a CastKeySystem.
On 2014/09/16 18:15:00, ddorwin wrote:
> TODO: Remove when prefixed EME is removed?

Done.

https://codereview.chromium.org/568243002/diff/40001/chromecast/shell/rendere...
File chromecast/shell/renderer/key_systems_cast.h (right):

https://codereview.chromium.org/568243002/diff/40001/chromecast/shell/rendere...
chromecast/shell/renderer/key_systems_cast.h:18: void
AddChromecastPlatformKeySystems(
On 2014/09/16 18:15:00, ddorwin wrote:
> TODO: Remove when prefixed EME is removed?

Done.

[lcwu1, 2014-09-17 01:44:22]

lgtm

[gunsch, 2014-09-17 02:59:23]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-09-17 03:00:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/568243002/60001

[commit-bot: I haz the power, 2014-09-17 04:05:35]

Committed patchset #4 (id:60001) as c0910999fe6e88f502e2494ddbecf1fa9d8a9c37

[commit-bot: I haz the power, 2014-09-17 04:06:26]

Patchset 4 (id:??) landed as
https://crrev.com/b50a4e3f22e64129176a9dccd4d280b9aba2f700
Cr-Commit-Position: refs/heads/master@{#295226}