Adds MediaClientAndroid to support embedder/MediaDrmBridge interaction.

Adds MediaClientAndroid to support embedder/MediaDrmBridge interaction.

This CL does a few things:
* Adds a MediaDrmBridgeDelegate interface to support per DRM-scheme logic for Android in MediaDrmBridge.
* Adds a MediaClientAndroid interface for embedders to provide MediaDrmBridgeDelegate implementations and key-system-UUID mappings
* Removes the Java-based key-system-UUID mapping path
* Embedders no longer call directly into MediaDrmBridge

R=qinmin@chromium.org,xhwang@chromium.org,boliu@chromium.org,jochen@chromium.org
BUG=462443
Committed: https://crrev.com/2288c539cc227ec262be28c4b684ff38527f7768
Cr-Commit-Position: refs/heads/master@{#327650}

[gunsch, 2015-03-04 19:28:15]

gunsch@chromium.org changed reviewers:
+ qinmin@chromium.org, xhwang@chromium.org

[gunsch, 2015-03-04 19:29:25]

PTAL. This addresses one of the needs of Cast-on-AndroidTV for custom handling
of PlayReady requests. On Chromecast, we are able to do this by providing our
own PlayReady DRM plugin.

[ddorwin, 2015-03-04 21:43:20]

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

[ddorwin, 2015-03-04 21:43:21]

Some high-level comments:

* What entity/layer calls addDelegateForUuid()?
* I know Java is closer to the MediaDrm plugin, but I think we'd prefer to
minimize the amount of Java code, especially when it comes to abstractions and
interfaces (see below).
* Is there a reason you always call the delegate then fall back to the default
behavior? Why not provide a default implementation that can be inherited or
perhaps an interface that the delegates can call to actually call the platform
(meaning all methods would need to be implemented in the delegate).
* Is getKeyRequest() really the only method that needs to be intercepted? (See
below.)
* If we're going to do something like this, we should extract the Widevine logic
into such an implementation (http://crbug.com/459400). If we do that, it would
be better to have this in C++ code as that logic is today.

[gunsch, 2015-03-05 00:48:49]

Thanks for your thoughts. Responses inline:

On 2015/03/04 21:43:21, ddorwin wrote:
> Some high-level comments:
> 
> * What entity/layer calls addDelegateForUuid()?

Presumably whichever entity/layer calls MediaDrm.addKeySystemUuidMapping
(existing API).

> * I know Java is closer to the MediaDrm plugin, but I think we'd prefer to
> minimize the amount of Java code, especially when it comes to abstractions and
> interfaces (see below).

Acknowledged. More thoughts further below.

> * Is there a reason you always call the delegate then fall back to the default
> behavior? Why not provide a default implementation that can be inherited or
> perhaps an interface that the delegates can call to actually call the platform
> (meaning all methods would need to be implemented in the delegate).

I only call the delegate if it's been registered for that key system. My gut
reaction is opposed to your first suggestion in a "composition-over-inheritance"
sense---I think that would make the interface more complicated to use for no
gain.

However, I could be open to the second idea (providing an interface to the
delegates to call)---sort of an adapter over the entire MediaDrmBridge
interface? Also seems more complicated than necessary.

> * Is getKeyRequest() really the only method that needs to be intercepted? (See
> below.)

Currently for us, yes. But I'm open to finding a solution that also addresses
the existing extra Widevine logic.

> * If we're going to do something like this, we should extract the Widevine
logic
> into such an implementation (http://crbug.com/459400). If we do that, it would
> be better to have this in C++ code as that logic is today.

I'm fine with generally preferring C++ over Java hooks.

Here's a pointer to our potential user of it (internal repo out of convenience,
we'd likely try to check this into chromium).
https://eureka-internal-review.git.corp.google.com/#/c/31377/

Our injection hook is mostly just a way to get at the "optionalParameters"
passed to MediaDrm.getKeyRequest.


Thinking about both use cases, we could create a C++ delegate that involves:
* Call from BrowserCdmFactoryAndroid immediately after MediaDrmBridge creation
(for SetSecurityLevel)
* Ability to intercept IsSecureDecoderRequired and provide a response
* Ability to intercept, or participate in, CreateSession, including writing to
an optional parameters map

However, for both our use cases, this still requires either:
(a) providing access to the MediaDrm instance that MediaDrmBridge owns, or
(b) adding "generic" interfaces into MediaDrmBridge for our specific use cases
(e.g. passing along optional parameters to createSession, and calling
MediaDrm.setPropertyString).

Option (a) feels particularly leaky, so I'd rather go with option (b). In either
case, the delegate API will likely end up rather tied to the specific needs of
the current Widevine hacks + Chromecast's PlayReady needs. How do you feel about
that approach?

[ddorwin, 2015-03-05 22:16:16]

qinmin, xhwang: Please review the discussion and comment.

[xhwang, 2015-03-05 23:13:11]

On 2015/03/05 22:16:16, ddorwin wrote:
> qinmin, xhwang: Please review the discussion and comment.

My 2-cents:
- MediaDrmBridge should really just be a bridge/adapter b/w C++ and Java
MediaDrm API. Given how much MediaDrm and EME differ, this class has already
been pretty complex. We shouldn't add non-MediaDrm-related stuff to this class.
- It seems to me the AddKeySystemUuidMapping call uses MediaDrmBridge only as a
path to reach C++ from Java. This seems bad to me but I don't have better
suggestions at this moment.
- With that, I think we should put our logic in C++ code as much as we can.

Since we already have some code in MediaDrmBridge.h to handle pssh [1], and we
are already doing something special for Widevine [2], can we do something
similar for now for PlayReady, and then later abstract them out to have some C++
delegate classes?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...

[gunsch, 2015-03-06 16:39:01]

On 2015/03/05 23:13:11, xhwang wrote:
> On 2015/03/05 22:16:16, ddorwin wrote:
> > qinmin, xhwang: Please review the discussion and comment.
> 
> My 2-cents:
> - MediaDrmBridge should really just be a bridge/adapter b/w C++ and Java
> MediaDrm API. Given how much MediaDrm and EME differ, this class has already
> been pretty complex. We shouldn't add non-MediaDrm-related stuff to this
class.
> - It seems to me the AddKeySystemUuidMapping call uses MediaDrmBridge only as
a
> path to reach C++ from Java. This seems bad to me but I don't have better
> suggestions at this moment.
> - With that, I think we should put our logic in C++ code as much as we can.
> 
> Since we already have some code in MediaDrmBridge.h to handle pssh [1], and we
> are already doing something special for Widevine [2], can we do something
> similar for now for PlayReady, and then later abstract them out to have some
C++
> delegate classes?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...

The code path is only used for Chromecast's PlayReady implementation. We could
temporarily add it to MediaDrmBridge conditioned by the PlayReady UUID and
assume that only Chromecast will trigger it by sending an extra PSSH box, but
it'd be possible for non-Chromecast users to hit that path. I think that's
probably undesirable here.

I'm willing to take a stab at what a native delegate class would look like that
handles both the Widevine/PlayReady cases, but the caveats I described in the
previous comment would still hold. Will be a week or so (OOO half of next week).

[gunsch, 2015-03-18 21:26:47]

PTAL: I've put together an example native-side delegate class, that would handle
both Chromecast's needs and the Widevine PSSH-parsing case. It registers based
on key system name (string) rather than UUID.

If this looks good, it could probably be expanded to extract (some of) the
Widevine SetSecurityLevel code-path as well.

One question: any thoughts on which part of the code should own the Widevine
PSSH-parsing code? I'd assume somewhere in content/, but am hesitant to just
dump the registration into, say, BrowserMainLoop. Where could be a) appropriate
and b) apply by default to Chromium-Android embedders?

[xhwang, 2015-03-23 04:59:47]

The Delegate in native code looks good. I added some comments.

For your question, can we put Widevine PSSH-parsing code under
src/components/cdm?

https://codereview.chromium.org/962793005/diff/80001/media/base/android/java/...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://codereview.chromium.org/962793005/diff/80001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:519:
optionalData.put(additionalData[i], additionalData[i + 1]);
hmm, can we just store addtionalData as HashMap<String, String>?

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

https://codereview.chromium.org/962793005/diff/80001/media/base/android/media...
media/base/android/media_drm_bridge.cc:320:
g_key_system_uuid_manager.Get().AddMapping(key_system, uuid);
Can we merge g_delegates and g_key_system_uuid_manager?

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

https://codereview.chromium.org/962793005/diff/80001/media/base/android/media...
media/base/android/media_drm_bridge.h:30: class Delegate {
Add comment please.

https://codereview.chromium.org/962793005/diff/80001/media/base/android/media...
media/base/android/media_drm_bridge.h:32: void OnCreateSession(
virtual?

https://codereview.chromium.org/962793005/diff/80001/media/base/android/media...
media/base/android/media_drm_bridge.h:32: void OnCreateSession(
How about SanitizeInitData()? (Search for "sanitized init data" in the latest
EME spec). The |init_data_out| could be |sanitized_init_data|.

Also, since Android MediaDrm takes an |optionalParameters|, we can probably just
reuse this name, instead of having a new |additional_data_out|.

[gunsch, 2015-04-09 01:36:39]

Getting back to this again. Thanks for the thorough review; it was a bit of a
toy CL last go-round to see how that delegate might look. I've cleaned it up
now, separated out the Widevine code into its own Delegate implementation, and
added a Chromecast-specific PlayReady implementation as the other expected user.

One issue I had was actually finding a location to put one-time browser-side CDM
initialization. The existing code paths I found seem to be one of the following:
1) one-time CDM initialization that's done renderer-side (registering key
systems)
2) browser-side Android-specific CDM or other media initialization is all done
at playback time per media-element or per RenderFrame (BrowserMediaPlayerManager
etc.)
3) the only one-time browser-side CDM setup I could find were the one-off calls
from Java to MediaDrmBridge.addKeySystemUuidMapping, which is clearly what we're
trying to get away from :)

So it ended up in the *BrowserMainParts classes instead.

https://codereview.chromium.org/962793005/diff/80001/media/base/android/java/...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://codereview.chromium.org/962793005/diff/80001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:519:
optionalData.put(additionalData[i], additionalData[i + 1]);
On 2015/03/23 04:59:47, xhwang wrote:
> hmm, can we just store addtionalData as HashMap<String, String>?

Mostly so: the hard part is the native->Java boundary---there doesn't seem to be
JNI utility code for building up a Java Map from native code. I'm still using
the vector<string> to pass here, but now the Java code uses HashMap<String,
String> where possible.

(Aside: what kind of API takes a HashMap rather than a Map? It feels a little
dirty to be passing an explicit HashMap around everywhere.)

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

https://codereview.chromium.org/962793005/diff/80001/media/base/android/media...
media/base/android/media_drm_bridge.cc:320:
g_key_system_uuid_manager.Get().AddMapping(key_system, uuid);
On 2015/03/23 04:59:47, xhwang wrote:
> Can we merge g_delegates and g_key_system_uuid_manager?

Done.

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

