Enable SurfaceView fullscreen video on Android with WebMediaPlayerImpl

Enable Android SurfaceView fullscreen video with WMPI

This CL enables SurfaceView fullscreen video with the unified media pipeline
in cases where we use the AVDA deferred rendering strategy today.

* There's a new media interface called SurfaceManager with two methods:
  CreateFullscreenSurface() and NaturalSizeChanged().
* SurfaceManager is implemented by RendererSurfaceViewManager, which is a
  RenderFrameObserver, and BrowserSurfaceViewManager, which is created for
  each RenderFrame by MediaWebContentsObserverAndroid.
* BrowserSurfaceViewManager creates a ContentVideoView in response to a
  CreateFullscreenSurface message, and registers it in a surface map that the
  decoder can look up in the GPU process.
* WMPI interacts with SurfaceManager on behalf of the decoder. It passes
  GpuVideoDecoder a callback for requesting surfaces which it calls before
  initializing VDAs.
* In response to the callback, if the player is in fullscreen WMPI will pass
  the request to the SurfaceManager which will return the surface id to GVD. If
  the player is not in fullscreen WMPI will return a null surface id.
* When GVD gets a surface id back it completes its initialization by calling
  VDA::Initialize with the surface id and the VDA will render to the given
  surface.
* WMPI records that it got a surface request callback so that it knows that
  future fullscreen transitions will require a restart of the decoder in order
  to switch surfaces.
* A future CL will allow us to switch the surface dynamically without
  requiring a restart.

