Encrypted Media: Confine UUID code to MediaDrmBridge.

Encrypted Media: Confine UUID code to MediaDrmBridge.

Currenly we have UUID in a lot of places in the EME implementation on Chromium
for Android. But UUID should really just be an implementation detail of
MediaDrmBridge. This CL moves all UUID related code to MediaDrmBridge and uses
key system elsewhere.

BUG=347596
R=boliu@chromium.org, dcheng@chromium.org, ddorwin@chromium.org, joi@chromium.org
TBR=dcheng@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255727

[xhwang, 2014-03-04 00:18:57]

PTAL

https://codereview.chromium.org/185993004/diff/20001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/185993004/diff/20001/media/media.gyp#newcode916
media/media.gyp:916:
'../third_party/widevine/cdm/widevine_cdm.gyp:widevine_cdm_version_h',
This is for kWidevineKeySystem, which is ugly... How about putting
kWidevineKeySystem in a common place in Chromium code base? e.g.
media/cdm/key_system_names.h

https://codereview.chromium.org/185993004/diff/20001/media/media.gyp#newcode1583
media/media.gyp:1583:
'../third_party/widevine/cdm/widevine_cdm.gyp:widevine_cdm_version_h',
ditto

[ddorwin, 2014-03-04 20:00:56]

This mostly LG. However, we need less key system-specific code in
media_drm_bridge.cc, not more. I'd also prefer not to have system-specific
values spread out in multiple locations if we can avoid it. Let's talk.

https://codereview.chromium.org/185993004/diff/20001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/185993004/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:45: const size_t
kEmeMaxKeySystemSize = 1024;
Too long. "Length" would probably be better.

https://codereview.chromium.org/185993004/diff/20001/content/renderer/media/a...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/185993004/diff/20001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.cc:40: #if
defined(ENABLE_PEPPER_CDMS)
Do we need any of these ifdefs? This is only Android, right?