https://codereview.chromium.org/962793005/diff/80001/media/base/android/media...
media/base/android/media_drm_bridge.h:30: class Delegate {
On 2015/03/23 04:59:47, xhwang wrote:
> Add comment please.

Done.

https://codereview.chromium.org/962793005/diff/80001/media/base/android/media...
media/base/android/media_drm_bridge.h:32: void OnCreateSession(
On 2015/03/23 04:59:47, xhwang wrote:
> virtual?

Done.

https://codereview.chromium.org/962793005/diff/80001/media/base/android/media...
media/base/android/media_drm_bridge.h:32: void OnCreateSession(
On 2015/03/23 04:59:47, xhwang wrote:
> How about SanitizeInitData()? (Search for "sanitized init data" in the latest
> EME spec). The |init_data_out| could be |sanitized_init_data|.
> 
> Also, since Android MediaDrm takes an |optionalParameters|, we can probably
just
> reuse this name, instead of having a new |additional_data_out|.

1) I feel like isn't, strictly speaking, "sanitization" of the initialization
data, especially given |additional_data_out|. I think that name might cause
additional confusion as to the purpose.

2) Renamed throughout the CL to |optionalParameters|.

[xhwang, 2015-04-09 22:37:10]

General question:

Will doing the work in BrowserMainParts affect the startup time?

Ideally, we can have a ContentBrowserClient call for this, but I am not sure
weather it's worth the effort.

Or, can you instead move it to somewhere that will be called later? e.g.

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

https://codereview.chromium.org/962793005/diff/120001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/962793005/diff/120001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:383: if (delegate_) {
Just call g_key_system_manager.Get().GetDelegate(key_system) here instead of
having a local variable?

https://codereview.chromium.org/962793005/diff/120001/media/base/android/medi...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/962793005/diff/120001/media/base/android/medi...
media/base/android/media_drm_bridge.h:231: Delegate* const delegate_;
nit: document about lifetime.

[gunsch, 2015-04-10 00:20:53]

Per BrowserMainParts:

Seeing as it's just creating a couple objects, that doesn't seem
substantial---it's certainly not doing any file reads or network access as part
of the startup time.

My concern with moving it to somewhere like RenderProcessWillLaunch is that this
must only be installed *once* per browser process, not per renderer. Everything
other media example is installed per renderer so far. So even if I add it into
ContentBrowserClient, either:

a) it can get called each page load, or when media playback starts, but then
each content embedder needs a flag to track if it has installed the delegates
already, or

b) it could be called on the very first media playback globally, at which point
we have a static flag somewhere like BrowserMediaPlayerManager and only invoke
it on ContentBrowserClient once.

I'm okay with option (b) (removes this path from BrowserMainParts) if you don't
think the static flag is too ugly. Option (a) feels worse to me.

https://codereview.chromium.org/962793005/diff/120001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/962793005/diff/120001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:383: if (delegate_) {
On 2015/04/09 22:37:10, xhwang wrote:
> Just call g_key_system_manager.Get().GetDelegate(key_system) here instead of
> having a local variable?

Can't right now: MediaDrmBridge doesn't actually track the key system string as
a member variable (feels intentional). MediaDrmBridge::Create maps the key
system string to a UUID, then passes the UUID into MediaDrmBridge instead. So I
see the following options:

1) Member variable for the delegate, mapped from key system string in
MediaDrmBridge::Create and passed into MediaDrmBridge instance (current
solution)

2) Extra member variable to also track the key system string in MediaDrmBridge
instance, then looking up the delegate using that

3) Registering delegates by UUID instead of by key system string.

I'm open to all three, and don't have any strong thoughts on the relative
pros/cons. What do you think?

[gunsch, 2015-04-10 00:21:11]

https://codereview.chromium.org/962793005/diff/120001/media/base/android/medi...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/962793005/diff/120001/media/base/android/medi...
media/base/android/media_drm_bridge.h:231: Delegate* const delegate_;
On 2015/04/09 22:37:10, xhwang wrote:
> nit: document about lifetime.

Done.

[xhwang, 2015-04-10 16:41:05]

On 2015/04/10 00:20:53, gunsch wrote:
> Per BrowserMainParts:
> 
> Seeing as it's just creating a couple objects, that doesn't seem
> substantial---it's certainly not doing any file reads or network access as
part
> of the startup time.
> 
> My concern with moving it to somewhere like RenderProcessWillLaunch is that
this
> must only be installed *once* per browser process, not per renderer.
Everything
> other media example is installed per renderer so far. So even if I add it into
> ContentBrowserClient, either:
> 
> a) it can get called each page load, or when media playback starts, but then
> each content embedder needs a flag to track if it has installed the delegates
> already, or
> 
> b) it could be called on the very first media playback globally, at which
point
> we have a static flag somewhere like BrowserMediaPlayerManager and only invoke
> it on ContentBrowserClient once.
> 
> I'm okay with option (b) (removes this path from BrowserMainParts) if you
don't
> think the static flag is too ugly. Option (a) feels worse to me.

How about add one call in ContentBrowserClient, then call it only at the first
time MediaDrmBridge is created in the browser process? You can do this by having
a per browser process singleton instance of the delegate, so you don't need a
flag to manually track this.

[xhwang, 2015-04-10 16:41:13]

https://codereview.chromium.org/962793005/diff/120001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/962793005/diff/120001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:383: if (delegate_) {
On 2015/04/10 00:20:53, gunsch wrote:
> On 2015/04/09 22:37:10, xhwang wrote:
> > Just call g_key_system_manager.Get().GetDelegate(key_system) here instead of
> > having a local variable?
> 
> Can't right now: MediaDrmBridge doesn't actually track the key system string
as
> a member variable (feels intentional). MediaDrmBridge::Create maps the key
> system string to a UUID, then passes the UUID into MediaDrmBridge instead. So
I
> see the following options:
> 
> 1) Member variable for the delegate, mapped from key system string in
> MediaDrmBridge::Create and passed into MediaDrmBridge instance (current
> solution)
> 
> 2) Extra member variable to also track the key system string in MediaDrmBridge
> instance, then looking up the delegate using that
> 
> 3) Registering delegates by UUID instead of by key system string.
> 
> I'm open to all three, and don't have any strong thoughts on the relative
> pros/cons. What do you think?

(3) makes sense to me. MediaDrmBridge only talks UUID and have no idea what a
key system is.

[gunsch, 2015-04-11 00:18:16]

Moved the registration to ContentBrowserClient, invoked the first time a
MediaDrmBridge instance is created (static flag tracking).

Also registered delegates by UUID now instead of by key system name. This
eliminates having MediaDrmBridge keep a reference to the appropriate Delegate.

https://codereview.chromium.org/962793005/diff/120001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/962793005/diff/120001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:383: if (delegate_) {
On 2015/04/10 16:41:13, xhwang wrote:
> On 2015/04/10 00:20:53, gunsch wrote:
> > On 2015/04/09 22:37:10, xhwang wrote:
> > > Just call g_key_system_manager.Get().GetDelegate(key_system) here instead
of
> > > having a local variable?
> > 
> > Can't right now: MediaDrmBridge doesn't actually track the key system string
> as
> > a member variable (feels intentional). MediaDrmBridge::Create maps the key
> > system string to a UUID, then passes the UUID into MediaDrmBridge instead.
So
> I
> > see the following options:
> > 
> > 1) Member variable for the delegate, mapped from key system string in
> > MediaDrmBridge::Create and passed into MediaDrmBridge instance (current
> > solution)
> > 
> > 2) Extra member variable to also track the key system string in
MediaDrmBridge
> > instance, then looking up the delegate using that
> > 
> > 3) Registering delegates by UUID instead of by key system string.
> > 
> > I'm open to all three, and don't have any strong thoughts on the relative
> > pros/cons. What do you think?
> 
> (3) makes sense to me. MediaDrmBridge only talks UUID and have no idea what a
> key system is.

Done.

[xhwang, 2015-04-13 18:43:44]

Thanks for the update! This looks much better.

I wonder, instead of having BrowserCdmManager to call into the client to
RegisterMediaDrmBridgeDelegates(), where the client will call into
MediaDrmBridge to register the delegate, can we make thinks simpler by following
how we set content clients
(https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...).

For example:
1. On the browser client interface, have a Delegate*
GetMediaDrmBridgeDelegate().
2. In g_key_system_manager.Get().GetDelegate(scheme_uuid_), we can have a
boolean map tracking whether we have tried to set the delegate. If so, return
the delegate we have (could be null if the embedder doesn't support a delegate).
Otherwise, call GetMediaDrmBridgeDelegate() and set the delegate.

This way, we don't need to touch BrowserCdmManager, which sounds better since
this delegate is really for MediaDrmBridge only. WDYT?

[gunsch, 2015-04-14 22:08:19]

On 2015/04/13 18:43:44, xhwang wrote:
> Thanks for the update! This looks much better.
> 
> I wonder, instead of having BrowserCdmManager to call into the client to
> RegisterMediaDrmBridgeDelegates(), where the client will call into
> MediaDrmBridge to register the delegate, can we make thinks simpler by
following
> how we set content clients
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...).
> 
> For example:
> 1. On the browser client interface, have a Delegate*
> GetMediaDrmBridgeDelegate().
> 2. In g_key_system_manager.Get().GetDelegate(scheme_uuid_), we can have a
> boolean map tracking whether we have tried to set the delegate. If so, return
> the delegate we have (could be null if the embedder doesn't support a
delegate).
> Otherwise, call GetMediaDrmBridgeDelegate() and set the delegate.
> 
> This way, we don't need to touch BrowserCdmManager, which sounds better since
> this delegate is really for MediaDrmBridge only. WDYT?

Hmm, I don't think that's really appropriate: that would introduce a dependency
from MediaDrmBridge (media/base/) onto ContentBrowserClient (content/public/). I
feel like BrowserCdmManager needs to be involved because of its role as the
bridge between content/browser/ and media/base/.

[xhwang, 2015-04-14 22:38:04]

On 2015/04/14 22:08:19, gunsch wrote:
> On 2015/04/13 18:43:44, xhwang wrote:
> > Thanks for the update! This looks much better.
> > 
> > I wonder, instead of having BrowserCdmManager to call into the client to
> > RegisterMediaDrmBridgeDelegates(), where the client will call into
> > MediaDrmBridge to register the delegate, can we make thinks simpler by
> following
> > how we set content clients
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...).
> > 
> > For example:
> > 1. On the browser client interface, have a Delegate*
> > GetMediaDrmBridgeDelegate().
> > 2. In g_key_system_manager.Get().GetDelegate(scheme_uuid_), we can have a
> > boolean map tracking whether we have tried to set the delegate. If so,
return
> > the delegate we have (could be null if the embedder doesn't support a
> delegate).
> > Otherwise, call GetMediaDrmBridgeDelegate() and set the delegate.
> > 
> > This way, we don't need to touch BrowserCdmManager, which sounds better
since
> > this delegate is really for MediaDrmBridge only. WDYT?
> 
> Hmm, I don't think that's really appropriate: that would introduce a
dependency
> from MediaDrmBridge (media/base/) onto ContentBrowserClient (content/public/).
I
> feel like BrowserCdmManager needs to be involved because of its role as the
> bridge between content/browser/ and media/base/.

Sigh, you are right.

Then how about having BrowserCdmManager getting the delegate from the embedder,
and sets it in the KeySystemManager the first time MediaDrmBridge is created?
This is pretty much what you are doing now, but we don't have the embedder code
talking to MediaDrmBridge directly.

[gunsch, 2015-04-14 23:36:13]

Alright, how about this?

* MediaDrmDelegate is now its own class (became sort of mandatory so I could
forward-declare it in the interface instead of having to add a DEPS entry from
content/ onto media/base/android/, but now that it's separated it feels cleaner)
* BrowserCdmManager gets a vector<MediaDrmDelegate> from the current
ContentBrowserClient, then performs the registrations
* MediaDrmDelegate now declares the UUID of the DRM scheme it accompanies as
part of the interface

[xhwang, 2015-04-15 00:05:18]

On 2015/04/14 23:36:13, gunsch wrote:
> Alright, how about this?
> 
> * MediaDrmDelegate is now its own class (became sort of mandatory so I could
> forward-declare it in the interface instead of having to add a DEPS entry from
> content/ onto media/base/android/, but now that it's separated it feels
cleaner)

Note that MediaDrmBridge is Android only and BrowserCdmManager should be able to
support other BrowserCdm implementations. So in theory BCM knows nothing about
MediaDrmBridge. That makes me wonder whether we should put MediaDrmBridge logic
somewhere else.... How about BrowserCdmFactoryAndroid? This is created only once
in the browser process
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/browser...),
and is the right place to do MediaDrmBridge related logic. The only issue is
that it's in media/base. But I think we can just move it over to
content/browser/media/android. It's a "Browser" CDM factory anyways. WDYT?

> * BrowserCdmManager gets a vector<MediaDrmDelegate> from the current
> ContentBrowserClient, then performs the registrations

See above about not doing this in BCM. Otherwise, using the vector sgtm.

> * MediaDrmDelegate now declares the UUID of the DRM scheme it accompanies as
> part of the interface

So this will replace the Java path where we register the UUID? The java code
definitely looks hacky. +1 for replacing it.

[gunsch, 2015-04-15 01:10:15]