BUG=533630
Committed: https://crrev.com/dee516f91d460baf825f25503063a898ec3aaf3a
Cr-Commit-Position: refs/heads/master@{#376068}

[watk, 2016-02-02 00:13:10]

watk@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[watk, 2016-02-02 00:13:11]

Dale, could you please take a high level look at this? I'm hoping to get some
early feedback in parallel with continuing to clean it up.

[DaleCurtis, 2016-02-02 00:22:10]

Before I take a look can you add a description which describes what the flow of
what you're trying to accomplish here?

[watk, 2016-02-02 00:41:42]

Description was changed from

==========
Enable SurfaceView fullscreen video on Android with WebMediaPlayerImpl

BUG=
==========

to

==========
Enable SurfaceView fullscreen video on Android with WebMediaPlayerImpl

* Introduces a new media interface called SurfaceManager with a single method
for now: CreateFullscreenSurface.
* SurfaceManager is implemented by RendererSurfaceViewManager which is a
RenderFrameObserver, and BrowserSurfaceViewManager which is created for each
RenderFrame by MediaWebContentsObserverAndroid.
* WMPI has a pointer to a SurfaceManager.
* WMPI gives GVD a new callback called RequestSurfaceCB which it calls before
initializing VDAs. If the player is in fullscreen, WMPI will call
SurfaceManager::CreateFullscreenSurface and the surface id will be returned to
GVD. If the player is not in fullscreen WMPI will return a null surface id.
* When GVD gets a surface id in response to this callback it completes
initialization by calling Initialize on the VDA, passing the surface id to it
through the config.
* As a side effect, WMPI records that it got a surface request callback so that
it knows on future fullscreen transitions to restart the renderer. This is the
only mode for now. In the future we can implement a system in which the decoder
gets a choice about whether to be restarted or not.
* The BrowserSurfaceManager creates a ContentVideoView in response to a
CreateFullscreenSurface call, and registers it to be looked up by the GPU
process.
==========

[watk, 2016-02-02 00:44:28]

On 2016/02/02 00:22:10, DaleCurtis wrote:
> Before I take a look can you add a description which describes what the flow
of
> what you're trying to accomplish here?

Tried to clarify a bit in the description. I would say the most interesting
files are GVD, WMPI, and browser_surface_view_manager. The rest is mostly
plumbing.

[DaleCurtis, 2016-02-02 01:01:33]

https://codereview.chromium.org/1655083002/diff/1/content/browser/media/andro...
File content/browser/media/android/browser_surface_view_manager.cc (right):

https://codereview.chromium.org/1655083002/diff/1/content/browser/media/andro...
content/browser/media/android/browser_surface_view_manager.cc:49: // XXX: plumb
through the size change event.
Do you have a vision for what this looks like? Another extension of the delegate
interface?

https://codereview.chromium.org/1655083002/diff/1/content/renderer/media/andr...
File content/renderer/media/android/renderer_surface_view_manager.h (right):

https://codereview.chromium.org/1655083002/diff/1/content/renderer/media/andr...
content/renderer/media/android/renderer_surface_view_manager.h:15: class
CONTENT_EXPORT RendererSurfaceViewManager
Seems like this might be okay to just put on the Delegate interface. The down
side would be changing from a callback interface to using the observer
interface. However, if you see future callbacks which may need to be delivered
to all delegate instances ... i.e. in the world of surfaceview everywhere, it
might be a more natural fit.

Another thing to think about is: if you go the route you have here the IPC team
might ask that you write this as a Mojo service instead of creating new
messages. Now that they're officially deprecated and all that :) I think if you
decide against extending the delegate interface it might be worth doing as Mojo.

https://codereview.chromium.org/1655083002/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1655083002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1242: pending_suspend_resume_cycle_ = true;
Why not just use |pending_resume| ?

[watk, 2016-02-02 20:38:14]

Thanks Dale, I'll work on moving it to the Delegate and adding the video size
plumbing.

+liberato: this is an early CL. I'd appreciate your high level feedback if you
have time.

I'll let you know when this is ready to do a complete review.

https://codereview.chromium.org/1655083002/diff/1/content/browser/media/andro...
File content/browser/media/android/browser_surface_view_manager.cc (right):

https://codereview.chromium.org/1655083002/diff/1/content/browser/media/andro...
content/browser/media/android/browser_surface_view_manager.cc:49: // XXX: plumb
through the size change event.
On 2016/02/02 01:01:32, DaleCurtis wrote:
> Do you have a vision for what this looks like? Another extension of the
delegate
> interface?

Yeah, that's what I was thinking, but haven't figured out what the source of
truth for the size is.

https://codereview.chromium.org/1655083002/diff/1/content/renderer/media/andr...
File content/renderer/media/android/renderer_surface_view_manager.h (right):

https://codereview.chromium.org/1655083002/diff/1/content/renderer/media/andr...
content/renderer/media/android/renderer_surface_view_manager.h:15: class
CONTENT_EXPORT RendererSurfaceViewManager
On 2016/02/02 01:01:33, DaleCurtis wrote:
> Seems like this might be okay to just put on the Delegate interface. The down
> side would be changing from a callback interface to using the observer
> interface. However, if you see future callbacks which may need to be delivered
> to all delegate instances ... i.e. in the world of surfaceview everywhere, it
> might be a more natural fit.
> 
> Another thing to think about is: if you go the route you have here the IPC
team
> might ask that you write this as a Mojo service instead of creating new
> messages. Now that they're officially deprecated and all that :) I think if
you
> decide against extending the delegate interface it might be worth doing as
Mojo.

I like the idea of putting it on the delegate actually. I'll give that a go.

https://codereview.chromium.org/1655083002/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1655083002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1242: pending_suspend_resume_cycle_ = true;
On 2016/02/02 01:01:33, DaleCurtis wrote:
> Why not just use |pending_resume| ?

I wish I wrote a comment because I've already forgotten the details :P 

This should change after Dan's pipeline controller CL lands anyway.

[watk, 2016-02-02 20:39:22]

watk@chromium.org changed reviewers:
+ liberato@chromium.org

[watk, 2016-02-02 20:39:22]

Actually +liberato. Please take a look at my previous comment Frank.

[liberato (no reviews please), 2016-02-02 22:46:57]

this is quite exciting!  still tracing through details.

i was looking over the design doc for this.  i think that our direction is,
understandably, a little different from when we wrote it.  i'll update it over
the next day or so, to see if you agree with what i think it looks like now.

i want to be sure that it accurately describes where we're headed, since we're
going to get there in many intermediate steps.

https://codereview.chromium.org/1655083002/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1655083002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1345:
decoder_requires_restart_for_fullscreen_ = true;
is this being set unconditionally so that EnterFullscreen knows to restart?  in
other words, when we start in non-fullscreen, GVD will request a surface, and
get NoSurface but cause |decoder_requires...| to be set.

then, when we enter full screen, WMPI will remember and restart the GVD so that
it can ask for a surface again (this time, successfully).

we don't want to have this sort of state in wmpi for too long.  as a follow-up
to this CL, let's figure out how to put it into at least GVD.  there, it has
access to the VDA config flags.  since WMPI<->GVD communication is easy, it
shouldn't be too hard to move this out of WMPI.

replacing |request_surface_cb| with an interface that exposes
SetEnterFullScreenCB, and RequestRestart that GVD can call will greatly clean up
WMPI, i believe.

but, again, i'm talking about the next CL.

https://codereview.chromium.org/1655083002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1356:
BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnSurfaceRequested));
should this be #if android?  otherwise, it looks like the gvd always gets a
|request_surface_cb|, which causes it to always request a surface, which causes
|decoder_requires_restart_for_fullscreen_|, which causes badness.

[watk, 2016-02-05 00:24:22]

Patchset #3 (id:40001) has been deleted

[watk, 2016-02-05 03:42:30]