https://codereview.chromium.org/185993004/diff/20001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/185993004/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.cc:67: if (key_system == kWidevineKeySystem)
{
We should probably state that for security reasons, we only do exact string
comparisons here - we don't try to parse the |key_system| in any way.

Preferably, we would do this on the browser side of the IPC. Maybe we should
check IsSupportedKeySystem() there. There's no reason to continue to here if
it's not.

https://codereview.chromium.org/185993004/diff/20001/media/base/android/media...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/185993004/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
There are no tests for this class. :(

Now that it is doing conversion to UUID, we should definitely have tests. (That
also means exposing GetUUID in this file, or via a -internal.h.)

https://codereview.chromium.org/185993004/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.h:47: static bool
IsCryptoSchemeSupported(const std::string& key_system,
This function name no longer makes sense. I assume "Crypto Scheme" comes from
Android and is identified by the UUID.

https://codereview.chromium.org/185993004/diff/20001/media/base/android/media...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/185993004/diff/20001/media/base/android/media...
media/base/android/media_source_player_unittest.cc:2081: if
(!MediaCodecBridge::IsAvailable() || !MediaDrmBridge::IsAvailable()) {
Do we need these checks for an invalid key system? Will the lower level calls
throw an exception or something? If so, is that something we should fix?

https://codereview.chromium.org/185993004/diff/20001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/185993004/diff/20001/media/media.gyp#newcode916
media/media.gyp:916:
'../third_party/widevine/cdm/widevine_cdm.gyp:widevine_cdm_version_h',
On 2014/03/04 00:18:58, xhwang wrote:
> This is for kWidevineKeySystem, which is ugly... How about putting
> kWidevineKeySystem in a common place in Chromium code base? e.g.
> media/cdm/key_system_names.h

Let's discuss. For one thing, including this file helps identify places where
key system-specific code exists.

https://codereview.chromium.org/185993004/diff/20001/media/media.gyp#newcode1052
media/media.gyp:1052: '<(SHARED_INTERMEDIATE_DIR)',
I believe this can be addressed by adding a dependent_settings item to
widevine_cdm.gyp:widevine_cdm_version_h. We just never got around to it. It
would clean up several other .gyp files too.

[xhwang, 2014-03-05 21:52:33]

PTAL again.

https://codereview.chromium.org/185993004/diff/20001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/185993004/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:45: const size_t
kEmeMaxKeySystemSize = 1024;
On 2014/03/04 20:00:56, ddorwin wrote:
> Too long. "Length" would probably be better.

Done.

https://codereview.chromium.org/185993004/diff/20001/content/renderer/media/a...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/185993004/diff/20001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.cc:40: #if
defined(ENABLE_PEPPER_CDMS)
On 2014/03/04 20:00:56, ddorwin wrote:
> Do we need any of these ifdefs? This is only Android, right?

Done.

https://codereview.chromium.org/185993004/diff/20001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/185993004/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.cc:67: if (key_system == kWidevineKeySystem)
{
On 2014/03/04 20:00:56, ddorwin wrote:
> We should probably state that for security reasons, we only do exact string
> comparisons here - we don't try to parse the |key_system| in any way.
> 
> Preferably, we would do this on the browser side of the IPC. Maybe we should
> check IsSupportedKeySystem() there. There's no reason to continue to here if
> it's not.

IsSupportedKeySystem() will actually call into this file in various ways. For
example, the handler of ChromeViewHostMsg_GetSupportedKeySystems will call
MediaDrmBridge::IsKeySystemSupported().

https://codereview.chromium.org/185993004/diff/20001/media/base/android/media...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/185993004/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2014/03/04 20:00:56, ddorwin wrote:
> There are no tests for this class. :(

I'll add tests (mainly for the static methods) in a follow up CL.

> Now that it is doing conversion to UUID, we should definitely have tests.
(That
> also means exposing GetUUID in this file, or via a -internal.h.)

UUID is an internal concept and GetUUID() just does the one-to-one mapping. I am
not sure whether it's worth exposing GetUUID() to test it.

https://codereview.chromium.org/185993004/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.h:47: static bool
IsCryptoSchemeSupported(const std::string& key_system,
On 2014/03/04 20:00:56, ddorwin wrote:
> This function name no longer makes sense. I assume "Crypto Scheme" comes from
> Android and is identified by the UUID.

Done.

https://codereview.chromium.org/185993004/diff/20001/media/base/android/media...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/185993004/diff/20001/media/base/android/media...
media/base/android/media_source_player_unittest.cc:2081: if
(!MediaCodecBridge::IsAvailable() || !MediaDrmBridge::IsAvailable()) {
On 2014/03/04 20:00:56, ddorwin wrote:
> Do we need these checks for an invalid key system? Will the lower level calls
> throw an exception or something? If so, is that something we should fix?

In the current implementation, we should always call IsAvailable() before we can
make any calls on MediaCodecBridge and MediaDrmBridge. I agree this is confusing
and unnecessary. This needs a lot of clean-up, which I'd like to do in a
separate CL.

https://codereview.chromium.org/185993004/diff/20001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/185993004/diff/20001/media/media.gyp#newcode1052
media/media.gyp:1052: '<(SHARED_INTERMEDIATE_DIR)',
On 2014/03/04 20:00:56, ddorwin wrote:
> I believe this can be addressed by adding a dependent_settings item to
> widevine_cdm.gyp:widevine_cdm_version_h. We just never got around to it. It
> would clean up several other .gyp files too.

Since we have a lot of places like this. I'll investigate this in a separate CL.

[ddorwin, 2014-03-05 22:11:29]

https://codereview.chromium.org/185993004/diff/40001/chrome/browser/media/enc...
File chrome/browser/media/encrypted_media_message_filter_android.cc (right):

https://codereview.chromium.org/185993004/diff/40001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_message_filter_android.cc:74:
SupportedKeySystemResponse* response) {
We should be validating SupportedKeySystemResponse here, including similar
checks as in the other filter .cc file. (I assume that code will eventually end
up here.)

https://codereview.chromium.org/185993004/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/185993004/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:586:
DCHECK(MediaDrmBridge::IsKeySystemSupported(key_system, ""));
If this is for security, we should probably return as well.
if (!MediaDrmBridge::IsKeySystemSupported(key_system, "")) {
  NOTREACHED() <<...
  return;
}

https://codereview.chromium.org/185993004/diff/40001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/185993004/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.cc:66: static std::vector<uint8>
GetUUID(const std::string& key_system) {
I still think it's worth commenting that we only do exact string comparisons
here for security reasons.

https://codereview.chromium.org/185993004/diff/40001/media/base/android/media...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/185993004/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.h:44: // TODO(xhwang): Add unit tests for
MediaDrmBridge.
Would we be better off just filing a bug?

https://codereview.chromium.org/185993004/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.h:48: static bool IsKeySystemSupported(const
std::string& key_system,
...WithType?

[xhwang, 2014-03-06 18:09:44]

comments addressed

[xhwang, 2014-03-06 18:09:57]

https://codereview.chromium.org/185993004/diff/40001/chrome/browser/media/enc...
File chrome/browser/media/encrypted_media_message_filter_android.cc (right):

https://codereview.chromium.org/185993004/diff/40001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_message_filter_android.cc:74:
SupportedKeySystemResponse* response) {
On 2014/03/05 22:11:29, ddorwin wrote:
> We should be validating SupportedKeySystemResponse here, including similar
> checks as in the other filter .cc file. (I assume that code will eventually
end
> up here.)

Done.

https://codereview.chromium.org/185993004/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/185993004/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:586:
DCHECK(MediaDrmBridge::IsKeySystemSupported(key_system, ""));
On 2014/03/05 22:11:29, ddorwin wrote:
> If this is for security, we should probably return as well.
> if (!MediaDrmBridge::IsKeySystemSupported(key_system, "")) {
>   NOTREACHED() <<...
>   return;
> }

Done.

https://codereview.chromium.org/185993004/diff/40001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/185993004/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.cc:66: static std::vector<uint8>
GetUUID(const std::string& key_system) {
On 2014/03/05 22:11:29, ddorwin wrote:
> I still think it's worth commenting that we only do exact string comparisons
> here for security reasons.

Done.

https://codereview.chromium.org/185993004/diff/40001/media/base/android/media...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/185993004/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.h:44: // TODO(xhwang): Add unit tests for
MediaDrmBridge.
On 2014/03/05 22:11:29, ddorwin wrote:
> Would we be better off just filing a bug?

Done.

https://codereview.chromium.org/185993004/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.h:48: static bool IsKeySystemSupported(const
std::string& key_system,
On 2014/03/05 22:11:29, ddorwin wrote:
> ...WithType?

Done.

[xhwang, 2014-03-06 18:10:18]

I'll fix webview code in the next patchset.

[ddorwin, 2014-03-06 18:28:47]

LGTM % nits. Thanks.

https://codereview.chromium.org/185993004/diff/60001/chrome/browser/media/enc...
File chrome/browser/media/encrypted_media_message_filter_android.cc (right):

https://codereview.chromium.org/185993004/diff/60001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_message_filter_android.cc:20: const size_t
kEmeMaxKeySystemLength = 256;
nit: "Max" should be first IMO.
nit: "Eme" is redundant in this file, and probably not necessary in the other
one either.

https://codereview.chromium.org/185993004/diff/60001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/185993004/diff/60001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:45: const size_t
kEmeMaxKeySystemLength = 256;
ditto

https://codereview.chromium.org/185993004/diff/60001/media/base/android/media...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/185993004/diff/60001/media/base/android/media...
media/base/android/media_drm_bridge.h:48: static bool
IsKeySystemSupportedWithType(
I now realize this name is a little misleading vs. some of the other functions
that actually take a full content type. We can resolve this once the spec issue
with initData type is resolved.

[xhwang, 2014-03-06 18:53:45]

comments addressed

[xhwang, 2014-03-06 18:53:48]

https://codereview.chromium.org/185993004/diff/60001/chrome/browser/media/enc...
File chrome/browser/media/encrypted_media_message_filter_android.cc (right):

https://codereview.chromium.org/185993004/diff/60001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_message_filter_android.cc:20: const size_t
kEmeMaxKeySystemLength = 256;
On 2014/03/06 18:28:48, ddorwin wrote:
> nit: "Max" should be first IMO.
> nit: "Eme" is redundant in this file, and probably not necessary in the other
> one either.

Done.

https://codereview.chromium.org/185993004/diff/60001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/185993004/diff/60001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:45: const size_t
kEmeMaxKeySystemLength = 256;
On 2014/03/06 18:28:48, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/185993004/diff/60001/media/base/android/media...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/185993004/diff/60001/media/base/android/media...
media/base/android/media_drm_bridge.h:48: static bool
IsKeySystemSupportedWithType(
On 2014/03/06 18:28:48, ddorwin wrote:
> I now realize this name is a little misleading vs. some of the other functions
> that actually take a full content type. We can resolve this once the spec
issue
> with initData type is resolved.

Added TODO.

[xhwang, 2014-03-06 18:59:26]

All changes are pretty trivial and should take <1min to review.

OWNERS review request:

joi@chromium.org: Please review changes in 
content/public/renderer/key_system_info.h

boliu@chromium.org: Please review changes in
android_webview/renderer/aw_key_systems.cc

dcheng@chromium.org: Please review changes in 
chrome/common/encrypted_media_messages_android.h and
content/common/media/cdm_messages.h

[boliu, 2014-03-07 06:05:06]

Sorry, not sure how I missed this today.

android_webivew lgtm

+ycheo fyi

[Jói, 2014-03-07 12:33:44]

//content/public LGTM

[xhwang, 2014-03-07 18:02:14]

TBRing dcheng...

[xhwang, 2014-03-07 18:02:22]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-03-07 18:03:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/185993004/100001

[commit-bot: I haz the power, 2014-03-07 20:20:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/185993004/100001

[dcheng, 2014-03-07 20:55:57]

chrome/common/encrypted_media_messages_android.h and
content/common/media/cdm_messages.h LGTM

[xhwang, 2014-03-08 01:07:20]

Committed patchset #6 manually as r255727 (presubmit successful).