On 2015/04/15 00:05:18, xhwang wrote:
> On 2015/04/14 23:36:13, gunsch wrote:
> > Alright, how about this?
> > 
> > * MediaDrmDelegate is now its own class (became sort of mandatory so I could
> > forward-declare it in the interface instead of having to add a DEPS entry
from
> > content/ onto media/base/android/, but now that it's separated it feels
> cleaner)
> 
> Note that MediaDrmBridge is Android only and BrowserCdmManager should be able
to
> support other BrowserCdm implementations. So in theory BCM knows nothing about
> MediaDrmBridge. That makes me wonder whether we should put MediaDrmBridge
logic
> somewhere else.... How about BrowserCdmFactoryAndroid? This is created only
once
> in the browser process
>
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/browser...),
> and is the right place to do MediaDrmBridge related logic. The only issue is
> that it's in media/base. But I think we can just move it over to
> content/browser/media/android. It's a "Browser" CDM factory anyways. WDYT?
> 

Now we're back to square one: where is appropriate to initialize something for
all content embedders that's Android-specific? I think moving it to
content/browser/media/android is reasonable, *but* then who calls
SetBrowserCdmFactory(BrowserCdmFactoryAndroid)? (currently in
media/base/browser_cdm_factory.cc).

We could call it from BrowserCdmManager before the first media::CreateBrowserCdm
call, but that doesn't actually address the concern you described (not calling
into anything Android-specific from BrowserCdmManager). It would at least be
lighter-weight than dealing with the MediaDrmDelegates in BrowserCdmManager.

> > * BrowserCdmManager gets a vector<MediaDrmDelegate> from the current
> > ContentBrowserClient, then performs the registrations
> 
> See above about not doing this in BCM. Otherwise, using the vector sgtm.
> 
> > * MediaDrmDelegate now declares the UUID of the DRM scheme it accompanies as
> > part of the interface
> 
> So this will replace the Java path where we register the UUID? The java code
> definitely looks hacky. +1 for replacing it.

Oof. No, I don't know that replacing it is in scope for this CL. We'd then
_also_ have to tie in key-system strings with the delegates, and support a
multiple-to-one mapping (key systems to UUIDs) with the delegates.

I had another conversation with ddorwin@ recently about that---I think
decoupling UUID from Key System registration was actually a mistake. For
example, we had a bug recently where a key system string was declared in
CastContentRendererClient::AddKeySystems but a UUID association not registered.
Unfortunately, this means it shows up as supported when you query for support,
but fails at runtime when you try to use it (!).

[xhwang, 2015-04-15 04:49:35]

On 2015/04/15 01:10:15, gunsch wrote:
> On 2015/04/15 00:05:18, xhwang wrote:
> > On 2015/04/14 23:36:13, gunsch wrote:
> > > Alright, how about this?
> > > 
> > > * MediaDrmDelegate is now its own class (became sort of mandatory so I
could
> > > forward-declare it in the interface instead of having to add a DEPS entry
> from
> > > content/ onto media/base/android/, but now that it's separated it feels
> > cleaner)
> > 
> > Note that MediaDrmBridge is Android only and BrowserCdmManager should be
able
> to
> > support other BrowserCdm implementations. So in theory BCM knows nothing
about
> > MediaDrmBridge. That makes me wonder whether we should put MediaDrmBridge
> logic
> > somewhere else.... How about BrowserCdmFactoryAndroid? This is created only
> once
> > in the browser process
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/browser...),
> > and is the right place to do MediaDrmBridge related logic. The only issue is
> > that it's in media/base. But I think we can just move it over to
> > content/browser/media/android. It's a "Browser" CDM factory anyways. WDYT?
> > 
> 
> Now we're back to square one: where is appropriate to initialize something for
> all content embedders that's Android-specific? I think moving it to
> content/browser/media/android is reasonable, *but* then who calls
> SetBrowserCdmFactory(BrowserCdmFactoryAndroid)? (currently in
> media/base/browser_cdm_factory.cc).

Can we move browser_cdm_factory.* to content/browser/media/ as well?

> We could call it from BrowserCdmManager before the first
media::CreateBrowserCdm
> call, but that doesn't actually address the concern you described (not calling
> into anything Android-specific from BrowserCdmManager). It would at least be
> lighter-weight than dealing with the MediaDrmDelegates in BrowserCdmManager.
> 
> > > * BrowserCdmManager gets a vector<MediaDrmDelegate> from the current
> > > ContentBrowserClient, then performs the registrations
> > 
> > See above about not doing this in BCM. Otherwise, using the vector sgtm.
> > 
> > > * MediaDrmDelegate now declares the UUID of the DRM scheme it accompanies
as
> > > part of the interface
> > 
> > So this will replace the Java path where we register the UUID? The java code
> > definitely looks hacky. +1 for replacing it.
> 
> Oof. No, I don't know that replacing it is in scope for this CL. We'd then
> _also_ have to tie in key-system strings with the delegates, and support a
> multiple-to-one mapping (key systems to UUIDs) with the delegates.

Agreed it's not in scope for this CL. Something we should think about though.

> I had another conversation with ddorwin@ recently about that---I think
> decoupling UUID from Key System registration was actually a mistake. For
> example, we had a bug recently where a key system string was declared in
> CastContentRendererClient::AddKeySystems but a UUID association not
registered.
> Unfortunately, this means it shows up as supported when you query for support,
> but fails at runtime when you try to use it (!).

Hmm, shouldn't you query the browser for key system support in your
CastContentRendererClient::AddKeySystems() call?

Something like
https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren...

[gunsch, 2015-04-18 00:36:27]

Just getting back to this, thanks for the review thoughts so far.

On 2015/04/15 04:49:35, xhwang wrote:
> On 2015/04/15 01:10:15, gunsch wrote:
> > On 2015/04/15 00:05:18, xhwang wrote:
> > > On 2015/04/14 23:36:13, gunsch wrote:
> > > > Alright, how about this?
> > > > 
> > > > * MediaDrmDelegate is now its own class (became sort of mandatory so I
> could
> > > > forward-declare it in the interface instead of having to add a DEPS
entry
> > from
> > > > content/ onto media/base/android/, but now that it's separated it feels
> > > cleaner)
> > > 
> > > Note that MediaDrmBridge is Android only and BrowserCdmManager should be
> able
> > to
> > > support other BrowserCdm implementations. So in theory BCM knows nothing
> about
> > > MediaDrmBridge. That makes me wonder whether we should put MediaDrmBridge
> > logic
> > > somewhere else.... How about BrowserCdmFactoryAndroid? This is created
only
> > once
> > > in the browser process
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/browser...),
> > > and is the right place to do MediaDrmBridge related logic. The only issue
is
> > > that it's in media/base. But I think we can just move it over to
> > > content/browser/media/android. It's a "Browser" CDM factory anyways. WDYT?
> > > 
> > 
> > Now we're back to square one: where is appropriate to initialize something
for
> > all content embedders that's Android-specific? I think moving it to
> > content/browser/media/android is reasonable, *but* then who calls
> > SetBrowserCdmFactory(BrowserCdmFactoryAndroid)? (currently in
> > media/base/browser_cdm_factory.cc).
> 
> Can we move browser_cdm_factory.* to content/browser/media/ as well?

That would mean moving it to content/public/browser/ actually, do you think
that's appropriate? That'll add a DEPS from content/public/browser/ to
media/base/ (currently isn't one, though I do see that content/public/renderer/
has one, so that's probably a reasonable addition).

I'd suggest moving BrowserCdm as well, but that would require uprooting
MediaDrmBridge.

> 
> > We could call it from BrowserCdmManager before the first
> media::CreateBrowserCdm
> > call, but that doesn't actually address the concern you described (not
calling
> > into anything Android-specific from BrowserCdmManager). It would at least be
> > lighter-weight than dealing with the MediaDrmDelegates in BrowserCdmManager.
> > 
> > > > * BrowserCdmManager gets a vector<MediaDrmDelegate> from the current
> > > > ContentBrowserClient, then performs the registrations
> > > 
> > > See above about not doing this in BCM. Otherwise, using the vector sgtm.
> > > 
> > > > * MediaDrmDelegate now declares the UUID of the DRM scheme it
accompanies
> as
> > > > part of the interface
> > > 
> > > So this will replace the Java path where we register the UUID? The java
code
> > > definitely looks hacky. +1 for replacing it.
> > 
> > Oof. No, I don't know that replacing it is in scope for this CL. We'd then
> > _also_ have to tie in key-system strings with the delegates, and support a
> > multiple-to-one mapping (key systems to UUIDs) with the delegates.
> 
> Agreed it's not in scope for this CL. Something we should think about though.
> 
> > I had another conversation with ddorwin@ recently about that---I think
> > decoupling UUID from Key System registration was actually a mistake. For
> > example, we had a bug recently where a key system string was declared in
> > CastContentRendererClient::AddKeySystems but a UUID association not
> registered.
> > Unfortunately, this means it shows up as supported when you query for
support,
> > but fails at runtime when you try to use it (!).
> 
> Hmm, shouldn't you query the browser for key system support in your
> CastContentRendererClient::AddKeySystems() call?
> 
> Something like
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren...

No, we don't. That's only applicable for Android, and right now the logic for
key-systems registration for both Cast Linux and Cast Android is the same (which
is a positive I'd like to keep). Is there any reason the common key-system code
doesn't do that query when checking if types are supported instead of expecting
each embedder to do it?

[gunsch, 2015-04-23 00:38:58]

On 2015/04/18 00:36:27, gunsch wrote:
> Just getting back to this, thanks for the review thoughts so far.
> 
> On 2015/04/15 04:49:35, xhwang wrote:
> > On 2015/04/15 01:10:15, gunsch wrote:
> > > On 2015/04/15 00:05:18, xhwang wrote:
> > > > On 2015/04/14 23:36:13, gunsch wrote:
> > > > > Alright, how about this?
> > > > > 
> > > > > * MediaDrmDelegate is now its own class (became sort of mandatory so I
> > could
> > > > > forward-declare it in the interface instead of having to add a DEPS
> entry
> > > from
> > > > > content/ onto media/base/android/, but now that it's separated it
feels
> > > > cleaner)
> > > > 
> > > > Note that MediaDrmBridge is Android only and BrowserCdmManager should be
> > able
> > > to
> > > > support other BrowserCdm implementations. So in theory BCM knows nothing
> > about
> > > > MediaDrmBridge. That makes me wonder whether we should put
MediaDrmBridge
> > > logic
> > > > somewhere else.... How about BrowserCdmFactoryAndroid? This is created
> only
> > > once
> > > > in the browser process
> > > >
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/browser...),
> > > > and is the right place to do MediaDrmBridge related logic. The only
issue
> is
> > > > that it's in media/base. But I think we can just move it over to
> > > > content/browser/media/android. It's a "Browser" CDM factory anyways.
WDYT?
> > > > 
> > > 
> > > Now we're back to square one: where is appropriate to initialize something
> for
> > > all content embedders that's Android-specific? I think moving it to
> > > content/browser/media/android is reasonable, *but* then who calls
> > > SetBrowserCdmFactory(BrowserCdmFactoryAndroid)? (currently in
> > > media/base/browser_cdm_factory.cc).
> > 
> > Can we move browser_cdm_factory.* to content/browser/media/ as well?
> 
> That would mean moving it to content/public/browser/ actually, do you think
> that's appropriate? That'll add a DEPS from content/public/browser/ to
> media/base/ (currently isn't one, though I do see that
content/public/renderer/
> has one, so that's probably a reasonable addition).
> 
> I'd suggest moving BrowserCdm as well, but that would require uprooting
> MediaDrmBridge.
> 
> > 
> > > We could call it from BrowserCdmManager before the first
> > media::CreateBrowserCdm
> > > call, but that doesn't actually address the concern you described (not
> calling
> > > into anything Android-specific from BrowserCdmManager). It would at least
be
> > > lighter-weight than dealing with the MediaDrmDelegates in
BrowserCdmManager.
> > > 
> > > > > * BrowserCdmManager gets a vector<MediaDrmDelegate> from the current
> > > > > ContentBrowserClient, then performs the registrations
> > > > 
> > > > See above about not doing this in BCM. Otherwise, using the vector sgtm.
> > > > 
> > > > > * MediaDrmDelegate now declares the UUID of the DRM scheme it
> accompanies
> > as
> > > > > part of the interface
> > > > 
> > > > So this will replace the Java path where we register the UUID? The java
> code
> > > > definitely looks hacky. +1 for replacing it.
> > > 
> > > Oof. No, I don't know that replacing it is in scope for this CL. We'd then
> > > _also_ have to tie in key-system strings with the delegates, and support a
> > > multiple-to-one mapping (key systems to UUIDs) with the delegates.
> > 
> > Agreed it's not in scope for this CL. Something we should think about
though.
> > 
> > > I had another conversation with ddorwin@ recently about that---I think
> > > decoupling UUID from Key System registration was actually a mistake. For
> > > example, we had a bug recently where a key system string was declared in
> > > CastContentRendererClient::AddKeySystems but a UUID association not
> > registered.
> > > Unfortunately, this means it shows up as supported when you query for
> support,
> > > but fails at runtime when you try to use it (!).
> > 
> > Hmm, shouldn't you query the browser for key system support in your
> > CastContentRendererClient::AddKeySystems() call?
> > 
> > Something like
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren...
> 
> No, we don't. That's only applicable for Android, and right now the logic for
> key-systems registration for both Cast Linux and Cast Android is the same
(which
> is a positive I'd like to keep). Is there any reason the common key-system
code
> doesn't do that query when checking if types are supported instead of
expecting
> each embedder to do it?

Per our discussions so far (including over chat today), what do you think about
reworking this to the following:

(1) Create new class |media::MediaClientAndroid|, similar to
|media::MediaClient| but a) Android-only and b) lives in the browser process (or
whichever process is managing Android media playback---for now browser process
is implied but won't be assumed in the code). General interface looks something
like:

class MediaClientAndroid {
  virtual std::vector<std::pair<std::string, UUID>> GetKeySystemUUIDPairs() = 0;
  virtual media::MediaDrmDelegate* GetMediaDrmBridgeDelegate(const UUID&
scheme_uuid) = 0;
};

(2) Content embedder can call |SetMediaClient(new CustomMediaClientAndroid)| at
basically any point. It does the following:
(2a) Contains any key-system-to-UUID mappings. media_drm_bridge.cc's
|KeySystemUuidManager| can ask for that list from |GetMediaClientAndroid()| the
first time |KeySystemUuidManager| needs them.
(2b) Returns a |MediaDrmDelegate| for a given UUID when queried. (Or just
returns a vector of |MediaDrmDelegate| instances like I currently have in
content_browser_client.h this CL, and |KeySystemUuidManager| also loads those
when first needed.)


This proposal would:
* be able to stay out of the content API
* be agnostic to which process Android's media code is running in (as long as
the call site of |SetMediaClientAndroid(MediaClientAndroid*)| occurred in the
same process)
* immediately provide a better path for the Java
"MediaDrmBridge.addKeySystemUuidMapping()" calls.