Thanks for the comments! I've cleaned things up, commented it, and fixed a few
bugs. It's ready to review for real now.

There are two outstanding tasks that I'll be working on:
1) Tests. I'm planning on getting MediaBrowserTests running on Android, but I
suspect that's going to be a large task in and of itself.
2) The mojo conversion.

https://codereview.chromium.org/1655083002/diff/1/content/browser/media/andro...
File content/browser/media/android/browser_surface_view_manager.cc (right):

https://codereview.chromium.org/1655083002/diff/1/content/browser/media/andro...
content/browser/media/android/browser_surface_view_manager.cc:49: // XXX: plumb
through the size change event.
On 2016/02/02 20:38:14, watk wrote:
> On 2016/02/02 01:01:32, DaleCurtis wrote:
> > Do you have a vision for what this looks like? Another extension of the
> delegate
> > interface?
> 
> Yeah, that's what I was thinking, but haven't figured out what the source of
> truth for the size is.

This turned out to be pretty simple. I just pass the size along with the surface
request. And then size changes are sent via a SizeChanged call.

https://codereview.chromium.org/1655083002/diff/1/content/renderer/media/andr...
File content/renderer/media/android/renderer_surface_view_manager.h (right):

https://codereview.chromium.org/1655083002/diff/1/content/renderer/media/andr...
content/renderer/media/android/renderer_surface_view_manager.h:15: class
CONTENT_EXPORT RendererSurfaceViewManager
On 2016/02/02 20:38:14, watk wrote:
> On 2016/02/02 01:01:33, DaleCurtis wrote:
> > Seems like this might be okay to just put on the Delegate interface. The
down
> > side would be changing from a callback interface to using the observer
> > interface. However, if you see future callbacks which may need to be
delivered
> > to all delegate instances ... i.e. in the world of surfaceview everywhere,
it
> > might be a more natural fit.
> > 
> > Another thing to think about is: if you go the route you have here the IPC
> team
> > might ask that you write this as a Mojo service instead of creating new
> > messages. Now that they're officially deprecated and all that :) I think if
> you
> > decide against extending the delegate interface it might be worth doing as
> Mojo.
> 
> I like the idea of putting it on the delegate actually. I'll give that a go.

I tried putting it in the delegate, but decided it wasn't the right fit for now.
We're planning to = move towards WMPI not knowing about surfaces at all, which
would mean something like giving the SurfaceManager to GVD directly, and that
gets harder if it's part of the delegate interface.

Mojo makes sense. I'll look into doing the Mojo conversion and see how long it
might take me.

https://codereview.chromium.org/1655083002/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1655083002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1242: pending_suspend_resume_cycle_ = true;
On 2016/02/02 20:38:14, watk wrote:
> On 2016/02/02 01:01:33, DaleCurtis wrote:
> > Why not just use |pending_resume| ?
> 
> I wish I wrote a comment because I've already forgotten the details :P 
> 
> This should change after Dan's pipeline controller CL lands anyway.

Added some comments for this.

https://codereview.chromium.org/1655083002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1345:
decoder_requires_restart_for_fullscreen_ = true;
On 2016/02/02 22:46:57, liberato wrote:
> is this being set unconditionally so that EnterFullscreen knows to restart? 
in
> other words, when we start in non-fullscreen, GVD will request a surface, and
> get NoSurface but cause |decoder_requires...| to be set.
> 
> then, when we enter full screen, WMPI will remember and restart the GVD so
that
> it can ask for a surface again (this time, successfully).
> 
> we don't want to have this sort of state in wmpi for too long.  as a follow-up
> to this CL, let's figure out how to put it into at least GVD.  there, it has
> access to the VDA config flags.  since WMPI<->GVD communication is easy, it
> shouldn't be too hard to move this out of WMPI.
> 
> replacing |request_surface_cb| with an interface that exposes
> SetEnterFullScreenCB, and RequestRestart that GVD can call will greatly clean
up
> WMPI, i believe.
> 
> but, again, i'm talking about the next CL.

Yep, your assessment was correct. I tried to comment it clearly now.

Also I totally agree with the future direction.

https://codereview.chromium.org/1655083002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1356:
BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnSurfaceRequested));
On 2016/02/02 22:46:57, liberato wrote:
> should this be #if android?  otherwise, it looks like the gvd always gets a
> |request_surface_cb|, which causes it to always request a surface, which
causes
> |decoder_requires_restart_for_fullscreen_|, which causes badness.

Done.

[liberato (no reviews please), 2016-02-05 22:09:47]

lgtm % nits.

i almost commented that it should be checked in without the flag in VDA
capabilities, but that's no fun.  :)

-fl

https://codereview.chromium.org/1655083002/diff/60001/content/browser/media/a...
File content/browser/media/android/browser_surface_view_manager.cc (right):

