Implement out-of-band video compositing on Android: Step 1

Implement out-of-band video compositing on Android: Step 1

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

[qinmin, 2013-03-05 15:11:53]

You probably need to change the logic in webmediaplayer_manager_android. By
default, we allow multiple embedded video. Bur with surfaceview, this is no
longer true. You need to pause all players that hasVideo() is true when start
playing a new video.

[no sievers, 2013-03-05 19:54:16]

What branch is this, where am I? :)

https://codereview.chromium.org/12443003/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/12443003/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.h:209: jobject jsurface);
Consider removing the unused arg 'jsurface' here.

https://codereview.chromium.org/12443003/diff/1/content/browser/android/media...
File content/browser/android/media_player_manager_android.cc (right):

https://codereview.chromium.org/12443003/diff/1/content/browser/android/media...
content/browser/android/media_player_manager_android.cc:201: if
(external_video_surface_user_id_ != -1)
nit: remove 'if()'

https://codereview.chromium.org/12443003/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/12443003/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:117:
mExternalVideoSurfaceView = new SurfaceView(getContext());
Not sure you always want to create the SurfaceView and add it to the view here.

[wonsik, 2013-03-06 02:45:43]

On 2013/03/05 19:54:16, Daniel Sievers wrote:
> What branch is this, where am I? :)
> 

I'm not quite sure what you mean here? I thought I was working on the master
branch.. :)

[wonsik, 2013-03-06 02:48:00]

On 2013/03/05 15:11:53, qinmin wrote:
> You probably need to change the logic in webmediaplayer_manager_android. By
> default, we allow multiple embedded video. Bur with surfaceview, this is no
> longer true. You need to pause all players that hasVideo() is true when start
> playing a new video.

I think the best-effort approach would be setting NULL surface to the media
players that are required to use external surfaces when other player also wants
the external surface. I updated
content/browser/android/media_player_manager_android.cc with the idea. Please
take another look. Thanks!

[wonsik, 2013-03-06 02:48:20]

https://codereview.chromium.org/12443003/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/12443003/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.h:209: jobject jsurface);
On 2013/03/05 19:54:16, Daniel Sievers wrote:
> Consider removing the unused arg 'jsurface' here.

Done.

https://codereview.chromium.org/12443003/diff/1/content/browser/android/media...
File content/browser/android/media_player_manager_android.cc (right):

https://codereview.chromium.org/12443003/diff/1/content/browser/android/media...
content/browser/android/media_player_manager_android.cc:201: if
(external_video_surface_user_id_ != -1)
On 2013/03/05 19:54:16, Daniel Sievers wrote:
> nit: remove 'if()'

Done.

https://codereview.chromium.org/12443003/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/12443003/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:117:
mExternalVideoSurfaceView = new SurfaceView(getContext());
On 2013/03/05 19:54:16, Daniel Sievers wrote:
> Not sure you always want to create the SurfaceView and add it to the view
here.

To create SurfaceView only when needed, ContentViewCore should communicate with
ContentView when the media player requests external surface. One way to do it is
using ContentViewCore.InternalAccessDelegate, but it looks awkward to me since
the delegate currently deals with View methods only. Do you think it would make
sense if I proceed with extending InternalAccessDelegate?

[scherkus (not reviewing), 2013-03-07 00:09:13]

https://codereview.chromium.org/12443003/diff/28001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/12443003/diff/28001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:1207: DCHECK(rvhi);
DCHECKs aren't useful when you immediately dereference them

https://codereview.chromium.org/12443003/diff/28001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:1216: DCHECK(rvhi);
ditto

https://codereview.chromium.org/12443003/diff/28001/media/base/media_switches.cc
File media/base/media_switches.cc (right):

https://codereview.chromium.org/12443003/diff/28001/media/base/media_switches...
media/base/media_switches.cc:73: "always-use-external-video-surface";
nit: remove "Always" prefix (it's more common to have switches that simply start
with "Use")

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:446: void
WebMediaPlayerAndroid::SetNeedsExternalSurface(
do you really need this method? it looks like you could inline it inside the
constructor

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_in_process_android.h (right):

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_in_process_android.h:55: class
WebMediaPlayerInProcessAndroid : public WebMediaPlayerAndroid {
qinmin: any ETA when can we delete this code?

[no sievers, 2013-03-07 00:16:16]

On 2013/03/06 02:45:43, wonsik wrote:
> On 2013/03/05 19:54:16, Daniel Sievers wrote:
> > What branch is this, where am I? :)
> > 
> 
> I'm not quite sure what you mean here? I thought I was working on the master
> branch.. :)