WDYT?

Also, the one question I would have with this proposal is, where would you be
comfortable with the call site for (2)? I think we're back to adding it
somewhere like {Aw,Chrome,Cast}BrowserMainParts, which isn't ideal but I think
is the only way to avoid circling back to the same questions we're currently
stuck at.

[xhwang, 2015-04-23 04:36:46]

On 2015/04/23 00:38:58, gunsch wrote:
> On 2015/04/18 00:36:27, gunsch wrote:
> > Just getting back to this, thanks for the review thoughts so far.
> > 
> > On 2015/04/15 04:49:35, xhwang wrote:
> > > On 2015/04/15 01:10:15, gunsch wrote:
> > > > On 2015/04/15 00:05:18, xhwang wrote:
> > > > > On 2015/04/14 23:36:13, gunsch wrote:
> > > > > > Alright, how about this?
> > > > > > 
> > > > > > * MediaDrmDelegate is now its own class (became sort of mandatory so
I
> > > could
> > > > > > forward-declare it in the interface instead of having to add a DEPS
> > entry
> > > > from
> > > > > > content/ onto media/base/android/, but now that it's separated it
> feels
> > > > > cleaner)
> > > > > 
> > > > > Note that MediaDrmBridge is Android only and BrowserCdmManager should
be
> > > able
> > > > to
> > > > > support other BrowserCdm implementations. So in theory BCM knows
nothing
> > > about
> > > > > MediaDrmBridge. That makes me wonder whether we should put
> MediaDrmBridge
> > > > logic
> > > > > somewhere else.... How about BrowserCdmFactoryAndroid? This is created
> > only
> > > > once
> > > > > in the browser process
> > > > >
> > > >
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/browser...),
> > > > > and is the right place to do MediaDrmBridge related logic. The only
> issue
> > is
> > > > > that it's in media/base. But I think we can just move it over to
> > > > > content/browser/media/android. It's a "Browser" CDM factory anyways.
> WDYT?
> > > > > 
> > > > 
> > > > Now we're back to square one: where is appropriate to initialize
something
> > for
> > > > all content embedders that's Android-specific? I think moving it to
> > > > content/browser/media/android is reasonable, *but* then who calls
> > > > SetBrowserCdmFactory(BrowserCdmFactoryAndroid)? (currently in
> > > > media/base/browser_cdm_factory.cc).
> > > 
> > > Can we move browser_cdm_factory.* to content/browser/media/ as well?
> > 
> > That would mean moving it to content/public/browser/ actually, do you think
> > that's appropriate? That'll add a DEPS from content/public/browser/ to
> > media/base/ (currently isn't one, though I do see that
> content/public/renderer/
> > has one, so that's probably a reasonable addition).
> > 
> > I'd suggest moving BrowserCdm as well, but that would require uprooting
> > MediaDrmBridge.
> > 
> > > 
> > > > We could call it from BrowserCdmManager before the first
> > > media::CreateBrowserCdm
> > > > call, but that doesn't actually address the concern you described (not
> > calling
> > > > into anything Android-specific from BrowserCdmManager). It would at
least
> be
> > > > lighter-weight than dealing with the MediaDrmDelegates in
> BrowserCdmManager.
> > > > 
> > > > > > * BrowserCdmManager gets a vector<MediaDrmDelegate> from the current
> > > > > > ContentBrowserClient, then performs the registrations
> > > > > 
> > > > > See above about not doing this in BCM. Otherwise, using the vector
sgtm.
> > > > > 
> > > > > > * MediaDrmDelegate now declares the UUID of the DRM scheme it
> > accompanies
> > > as
> > > > > > part of the interface
> > > > > 
> > > > > So this will replace the Java path where we register the UUID? The
java
> > code
> > > > > definitely looks hacky. +1 for replacing it.
> > > > 
> > > > Oof. No, I don't know that replacing it is in scope for this CL. We'd
then
> > > > _also_ have to tie in key-system strings with the delegates, and support
a
> > > > multiple-to-one mapping (key systems to UUIDs) with the delegates.
> > > 
> > > Agreed it's not in scope for this CL. Something we should think about
> though.
> > > 
> > > > I had another conversation with ddorwin@ recently about that---I think
> > > > decoupling UUID from Key System registration was actually a mistake. For
> > > > example, we had a bug recently where a key system string was declared in
> > > > CastContentRendererClient::AddKeySystems but a UUID association not
> > > registered.
> > > > Unfortunately, this means it shows up as supported when you query for
> > support,
> > > > but fails at runtime when you try to use it (!).
> > > 
> > > Hmm, shouldn't you query the browser for key system support in your
> > > CastContentRendererClient::AddKeySystems() call?
> > > 
> > > Something like
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren...
> > 
> > No, we don't. That's only applicable for Android, and right now the logic
for
> > key-systems registration for both Cast Linux and Cast Android is the same
> (which
> > is a positive I'd like to keep). Is there any reason the common key-system
> code
> > doesn't do that query when checking if types are supported instead of
> expecting
> > each embedder to do it?
> 
> Per our discussions so far (including over chat today), what do you think
about
> reworking this to the following:
> 
> (1) Create new class |media::MediaClientAndroid|, similar to
> |media::MediaClient| but a) Android-only and b) lives in the browser process
(or
> whichever process is managing Android media playback---for now browser process
> is implied but won't be assumed in the code). General interface looks
something
> like:
> 
> class MediaClientAndroid {
>   virtual std::vector<std::pair<std::string, UUID>> GetKeySystemUUIDPairs() =
0;
>   virtual media::MediaDrmDelegate* GetMediaDrmBridgeDelegate(const UUID&
> scheme_uuid) = 0;
> };
> 
> (2) Content embedder can call |SetMediaClient(new CustomMediaClientAndroid)|
at
> basically any point. It does the following:
> (2a) Contains any key-system-to-UUID mappings. media_drm_bridge.cc's
> |KeySystemUuidManager| can ask for that list from |GetMediaClientAndroid()|
the
> first time |KeySystemUuidManager| needs them.
> (2b) Returns a |MediaDrmDelegate| for a given UUID when queried. (Or just
> returns a vector of |MediaDrmDelegate| instances like I currently have in
> content_browser_client.h this CL, and |KeySystemUuidManager| also loads those
> when first needed.)
> 
> 
> This proposal would:
> * be able to stay out of the content API
> * be agnostic to which process Android's media code is running in (as long as
> the call site of |SetMediaClientAndroid(MediaClientAndroid*)| occurred in the
> same process)
> * immediately provide a better path for the Java
> "MediaDrmBridge.addKeySystemUuidMapping()" calls.
> 
> WDYT?
> 
> Also, the one question I would have with this proposal is, where would you be
> comfortable with the call site for (2)? I think we're back to adding it
> somewhere like {Aw,Chrome,Cast}BrowserMainParts, which isn't ideal but I think
> is the only way to avoid circling back to the same questions we're currently
> stuck at.

Thanks for the proposal and patience! It's more consistent with how we support
embedders in Chromium and I like it!

Two minor comments:
1. This delegate is so MediaDrmBridge specific that I feel something like
MediaDrmBridgeDelegate would be more accurate.
2. Chromium content clients are set during *Main* as well so *BrowserMainParts
sounds good to me now:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...

[gunsch, 2015-04-23 23:43:56]