https://codereview.chromium.org/1655083002/diff/60001/content/browser/media/a...
content/browser/media/android/browser_surface_view_manager.cc:53:
SendSurfaceID(surface_id_);
do we need to OnFullscreenVideoSizeChanged() too?

https://codereview.chromium.org/1655083002/diff/60001/content/browser/media/a...
content/browser/media/android/browser_surface_view_manager.cc:70: void
BrowserSurfaceViewManager::OnFullscreenVideoSizeChanged(gfx::Size size) {
OnNaturalSizeChanged (here and elsewhere)?  i see that you're matching it to
|content_video_video_|, but it's a confusing name.

https://codereview.chromium.org/1655083002/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/1655083002/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:253: void OnSurfaceRequested(const
SurfaceCreatedCB& surface_created_cb);
probably want to #if android this, since the definition is already compiled out.

[DaleCurtis, 2016-02-06 00:17:39]

https://codereview.chromium.org/1655083002/diff/60001/content/browser/media/a...
File content/browser/media/android/browser_surface_view_manager.h (right):

https://codereview.chromium.org/1655083002/diff/60001/content/browser/media/a...
content/browser/media/android/browser_surface_view_manager.h:32: void
OnCreateFullscreenSurface(gfx::Size video_size);
Should we pass these by const& ?

https://codereview.chromium.org/1655083002/diff/60001/content/browser/media/a...
content/browser/media/android/browser_surface_view_manager.h:40: // The surface
id of the ContentVideoView surface.
Blank lines between comment and previous code.

https://codereview.chromium.org/1655083002/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/1655083002/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:462: // For requesting surfaces on behalf of
the Android H/W decoder in fullscreen.
Lets avoid the OS_ANDROID #define where possible and instead just let this be a
nullptr.

[watk, 2016-02-11 04:01:18]

Patchset #4 (id:80001) has been deleted

[watk, 2016-02-11 04:09:41]

Patchset #5 (id:120001) has been deleted

[watk, 2016-02-11 04:28:00]

Description was changed from

==========
Enable SurfaceView fullscreen video on Android with WebMediaPlayerImpl

* Introduces a new media interface called SurfaceManager with a single method
for now: CreateFullscreenSurface.
* SurfaceManager is implemented by RendererSurfaceViewManager which is a
RenderFrameObserver, and BrowserSurfaceViewManager which is created for each
RenderFrame by MediaWebContentsObserverAndroid.
* WMPI has a pointer to a SurfaceManager.
* WMPI gives GVD a new callback called RequestSurfaceCB which it calls before
initializing VDAs. If the player is in fullscreen, WMPI will call
SurfaceManager::CreateFullscreenSurface and the surface id will be returned to
GVD. If the player is not in fullscreen WMPI will return a null surface id.
* When GVD gets a surface id in response to this callback it completes
initialization by calling Initialize on the VDA, passing the surface id to it
through the config.
* As a side effect, WMPI records that it got a surface request callback so that
it knows on future fullscreen transitions to restart the renderer. This is the
only mode for now. In the future we can implement a system in which the decoder
gets a choice about whether to be restarted or not.
* The BrowserSurfaceManager creates a ContentVideoView in response to a
CreateFullscreenSurface call, and registers it to be looked up by the GPU
process.
==========

to

==========
Enable Android SurfaceView fullscreen video with WMPI

This CL enables SurfaceView fullscreen video with the unified media pipeline
in cases where we use the AVDA deferred rendering strategy today, i.e., when
the --enable-unified-media-pipeline flag is passed.

* There's a new media interface called SurfaceManager with two methods:
  CreateFullscreenSurface() and NaturalSizeChanged().
* SurfaceManager is implemented by RendererSurfaceViewManager, which is a
  RenderFrameObserver, and BrowserSurfaceViewManager, which is created for
  each RenderFrame by MediaWebContentsObserverAndroid.
* BrowserSurfaceViewManager creates a ContentVideoView in response to a
  CreateFullscreenSurface message, and registers it in a surface map that the
  decoder can look up in the GPU process.
* WMPI interacts with SurfaceManager on behalf of the decoder. It passes
  GpuVideoDecoder a callback for requesting surfaces which it calls before
  initializing VDAs.
* In response to the callback, if the player is in fullscreen WMPI will pass
  the request to the SurfaceManager which will return the surface id to GVD. If
  the player is not in fullscreen WMPI will return a null surface id.
* When GVD gets a surface id back it completes its initialization by calling
  VDA::Initialize with the surface id and the VDA will render to the given
  surface.
* WMPI records that it got a surface request callback so that it knows that
  future fullscreen transitions will require a restart of the decoder in order
  to switch surfaces.
