content: Out-of-process GPU service support for SurfaceTexture backed GpuMemoryBuffers.

content: Out-of-process GPU service support for SurfaceTexture backed GpuMemoryBuffers.

This implements proper out-of-process support for SurfaceTexture backed
GpuMemoryBuffers using our generic GPU service side GpuMemoryBuffer
allocation mechanism. Both renderer and browser compositor usage is
supported.

BUG=368716, 418553
TBR=jln@chromium.org,sky@chromium.org

Committed: https://crrev.com/6f3e365094fbeeb0340f90917bde6a0a5889cce3
Cr-Commit-Position: refs/heads/master@{#299052}

[reveman, 2014-10-06 19:05:14]

reveman@chromium.org changed reviewers:
+ piman@chromium.org, sievers@chromium.org

[reveman, 2014-10-06 19:05:15]

Please take a look.

https://codereview.chromium.org/634643002/diff/1/content/app/android/child_pr...
File content/app/android/child_process_service.cc (right):

https://codereview.chromium.org/634643002/diff/1/content/app/android/child_pr...
content/app/android/child_process_service.cc:35: public GpuSurfaceLookup {
I decided to consolidate these into one implementation with the "Manager"
interface as the primary interface. My hope is that the second two can be
eliminated in favor of using GpuMemoryBuffers and CHROMIUM_image extension.

[no sievers, 2014-10-06 22:28:27]

https://codereview.chromium.org/634643002/diff/1/content/browser/android/chil...
File content/browser/android/child_process_launcher_android.cc (right):

https://codereview.chromium.org/634643002/diff/1/content/browser/android/chil...
content/browser/android/child_process_launcher_android.cc:34: static void
SetSurfacePeer(
While you're in here .... :)
I think you can remove this duplicated code now if you just make the
implementation in browser_surface_texture_manager take a Surface (instead of
SurfaceTexture).

https://codereview.chromium.org/634643002/diff/1/content/common/android/surfa...
File content/common/android/surface_texture_manager.h (right):

https://codereview.chromium.org/634643002/diff/1/content/common/android/surfa...
content/common/android/surface_texture_manager.h:16: class SurfaceTextureManager
{
Do you think this would fit better under content/common/gpu maybe?
There are really two related interfaces in here: one for the client
(AcquireNativeWidget) and one (register/unregister) for the service to call.

https://codereview.chromium.org/634643002/diff/1/content/common/gpu/client/gp...
File content/common/gpu/client/gpu_memory_buffer_impl_surface_texture.cc
(right):

https://codereview.chromium.org/634643002/diff/1/content/common/gpu/client/gp...
content/common/gpu/client/gpu_memory_buffer_impl_surface_texture.cc:69:
handle.global_id.secondary_id = client_id;
Can we avoid using the very opaque identifiers primary/secondary_id? It gets a
bit confusing (and easy to mix up client vs. surface_texture_id)?
Maybe either a platform-specific type definition like we do for the native
widget types or ifdef'd fields?

https://codereview.chromium.org/634643002/diff/1/content/common/gpu/gpu_memor...
File content/common/gpu/gpu_memory_buffer_factory_surface_texture.cc (right):

https://codereview.chromium.org/634643002/diff/1/content/common/gpu/gpu_memor...
content/common/gpu/gpu_memory_buffer_factory_surface_texture.cc:38:
DCHECK(surface_textures_.find(key) == surface_textures_.end());
nit: maybe LOG(ERROR) since a misbehaving client could actually trigger this?
And maybe fail and return?

https://codereview.chromium.org/634643002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/634643002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:539:
Pair<Integer, Integer> key = new Pair<Integer, Integer>(surfaceTextureId,
clientId);
nit: Can you call the methods above in ChildProcessLauncher for
register/unregister/get?

[reveman, 2014-10-07 01:00:21]

PTAL

https://codereview.chromium.org/634643002/diff/1/content/browser/android/chil...
File content/browser/android/child_process_launcher_android.cc (right):

https://codereview.chromium.org/634643002/diff/1/content/browser/android/chil...
content/browser/android/child_process_launcher_android.cc:34: static void
SetSurfacePeer(
On 2014/10/06 22:28:27, sievers wrote:
> While you're in here .... :)
> I think you can remove this duplicated code now if you just make the
> implementation in browser_surface_texture_manager take a Surface (instead of
> SurfaceTexture).

I'm not sure I understand exactly how that would work. I changed so all Java
code deals with Surfaces instead of SurfaceTextures in latest patch but I didn't
change the code here. Unless it's very trivial, I'd prefer if it was left for a
follow up patch. Wdyt?

https://codereview.chromium.org/634643002/diff/1/content/common/android/surfa...
File content/common/android/surface_texture_manager.h (right):

https://codereview.chromium.org/634643002/diff/1/content/common/android/surfa...
content/common/android/surface_texture_manager.h:16: class SurfaceTextureManager
{
On 2014/10/06 22:28:27, sievers wrote:
> Do you think this would fit better under content/common/gpu maybe?
> There are really two related interfaces in here: one for the client
> (AcquireNativeWidget) and one (register/unregister) for the service to call.

Maybe. I decided to keep these interfaces together for now and leave it up to
the user of this interface to decide what part is the service and what part is
the client. Both need to be available in the browser process and both need to be
available in the renderer process so it seemed simple to just think of it as the
interface to some instance that manages all our surface textures.

https://codereview.chromium.org/634643002/diff/1/content/common/gpu/client/gp...
File content/common/gpu/client/gpu_memory_buffer_impl_surface_texture.cc
(right):

https://codereview.chromium.org/634643002/diff/1/content/common/gpu/client/gp...
content/common/gpu/client/gpu_memory_buffer_impl_surface_texture.cc:69:
handle.global_id.secondary_id = client_id;
On 2014/10/06 22:28:27, sievers wrote:
> Can we avoid using the very opaque identifiers primary/secondary_id? It gets a
> bit confusing (and easy to mix up client vs. surface_texture_id)?
> Maybe either a platform-specific type definition like we do for the native
> widget types or ifdef'd fields?

gfx::GpuMemoryBufferId needs to live in ui/gfx and gfx shouldn't really be aware
of multi-process concepts such as process ids and gpu client ids. Maybe we can
figure out a way to make this less opaque without exposing such concepts to gfx/
but it should in that case be done as a separate patch as this doesn't just
apply to surface textures.

https://codereview.chromium.org/634643002/diff/1/content/common/gpu/gpu_memor...
File content/common/gpu/gpu_memory_buffer_factory_surface_texture.cc (right):

https://codereview.chromium.org/634643002/diff/1/content/common/gpu/gpu_memor...
content/common/gpu/gpu_memory_buffer_factory_surface_texture.cc:38:
DCHECK(surface_textures_.find(key) == surface_textures_.end());
On 2014/10/06 22:28:27, sievers wrote:
> nit: maybe LOG(ERROR) since a misbehaving client could actually trigger this?
> And maybe fail and return?

Only the browser process can use IPC that will cause this interface to be used.
The browser is misbehaving if this DCHECK is triggered.

https://codereview.chromium.org/634643002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/634643002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:539:
Pair<Integer, Integer> key = new Pair<Integer, Integer>(surfaceTextureId,
clientId);
On 2014/10/06 22:28:27, sievers wrote:
> nit: Can you call the methods above in ChildProcessLauncher for
> register/unregister/get?

Done.

[no sievers, 2014-10-07 22:12:45]

https://codereview.chromium.org/634643002/diff/1/content/browser/android/chil...
File content/browser/android/child_process_launcher_android.cc (right):

https://codereview.chromium.org/634643002/diff/1/content/browser/android/chil...
content/browser/android/child_process_launcher_android.cc:34: static void
SetSurfacePeer(
On 2014/10/07 01:00:21, reveman wrote:
> On 2014/10/06 22:28:27, sievers wrote:
> > While you're in here .... :)
> > I think you can remove this duplicated code now if you just make the
> > implementation in browser_surface_texture_manager take a Surface (instead of
> > SurfaceTexture).
> 
> I'm not sure I understand exactly how that would work. I changed so all Java
> code deals with Surfaces instead of SurfaceTextures in latest patch but I
didn't
> change the code here. Unless it's very trivial, I'd prefer if it was left for
a
> follow up patch. Wdyt?

Sounds good, I can take a look as a follow-up.

https://codereview.chromium.org/634643002/diff/1/content/common/android/surfa...
File content/common/android/surface_texture_manager.h (right):

https://codereview.chromium.org/634643002/diff/1/content/common/android/surfa...
content/common/android/surface_texture_manager.h:16: class SurfaceTextureManager
{
On 2014/10/07 01:00:21, reveman wrote:
> On 2014/10/06 22:28:27, sievers wrote:
> > Do you think this would fit better under content/common/gpu maybe?
> > There are really two related interfaces in here: one for the client
> > (AcquireNativeWidget) and one (register/unregister) for the service to call.
> 
> Maybe. I decided to keep these interfaces together for now and leave it up to
> the user of this interface to decide what part is the service and what part is
> the client. Both need to be available in the browser process and both need to
be
> available in the renderer process so it seemed simple to just think of it as
the
> interface to some instance that manages all our surface textures.

Yea no problem. I only mentioned it because it's slightly confusing that we
always have to instantiate BrowserSurfaceTextureManager although
Register/Unregister is only used from there when running in-process-gl.

https://codereview.chromium.org/634643002/diff/1/content/common/gpu/gpu_memor...
File content/common/gpu/gpu_memory_buffer_factory_surface_texture.cc (right):

https://codereview.chromium.org/634643002/diff/1/content/common/gpu/gpu_memor...
content/common/gpu/gpu_memory_buffer_factory_surface_texture.cc:38:
DCHECK(surface_textures_.find(key) == surface_textures_.end());
On 2014/10/07 01:00:21, reveman wrote:
> On 2014/10/06 22:28:27, sievers wrote:
> > nit: maybe LOG(ERROR) since a misbehaving client could actually trigger
this?
> > And maybe fail and return?
> 
> Only the browser process can use IPC that will cause this interface to be
used.
> The browser is misbehaving if this DCHECK is triggered.

Ok, if it's a request from the renderer, the browser will have generated
primary_id in GpuMemoryBufferImplSurfaceTexture::AllocateForChildProcess().

https://codereview.chromium.org/634643002/diff/20001/content/app/android/chil...
File content/app/android/child_process_service.cc (right):

https://codereview.chromium.org/634643002/diff/20001/content/app/android/chil...
content/app/android/child_process_service.cc:44: }
One thing I don't like so much is that this class mixes functions that are
exclusively used either by renderers (EstablishPeer, AcquireNativeWidget for ST)
or by the gpu process (AcquireNativeWidget for view surface, Register,
Unregister).
But it was like that before this patch. Maybe I can follow up and see if there
is an easy way to find to instantiate one of two implementations here. I think
it would make it easier to follow things. Mind putting a TODO?

https://codereview.chromium.org/634643002/diff/20001/content/app/android/chil...
content/app/android/child_process_service.cc:47:
GpuSurfaceLookup::InitInstance(NULL);
Should it also do SurfaceTextureManager::InitInstance(NULL)?

https://codereview.chromium.org/634643002/diff/20001/content/browser/android/...
File content/browser/android/browser_surface_texture_manager.cc (right):

https://codereview.chromium.org/634643002/diff/20001/content/browser/android/...
content/browser/android/browser_surface_texture_manager.cc:87:
gfx::ScopedJavaSurface surface(surface_texture);
Does the surface still work when you pass it to Java, since ~ScopedJavaSurface()
will call release() on it.
Wouldn't the assert for surface.isValid() in
ChildProcessLauncher.getSurfaceTextureSurface() fail?

You could add a leak() method to ScopedJavaSurface. (I wouldn't call it
release() since that makes it easy to confuse with Surface.release()).

https://codereview.chromium.org/634643002/diff/20001/content/browser/android/...
content/browser/android/browser_surface_texture_manager.cc:101:
gfx::ScopedJavaSurface surface(
Similar problem here. I think it works the first time since you are creating the
native_window before the ref is released. But I think you would not be able to
call this method here a second time. Not sure if you need to support that, but
since you are still tracking the surfaces might as well.
I think here you can actually use:

gfx::ScopedJavaSurface surface(ScopedJavaSurface::AcquireExternalSurface(...

which will keep it from releasing it.

[no sievers, 2014-10-07 22:23:25]

https://codereview.chromium.org/634643002/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/634643002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:268:
Pair<Integer, Integer> key = new Pair<Integer, Integer>(surfaceTextureId,
clientId);
Also here I think you should call release() on the Surface to make sure the
graphics memory gets freed immediately and we don't have to wait until GC.

[reveman, 2014-10-08 15:56:39]

PTAL

https://codereview.chromium.org/634643002/diff/1/content/common/android/surfa...
File content/common/android/surface_texture_manager.h (right):

https://codereview.chromium.org/634643002/diff/1/content/common/android/surfa...
content/common/android/surface_texture_manager.h:16: class SurfaceTextureManager
{
On 2014/10/07 22:12:44, sievers wrote:
> On 2014/10/07 01:00:21, reveman wrote:
> > On 2014/10/06 22:28:27, sievers wrote:
> > > Do you think this would fit better under content/common/gpu maybe?
> > > There are really two related interfaces in here: one for the client
> > > (AcquireNativeWidget) and one (register/unregister) for the service to
call.
> > 
> > Maybe. I decided to keep these interfaces together for now and leave it up
to
> > the user of this interface to decide what part is the service and what part
is
> > the client. Both need to be available in the browser process and both need
to
> be
> > available in the renderer process so it seemed simple to just think of it as
> the
> > interface to some instance that manages all our surface textures.
> 
> Yea no problem. I only mentioned it because it's slightly confusing that we
> always have to instantiate BrowserSurfaceTextureManager although
> Register/Unregister is only used from there when running in-process-gl.

Correct, register/unregister are only used with in-process gpu service. Or in
more generic terms; they're used whenever we create surface texture instances in
the browser process. AcquireNativeWidget is used independent of that.

https://codereview.chromium.org/634643002/diff/20001/content/app/android/chil...
File content/app/android/child_process_service.cc (right):

https://codereview.chromium.org/634643002/diff/20001/content/app/android/chil...
content/app/android/child_process_service.cc:44: }
On 2014/10/07 22:12:45, sievers wrote:
> One thing I don't like so much is that this class mixes functions that are
> exclusively used either by renderers (EstablishPeer, AcquireNativeWidget for
ST)
> or by the gpu process (AcquireNativeWidget for view surface, Register,
> Unregister).
> But it was like that before this patch. Maybe I can follow up and see if there
> is an easy way to find to instantiate one of two implementations here. I think
> it would make it easier to follow things. Mind putting a TODO?

Done.

https://codereview.chromium.org/634643002/diff/20001/content/app/android/chil...
content/app/android/child_process_service.cc:47:
GpuSurfaceLookup::InitInstance(NULL);
On 2014/10/07 22:12:45, sievers wrote:
> Should it also do SurfaceTextureManager::InitInstance(NULL)?

That's the responsibility of InternalInitChildProcess. Currently it just sets
and forgets this interface. Which is a bit nasty and something we might want to
clean up but that's not new to this patch.

https://codereview.chromium.org/634643002/diff/20001/content/browser/android/...
File content/browser/android/browser_surface_texture_manager.cc (right):

https://codereview.chromium.org/634643002/diff/20001/content/browser/android/...
content/browser/android/browser_surface_texture_manager.cc:87:
gfx::ScopedJavaSurface surface(surface_texture);
On 2014/10/07 22:12:45, sievers wrote:
> Does the surface still work when you pass it to Java, since
~ScopedJavaSurface()
> will call release() on it.
> Wouldn't the assert for surface.isValid() in
> ChildProcessLauncher.getSurfaceTextureSurface() fail?
> 
> You could add a leak() method to ScopedJavaSurface. (I wouldn't call it
> release() since that makes it easy to confuse with Surface.release()).

Ah, I was too quick to change this without actually verifying that it worked. I
think it's easier and cleaner to handle this in java land so I changed things
around a bit in latest patch. The java API is now
create/destroySurfaceTextureSurface instead. PTAL.

https://codereview.chromium.org/634643002/diff/20001/content/browser/android/...
content/browser/android/browser_surface_texture_manager.cc:101:
gfx::ScopedJavaSurface surface(
On 2014/10/07 22:12:45, sievers wrote:
> Similar problem here. I think it works the first time since you are creating
the
> native_window before the ref is released. But I think you would not be able to
> call this method here a second time. Not sure if you need to support that, but
> since you are still tracking the surfaces might as well.
> I think here you can actually use:
> 
> gfx::ScopedJavaSurface surface(ScopedJavaSurface::AcquireExternalSurface(...
> 
> which will keep it from releasing it.

Nice catch. Should be fixed in latest patch. Including the child process case.

https://codereview.chromium.org/634643002/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/634643002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:268:
Pair<Integer, Integer> key = new Pair<Integer, Integer>(surfaceTextureId,
clientId);
On 2014/10/07 22:23:25, sievers wrote:
> Also here I think you should call release() on the Surface to make sure the
> graphics memory gets freed immediately and we don't have to wait until GC.

Done.

[reveman, 2014-10-09 18:54:11]

reveman@chromium.org changed reviewers:
+ jln@chromium.org, jschuh@chromium.org

[reveman, 2014-10-09 18:54:12]

piman, sievers, does latest patch look good? I prefer to land this before my big
CHROMIUM_image refactor.

+jschuh,jln for content/common/child_process_messages.h

[piman, 2014-10-09 19:11:30]

On 2014/10/09 18:54:12, reveman wrote:
> piman, sievers, does latest patch look good? I prefer to land this before my
big
> CHROMIUM_image refactor.
> 
> +jschuh,jln for content/common/child_process_messages.h

I'm happy as soon as Daniel is (he's a content and ui/gl OWNER so his LGTM is as
good as mine).

[piman, 2014-10-09 19:11:52]

piman@chromium.org changed reviewers:
+ sky@chromium.org

[piman, 2014-10-09 19:11:52]

also, +sky for ui/gfx

[no sievers, 2014-10-09 19:31:40]

LGTM with register from GPU child process behavior changed back, see comment.

https://codereview.chromium.org/634643002/diff/80001/content/app/android/chil...
File content/app/android/child_process_service.cc (right):

https://codereview.chromium.org/634643002/diff/80001/content/app/android/chil...
content/app/android/child_process_service.cc:58:
Java_ChildProcessService_createSurfaceTextureSurface(
Can you make this Java_ChildProcessService_registerSurfaceTextureSurface() again
and use a ScoepdJavaSurface here? Here I think we do actually want to release()
the Surface in this process after we have pushed it to the browser. Otherwise
we'd be leaking a ref until GC from the GPU process here for the temporary
Surface.

https://codereview.chromium.org/634643002/diff/80001/content/app/android/chil...
content/app/android/child_process_service.cc:64:
DCHECK(!base::android::HasException(env));
nit: doesn't the generated boilerplate already do this, even in release builds?
same for call sites below.

[reveman, 2014-10-10 02:25:19]

https://codereview.chromium.org/634643002/diff/80001/content/app/android/chil...
File content/app/android/child_process_service.cc (right):

https://codereview.chromium.org/634643002/diff/80001/content/app/android/chil...
content/app/android/child_process_service.cc:58:
Java_ChildProcessService_createSurfaceTextureSurface(
On 2014/10/09 19:31:40, sievers wrote:
> Can you make this Java_ChildProcessService_registerSurfaceTextureSurface()
again
> and use a ScoepdJavaSurface here? Here I think we do actually want to
release()
> the Surface in this process after we have pushed it to the browser. Otherwise
> we'd be leaking a ref until GC from the GPU process here for the temporary
> Surface.

Added a "release" call to the Java code as discussed.

https://codereview.chromium.org/634643002/diff/80001/content/app/android/chil...
content/app/android/child_process_service.cc:64:
DCHECK(!base::android::HasException(env));
On 2014/10/09 19:31:40, sievers wrote:
> nit: doesn't the generated boilerplate already do this, even in release
builds?
> same for call sites below.

Removed this and DCHECKs below.

[reveman, 2014-10-10 02:32:48]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-10-10 02:34:13]

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

[sky, 2014-10-10 02:53:18]

LGTM

[commit-bot: I haz the power, 2014-10-10 03:32:10]

Committed patchset #8 (id:440001)

[commit-bot: I haz the power, 2014-10-10 03:33:11]

Patchset 8 (id:??) landed as
https://crrev.com/6f3e365094fbeeb0340f90917bde6a0a5889cce3
Cr-Commit-Position: refs/heads/master@{#299052}