On 2015/04/23 04:36:46, xhwang wrote:
> On 2015/04/23 00:38:58, gunsch wrote:
> > On 2015/04/18 00:36:27, gunsch wrote:
> > > Just getting back to this, thanks for the review thoughts so far.
> > > 
> > > On 2015/04/15 04:49:35, xhwang wrote:
> > > > On 2015/04/15 01:10:15, gunsch wrote:
> > > > > On 2015/04/15 00:05:18, xhwang wrote:
> > > > > > On 2015/04/14 23:36:13, gunsch wrote:
> > > > > > > Alright, how about this?
> > > > > > > 
> > > > > > > * MediaDrmDelegate is now its own class (became sort of mandatory
so
> I
> > > > could
> > > > > > > forward-declare it in the interface instead of having to add a
DEPS
> > > entry
> > > > > from
> > > > > > > content/ onto media/base/android/, but now that it's separated it
> > feels
> > > > > > cleaner)
> > > > > > 
> > > > > > Note that MediaDrmBridge is Android only and BrowserCdmManager
should
> be
> > > > able
> > > > > to
> > > > > > support other BrowserCdm implementations. So in theory BCM knows
> nothing
> > > > about
> > > > > > MediaDrmBridge. That makes me wonder whether we should put
> > MediaDrmBridge
> > > > > logic
> > > > > > somewhere else.... How about BrowserCdmFactoryAndroid? This is
created
> > > only
> > > > > once
> > > > > > in the browser process
> > > > > >
> > > > >
> > > >
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/browser...),
> > > > > > and is the right place to do MediaDrmBridge related logic. The only
> > issue
> > > is
> > > > > > that it's in media/base. But I think we can just move it over to
> > > > > > content/browser/media/android. It's a "Browser" CDM factory anyways.
> > WDYT?
> > > > > > 
> > > > > 
> > > > > Now we're back to square one: where is appropriate to initialize
> something
> > > for
> > > > > all content embedders that's Android-specific? I think moving it to
> > > > > content/browser/media/android is reasonable, *but* then who calls
> > > > > SetBrowserCdmFactory(BrowserCdmFactoryAndroid)? (currently in
> > > > > media/base/browser_cdm_factory.cc).
> > > > 
> > > > Can we move browser_cdm_factory.* to content/browser/media/ as well?
> > > 
> > > That would mean moving it to content/public/browser/ actually, do you
think
> > > that's appropriate? That'll add a DEPS from content/public/browser/ to
> > > media/base/ (currently isn't one, though I do see that
> > content/public/renderer/
> > > has one, so that's probably a reasonable addition).
> > > 
> > > I'd suggest moving BrowserCdm as well, but that would require uprooting
> > > MediaDrmBridge.
> > > 
> > > > 
> > > > > We could call it from BrowserCdmManager before the first
> > > > media::CreateBrowserCdm
> > > > > call, but that doesn't actually address the concern you described (not
> > > calling
> > > > > into anything Android-specific from BrowserCdmManager). It would at
> least
> > be
> > > > > lighter-weight than dealing with the MediaDrmDelegates in
> > BrowserCdmManager.
> > > > > 
> > > > > > > * BrowserCdmManager gets a vector<MediaDrmDelegate> from the
current
> > > > > > > ContentBrowserClient, then performs the registrations
> > > > > > 
> > > > > > See above about not doing this in BCM. Otherwise, using the vector
> sgtm.
> > > > > > 
> > > > > > > * MediaDrmDelegate now declares the UUID of the DRM scheme it
> > > accompanies
> > > > as
> > > > > > > part of the interface
> > > > > > 
> > > > > > So this will replace the Java path where we register the UUID? The
> java
> > > code
> > > > > > definitely looks hacky. +1 for replacing it.
> > > > > 
> > > > > Oof. No, I don't know that replacing it is in scope for this CL. We'd
> then
> > > > > _also_ have to tie in key-system strings with the delegates, and
support
> a
> > > > > multiple-to-one mapping (key systems to UUIDs) with the delegates.
> > > > 
> > > > Agreed it's not in scope for this CL. Something we should think about
> > though.
> > > > 
> > > > > I had another conversation with ddorwin@ recently about that---I think
> > > > > decoupling UUID from Key System registration was actually a mistake.
For
> > > > > example, we had a bug recently where a key system string was declared
in
> > > > > CastContentRendererClient::AddKeySystems but a UUID association not
> > > > registered.
> > > > > Unfortunately, this means it shows up as supported when you query for
> > > support,
> > > > > but fails at runtime when you try to use it (!).
> > > > 
> > > > Hmm, shouldn't you query the browser for key system support in your
> > > > CastContentRendererClient::AddKeySystems() call?
> > > > 
> > > > Something like
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren...
> > > 
> > > No, we don't. That's only applicable for Android, and right now the logic
> for
> > > key-systems registration for both Cast Linux and Cast Android is the same
> > (which
> > > is a positive I'd like to keep). Is there any reason the common key-system
> > code
> > > doesn't do that query when checking if types are supported instead of
> > expecting
> > > each embedder to do it?
> > 
> > Per our discussions so far (including over chat today), what do you think
> about
> > reworking this to the following:
> > 
> > (1) Create new class |media::MediaClientAndroid|, similar to
> > |media::MediaClient| but a) Android-only and b) lives in the browser process
> (or
> > whichever process is managing Android media playback---for now browser
process
> > is implied but won't be assumed in the code). General interface looks
> something
> > like:
> > 
> > class MediaClientAndroid {
> >   virtual std::vector<std::pair<std::string, UUID>> GetKeySystemUUIDPairs()
=
> 0;
> >   virtual media::MediaDrmDelegate* GetMediaDrmBridgeDelegate(const UUID&
> > scheme_uuid) = 0;
> > };
> > 
> > (2) Content embedder can call |SetMediaClient(new CustomMediaClientAndroid)|
> at
> > basically any point. It does the following:
> > (2a) Contains any key-system-to-UUID mappings. media_drm_bridge.cc's
> > |KeySystemUuidManager| can ask for that list from |GetMediaClientAndroid()|
> the
> > first time |KeySystemUuidManager| needs them.
> > (2b) Returns a |MediaDrmDelegate| for a given UUID when queried. (Or just
> > returns a vector of |MediaDrmDelegate| instances like I currently have in
> > content_browser_client.h this CL, and |KeySystemUuidManager| also loads
those
> > when first needed.)
> > 
> > 
> > This proposal would:
> > * be able to stay out of the content API
> > * be agnostic to which process Android's media code is running in (as long
as
> > the call site of |SetMediaClientAndroid(MediaClientAndroid*)| occurred in
the
> > same process)
> > * immediately provide a better path for the Java
> > "MediaDrmBridge.addKeySystemUuidMapping()" calls.
> > 
> > WDYT?
> > 
> > Also, the one question I would have with this proposal is, where would you
be
> > comfortable with the call site for (2)? I think we're back to adding it
> > somewhere like {Aw,Chrome,Cast}BrowserMainParts, which isn't ideal but I
think
> > is the only way to avoid circling back to the same questions we're currently
> > stuck at.
> 
> Thanks for the proposal and patience! It's more consistent with how we support
> embedders in Chromium and I like it!
> 
> Two minor comments:
> 1. This delegate is so MediaDrmBridge specific that I feel something like
> MediaDrmBridgeDelegate would be more accurate.
> 2. Chromium content clients are set during *Main* as well so *BrowserMainParts
> sounds good to me now:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...

Alright, ready for another round :)

Highlights:
* MediaDrmBridge.addKeySystemUuidMapping is now gone!
* MediaClientAndroid is introduced as an Android-only, browser-process (implied,
but not assumed in the code) client to communicate key-system/UUID mappings and
provide MediaDrmBridge delegates.
* Client is set during browser-process startup for each embedder. All changes to
content/ reverted.

[xhwang, 2015-04-24 04:52:24]

I love this CL. It's much cleaner and it fixed the UUID mapping hack! Thank you
for the patience and hard work!

The CL looks good in general. I just have some minor comments/nits.

https://codereview.chromium.org/962793005/diff/220001/android_webview/browser...
File android_webview/browser/aw_media_client_android.h (right):

https://codereview.chromium.org/962793005/diff/220001/android_webview/browser...
android_webview/browser/aw_media_client_android.h:30:
scoped_ptr<media::MediaDrmBridgeDelegate> widevine_delegate_;
nit: just store cdm::WidevineDrmDelegateAndroid widevine_delegate_?

https://codereview.chromium.org/962793005/diff/220001/chrome/browser/android/...
File chrome/browser/android/chrome_media_client_android.cc (right):

https://codereview.chromium.org/962793005/diff/220001/chrome/browser/android/...
chrome/browser/android/chrome_media_client_android.cc:10: :
widevine_delegate_(new cdm::WidevineDrmDelegateAndroid) {
As commented below, use new Foo() style.

https://codereview.chromium.org/962793005/diff/220001/chrome/browser/android/...
File chrome/browser/android/chrome_media_client_android.h (right):

https://codereview.chromium.org/962793005/diff/220001/chrome/browser/android/...
chrome/browser/android/chrome_media_client_android.h:22:
scoped_ptr<media::MediaDrmBridgeDelegate> widevine_delegate_;
nit: just store cdm::WidevineDrmDelegateAndroid widevine_delegate_?

https://codereview.chromium.org/962793005/diff/220001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main_android.cc (right):

https://codereview.chromium.org/962793005/diff/220001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main_android.cc:79:
media::SetMediaClientAndroid(new ChromeMediaClientAndroid);
nit: Here and throughout the CL, usually we use new Foo() instead of new Foo:

http://stackoverflow.com/questions/620137/do-the-parentheses-after-the-type-n...

https://codereview.chromium.org/962793005/diff/220001/chromecast/browser/cast...
File chromecast/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/962793005/diff/220001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.cc:266:
::media::SetMediaClientAndroid(new media::CastMediaClientAndroid);
ditto

https://codereview.chromium.org/962793005/diff/220001/chromecast/browser/medi...
File chromecast/browser/media/cast_media_client_android.cc (right):

https://codereview.chromium.org/962793005/diff/220001/chromecast/browser/medi...
chromecast/browser/media/cast_media_client_android.cc:29: new
cdm::WidevineDrmDelegateAndroid;
ditto about new Foo().

https://codereview.chromium.org/962793005/diff/220001/chromecast/browser/medi...
chromecast/browser/media/cast_media_client_android.cc:39:
STLDeleteContainerPairSecondPointers(delegates_.begin(), delegates_.end());
If you use ScopedPtrHashMap, then you don't need to worry about deleting here,
and hash map would be more efficient than regular std::map when using vector as
keys (I guess).

https://codereview.chromium.org/962793005/diff/220001/chromecast/media/cdm/br...
File chromecast/media/cdm/browser_cdm_cast.cc (right):

https://codereview.chromium.org/962793005/diff/220001/chromecast/media/cdm/br...
chromecast/media/cdm/browser_cdm_cast.cc:108: }
You need to rebase :)

https://codereview.chromium.org/962793005/diff/220001/chromecast/media/cdm/pl...
File chromecast/media/cdm/playready_drm_delegate_android.cc (right):

https://codereview.chromium.org/962793005/diff/220001/chromecast/media/cdm/pl...
chromecast/media/cdm/playready_drm_delegate_android.cc:21: 0xbe, 0x23, 0x0f,
0x81, 0x5a, 0x60, 0x6e, 0xb2 };
alignments are not consistent

https://codereview.chromium.org/962793005/diff/220001/chromecast/media/cdm/pl...
chromecast/media/cdm/playready_drm_delegate_android.cc:26: }  // namespace
tiny nit: no need to use anonymous namespace for consts. const variables
declared at namespace scope have internal linkage.

https://codereview.chromium.org/962793005/diff/220001/chromecast/media/cdm/pl...
chromecast/media/cdm/playready_drm_delegate_android.cc:36: kPlayreadyUuid +
arraysize(kPlayreadyUuid));
alignment is off?

https://codereview.chromium.org/962793005/diff/220001/chromecast/media/cdm/pl...
chromecast/media/cdm/playready_drm_delegate_android.cc:43: std::vector<uint8_t>&
init_data_out,
/* init_data_out */ to be clear that we don't touch init_data_out.

https://codereview.chromium.org/962793005/diff/220001/components/cdm/browser/...
File components/cdm/browser/widevine_drm_delegate_android.cc (right):

https://codereview.chromium.org/962793005/diff/220001/components/cdm/browser/...
components/cdm/browser/widevine_drm_delegate_android.cc:147: kWidevineUuid +
arraysize(kWidevineUuid));
alignment is off?

https://codereview.chromium.org/962793005/diff/220001/components/cdm/browser/...
components/cdm/browser/widevine_drm_delegate_android.cc:162: return true;
tiny nit: we like to return early, so 

if (init_data_type != media::EmeInitDataType::CENC)
  return true;

return GetPsshData(init_data, init_data_length, &init_data_out);

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
File media/base/android/media_client_android.h (right):

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_client_android.h:18: class MediaClientAndroid;
nit: order

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_client_android.h:37: virtual
std::vector<std::pair<std::string, std::vector<uint8_t>>>
How about:

using UUID = std::vector<uint8_t>;

using UUIDMapping = std::vector<std::pair<std::string, UUID>>

Actually, why this can't be a std::map or hash map?

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_client_android.h:38: GetKeySystemUUIDMappings() const;
Returning a vector of string/vector involves copy which is expensive. Can we
return a reference since all implementations have the mapping a member variable?

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_client_android.h:41: const std::vector<uint8_t>&
scheme_uuid) const;
If you follow the comment about, you can s/std::vector<unt8_t>/UUID/ here

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (left):

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:298: }
Yay! Thank you for cleaning this up!

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:92: UUID(kWidevineUuid, kWidevineUuid +
arraysize(kWidevineUuid));
hmm, can we AddMappingsFromMediaClient() here?

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:107: mappings_added = true;
If we AddMappingsFromMediaClient() in the constructor, we don't need
mappings_added here because KeySystemManager can only be constructed once.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:112: client->GetKeySystemUUIDMappings();
As mentioned above, this causes a copy of the vector.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:348:
promise->reject(INVALID_ACCESS_ERROR, 0, "Invalid data.");
nit: Invalid init data.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
File media/base/android/media_drm_bridge_delegate.cc (right):

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge_delegate.cc:21: return true;
DCHECK(init_data_out->empty())

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
File media/base/android/media_drm_bridge_delegate.h (right):

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge_delegate.h:17: // Allows embedders to modify
the Android DRM flow. Delegates are registered
s/DRM/MediaDrm

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge_delegate.h:31: // even-length list of (key,
value) pairs to send to the DRM plugin.
s/DRM plugin/MediaDrm. We only talk to Android API, where there's no concept of
a plugin.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge_delegate.h:36: int init_data_length,
nit: use std::vector<uint8_t> now.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge_delegate.h:38: std::vector<std::string>&
optional_parameters_out);
output parameters should be pointers. We do not use non-const references for
functions parameters.

