Register WideVine keysystem for WebView.

Register WideVine keysystem for WebView.

BUG=322395
Change-Id: I7b4a52824c0b82b3cf9d1e6b6d133bb741d0d96a

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

[joth, 2013-11-22 06:59:49]

-> qinmin will you be main reviewer for this area?

(+benm fyi)

https://codereview.chromium.org/82803002/diff/1/android_webview/android_webvi...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/82803002/diff/1/android_webview/android_webvi...
android_webview/android_webview.gyp:112: '<(SHARED_INTERMEDIATE_DIR)',
surprised this is needed. what needs it?

https://codereview.chromium.org/82803002/diff/1/android_webview/renderer/aw_k...
File android_webview/renderer/aw_key_systems.cc (right):

https://codereview.chromium.org/82803002/diff/1/android_webview/renderer/aw_k...
android_webview/renderer/aw_key_systems.cc:28: static const uint8
kWidevineUuid[16] = {
there's 3 copies of this uuid already in the code, one with a TODO to move it
somewhere common. Seems time we did that :)

https://codereview.chromium.org/82803002/diff/1/android_webview/renderer/aw_k...
android_webview/renderer/aw_key_systems.cc:33: static std::string
GetDirectParentName(std::string name) {
pass as const&

(although I think you could get away with const char* here)

[joth, 2013-11-22 06:59:49]

-> qinmin will you be main reviewer for this area?

(+benm fyi)

https://codereview.chromium.org/82803002/diff/1/android_webview/android_webvi...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/82803002/diff/1/android_webview/android_webvi...
android_webview/android_webview.gyp:112: '<(SHARED_INTERMEDIATE_DIR)',
surprised this is needed. what needs it?

https://codereview.chromium.org/82803002/diff/1/android_webview/renderer/aw_k...
File android_webview/renderer/aw_key_systems.cc (right):

https://codereview.chromium.org/82803002/diff/1/android_webview/renderer/aw_k...
android_webview/renderer/aw_key_systems.cc:28: static const uint8
kWidevineUuid[16] = {
there's 3 copies of this uuid already in the code, one with a TODO to move it
somewhere common. Seems time we did that :)

https://codereview.chromium.org/82803002/diff/1/android_webview/renderer/aw_k...
android_webview/renderer/aw_key_systems.cc:33: static std::string
GetDirectParentName(std::string name) {
pass as const&

(although I think you could get away with const char* here)

[ycheo (away), 2013-11-22 11:49:35]

https://codereview.chromium.org/82803002/diff/1/android_webview/android_webvi...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/82803002/diff/1/android_webview/android_webvi...
android_webview/android_webview.gyp:112: '<(SHARED_INTERMEDIATE_DIR)',
On 2013/11/22 06:59:49, joth wrote:
> surprised this is needed. what needs it?
According to widevine_cdm.gyp, widevine_cdm_version.h is supposed to generated
here:
https://code.google.com/p/chromium/codesearch#search/&q=widevine_cdm_version....

https://codereview.chromium.org/82803002/diff/1/android_webview/renderer/aw_k...
File android_webview/renderer/aw_key_systems.cc (right):

https://codereview.chromium.org/82803002/diff/1/android_webview/renderer/aw_k...
android_webview/renderer/aw_key_systems.cc:28: static const uint8
kWidevineUuid[16] = {
On 2013/11/22 06:59:49, joth wrote:
> there's 3 copies of this uuid already in the code, one with a TODO to move it
> somewhere common. Seems time we did that :)

I'll handle this in another CL.
Filed the bug: http://crbug.com/322464

https://codereview.chromium.org/82803002/diff/1/android_webview/renderer/aw_k...
android_webview/renderer/aw_key_systems.cc:33: static std::string
GetDirectParentName(std::string name) {
On 2013/11/22 06:59:49, joth wrote:
> pass as const&
> 
> (although I think you could get away with const char* here)

Done.

[qinmin, 2013-11-22 18:36:34]

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
File android_webview/renderer/aw_key_systems.cc (right):

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
android_webview/renderer/aw_key_systems.cc:44:
info.supported_types.push_back(std::make_pair(kAudioWebM, kVorbis));
chrome on android does not support webM for widevine
Check GetSupportedCodecs() in encrypted_media_message_filter_android.cc or
third_party/widevine/cdm/android/WIDEVINE_CDM_VERSION_H_

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
File android_webview/renderer/aw_key_systems.h (right):

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
android_webview/renderer/aw_key_systems.h:9: 
namespace android_webview ?

[ddorwin, 2013-11-22 21:21:08]

Is there a use case where Chrome for Android and AW should have different
behaviors for key systems? It seems that both would have the same requirements
and checks for platform capabilities.

Is anything other than the location of the existing Android code stopping AW
from using it? Is there a place common for all content clients that we could
move that logic?

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
File android_webview/renderer/aw_key_systems.cc (right):

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
android_webview/renderer/aw_key_systems.cc:18: const char kAudioWebM[] =
"audio/webm";
We now have these types (and UUID) and maybe some of the logic in at least 3
different places. I wonder if we should have key_system_info_types/helpers.h or
something.

This CL is fine, but it's something for others to consider.

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
android_webview/renderer/aw_key_systems.cc:58: std::string standard_widevine =
GetDirectParentName(kWidevineKeySystem);
"standard" is confusing. It's really just widevine_parent.

[ycheo (away), 2013-11-23 01:27:02]

On 2013/11/22 21:21:08, ddorwin wrote:
> Is there a use case where Chrome for Android and AW should have different
> behaviors for key systems? It seems that both would have the same requirements
> and checks for platform capabilities.
What I consider as the next step in AW is to enumerate all DRM installed in the
Android framework and register their keysystems.
1. I wonder if this feature is necessary in Chrome for Android, because Chrome
for Android is for more general use case (i.e. for the universal DRM, not the
vendor specific DRM).
2. In implementing the feature, I'm considering the way to register the
keysystem from the Android platform to AW, not to enumerate all DRMs in AW
itself, that's because, IMO, the mapping-table between the keysystem name and
its UUID should exist in the Android framework, not in Chrome code base.
> 
> Is anything other than the location of the existing Android code stopping AW
> from using it? Is there a place common for all content clients that we could
> move that logic?
> 
>
https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
> File android_webview/renderer/aw_key_systems.cc (right):
> 
>
https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
> android_webview/renderer/aw_key_systems.cc:18: const char kAudioWebM[] =
> "audio/webm";
> We now have these types (and UUID) and maybe some of the logic in at least 3
> different places. I wonder if we should have key_system_info_types/helpers.h
or
> something.
> 
> This CL is fine, but it's something for others to consider.
> 
>
https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
> android_webview/renderer/aw_key_systems.cc:58: std::string standard_widevine =
> GetDirectParentName(kWidevineKeySystem);
> "standard" is confusing. It's really just widevine_parent.

[ycheo (away), 2013-11-23 01:27:18]

https://codereview.chromium.org/82803002/diff/1/android_webview/android_webvi...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/82803002/diff/1/android_webview/android_webvi...
android_webview/android_webview.gyp:112: '<(SHARED_INTERMEDIATE_DIR)',
On 2013/11/22 11:49:35, Yuncheol Heo wrote:
> On 2013/11/22 06:59:49, joth wrote:
> > surprised this is needed. what needs it?
> According to widevine_cdm.gyp, widevine_cdm_version.h is supposed to generated
> here:
>
https://code.google.com/p/chromium/codesearch#search/&q=widevine_cdm_version....

Actually, what I need is widevine_cdm_common.h, not widevine_cdm_version.h.
So I changed the code to include widevine_cdm_common.h directly.

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
File android_webview/renderer/aw_key_systems.cc (right):

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
android_webview/renderer/aw_key_systems.cc:18: const char kAudioWebM[] =
"audio/webm";
On 2013/11/22 21:21:08, ddorwin wrote:
> We now have these types (and UUID) and maybe some of the logic in at least 3
> different places. I wonder if we should have key_system_info_types/helpers.h
or
> something.
> 
> This CL is fine, but it's something for others to consider.

Thanks for the suggestion.
I'll consider these also when I remove the duplication of kWidevineUuid.

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
android_webview/renderer/aw_key_systems.cc:44:
info.supported_types.push_back(std::make_pair(kAudioWebM, kVorbis));
On 2013/11/22 18:36:34, qinmin wrote:
> chrome on android does not support webM for widevine
> Check GetSupportedCodecs() in encrypted_media_message_filter_android.cc or
> third_party/widevine/cdm/android/WIDEVINE_CDM_VERSION_H_

Thanks for letting me know this.
BTW, the MediaCodec supports the vorbis and vp8 codec:
http://developer.android.com/reference/android/media/MediaCodec.html#createDe...
Do you know what're the issues to support WebM in Clank?

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
android_webview/renderer/aw_key_systems.cc:58: std::string standard_widevine =
GetDirectParentName(kWidevineKeySystem);
On 2013/11/22 21:21:08, ddorwin wrote:
> "standard" is confusing. It's really just widevine_parent.

Done.

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
File android_webview/renderer/aw_key_systems.h (right):

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
android_webview/renderer/aw_key_systems.h:9: 
On 2013/11/22 18:36:34, qinmin wrote:
> namespace android_webview ?

Done.

[qinmin, 2013-11-23 01:36:13]

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
File android_webview/renderer/aw_key_systems.cc (right):

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
android_webview/renderer/aw_key_systems.cc:44:
info.supported_types.push_back(std::make_pair(kAudioWebM, kVorbis));
although MediaCodec supports vorbis and vp8, widevine does not support them
since it only supports CENC. 
ddorwin probably knows this better than me.
 
On 2013/11/23 01:27:19, Yuncheol Heo wrote:
> On 2013/11/22 18:36:34, qinmin wrote:
> > chrome on android does not support webM for widevine
> > Check GetSupportedCodecs() in encrypted_media_message_filter_android.cc or
> > third_party/widevine/cdm/android/WIDEVINE_CDM_VERSION_H_
> 
> Thanks for letting me know this.
> BTW, the MediaCodec supports the vorbis and vp8 codec:
>
http://developer.android.com/reference/android/media/MediaCodec.html#createDe...)
> Do you know what're the issues to support WebM in Clank?

[joth, 2013-11-23 23:00:14]

changes vs. my comments on PS1 lg2m, but holding for the other reviewer's to
confirm

https://codereview.chromium.org/82803002/diff/130001/android_webview/renderer...
File android_webview/renderer/aw_key_systems.cc (right):

https://codereview.chromium.org/82803002/diff/130001/android_webview/renderer...
android_webview/renderer/aw_key_systems.cc:19: static const char kAudioMp4[] =
"audio/mp4";
nit: prefer anonymous namespace rather than static (for everything down to line
47)

[ddorwin, 2013-11-25 19:14:02]

LGTM. It'd be nice if AW and Android shared this code, but I understand AW may
be headed in a different direction. Please keep reuse in mind and maybe revisit
it later. As it stands, AW may fall behind when new capabilities are added for
Android.

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
File android_webview/renderer/aw_key_systems.cc (right):

https://codereview.chromium.org/82803002/diff/70001/android_webview/renderer/...
android_webview/renderer/aw_key_systems.cc:44:
info.supported_types.push_back(std::make_pair(kAudioWebM, kVorbis));
On 2013/11/23 01:36:14, qinmin wrote:
> although MediaCodec supports vorbis and vp8, widevine does not support them
> since it only supports CENC. 
> ddorwin probably knows this better than me.
>  
> On 2013/11/23 01:27:19, Yuncheol Heo wrote:
> > On 2013/11/22 18:36:34, qinmin wrote:
> > > chrome on android does not support webM for widevine
> > > Check GetSupportedCodecs() in encrypted_media_message_filter_android.cc or
> > > third_party/widevine/cdm/android/WIDEVINE_CDM_VERSION_H_
> > 
> > Thanks for letting me know this.
> > BTW, the MediaCodec supports the vorbis and vp8 codec:
> >
>
http://developer.android.com/reference/android/media/MediaCodec.html#createDe...)
> > Do you know what're the issues to support WebM in Clank?
> 

Yes, it's the container that is not currently supported by MediaDrm.

https://codereview.chromium.org/82803002/diff/270001/android_webview/renderer...
File android_webview/renderer/aw_key_systems.h (right):

https://codereview.chromium.org/82803002/diff/270001/android_webview/renderer...
android_webview/renderer/aw_key_systems.h:11: void
AwAddKeySystems(std::vector<content::KeySystemInfo>* key_systems_info);
nit: empty lines

[ycheo (away), 2013-11-26 00:48:49]

Thanks for the advice.
I'll keep reuse in mind.

https://codereview.chromium.org/82803002/diff/270001/android_webview/renderer...
File android_webview/renderer/aw_key_systems.h (right):

https://codereview.chromium.org/82803002/diff/270001/android_webview/renderer...
android_webview/renderer/aw_key_systems.h:11: void
AwAddKeySystems(std::vector<content::KeySystemInfo>* key_systems_info);
On 2013/11/25 19:14:03, ddorwin wrote:
> nit: empty lines

Done.

[qinmin, 2013-11-26 02:10:30]

lgtm

[ycheo (away), 2013-11-26 23:07:56]

@joth,
PTAL.

[joth, 2013-11-26 23:21:06]

lgtm

[commit-bot: I haz the power, 2013-11-26 23:25:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ycheo@chromium.org/82803002/310001

[commit-bot: I haz the power, 2013-11-27 00:29:05]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-11-27 12:23:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ycheo@chromium.org/82803002/330001

[ycheo (away), 2013-11-27 12:31:53]

To reviewers,

I removed ".hrnoncompositing" keysystem because the trybot (android_clang_dbg)
emits the link error on switches::kMediaDrmEnableNonCompositing in
media_switches.h.

Frankly, I can't understand why the link error happens and I can't reproduce it
in my local build.
So, I removed the logic as the stopgap solution.
I'll revisit this later when handling the other DRM.
Please let me know if you have any concerns on removing the keysystem.

Thanks,
Yuncheol

[commit-bot: I haz the power, 2013-11-27 14:35:06]

Change committed as 237572

[qinmin, 2013-11-27 18:28:39]

On 2013/11/27 12:31:53, Yuncheol Heo wrote:
> To reviewers,
> 
> I removed ".hrnoncompositing" keysystem because the trybot (android_clang_dbg)
> emits the link error on switches::kMediaDrmEnableNonCompositing in
> media_switches.h.
> 
> Frankly, I can't understand why the link error happens and I can't reproduce
it
> in my local build.
> So, I removed the logic as the stopgap solution.
> I'll revisit this later when handling the other DRM.
> Please let me know if you have any concerns on removing the keysystem.
> 
> Thanks,
> Yuncheol

what about changing the include rules to media/base/? does that help?