* A future CL will allow us to switch the surface dynamically without
  requiring a restart.

BUG=533630
==========

[watk, 2016-02-11 04:32:56]

Patchset #5 (id:140001) has been deleted

[watk, 2016-02-11 04:33:04]

Patchset #4 (id:100001) has been deleted

[watk, 2016-02-11 23:03:25]

PTAL. GVD changed a bit to rebase on xhwang's change. The 4->5 diff is hard to
read so I'd probably just review the 5th revision alone for that file. 

I moved the HTMLME and WebMediaPlayer changes here
https://codereview.chromium.org/1689043002/

I think we should land this and start experimenting with it even though it looks
janky right now. We can hopefully reduce the jank before M50, and if not I can
put it behind a flag.

https://codereview.chromium.org/1655083002/diff/60001/content/browser/media/a...
File content/browser/media/android/browser_surface_view_manager.cc (right):

https://codereview.chromium.org/1655083002/diff/60001/content/browser/media/a...
content/browser/media/android/browser_surface_view_manager.cc:53:
SendSurfaceID(surface_id_);
On 2016/02/05 22:09:46, liberato wrote:
> do we need to OnFullscreenVideoSizeChanged() too?

Oops, Done

https://codereview.chromium.org/1655083002/diff/60001/content/browser/media/a...
content/browser/media/android/browser_surface_view_manager.cc:70: void
BrowserSurfaceViewManager::OnFullscreenVideoSizeChanged(gfx::Size size) {
On 2016/02/05 22:09:46, liberato wrote:
> OnNaturalSizeChanged (here and elsewhere)?  i see that you're matching it to
> |content_video_video_|, but it's a confusing name.

Done.

https://codereview.chromium.org/1655083002/diff/60001/content/browser/media/a...
File content/browser/media/android/browser_surface_view_manager.h (right):

https://codereview.chromium.org/1655083002/diff/60001/content/browser/media/a...
content/browser/media/android/browser_surface_view_manager.h:32: void
OnCreateFullscreenSurface(gfx::Size video_size);
On 2016/02/06 00:17:39, DaleCurtis wrote:
> Should we pass these by const& ?

Done.

https://codereview.chromium.org/1655083002/diff/60001/content/browser/media/a...
content/browser/media/android/browser_surface_view_manager.h:40: // The surface
id of the ContentVideoView surface.
On 2016/02/06 00:17:39, DaleCurtis wrote:
> Blank lines between comment and previous code.

Done.

https://codereview.chromium.org/1655083002/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/1655083002/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:462: // For requesting surfaces on behalf of
the Android H/W decoder in fullscreen.
On 2016/02/06 00:17:39, DaleCurtis wrote:
> Lets avoid the OS_ANDROID #define where possible and instead just let this be
a
> nullptr.

Done.

[DaleCurtis, 2016-02-12 00:42:58]

lgtm

[watk, 2016-02-12 02:14:47]

watk@chromium.org changed reviewers:
+ kenrb@chromium.org, sievers@chromium.org

[watk, 2016-02-12 02:14:48]

sievers@chromium.org: Please review changes in content/renderer

kenrb@chromium.org: Please review changes in content/common and ipc/

[no sievers, 2016-02-16 19:03:47]

https://codereview.chromium.org/1655083002/diff/180001/content/renderer/media...
File content/renderer/media/android/renderer_surface_view_manager.cc (right):

https://codereview.chromium.org/1655083002/diff/180001/content/renderer/media...
content/renderer/media/android/renderer_surface_view_manager.cc:32:
DCHECK(!surface_created_cb.is_null());
You mean  DCHECK(surface_created_cb.is_null())?

How is it avoided that some other media player tries to enter fullscreen while
another request is pending?

[watk, 2016-02-16 20:04:00]

https://codereview.chromium.org/1655083002/diff/180001/content/renderer/media...
File content/renderer/media/android/renderer_surface_view_manager.cc (right):

https://codereview.chromium.org/1655083002/diff/180001/content/renderer/media...
content/renderer/media/android/renderer_surface_view_manager.cc:32:
DCHECK(!surface_created_cb.is_null());
On 2016/02/16 19:03:47, sievers wrote:
> You mean  DCHECK(surface_created_cb.is_null())?

This is actually DCHECKing the argument, not the member variable.

> How is it avoided that some other media player tries to enter fullscreen while
> another request is pending?

That's actually OK, but it's not clear. The second player will get the surface.
I added some comments about this to surface_view_manager.h

[no sievers, 2016-02-16 20:07:54]

content/renderer lgtm

[kenrb, 2016-02-16 21:18:44]

The names RendererSurfaceViewManager and BrowserSurfaceViewManager seem to go
against convention, where they would normally be named SurfaceViewManager and
SurfaceViewManagerHost respectively. Is there any reason for that?

[watk, 2016-02-16 21:36:02]

On 2016/02/16 21:18:44, kenrb wrote:
> The names RendererSurfaceViewManager and BrowserSurfaceViewManager seem to go
> against convention, where they would normally be named SurfaceViewManager and
> SurfaceViewManagerHost respectively. Is there any reason for that?

True. I was copying the naming of BrowserMediaPlayerManager,
BrowserMediaSessionManager and BrowserDemuxerAndroid (which each have a Renderer
peer). It seems like the media code has diverged from the "host" convention.

Do you think we should rename them?

[kenrb, 2016-02-17 18:43:02]

On 2016/02/16 21:36:02, watk wrote:
> On 2016/02/16 21:18:44, kenrb wrote:
> > The names RendererSurfaceViewManager and BrowserSurfaceViewManager seem to
go
> > against convention, where they would normally be named SurfaceViewManager
and
> > SurfaceViewManagerHost respectively. Is there any reason for that?
> 
> True. I was copying the naming of BrowserMediaPlayerManager,
> BrowserMediaSessionManager and BrowserDemuxerAndroid (which each have a
Renderer
> peer). It seems like the media code has diverged from the "host" convention.
> 
> Do you think we should rename them?

Interesting, I hadn't seen those. It looks like BrowserMediaPlayerManager still
uses IPC messages according to the convention, though (i.e.
MediaPlayer(Host)_*). Can you rename just the IPC messages to support that, and
also add section header comments to surface_view_manager_messages_android.h
with, e.g.:
// Messages sent from the browser to the renderer.

It helps encourage people to not intermix the different kinds of messages later
on.

[watk, 2016-02-17 21:06:08]

On 2016/02/17 18:43:02, kenrb wrote:
> On 2016/02/16 21:36:02, watk wrote:
> > On 2016/02/16 21:18:44, kenrb wrote:
> > > The names RendererSurfaceViewManager and BrowserSurfaceViewManager seem to
> go
> > > against convention, where they would normally be named SurfaceViewManager
> and
> > > SurfaceViewManagerHost respectively. Is there any reason for that?
> > 
> > True. I was copying the naming of BrowserMediaPlayerManager,
> > BrowserMediaSessionManager and BrowserDemuxerAndroid (which each have a
> Renderer
> > peer). It seems like the media code has diverged from the "host" convention.
> > 
> > Do you think we should rename them?
> 
> Interesting, I hadn't seen those. It looks like BrowserMediaPlayerManager
still
> uses IPC messages according to the convention, though (i.e.
> MediaPlayer(Host)_*). Can you rename just the IPC messages to support that,
and
> also add section header comments to surface_view_manager_messages_android.h
> with, e.g.:
> // Messages sent from the browser to the renderer.
> 
> It helps encourage people to not intermix the different kinds of messages
later
> on.

Yep sure, done.

[watk, 2016-02-17 21:06:18]

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

[commit-bot: I haz the power, 2016-02-17 21:08:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1655083002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1655083002/220001

[kenrb, 2016-02-17 21:11:47]

ipc lgtm

[watk, 2016-02-17 21:14:10]

Description was changed from

==========
Enable Android SurfaceView fullscreen video with WMPI

This CL enables SurfaceView fullscreen video with the unified media pipeline
in cases where we use the AVDA deferred rendering strategy today, i.e., when
the --enable-unified-media-pipeline flag is passed.

* There's a new media interface called SurfaceManager with two methods:
  CreateFullscreenSurface() and NaturalSizeChanged().
* SurfaceManager is implemented by RendererSurfaceViewManager, which is a
  RenderFrameObserver, and BrowserSurfaceViewManager, which is created for
  each RenderFrame by MediaWebContentsObserverAndroid.
* BrowserSurfaceViewManager creates a ContentVideoView in response to a
  CreateFullscreenSurface message, and registers it in a surface map that the
  decoder can look up in the GPU process.
* WMPI interacts with SurfaceManager on behalf of the decoder. It passes
  GpuVideoDecoder a callback for requesting surfaces which it calls before
  initializing VDAs.
* In response to the callback, if the player is in fullscreen WMPI will pass
  the request to the SurfaceManager which will return the surface id to GVD. If
  the player is not in fullscreen WMPI will return a null surface id.
* When GVD gets a surface id back it completes its initialization by calling
  VDA::Initialize with the surface id and the VDA will render to the given
  surface.
* WMPI records that it got a surface request callback so that it knows that
  future fullscreen transitions will require a restart of the decoder in order
  to switch surfaces.
* A future CL will allow us to switch the surface dynamically without
  requiring a restart.

BUG=533630
==========

to

==========
Enable Android SurfaceView fullscreen video with WMPI

This CL enables SurfaceView fullscreen video with the unified media pipeline
in cases where we use the AVDA deferred rendering strategy today.

* There's a new media interface called SurfaceManager with two methods:
  CreateFullscreenSurface() and NaturalSizeChanged().
* SurfaceManager is implemented by RendererSurfaceViewManager, which is a
  RenderFrameObserver, and BrowserSurfaceViewManager, which is created for
  each RenderFrame by MediaWebContentsObserverAndroid.
* BrowserSurfaceViewManager creates a ContentVideoView in response to a
  CreateFullscreenSurface message, and registers it in a surface map that the
  decoder can look up in the GPU process.
* WMPI interacts with SurfaceManager on behalf of the decoder. It passes
  GpuVideoDecoder a callback for requesting surfaces which it calls before
  initializing VDAs.
* In response to the callback, if the player is in fullscreen WMPI will pass
  the request to the SurfaceManager which will return the surface id to GVD. If
  the player is not in fullscreen WMPI will return a null surface id.
* When GVD gets a surface id back it completes its initialization by calling
  VDA::Initialize with the surface id and the VDA will render to the given
  surface.
* WMPI records that it got a surface request callback so that it knows that
  future fullscreen transitions will require a restart of the decoder in order
  to switch surfaces.
* A future CL will allow us to switch the surface dynamically without
  requiring a restart.

BUG=533630
==========

[commit-bot: I haz the power, 2016-02-17 21:15:53]

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

[commit-bot: I haz the power, 2016-02-17 21:15:55]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  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_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[watk, 2016-02-17 22:26:18]

dalecurtis@ PTAL at the changes in media/mojo

[watk, 2016-02-17 23:04:00]

watk@chromium.org changed reviewers:
+ halliwell@chromium.org

[watk, 2016-02-17 23:04:02]

halliwell@chromium.org: Please review changes in chromecast/

[halliwell, 2016-02-17 23:11:54]

On 2016/02/17 23:04:02, watk wrote:
> mailto:halliwell@chromium.org: Please review changes in chromecast/

chromecast/ lgtm

[DaleCurtis, 2016-02-17 23:27:16]

lgtm

[watk, 2016-02-17 23:53:14]

The CQ bit was checked by watk@chromium.org

[watk, 2016-02-17 23:53:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org, sievers@chromium.org, kenrb@chromium.org,
halliwell@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1655083002/#ps300001
(title: "s/SurfaceCreated/RequestSurface/ in chromecast")

[commit-bot: I haz the power, 2016-02-17 23:53:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1655083002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1655083002/300001

[commit-bot: I haz the power, 2016-02-18 00:17:13]

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

[commit-bot: I haz the power, 2016-02-18 00:17:15]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[watk, 2016-02-18 00:28:35]

The CQ bit was checked by watk@chromium.org

[watk, 2016-02-18 00:28:36]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
liberato@chromium.org, halliwell@chromium.org, sievers@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1655083002/#ps320001
(title: "Add an empty destructor to satisfy chromium-style")

[commit-bot: I haz the power, 2016-02-18 00:29:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1655083002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1655083002/320001

[commit-bot: I haz the power, 2016-02-18 02:22:28]

Description was changed from

==========
Enable Android SurfaceView fullscreen video with WMPI

This CL enables SurfaceView fullscreen video with the unified media pipeline
in cases where we use the AVDA deferred rendering strategy today.

* There's a new media interface called SurfaceManager with two methods:
  CreateFullscreenSurface() and NaturalSizeChanged().
* SurfaceManager is implemented by RendererSurfaceViewManager, which is a
  RenderFrameObserver, and BrowserSurfaceViewManager, which is created for
  each RenderFrame by MediaWebContentsObserverAndroid.
* BrowserSurfaceViewManager creates a ContentVideoView in response to a
  CreateFullscreenSurface message, and registers it in a surface map that the
  decoder can look up in the GPU process.
* WMPI interacts with SurfaceManager on behalf of the decoder. It passes
  GpuVideoDecoder a callback for requesting surfaces which it calls before
  initializing VDAs.
* In response to the callback, if the player is in fullscreen WMPI will pass
  the request to the SurfaceManager which will return the surface id to GVD. If
  the player is not in fullscreen WMPI will return a null surface id.
* When GVD gets a surface id back it completes its initialization by calling
  VDA::Initialize with the surface id and the VDA will render to the given
  surface.
* WMPI records that it got a surface request callback so that it knows that
  future fullscreen transitions will require a restart of the decoder in order
  to switch surfaces.
* A future CL will allow us to switch the surface dynamically without
  requiring a restart.

BUG=533630
==========

to

==========
Enable Android SurfaceView fullscreen video with WMPI

This CL enables SurfaceView fullscreen video with the unified media pipeline
in cases where we use the AVDA deferred rendering strategy today.

* There's a new media interface called SurfaceManager with two methods:
  CreateFullscreenSurface() and NaturalSizeChanged().
* SurfaceManager is implemented by RendererSurfaceViewManager, which is a
  RenderFrameObserver, and BrowserSurfaceViewManager, which is created for
  each RenderFrame by MediaWebContentsObserverAndroid.
* BrowserSurfaceViewManager creates a ContentVideoView in response to a
  CreateFullscreenSurface message, and registers it in a surface map that the
  decoder can look up in the GPU process.
* WMPI interacts with SurfaceManager on behalf of the decoder. It passes
  GpuVideoDecoder a callback for requesting surfaces which it calls before
  initializing VDAs.
* In response to the callback, if the player is in fullscreen WMPI will pass
  the request to the SurfaceManager which will return the surface id to GVD. If
  the player is not in fullscreen WMPI will return a null surface id.
* When GVD gets a surface id back it completes its initialization by calling
  VDA::Initialize with the surface id and the VDA will render to the given
  surface.
* WMPI records that it got a surface request callback so that it knows that
  future fullscreen transitions will require a restart of the decoder in order
  to switch surfaces.
* A future CL will allow us to switch the surface dynamically without
  requiring a restart.

BUG=533630
==========

[commit-bot: I haz the power, 2016-02-18 02:22:29]

Committed patchset #12 (id:320001)

[commit-bot: I haz the power, 2016-02-18 02:23:26]

Description was changed from

==========
Enable Android SurfaceView fullscreen video with WMPI

This CL enables SurfaceView fullscreen video with the unified media pipeline
in cases where we use the AVDA deferred rendering strategy today.

* There's a new media interface called SurfaceManager with two methods:
  CreateFullscreenSurface() and NaturalSizeChanged().
* SurfaceManager is implemented by RendererSurfaceViewManager, which is a
  RenderFrameObserver, and BrowserSurfaceViewManager, which is created for
  each RenderFrame by MediaWebContentsObserverAndroid.
* BrowserSurfaceViewManager creates a ContentVideoView in response to a
  CreateFullscreenSurface message, and registers it in a surface map that the
  decoder can look up in the GPU process.
* WMPI interacts with SurfaceManager on behalf of the decoder. It passes
  GpuVideoDecoder a callback for requesting surfaces which it calls before
  initializing VDAs.
* In response to the callback, if the player is in fullscreen WMPI will pass
  the request to the SurfaceManager which will return the surface id to GVD. If
  the player is not in fullscreen WMPI will return a null surface id.
* When GVD gets a surface id back it completes its initialization by calling
  VDA::Initialize with the surface id and the VDA will render to the given
  surface.
* WMPI records that it got a surface request callback so that it knows that
  future fullscreen transitions will require a restart of the decoder in order
  to switch surfaces.
* A future CL will allow us to switch the surface dynamically without
  requiring a restart.

BUG=533630
==========

to

==========
Enable Android SurfaceView fullscreen video with WMPI

This CL enables SurfaceView fullscreen video with the unified media pipeline
in cases where we use the AVDA deferred rendering strategy today.

* There's a new media interface called SurfaceManager with two methods:
  CreateFullscreenSurface() and NaturalSizeChanged().
* SurfaceManager is implemented by RendererSurfaceViewManager, which is a
  RenderFrameObserver, and BrowserSurfaceViewManager, which is created for
  each RenderFrame by MediaWebContentsObserverAndroid.
* BrowserSurfaceViewManager creates a ContentVideoView in response to a
  CreateFullscreenSurface message, and registers it in a surface map that the
  decoder can look up in the GPU process.
* WMPI interacts with SurfaceManager on behalf of the decoder. It passes
  GpuVideoDecoder a callback for requesting surfaces which it calls before
  initializing VDAs.
* In response to the callback, if the player is in fullscreen WMPI will pass
  the request to the SurfaceManager which will return the surface id to GVD. If
  the player is not in fullscreen WMPI will return a null surface id.
* When GVD gets a surface id back it completes its initialization by calling
  VDA::Initialize with the surface id and the VDA will render to the given
  surface.
* WMPI records that it got a surface request callback so that it knows that
  future fullscreen transitions will require a restart of the decoder in order
  to switch surfaces.
* A future CL will allow us to switch the surface dynamically without
  requiring a restart.

BUG=533630

Committed: https://crrev.com/dee516f91d460baf825f25503063a898ec3aaf3a
Cr-Commit-Position: refs/heads/master@{#376068}
==========

[commit-bot: I haz the power, 2016-02-18 02:23:28]

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