[gunsch, 2015-04-27 23:44:13]

PTAL!

https://codereview.chromium.org/962793005/diff/220001/android_webview/browser...
File android_webview/browser/aw_media_client_android.h (right):

https://codereview.chromium.org/962793005/diff/220001/android_webview/browser...
android_webview/browser/aw_media_client_android.h:30:
scoped_ptr<media::MediaDrmBridgeDelegate> widevine_delegate_;
On 2015/04/24 04:52:22, xhwang wrote:
> nit: just store cdm::WidevineDrmDelegateAndroid widevine_delegate_?

Done.

https://codereview.chromium.org/962793005/diff/220001/chrome/browser/android/...
File chrome/browser/android/chrome_media_client_android.cc (right):

https://codereview.chromium.org/962793005/diff/220001/chrome/browser/android/...
chrome/browser/android/chrome_media_client_android.cc:10: :
widevine_delegate_(new cdm::WidevineDrmDelegateAndroid) {
On 2015/04/24 04:52:23, xhwang wrote:
> As commented below, use new Foo() style.

Done.

https://codereview.chromium.org/962793005/diff/220001/chrome/browser/android/...
File chrome/browser/android/chrome_media_client_android.h (right):

https://codereview.chromium.org/962793005/diff/220001/chrome/browser/android/...
chrome/browser/android/chrome_media_client_android.h:22:
scoped_ptr<media::MediaDrmBridgeDelegate> widevine_delegate_;
On 2015/04/24 04:52:23, xhwang wrote:
> nit: just store cdm::WidevineDrmDelegateAndroid widevine_delegate_?

Done.

https://codereview.chromium.org/962793005/diff/220001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main_android.cc (right):

https://codereview.chromium.org/962793005/diff/220001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main_android.cc:79:
media::SetMediaClientAndroid(new ChromeMediaClientAndroid);
On 2015/04/24 04:52:23, xhwang wrote:
> nit: Here and throughout the CL, usually we use new Foo() instead of new Foo:
> 
>
http://stackoverflow.com/questions/620137/do-the-parentheses-after-the-type-n...

Done. (TIL! Thanks for the link.)

https://codereview.chromium.org/962793005/diff/220001/chromecast/browser/medi...
File chromecast/browser/media/cast_media_client_android.cc (right):

https://codereview.chromium.org/962793005/diff/220001/chromecast/browser/medi...
chromecast/browser/media/cast_media_client_android.cc:29: new
cdm::WidevineDrmDelegateAndroid;
On 2015/04/24 04:52:23, xhwang wrote:
> ditto about new Foo().

Done.

https://codereview.chromium.org/962793005/diff/220001/chromecast/browser/medi...
chromecast/browser/media/cast_media_client_android.cc:39:
STLDeleteContainerPairSecondPointers(delegates_.begin(), delegates_.end());
On 2015/04/24 04:52:23, xhwang wrote:
> If you use ScopedPtrHashMap, then you don't need to worry about deleting here,
> and hash map would be more efficient than regular std::map when using vector
as
> keys (I guess).

Unfortunately there's no hash function for std::vector, so I can't use a
base::hash_map (and thus ScopedPtrHashMap). Definitely would have preferred to.

https://codereview.chromium.org/962793005/diff/220001/chromecast/media/cdm/pl...
File chromecast/media/cdm/playready_drm_delegate_android.cc (right):

https://codereview.chromium.org/962793005/diff/220001/chromecast/media/cdm/pl...
chromecast/media/cdm/playready_drm_delegate_android.cc:21: 0xbe, 0x23, 0x0f,
0x81, 0x5a, 0x60, 0x6e, 0xb2 };
On 2015/04/24 04:52:23, xhwang wrote:
> alignments are not consistent

Done.

https://codereview.chromium.org/962793005/diff/220001/chromecast/media/cdm/pl...
chromecast/media/cdm/playready_drm_delegate_android.cc:26: }  // namespace
On 2015/04/24 04:52:23, xhwang wrote:
> tiny nit: no need to use anonymous namespace for consts. const variables
> declared at namespace scope have internal linkage.

Done.

https://codereview.chromium.org/962793005/diff/220001/chromecast/media/cdm/pl...
chromecast/media/cdm/playready_drm_delegate_android.cc:36: kPlayreadyUuid +
arraysize(kPlayreadyUuid));
On 2015/04/24 04:52:23, xhwang wrote:
> alignment is off?

Done.

https://codereview.chromium.org/962793005/diff/220001/chromecast/media/cdm/pl...
chromecast/media/cdm/playready_drm_delegate_android.cc:43: std::vector<uint8_t>&
init_data_out,
On 2015/04/24 04:52:23, xhwang wrote:
> /* init_data_out */ to be clear that we don't touch init_data_out.

Done.

https://codereview.chromium.org/962793005/diff/220001/components/cdm/browser/...
File components/cdm/browser/widevine_drm_delegate_android.cc (right):

https://codereview.chromium.org/962793005/diff/220001/components/cdm/browser/...
components/cdm/browser/widevine_drm_delegate_android.cc:147: kWidevineUuid +
arraysize(kWidevineUuid));
On 2015/04/24 04:52:23, xhwang wrote:
> alignment is off?

Heh, I think I had done a find/replace in the CL of uint8 --> uint8_t...

https://codereview.chromium.org/962793005/diff/220001/components/cdm/browser/...
components/cdm/browser/widevine_drm_delegate_android.cc:162: return true;
On 2015/04/24 04:52:23, xhwang wrote:
> tiny nit: we like to return early, so 
> 
> if (init_data_type != media::EmeInitDataType::CENC)
>   return true;
> 
> return GetPsshData(init_data, init_data_length, &init_data_out);

Done.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
File media/base/android/media_client_android.h (right):

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_client_android.h:18: class MediaClientAndroid;
On 2015/04/24 04:52:23, xhwang wrote:
> nit: order

Done.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_client_android.h:37: virtual
std::vector<std::pair<std::string, std::vector<uint8_t>>>
On 2015/04/24 04:52:23, xhwang wrote:
> How about:
> 
> using UUID = std::vector<uint8_t>;
> 
> using UUIDMapping = std::vector<std::pair<std::string, UUID>>
> 
> Actually, why this can't be a std::map or hash map?

Done.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_client_android.h:38: GetKeySystemUUIDMappings() const;
On 2015/04/24 04:52:23, xhwang wrote:
> Returning a vector of string/vector involves copy which is expensive. Can we
> return a reference since all implementations have the mapping a member
variable?
> 

Not all implementations do, see the default MediaClientAndroid implementation.

Though, it seems like a good enough idea that I've reworked to have:
* MediaClientAndroid base class create its own map (it already needs one anyways
if it's going to return a const ref)
* Make GetKeySystemUUIDMappings non-overridable
* Provide a protected method for implementations to add mappings to the base
class

Now implementations aren't required to worry about how this value is returned.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_client_android.h:41: const std::vector<uint8_t>&
scheme_uuid) const;
On 2015/04/24 04:52:23, xhwang wrote:
> If you follow the comment about, you can s/std::vector<unt8_t>/UUID/ here

Done.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:92: UUID(kWidevineUuid, kWidevineUuid +
arraysize(kWidevineUuid));
On 2015/04/24 04:52:24, xhwang wrote:
> hmm, can we AddMappingsFromMediaClient() here?

Done.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:107: mappings_added = true;
On 2015/04/24 04:52:23, xhwang wrote:
> If we AddMappingsFromMediaClient() in the constructor, we don't need
> mappings_added here because KeySystemManager can only be constructed once.

The intention was to have this done lazily, but I forgot to account for
KeySystemManager already being constructed lazily when it's first used.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:112: client->GetKeySystemUUIDMappings();
On 2015/04/24 04:52:23, xhwang wrote:
> As mentioned above, this causes a copy of the vector.

Done.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:348:
promise->reject(INVALID_ACCESS_ERROR, 0, "Invalid data.");
On 2015/04/24 04:52:24, xhwang wrote:
> nit: Invalid init data.

Done.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
File media/base/android/media_drm_bridge_delegate.cc (right):

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge_delegate.cc:21: return true;
On 2015/04/24 04:52:24, xhwang wrote:
> DCHECK(init_data_out->empty())

Done.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
File media/base/android/media_drm_bridge_delegate.h (right):

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge_delegate.h:17: // Allows embedders to modify
the Android DRM flow. Delegates are registered
On 2015/04/24 04:52:24, xhwang wrote:
> s/DRM/MediaDrm

Done.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge_delegate.h:31: // even-length list of (key,
value) pairs to send to the DRM plugin.
On 2015/04/24 04:52:24, xhwang wrote:
> s/DRM plugin/MediaDrm. We only talk to Android API, where there's no concept
of
> a plugin.

Done.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge_delegate.h:36: int init_data_length,
On 2015/04/24 04:52:24, xhwang wrote:
> nit: use std::vector<uint8_t> now.

Done.

https://codereview.chromium.org/962793005/diff/220001/media/base/android/medi...
media/base/android/media_drm_bridge_delegate.h:38: std::vector<std::string>&
optional_parameters_out);
On 2015/04/24 04:52:24, xhwang wrote:
> output parameters should be pointers. We do not use non-const references for
> functions parameters.

Done.

[xhwang, 2015-04-28 05:25:11]

Thanks! This looks pretty good. I think we are getting very close. I just have a
few more comments/nits.

https://codereview.chromium.org/962793005/diff/220001/chromecast/browser/medi...
File chromecast/browser/media/cast_media_client_android.cc (right):

https://codereview.chromium.org/962793005/diff/220001/chromecast/browser/medi...
chromecast/browser/media/cast_media_client_android.cc:39:
STLDeleteContainerPairSecondPointers(delegates_.begin(), delegates_.end());
On 2015/04/27 23:44:12, gunsch wrote:
> On 2015/04/24 04:52:23, xhwang wrote:
> > If you use ScopedPtrHashMap, then you don't need to worry about deleting
here,
> > and hash map would be more efficient than regular std::map when using vector
> as
> > keys (I guess).
> 
> Unfortunately there's no hash function for std::vector, so I can't use a
> base::hash_map (and thus ScopedPtrHashMap). Definitely would have preferred
to.

Acknowledged.

https://codereview.chromium.org/962793005/diff/260001/android_webview/browser...
File android_webview/browser/aw_media_client_android.h (right):

https://codereview.chromium.org/962793005/diff/260001/android_webview/browser...
android_webview/browser/aw_media_client_android.h:31:
scoped_ptr<cdm::WidevineDrmDelegateAndroid> widevine_delegate_;
nit: I was saying just have:

cdm::WidevineDrmDelegateAndroid widevine_delegate_;

https://codereview.chromium.org/962793005/diff/260001/chrome/browser/android/...
File chrome/browser/android/chrome_media_client_android.h (right):

https://codereview.chromium.org/962793005/diff/260001/chrome/browser/android/...
chrome/browser/android/chrome_media_client_android.h:26:
scoped_ptr<cdm::WidevineDrmDelegateAndroid> widevine_delegate_;
ditto

https://codereview.chromium.org/962793005/diff/260001/components/cdm/browser/...
File components/cdm/browser/widevine_drm_delegate_android.cc (right):

https://codereview.chromium.org/962793005/diff/260001/components/cdm/browser/...
components/cdm/browser/widevine_drm_delegate_android.cc:67: const uint8*
box_head = cur;
uint8_t