The base URL on the left is weird. For other code reviews it says
svn://svn.chromium.org/chrome/trunk/src

[qinmin, 2013-03-07 00:22:18]

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_in_process_android.h (right):

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_in_process_android.h:55: class
WebMediaPlayerInProcessAndroid : public WebMediaPlayerAndroid {
On 2013/03/07 00:09:13, scherkus wrote:
> qinmin: any ETA when can we delete this code?

hmm... I want to keep it for DRT tests. Sievers is working on the in-process
graphicsContext stuff for this. So if that works, and we can run DRT tests with
image diff on

[scherkus (not reviewing), 2013-03-07 00:28:56]

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_in_process_android.h (right):

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_in_process_android.h:55: class
WebMediaPlayerInProcessAndroid : public WebMediaPlayerAndroid {
On 2013/03/07 00:22:19, qinmin wrote:
> On 2013/03/07 00:09:13, scherkus wrote:
> > qinmin: any ETA when can we delete this code?
> 
> hmm... I want to keep it for DRT tests. Sievers is working on the in-process
> graphicsContext stuff for this. So if that works, and we can run DRT tests
with
> image diff on

Will using content_shell for layout tests change the situation?

If this is a more involved Q&A we can chat about it offline / outside of this CL
:)

[qinmin, 2013-03-07 00:31:55]

On 2013/03/07 00:28:56, scherkus wrote:
>
https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
> File webkit/media/android/webmediaplayer_in_process_android.h (right):
> 
>
https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
> webkit/media/android/webmediaplayer_in_process_android.h:55: class
> WebMediaPlayerInProcessAndroid : public WebMediaPlayerAndroid {
> On 2013/03/07 00:22:19, qinmin wrote:
> > On 2013/03/07 00:09:13, scherkus wrote:
> > > qinmin: any ETA when can we delete this code?
> > 
> > hmm... I want to keep it for DRT tests. Sievers is working on the in-process
> > graphicsContext stuff for this. So if that works, and we can run DRT tests
> with
> > image diff on
> 
> Will using content_shell for layout tests change the situation?
> 
> If this is a more involved Q&A we can chat about it offline / outside of this
CL
> :)

Ya, if we use content_shell, then we don't need the in_process player,

[no sievers, 2013-03-07 00:32:02]

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_in_process_android.h (right):

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_in_process_android.h:55: class
WebMediaPlayerInProcessAndroid : public WebMediaPlayerAndroid {
On 2013/03/07 00:28:56, scherkus wrote:
> On 2013/03/07 00:22:19, qinmin wrote:
> > On 2013/03/07 00:09:13, scherkus wrote:
> > > qinmin: any ETA when can we delete this code?
> > 
> > hmm... I want to keep it for DRT tests. Sievers is working on the in-process
> > graphicsContext stuff for this. So if that works, and we can run DRT tests
> with
> > image diff on
> 
> Will using content_shell for layout tests change the situation?
> 
> If this is a more involved Q&A we can chat about it offline / outside of this
CL
> :)

Yes, I'd be a fan of running layout tests with the same architecture and
codepath as is normally used. 
I don't think it's worth keeping a separate media player implementation just for
tests. At some point you have to wonder if you are testing the right thing.

[scherkus (not reviewing), 2013-03-07 00:38:50]

On 2013/03/07 00:32:02, Daniel Sievers wrote:
>
https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
> File webkit/media/android/webmediaplayer_in_process_android.h (right):
> 
>
https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
> webkit/media/android/webmediaplayer_in_process_android.h:55: class
> WebMediaPlayerInProcessAndroid : public WebMediaPlayerAndroid {
> On 2013/03/07 00:28:56, scherkus wrote:
> > On 2013/03/07 00:22:19, qinmin wrote:
> > > On 2013/03/07 00:09:13, scherkus wrote:
> > > > qinmin: any ETA when can we delete this code?
> > > 
> > > hmm... I want to keep it for DRT tests. Sievers is working on the
in-process
> > > graphicsContext stuff for this. So if that works, and we can run DRT tests
> > with
> > > image diff on
> > 
> > Will using content_shell for layout tests change the situation?
> > 
> > If this is a more involved Q&A we can chat about it offline / outside of
this
> CL
> > :)
> 
> Yes, I'd be a fan of running layout tests with the same architecture and
> codepath as is normally used. 
> I don't think it's worth keeping a separate media player implementation just
for
> tests. At some point you have to wonder if you are testing the right thing.

I wholeheartedly agree.

[palmer, 2013-03-07 01:24:55]

IPC security LGTM.

[wonsik, 2013-03-07 02:54:08]

PTAL

https://codereview.chromium.org/12443003/diff/28001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/12443003/diff/28001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:1207: DCHECK(rvhi);
On 2013/03/07 00:09:13, scherkus wrote:
> DCHECKs aren't useful when you immediately dereference them

Removed & added the condition in if clause.

https://codereview.chromium.org/12443003/diff/28001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:1216: DCHECK(rvhi);
On 2013/03/07 00:09:13, scherkus wrote:
> ditto

Removed & added the condition in if clause.

https://codereview.chromium.org/12443003/diff/28001/media/base/media_switches.cc
File media/base/media_switches.cc (right):

https://codereview.chromium.org/12443003/diff/28001/media/base/media_switches...
media/base/media_switches.cc:73: "always-use-external-video-surface";
On 2013/03/07 00:09:13, scherkus wrote:
> nit: remove "Always" prefix (it's more common to have switches that simply
start
> with "Use")

Done.

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:446: void
WebMediaPlayerAndroid::SetNeedsExternalSurface(
On 2013/03/07 00:09:13, scherkus wrote:
> do you really need this method? it looks like you could inline it inside the
> constructor

I expect to have more criteria for using external surface to come (for example,
CDM protected media), and those conditions may not be detectable at the
constructor.

[qinmin, 2013-03-07 18:47:24]

lgtm

On 2013/03/07 02:54:08, wonsik wrote:
> PTAL
> 
>
https://codereview.chromium.org/12443003/diff/28001/content/browser/android/c...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
https://codereview.chromium.org/12443003/diff/28001/content/browser/android/c...
> content/browser/android/content_view_core_impl.cc:1207: DCHECK(rvhi);
> On 2013/03/07 00:09:13, scherkus wrote:
> > DCHECKs aren't useful when you immediately dereference them
> 
> Removed & added the condition in if clause.
> 
>
https://codereview.chromium.org/12443003/diff/28001/content/browser/android/c...
> content/browser/android/content_view_core_impl.cc:1216: DCHECK(rvhi);
> On 2013/03/07 00:09:13, scherkus wrote:
> > ditto
> 
> Removed & added the condition in if clause.
> 
>
https://codereview.chromium.org/12443003/diff/28001/media/base/media_switches.cc
> File media/base/media_switches.cc (right):
> 
>
https://codereview.chromium.org/12443003/diff/28001/media/base/media_switches...
> media/base/media_switches.cc:73: "always-use-external-video-surface";
> On 2013/03/07 00:09:13, scherkus wrote:
> > nit: remove "Always" prefix (it's more common to have switches that simply
> start
> > with "Use")
> 
> Done.
> 
>
https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
> File webkit/media/android/webmediaplayer_android.cc (right):
> 
>
https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
> webkit/media/android/webmediaplayer_android.cc:446: void
> WebMediaPlayerAndroid::SetNeedsExternalSurface(
> On 2013/03/07 00:09:13, scherkus wrote:
> > do you really need this method? it looks like you could inline it inside the
> > constructor
> 
> I expect to have more criteria for using external surface to come (for
example,
> CDM protected media), and those conditions may not be detectable at the
> constructor.

[scherkus (not reviewing), 2013-03-07 18:59:10]

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:446: void
WebMediaPlayerAndroid::SetNeedsExternalSurface(
On 2013/03/07 02:54:08, wonsik wrote:
> On 2013/03/07 00:09:13, scherkus wrote:
> > do you really need this method? it looks like you could inline it inside the
> > constructor
> 
> I expect to have more criteria for using external surface to come (for
example,
> CDM protected media), and those conditions may not be detectable at the
> constructor.

Unless you have a CL ready to review that uses this method, I'd rather have you
introduce this method when you actually need it.

Not only is it hard to predict the future, but it also makes it easier for the
code reviewer to understand the change when you introduce the function along
with the code that uses it.

https://codereview.chromium.org/12443003/diff/38001/media/base/media_switches.cc
File media/base/media_switches.cc (right):

https://codereview.chromium.org/12443003/diff/38001/media/base/media_switches...
media/base/media_switches.cc:72: const char kUseExternalVideoSurface[] =
"use-external-video-surface";
sorry for not spotting this earlier, but this switch should be defined closer to
where it's actually used (nothing inside src/media/ inspects this switch).

can you create webkit/media/media_switches.{cc,h} and place the switch there?

[no sievers, 2013-03-07 19:14:07]

https://codereview.chromium.org/12443003/diff/38001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/12443003/diff/38001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:138:
mExternalVideoSurfaceView.setZOrderMediaOverlay(false);
I think this should all move to the app/embedder. Then you don't need a hook
back and forth either. What do you think?
(i.e. simply remove the changes to this file from the patch)

Also you might want to only create one SurfaceView for video (and not one per
tab), and move/resize it as needed when switching tabs.

In that case, I think renaming externalVideoSurfaceCreated/Destroyed() to
attach/detachExternalVideoSurface() might also be more fitting as in that it
expressed that you are redirecting the output.

[wonsik, 2013-03-11 12:32:44]

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/12443003/diff/28001/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:446: void
WebMediaPlayerAndroid::SetNeedsExternalSurface(
On 2013/03/07 18:59:10, scherkus wrote:
> On 2013/03/07 02:54:08, wonsik wrote:
> > On 2013/03/07 00:09:13, scherkus wrote:
> > > do you really need this method? it looks like you could inline it inside
the
> > > constructor
> > 
> > I expect to have more criteria for using external surface to come (for
> example,
> > CDM protected media), and those conditions may not be detectable at the
> > constructor.
> 
> Unless you have a CL ready to review that uses this method, I'd rather have
you
> introduce this method when you actually need it.
> 
> Not only is it hard to predict the future, but it also makes it easier for the
> code reviewer to understand the change when you introduce the function along
> with the code that uses it.

Got it. Done.

https://codereview.chromium.org/12443003/diff/38001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/12443003/diff/38001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:138:
mExternalVideoSurfaceView.setZOrderMediaOverlay(false);
On 2013/03/07 19:14:07, Daniel Sievers wrote:
> I think this should all move to the app/embedder. Then you don't need a hook
> back and forth either. What do you think?
> (i.e. simply remove the changes to this file from the patch)

Could you point me to a starting point to investigate? I think I need some hint
here...

> 
> Also you might want to only create one SurfaceView for video (and not one per
> tab), and move/resize it as needed when switching tabs.
> 
> In that case, I think renaming externalVideoSurfaceCreated/Destroyed() to
> attach/detachExternalVideoSurface() might also be more fitting as in that it
> expressed that you are redirecting the output.

https://codereview.chromium.org/12443003/diff/38001/media/base/media_switches.cc
File media/base/media_switches.cc (right):

https://codereview.chromium.org/12443003/diff/38001/media/base/media_switches...
media/base/media_switches.cc:72: const char kUseExternalVideoSurface[] =
"use-external-video-surface";
On 2013/03/07 18:59:11, scherkus wrote:
> sorry for not spotting this earlier, but this switch should be defined closer
to
> where it's actually used (nothing inside src/media/ inspects this switch).
> 
> can you create webkit/media/media_switches.{cc,h} and place the switch there?

Done.

[no sievers, 2013-03-11 17:26:27]

https://codereview.chromium.org/12443003/diff/38001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/12443003/diff/38001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:138:
mExternalVideoSurfaceView.setZOrderMediaOverlay(false);
On 2013/03/11 12:32:44, wonsik wrote:
> On 2013/03/07 19:14:07, Daniel Sievers wrote:
> > I think this should all move to the app/embedder. Then you don't need a hook
> > back and forth either. What do you think?
> > (i.e. simply remove the changes to this file from the patch)
> 
> Could you point me to a starting point to investigate? I think I need some
hint
> here...
> 

I mean this code to create the external SurfaceView does not have to be in
content/, just like the main SurfaceView is not getting created from here.
The application can create the external video surface (on demand) and then use
the ChromeView API to redirect the video output to that.

> > 
> > Also you might want to only create one SurfaceView for video (and not one
per
> > tab), and move/resize it as needed when switching tabs.
> > 
> > In that case, I think renaming externalVideoSurfaceCreated/Destroyed() to
> > attach/detachExternalVideoSurface() might also be more fitting as in that it
> > expressed that you are redirecting the output.
>

[scherkus (not reviewing), 2013-03-11 20:31:00]

lgtm

thanks!

[wonsik, 2013-03-14 06:28:14]

PTAL

https://codereview.chromium.org/12443003/diff/38001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/12443003/diff/38001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:138:
mExternalVideoSurfaceView.setZOrderMediaOverlay(false);
On 2013/03/11 17:26:28, Daniel Sievers wrote:
> On 2013/03/11 12:32:44, wonsik wrote:
> > On 2013/03/07 19:14:07, Daniel Sievers wrote:
> > > I think this should all move to the app/embedder. Then you don't need a
hook
> > > back and forth either. What do you think?
> > > (i.e. simply remove the changes to this file from the patch)
> > 
> > Could you point me to a starting point to investigate? I think I need some
> hint
> > here...
> > 
> 
> I mean this code to create the external SurfaceView does not have to be in
> content/, just like the main SurfaceView is not getting created from here.
> The application can create the external video surface (on demand) and then use
> the ChromeView API to redirect the video output to that.
> 
> > > 
> > > Also you might want to only create one SurfaceView for video (and not one
> per
> > > tab), and move/resize it as needed when switching tabs.
> > > 
> > > In that case, I think renaming externalVideoSurfaceCreated/Destroyed() to
> > > attach/detachExternalVideoSurface() might also be more fitting as in that
it
> > > expressed that you are redirecting the output.
> > 
> 

Created ExternalVideoSurfaceViewHolder to be injected from app or embedder.
Please review my approach and let me know if I can do better. Thanks!

[ycheo, 2013-03-14 09:45:52]

https://codereview.chromium.org/12443003/diff/66012/content/browser/android/m...
File content/browser/android/media_player_manager_android.h (right):

https://codereview.chromium.org/12443003/diff/66012/content/browser/android/m...
content/browser/android/media_player_manager_android.h:96: int
external_video_surface_user_id_;
IMO, it would be better that this variable is managed in
ExternalVideoSurfaceViewHolder and you can add some method like isOwner() to
check if it is the current user.

https://codereview.chromium.org/12443003/diff/66012/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/12443003/diff/66012/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:405: if
(!natural_size_.isEmpty())
For multi-lines, you need a bracket.

https://codereview.chromium.org/12443003/diff/66012/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/12443003/diff/66012/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.h:185: void
SetNeedsExternalSurface(bool needs_external_surface);
Do you need this api?

[wonsik, 2013-03-14 10:57:01]

PTAL

https://codereview.chromium.org/12443003/diff/66012/content/browser/android/m...
File content/browser/android/media_player_manager_android.h (right):

https://codereview.chromium.org/12443003/diff/66012/content/browser/android/m...
content/browser/android/media_player_manager_android.h:96: int
external_video_surface_user_id_;
On 2013/03/14 09:45:52, ycheo wrote:
> IMO, it would be better that this variable is managed in
> ExternalVideoSurfaceViewHolder and you can add some method like isOwner() to
> check if it is the current user.

Done.

https://codereview.chromium.org/12443003/diff/66012/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/12443003/diff/66012/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.cc:405: if
(!natural_size_.isEmpty())
On 2013/03/14 09:45:52, ycheo wrote:
> For multi-lines, you need a bracket.

Done.

https://codereview.chromium.org/12443003/diff/66012/webkit/media/android/webm...
File webkit/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/12443003/diff/66012/webkit/media/android/webm...
webkit/media/android/webmediaplayer_android.h:185: void
SetNeedsExternalSurface(bool needs_external_surface);
On 2013/03/14 09:45:52, ycheo wrote:
> Do you need this api?
Removed.

[ycheo, 2013-03-14 12:11:29]

lgtm

[wonsik, 2013-03-18 05:15:06]

Gentle reminder --- PTAL.

[scherkus (not reviewing), 2013-03-18 17:11:17]

On 2013/03/18 05:15:06, wonsik wrote:
> Gentle reminder --- PTAL.

wonsik: can you clarify who should be looking at the new code?

[wonsik, 2013-03-19 01:39:01]

On 2013/03/18 17:11:17, scherkus wrote:
> On 2013/03/18 05:15:06, wonsik wrote:
> > Gentle reminder --- PTAL.
> 
> wonsik: can you clarify who should be looking at the new code?

I just had a brief chat with Daniel, and will modify the CL according to the
feedback I got from him.

[wonsik, 2013-03-20 12:36:44]

PTAL

sievers: content/browser/android/* ;
content/browser/renderer_host/render_process_host_impl.cc
yfriedman: content/public/android/*
sky: content/browser/web_contents/*

[sky, 2013-03-20 14:39:08]

web_contents LGTM

[Yaron, 2013-03-21 00:48:52]

That's a lot of layers! content/public lgtm

[no sievers, 2013-03-21 01:16:52]

lgtm

[commit-bot: I haz the power, 2013-03-21 01:28:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wonsik@chromium.org/12443003/87001

[commit-bot: I haz the power, 2013-03-21 01:28:40]

Failed to apply patch for
content/browser/android/media_player_manager_android.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/android/media_player_manager_android.cc
  Hunk #1 FAILED at 6.
  Hunk #2 succeeded at 24 (offset 1 line).
  Hunk #3 succeeded at 44 (offset 1 line).
  Hunk #4 succeeded at 202 (offset 4 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
content/browser/android/media_player_manager_android.cc.rej

Patch:       content/browser/android/media_player_manager_android.cc
Index: content/browser/android/media_player_manager_android.cc
diff --git a/content/browser/android/media_player_manager_android.cc
b/content/browser/android/media_player_manager_android.cc
index
6277ecdc586b36a7650b08963be00bae7dd99201..ebc358950eb4195aad83ba8115d6b7b05d48dddc
100644
--- a/content/browser/android/media_player_manager_android.cc
+++ b/content/browser/android/media_player_manager_android.cc
@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "content/browser/android/cookie_getter_impl.h"
+#include "content/browser/web_contents/web_contents_view_android.h"
 #include "content/common/media/media_player_messages.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/render_process_host.h"
@@ -23,7 +24,8 @@ MediaPlayerManagerAndroid::MediaPlayerManagerAndroid(
     RenderViewHost* render_view_host)
     : RenderViewHostObserver(render_view_host),
       ALLOW_THIS_IN_INITIALIZER_LIST(video_view_(this)),
-      fullscreen_player_id_(-1) {
+      fullscreen_player_id_(-1),
+      web_contents_(WebContents::FromRenderViewHost(render_view_host)) {
 }
 
 MediaPlayerManagerAndroid::~MediaPlayerManagerAndroid() {}
@@ -42,6 +44,8 @@ bool MediaPlayerManagerAndroid::OnMessageReceived(const
IPC::Message& msg) {
     IPC_MESSAGE_HANDLER(MediaPlayerHostMsg_DestroyMediaPlayer, OnDestroyPlayer)
     IPC_MESSAGE_HANDLER(MediaPlayerHostMsg_DestroyAllMediaPlayers,
                         DestroyAllMediaPlayers)
+    IPC_MESSAGE_HANDLER(MediaPlayerHostMsg_RequestExternalSurface,
+                        OnRequestExternalSurface)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -195,6 +199,28 @@ void MediaPlayerManagerAndroid::DestroyAllMediaPlayers() {
   }
 }
 
+void MediaPlayerManagerAndroid::AttachExternalVideoSurface(int player_id,
+                                                           jobject surface) {
+  MediaPlayerBridge* player = GetPlayer(player_id);
+  if (player)
+    player->SetVideoSurface(surface);
+}
+
+void MediaPlayerManagerAndroid::DetachExternalVideoSurface(int player_id) {
+  MediaPlayerBridge* player = GetPlayer(player_id);
+  if (player)
+    player->SetVideoSurface(NULL);
+}
+
+void MediaPlayerManagerAndroid::OnRequestExternalSurface(int player_id) {
+  if (web_contents_) {
+    WebContentsViewAndroid* view =
+        static_cast<WebContentsViewAndroid*>(web_contents_->GetView());
+    if (view)
+      view->RequestExternalVideoSurface(player_id);
+  }
+}
+
 MediaPlayerBridge* MediaPlayerManagerAndroid::GetPlayer(int player_id) {
   for (ScopedVector<MediaPlayerBridge>::iterator it = players_.begin();
       it != players_.end(); ++it) {

[commit-bot: I haz the power, 2013-03-21 02:03:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wonsik@chromium.org/12443003/86002

[commit-bot: I haz the power, 2013-03-21 06:42:03]

Change committed as 189537