https://codereview.chromium.org/962793005/diff/260001/components/cdm/browser/...
components/cdm/browser/widevine_drm_delegate_android.cc:73: uint32 type =
ReadUint32(cur + 4);
Now it's recommended to use *_t integer types. So in this function/file, we
should use uint32_t instead of uint32 (same for other types).

https://codereview.chromium.org/962793005/diff/260001/components/cdm/browser/...
components/cdm/browser/widevine_drm_delegate_android.cc:88: const uint8* box_end
= box_head + box_size;
ditto

https://codereview.chromium.org/962793005/diff/260001/components/cdm/browser/...
components/cdm/browser/widevine_drm_delegate_android.cc:106: uint32
version_and_flags = ReadUint32(cur);
ditto

https://codereview.chromium.org/962793005/diff/260001/components/cdm/browser/...
components/cdm/browser/widevine_drm_delegate_android.cc:123: uint32 data_size =
ReadUint32(cur);
ditto

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
File media/base/android/media_client_android.h (right):

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
media/base/android/media_client_android.h:42: // Returns a
MediaDrmBridgeDelegate that corresponds to |scheme_uuid|.
Document about the ownership of the returned object.

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
media/base/android/media_client_android.h:51: base::hash_map<std::string, UUID>
key_system_uuid_map_;
In MediaDrmBridge we have [1]:

typedef base::hash_map<std::string, UUID> KeySystemUuidMap;

Can we do it here and let MediaDrmBridge just use
MediaClientAndroid::KeySystemUuidMap?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:71: class KeySystemManager {
I wonder whether we still need this class, which looks very similar to
MediaClientAndroid. Can this be replaced by MediaClientAndroid? The only
difference I see is that we add kWidevineKeySystem here, but we can also add it
to the map of the MediaClientAndroid member map.

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
File media/base/android/media_drm_bridge_delegate.h (right):

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
media/base/android/media_drm_bridge_delegate.h:25: virtual const
std::vector<uint8_t> GetUUID() const = 0;
s/std::vector<uint8_t>/UUID

[xhwang, 2015-04-28 17:06:11]

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:71: class KeySystemManager {
On 2015/04/28 05:25:11, xhwang wrote:
> I wonder whether we still need this class, which looks very similar to
> MediaClientAndroid. Can this be replaced by MediaClientAndroid? The only
> difference I see is that we add kWidevineKeySystem here, but we can also add
it
> to the map of the MediaClientAndroid member map. 

If not, maybe we can do something like:

MediaClientAndroid::AddKeySystemsUUIDMappings(KeySystemUuidMap*
key_system_uuid_map)

So that we don't copy the map here...

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:105: base::hash_map<std::string, UUID>
mappings =
Use KeySystemUuidMap

[gunsch, 2015-04-28 18:50:51]

https://codereview.chromium.org/962793005/diff/260001/android_webview/browser...
File android_webview/browser/aw_media_client_android.h (right):

https://codereview.chromium.org/962793005/diff/260001/android_webview/browser...
android_webview/browser/aw_media_client_android.h:31:
scoped_ptr<cdm::WidevineDrmDelegateAndroid> widevine_delegate_;
On 2015/04/28 05:25:11, xhwang wrote:
> nit: I was saying just have:
> 
> cdm::WidevineDrmDelegateAndroid widevine_delegate_;

Can't do without either:
a) making GetMediaDrmBridgeDelegate non-const (|&widevine_delegate_| in a
|const| method gives me a |const cdm::WidevineDrmDelegateAndroid*|)
b) using a const_cast in the implementation

I'd rather leave the scoped_ptr than either (a) or (b), but if you feel either
of those are cleaner I'll switch.

https://codereview.chromium.org/962793005/diff/260001/chrome/browser/android/...
File chrome/browser/android/chrome_media_client_android.h (right):

https://codereview.chromium.org/962793005/diff/260001/chrome/browser/android/...
chrome/browser/android/chrome_media_client_android.h:26:
scoped_ptr<cdm::WidevineDrmDelegateAndroid> widevine_delegate_;
On 2015/04/28 05:25:11, xhwang wrote:
> ditto

Same comment.

https://codereview.chromium.org/962793005/diff/260001/components/cdm/browser/...
File components/cdm/browser/widevine_drm_delegate_android.cc (right):

https://codereview.chromium.org/962793005/diff/260001/components/cdm/browser/...
components/cdm/browser/widevine_drm_delegate_android.cc:73: uint32 type =
ReadUint32(cur + 4);
On 2015/04/28 05:25:11, xhwang wrote:
> Now it's recommended to use *_t integer types. So in this function/file, we
> should use uint32_t instead of uint32 (same for other types).

Oops, thanks for catching---I copied the new implementation after rebasing the
vector<uint8> change

https://codereview.chromium.org/962793005/diff/260001/components/cdm/browser/...
components/cdm/browser/widevine_drm_delegate_android.cc:88: const uint8* box_end
= box_head + box_size;
On 2015/04/28 05:25:11, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/962793005/diff/260001/components/cdm/browser/...
components/cdm/browser/widevine_drm_delegate_android.cc:106: uint32
version_and_flags = ReadUint32(cur);
On 2015/04/28 05:25:11, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/962793005/diff/260001/components/cdm/browser/...
components/cdm/browser/widevine_drm_delegate_android.cc:123: uint32 data_size =
ReadUint32(cur);
On 2015/04/28 05:25:11, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
File media/base/android/media_client_android.h (right):

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
media/base/android/media_client_android.h:42: // Returns a
MediaDrmBridgeDelegate that corresponds to |scheme_uuid|.
On 2015/04/28 05:25:11, xhwang wrote:
> Document about the ownership of the returned object.

Done.

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:71: class KeySystemManager {
On 2015/04/28 17:06:10, xhwang wrote:
> On 2015/04/28 05:25:11, xhwang wrote:
> > I wonder whether we still need this class, which looks very similar to
> > MediaClientAndroid. Can this be replaced by MediaClientAndroid? The only
> > difference I see is that we add kWidevineKeySystem here, but we can also add
> it
> > to the map of the MediaClientAndroid member map. 
> 
> If not, maybe we can do something like:
> 
> MediaClientAndroid::AddKeySystemsUUIDMappings(KeySystemUuidMap*
> key_system_uuid_map)
> 
> So that we don't copy the map here...

Right now, I assume that MediaClientAndroid is optional, so it can't be the
single source of truth. Changing that assumption to make MediaClientAndroid hold
the source of truth for all the mappings would _require_ every embedder to
create a MediaClientAndroid, which seems to go against the concept of it being a
"client".

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:105: base::hash_map<std::string, UUID>
mappings =
On 2015/04/28 17:06:10, xhwang wrote:
> Use KeySystemUuidMap

Done.

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
File media/base/android/media_drm_bridge_delegate.h (right):

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
media/base/android/media_drm_bridge_delegate.h:25: virtual const
std::vector<uint8_t> GetUUID() const = 0;
On 2015/04/28 05:25:11, xhwang wrote:
> s/std::vector<uint8_t>/UUID

Done.

[xhwang, 2015-04-28 19:00:38]

https://codereview.chromium.org/962793005/diff/260001/android_webview/browser...
File android_webview/browser/aw_media_client_android.h (right):

https://codereview.chromium.org/962793005/diff/260001/android_webview/browser...
android_webview/browser/aw_media_client_android.h:31:
scoped_ptr<cdm::WidevineDrmDelegateAndroid> widevine_delegate_;
On 2015/04/28 18:50:51, gunsch wrote:
> On 2015/04/28 05:25:11, xhwang wrote:
> > nit: I was saying just have:
> > 
> > cdm::WidevineDrmDelegateAndroid widevine_delegate_;
> 
> Can't do without either:
> a) making GetMediaDrmBridgeDelegate non-const (|&widevine_delegate_| in a
> |const| method gives me a |const cdm::WidevineDrmDelegateAndroid*|)
> b) using a const_cast in the implementation
> 
> I'd rather leave the scoped_ptr than either (a) or (b), but if you feel either
> of those are cleaner I'll switch.

I didn't notice the "const" part :)

Having a const method returning a non-const pointer to a class member isn't a
great pattern either. There's no fundamental differences here about what we are
really doing. So dropping the "const" in  GetMediaDrmBridgeDelegate() sgtm. But
I don't have strong opinions on this :)

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:71: class KeySystemManager {
On 2015/04/28 18:50:51, gunsch wrote:
> On 2015/04/28 17:06:10, xhwang wrote:
> > On 2015/04/28 05:25:11, xhwang wrote:
> > > I wonder whether we still need this class, which looks very similar to
> > > MediaClientAndroid. Can this be replaced by MediaClientAndroid? The only
> > > difference I see is that we add kWidevineKeySystem here, but we can also
add
> > it
> > > to the map of the MediaClientAndroid member map. 
> > 
> > If not, maybe we can do something like:
> > 
> > MediaClientAndroid::AddKeySystemsUUIDMappings(KeySystemUuidMap*
> > key_system_uuid_map)
> > 
> > So that we don't copy the map here...
> 
> Right now, I assume that MediaClientAndroid is optional, so it can't be the
> single source of truth. Changing that assumption to make MediaClientAndroid
hold
> the source of truth for all the mappings would _require_ every embedder to
> create a MediaClientAndroid, which seems to go against the concept of it being
a
> "client".

Agreed. That makes sense and is good layering.

How about my second comment to avoid copying the map?

[gunsch, 2015-04-28 19:56:32]

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:71: class KeySystemManager {
On 2015/04/28 19:00:38, xhwang wrote:
> On 2015/04/28 18:50:51, gunsch wrote:
> > On 2015/04/28 17:06:10, xhwang wrote:
> > > On 2015/04/28 05:25:11, xhwang wrote:
> > > > I wonder whether we still need this class, which looks very similar to
> > > > MediaClientAndroid. Can this be replaced by MediaClientAndroid? The only
> > > > difference I see is that we add kWidevineKeySystem here, but we can also
> add
> > > it
> > > > to the map of the MediaClientAndroid member map. 
> > > 
> > > If not, maybe we can do something like:
> > > 
> > > MediaClientAndroid::AddKeySystemsUUIDMappings(KeySystemUuidMap*
> > > key_system_uuid_map)
> > > 
> > > So that we don't copy the map here...
> > 
> > Right now, I assume that MediaClientAndroid is optional, so it can't be the
> > single source of truth. Changing that assumption to make MediaClientAndroid
> hold
> > the source of truth for all the mappings would _require_ every embedder to
> > create a MediaClientAndroid, which seems to go against the concept of it
being
> a
> > "client".
> 
> Agreed. That makes sense and is good layering.
> 
> How about my second comment to avoid copying the map?

I guess I'm unclear on the API you're asking for: is this to have
MediaClientAndroid populate key_system_uuid_map_ instead of asking it for a copy
of the map, just to avoid the transient copy? Or are you asking for a way to not
have MediaClientAndroid have a separate map from KeySystemManager? ISTM that the
first case is avoided by using a reference for GetKeySystemUUIDMappings (which I
forgot to do, fixed below). But the second one might need a slightly different
approach, such as querying MediaClientAndroid on the fly when calling GetUUID.

[xhwang, 2015-04-28 22:34:36]

On 2015/04/28 19:56:32, gunsch wrote:
>
https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
> File media/base/android/media_drm_bridge.cc (right):
> 
>
https://codereview.chromium.org/962793005/diff/260001/media/base/android/medi...
> media/base/android/media_drm_bridge.cc:71: class KeySystemManager {
> On 2015/04/28 19:00:38, xhwang wrote:
> > On 2015/04/28 18:50:51, gunsch wrote:
> > > On 2015/04/28 17:06:10, xhwang wrote:
> > > > On 2015/04/28 05:25:11, xhwang wrote:
> > > > > I wonder whether we still need this class, which looks very similar to
> > > > > MediaClientAndroid. Can this be replaced by MediaClientAndroid? The
only
> > > > > difference I see is that we add kWidevineKeySystem here, but we can
also
> > add
> > > > it
> > > > > to the map of the MediaClientAndroid member map. 
> > > > 
> > > > If not, maybe we can do something like:
> > > > 
> > > > MediaClientAndroid::AddKeySystemsUUIDMappings(KeySystemUuidMap*
> > > > key_system_uuid_map)
> > > > 
> > > > So that we don't copy the map here...
> > > 
> > > Right now, I assume that MediaClientAndroid is optional, so it can't be
the
> > > single source of truth. Changing that assumption to make
MediaClientAndroid
> > hold
> > > the source of truth for all the mappings would _require_ every embedder to
> > > create a MediaClientAndroid, which seems to go against the concept of it
> being
> > a
> > > "client".
> > 
> > Agreed. That makes sense and is good layering.
> > 
> > How about my second comment to avoid copying the map?
> 
> I guess I'm unclear on the API you're asking for: is this to have
> MediaClientAndroid populate key_system_uuid_map_ instead of asking it for a
copy
> of the map, just to avoid the transient copy? 

Pretty much yes. It's similar to the current way how we populate KeySystemInfo:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...

So the KeySystemManager will put Widevine in the map, then ask embedders to add
platform specific mappings.

> Or are you asking for a way to not
> have MediaClientAndroid have a separate map from KeySystemManager? ISTM that
the
> first case is avoided by using a reference for GetKeySystemUUIDMappings (which
I
> forgot to do, fixed below). But the second one might need a slightly different
> approach, such as querying MediaClientAndroid on the fly when calling GetUUID.

[xhwang, 2015-04-28 22:37:07]

https://codereview.chromium.org/962793005/diff/340001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/962793005/diff/340001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:107: key_system_uuid_map_[mapping.first]
= mapping.second;
You still have to copy the map over. My previous comment was trying avoid this.
Sorry for not being clear about this.

[xhwang, 2015-04-28 22:38:31]

Thanks again for fixing this and thanks for the patience :) I just had one last
comment, but it's minor.

LGTM!

[gunsch, 2015-04-29 01:14:08]

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

[gunsch, 2015-04-29 01:15:38]

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

[gunsch, 2015-04-29 01:17:51]

@xhwang: thanks for all the review! Updated the MediaClientAndroid API to allow
the implementer to avoid copying the entire map (and changed the implementations
slightly to avoid it as well).

Plus some OWNERS:
+boliu: PTAL at android_webview/. The main changes in this CL affecting AW are
a) working toward decoupling the Widevine dependencies from the MediaDrmBridge
code and b) changing how the key-system to UUID mapping is propagated (no longer
through Java).
+jochen: PTAL for components/ and chrome/browser/ OWNERS.

https://codereview.chromium.org/962793005/diff/340001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/962793005/diff/340001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:107: key_system_uuid_map_[mapping.first]
= mapping.second;
On 2015/04/28 22:37:07, xhwang wrote:
> You still have to copy the map over. My previous comment was trying avoid
this.
> Sorry for not being clear about this.

Done.

[xhwang, 2015-04-29 03:14:53]

LGTM++ Thanks for the work!

[jochen (gone - plz use gerrit), 2015-04-29 09:53:28]

lgtm

[boliu, 2015-04-29 18:35:15]

First comment before reading code: better CL description please? What does this
actually do? Makes reviewing a bit easier :)

[gunsch, 2015-04-29 18:42:10]

@boliu: good point, the CL evolved a lot since the first description and the
description did not keep up. Updated that to more thoroughly describe what's
going on here.

[boliu, 2015-04-29 19:24:25]

Mostly minor things I think.

One follow up suggestion: Looks like this paves the path lazily creating
media_client_android on first use. The less things we do during start up, the
better!

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
File android_webview/browser/aw_browser_main_parts.cc (right):

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
android_webview/browser/aw_browser_main_parts.cc:11: #include
"android_webview/common/aw_resource.h"
include not necessary?

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
android_webview/browser/aw_browser_main_parts.cc:107: new
AwMediaClientAndroid(AwResource::GetConfigKeySystemUuidMapping()));
So chrome does this in PreEarlyInitialization, and webview and chromecast does
this in PreMainMessageLoopRun.

I'm guessing it doesn't matter which one it happens in? So maybe just be
consistent across platforms?

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:26: #include
"components/cdm/browser/widevine_drm_delegate_android.h"
spurious change not needed?

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
File android_webview/browser/aw_media_client_android.cc (right):

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
android_webview/browser/aw_media_client_android.cc:28: std::string
guid(tokens[1]);
Is it safe to assume key_system_uuid_mapping can always to tokenized correctly,
or parsed as hex string correctly?

Previous java code catches any parsing exceptions. This will just crash.

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
android_webview/browser/aw_media_client_android.cc:50: (*map)[mapping.first] =
mapping.second;
map->insert?

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
android_webview/browser/aw_media_client_android.cc:59: return nullptr;
Pure question on my part since I don't actually know the whole system. So looks
like AddKeySystemUUIDMappings can potentially advertise other systems with
different UUIDs, but here there's only a delegate for the widevine one? So how
does that magically work? Was it behaving like this before this patch?

https://codereview.chromium.org/962793005/diff/380001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwResource.java
(right):

https://codereview.chromium.org/962793005/diff/380001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwResource.java:73: public
static String[] getConfigKeySystemUuidMapping() {
I think the existing pattern is pure native->java methods can be private, but
looks there's more than one that should be fixed here :)

[gunsch, 2015-04-29 20:29:44]

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
File android_webview/browser/aw_browser_main_parts.cc (right):

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
android_webview/browser/aw_browser_main_parts.cc:11: #include
"android_webview/common/aw_resource.h"
On 2015/04/29 19:24:25, boliu wrote:
> include not necessary?

Necessary for AwResource::GetConfigKeySystemUuidMapping call below

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
android_webview/browser/aw_browser_main_parts.cc:107: new
AwMediaClientAndroid(AwResource::GetConfigKeySystemUuidMapping()));
On 2015/04/29 19:24:25, boliu wrote:
> So chrome does this in PreEarlyInitialization, and webview and chromecast does
> this in PreMainMessageLoopRun.
> 
> I'm guessing it doesn't matter which one it happens in? So maybe just be
> consistent across platforms?

Sure, updated Chrome to do this in PreMainMessageLoopRun for consistency.

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:26: #include
"components/cdm/browser/widevine_drm_delegate_android.h"
On 2015/04/29 19:24:25, boliu wrote:
> spurious change not needed?

Done.

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
File android_webview/browser/aw_media_client_android.cc (right):

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
android_webview/browser/aw_media_client_android.cc:28: std::string
guid(tokens[1]);
On 2015/04/29 19:24:25, boliu wrote:
> Is it safe to assume key_system_uuid_mapping can always to tokenized
correctly,
> or parsed as hex string correctly?
> 
> Previous java code catches any parsing exceptions. This will just crash.

I was hoping you could answer that: I don't really have insight into what
content AW passes here. But since this string in built into the package itself
and not user-provided, ISTM that it's actually a bug to be fixed if anyone uses
a resource string that isn't correct.

That said, it may be better to keep the behavior to log an error and move on, so
I've adjusted the code to do that.

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
android_webview/browser/aw_media_client_android.cc:50: (*map)[mapping.first] =
mapping.second;
On 2015/04/29 19:24:25, boliu wrote:
> map->insert?

Done.

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
android_webview/browser/aw_media_client_android.cc:59: return nullptr;
On 2015/04/29 19:24:25, boliu wrote:
> Pure question on my part since I don't actually know the whole system. So
looks
> like AddKeySystemUUIDMappings can potentially advertise other systems with
> different UUIDs, but here there's only a delegate for the widevine one? So how
> does that magically work? Was it behaving like this before this patch?

This patch shouldn't change any user-facing behavior from before.

KeySystemUUIDMappings is used to make sure all key systems are still registered.

Delegates are specifically to add additional functionality. For example, the
Widevine delegate's primary purpose is to strip the PSSH box and pass only the
license data itself to the  MediaDrm instance.

https://codereview.chromium.org/962793005/diff/380001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwResource.java
(right):

https://codereview.chromium.org/962793005/diff/380001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwResource.java:73: public
static String[] getConfigKeySystemUuidMapping() {
On 2015/04/29 19:24:25, boliu wrote:
> I think the existing pattern is pure native->java methods can be private, but
> looks there's more than one that should be fixed here :)

Done.

[boliu, 2015-04-29 23:15:44]

lgtm % last suggestion

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
File android_webview/browser/aw_media_client_android.cc (right):

https://codereview.chromium.org/962793005/diff/380001/android_webview/browser...
android_webview/browser/aw_media_client_android.cc:28: std::string
guid(tokens[1]);
On 2015/04/29 20:29:43, gunsch wrote:
> On 2015/04/29 19:24:25, boliu wrote:
> > Is it safe to assume key_system_uuid_mapping can always to tokenized
> correctly,
> > or parsed as hex string correctly?
> > 
> > Previous java code catches any parsing exceptions. This will just crash.
> 
> I was hoping you could answer that: I don't really have insight into what
> content AW passes here. But since this string in built into the package itself
> and not user-provided, ISTM that it's actually a bug to be fixed if anyone
uses
> a resource string that isn't correct.

It comes from a resource that individual devices can then override/add to. Eg
apparently nexus player supports microsoft playready:
https://android.googlesource.com/device/asus/fugu/+/lollipop-release/overlay/...

> 
> That said, it may be better to keep the behavior to log an error and move on,
so
> I've adjusted the code to do that.

Yeah, better not crash first.

https://codereview.chromium.org/962793005/diff/400001/android_webview/browser...
File android_webview/browser/aw_media_client_android.cc (right):

https://codereview.chromium.org/962793005/diff/400001/android_webview/browser...
android_webview/browser/aw_media_client_android.cc:61:
map->insert(std::make_pair(mapping.first, mapping.second));
|mapping| is already a pair, no?

And probably should use KeySystemUuidMap::value_type instead of using
std::pair<...> everywhere.

[gunsch, 2015-04-29 23:32:16]

https://codereview.chromium.org/962793005/diff/400001/android_webview/browser...
File android_webview/browser/aw_media_client_android.cc (right):

https://codereview.chromium.org/962793005/diff/400001/android_webview/browser...
android_webview/browser/aw_media_client_android.cc:61:
map->insert(std::make_pair(mapping.first, mapping.second));
On 2015/04/29 23:15:44, boliu wrote:
> |mapping| is already a pair, no?
> 
> And probably should use KeySystemUuidMap::value_type instead of using
> std::pair<...> everywhere.

Done! Guess I learn something new about C++ every week

[gunsch, 2015-04-29 23:32:44]

The CQ bit was checked by gunsch@chromium.org to run a CQ dry run

[gunsch, 2015-04-29 23:32:45]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
xhwang@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/962793005/#ps420001
(title: "use value_type where possible")

[commit-bot: I haz the power, 2015-04-29 23:33:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/962793005/420001

[commit-bot: I haz the power, 2015-04-29 23:38:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-29 23:38:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gunsch, 2015-04-29 23:41:06]

The CQ bit was checked by gunsch@chromium.org to run a CQ dry run

[gunsch, 2015-04-29 23:41:07]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
xhwang@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/962793005/#ps440001
(title: "rebased")

[commit-bot: I haz the power, 2015-04-29 23:41:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/962793005/440001

[commit-bot: I haz the power, 2015-04-30 00:27:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-30 00:27:34]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[gunsch, 2015-04-30 01:26:24]

The CQ bit was checked by gunsch@chromium.org to run a CQ dry run

[gunsch, 2015-04-30 01:26:24]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
xhwang@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/962793005/#ps460001
(title: "rename createSession to createSessionFromNative to avoid bug in
FindBugs")

[commit-bot: I haz the power, 2015-04-30 01:26:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/962793005/460001

[commit-bot: I haz the power, 2015-04-30 02:47:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-30 02:47:59]

Dry run: This issue passed the CQ dry run.

[gunsch, 2015-04-30 03:57:26]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2015-04-30 03:57:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/962793005/460001

[commit-bot: I haz the power, 2015-04-30 03:58:52]

Committed patchset #24 (id:460001)

[commit-bot: I haz the power, 2015-04-30 03:59:42]

Patchset 24 (id:??) landed as
https://crrev.com/2288c539cc227ec262be28c4b684ff38527f7768
Cr-Commit-Position: refs/heads/master@{#327650}