Add initial GpuMemoryBufferSurfaceTexture implementation.

Add initial GpuMemoryBufferSurfaceTexture implementation.

This adds the plumbing and allocation logic needed to implement
a surface texture backed GpuMemoryBuffer on Android. The allocation
flow is the same as other GpuMemoryBuffer implementations. That is;
the browser process decides if a renderer is allowed to allocate
a new GpuMemoryBuffer or not. Successful allocation results in
a valid GpuMemoryBufferHandle being returned to the renderer. This
handle can be used to create a GpuMemoryBufferImpl instance on the
renderer side, which can be mapped into the renderer address space
and used as existing GpuMemoryBufferImpls. The same handle can also
be used to register the GpuMemoryBuffer with the GPU process and
and create a GLImage that can be used for sampling.

Two new interfaces are added to allow sharing of surface textures
across process boundaries:

SurfaceTextureLookup, this interface is used by a renderer to
acquire a native widget for a surface texture. The native widget
allows the renderer to map the storage of the surface texture into
its address space. The current implementation of this interface uses
Java Binder IPC to share the surface texture surface with a
renderer process.

SurfaceTextureTracker, this interface is used by GLImage
implementation to acquire ownership of surface textures allocated
by the browser process. Current implementation of this interface
only works with GLImage instances in the browser process. A
different implementation can be added to support out of process
GLImage instances.

This is currently limited to buffered surface textures and
GL_TEXTURE_EXTERNAL_OES texture target.

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

[reveman, 2014-03-11 22:59:35]

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:168: jobject
GetSurface(int surface_texture_id) const {
This should probably take a renderer_id to prevent a renderer from stealing a
surface from another renderer.

[reveman, 2014-03-11 23:12:49]

This initial patch is focused on getting the allocation and integration with the
rest of the zero-copy framework right. Performance and especially memory usage
is expected to be bad.

This doesn't implement GPU-copy. It implements true zero-copy using buffered
surface textures as a simple first step.

This design doc describes the implementation in more detail:
https://docs.google.com/a/chromium.org/document/d/1jHmGdxwCZdo9Ypeeh-u8DPsXgt...

Please take a look.

[epennerAtGoogle, 2014-03-12 00:45:06]

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:191:
base::LazyInstance<SurfaceTextureTrackerImpl> g_surface_texture_tracker =
Hold on, I thought the gfx::SurfaceTextureTracker had a global pointer stored in
gfx::SurfaceTextureTracker::InitInstance?   Why does this also store a global
instance?

Anyway, since this is LazyInstance, could it just be moved into
gfx::SurfaceTextureTracker and this one removed?

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:237:
g_surface_texture_tracker.Pointer()->GetSurface(surface_texture_id);
Could CompositorImpl just own this and avoid a global instance? It's nice if it
doesn't need to do thread-safe stuff on every access to the object.

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:248:
make_scoped_refptr(new gfx::SurfaceTexture(kDummyTextureId));
What thread will this happen on? I believe it has to be on a thread that doesn't
have a GL context (in order to do the kDummyTextureId trick).

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.h:55: static int
AllocateSurfaceTexture();
Could this and the above possibly be fused? Is the issue that the allocation
goes through our normal sync IPC and then GetSurfaceTextureSurface goes
synchronously through binder?  Even if so, why not do both at the same time?

A comment about this flow (and why) would help (for posterity).

https://codereview.chromium.org/195583003/diff/20001/content/common/android/s...
File content/common/android/surface_texture_lookup.cc (right):

https://codereview.chromium.org/195583003/diff/20001/content/common/android/s...
content/common/android/surface_texture_lookup.cc:12: SurfaceTextureLookup*
g_instance_ = NULL;
It would be nice if someone could own this. Otherwise can you comment why
initialization races are impossible (or change to LazyInstance).

https://codereview.chromium.org/195583003/diff/20001/ui/gfx/gpu_memory_buffer.h
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/195583003/diff/20001/ui/gfx/gpu_memory_buffer...
ui/gfx/gpu_memory_buffer.h:52: uint32 surface_texture_id;
Throughout this patch, I'd like it if we can make it very clear that these ID's
come from a certain 'name-space' of SurfaceTextures. They will have more and
more uses of SurfaceTexture, and not all of them have an ID from this source of
IDs and you can't find the source of 'int' as easily in code-search. Possibly
longer names like name_space_surface_texture_id, or even a real type with
::GetID(), might help?

https://codereview.chromium.org/195583003/diff/20001/ui/gl/android/surface_te...
File ui/gl/android/surface_texture_tracker.cc (right):

https://codereview.chromium.org/195583003/diff/20001/ui/gl/android/surface_te...
ui/gl/android/surface_texture_tracker.cc:9: SurfaceTextureTracker* g_instance =
NULL;
Same on this one. Can you use LazyInstance or comment why initialization races
are impossible?

https://codereview.chromium.org/195583003/diff/20001/ui/gl/android/surface_te...
ui/gl/android/surface_texture_tracker.cc:13: SurfaceTextureTracker*
SurfaceTextureTracker::GetInstance() {
This is a lot better than an 'extern' but not _huge_ amount better ;). What I'm
worried about is this becoming an extra item that prevents us from using
out-of-proc GPU which otherwise just has one blocking issue IIUC.

I suppose we can just disable this feature if we do that, so I'm okay with it
for now. Sievers@, what's your thoughts?

https://codereview.chromium.org/195583003/diff/20001/ui/gl/gl_image_surface_t...
File ui/gl/gl_image_surface_texture.cc (right):

https://codereview.chromium.org/195583003/diff/20001/ui/gl/gl_image_surface_t...
ui/gl/gl_image_surface_texture.cc:41: void DetachFromGLContext(SurfaceTexture*
surface_texture) {
I'm not sure this works 100% the way we want, at least for the use cases we
discussed. The entire bit about 'early fencing' being caused by detach also
won't happen if there is no context current. In fact I'm not sure this can even
be called at all with no context current.

In any case, without that first bit I don't see why we need it at all yet. Is
there cases where we need to delete the GLImage but not service texture (along
with it's ID)?

[reveman, 2014-03-12 16:07:05]

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:191:
base::LazyInstance<SurfaceTextureTrackerImpl> g_surface_texture_tracker =
On 2014/03/12 00:45:06, epennerAtGoogle wrote:
> Hold on, I thought the gfx::SurfaceTextureTracker had a global pointer stored
in
> gfx::SurfaceTextureTracker::InitInstance?   Why does this also store a global
> instance?
> 
> Anyway, since this is LazyInstance, could it just be moved into
> gfx::SurfaceTextureTracker and this one removed?

gfx::SurfaceTextureTracker is just an interface and this is one implementation
of that interface. We could have more implementations. ie. one that works with
an out-of-process GPU service.

This doesn't have to be a lazy instance but I found it consistent with
g_surface_map below, which is the other set of surfaces that can be acquired by
the renderer using Java IPC.

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:237:
g_surface_texture_tracker.Pointer()->GetSurface(surface_texture_id);
On 2014/03/12 00:45:06, epennerAtGoogle wrote:
> Could CompositorImpl just own this and avoid a global instance? It's nice if
it
> doesn't need to do thread-safe stuff on every access to the object.

We need some thread-safe code here to allow the Java IPC handler to access this.
If this is not global, then the Java IPC handler would have to figure out what
compositorimpl to use. This seems a lot easier and consistent with
g_surface_map..

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:248:
make_scoped_refptr(new gfx::SurfaceTexture(kDummyTextureId));
On 2014/03/12 00:45:06, epennerAtGoogle wrote:
> What thread will this happen on? I believe it has to be on a thread that
doesn't
> have a GL context (in order to do the kDummyTextureId trick).

On the IO thread.

Having looked at the SurfaceTexture code, this doesn't seem necessary but that's
of course very fragile as we rely on specifics of the implementation.

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.h:55: static int
AllocateSurfaceTexture();
On 2014/03/12 00:45:06, epennerAtGoogle wrote:
> Could this and the above possibly be fused? Is the issue that the allocation
> goes through our normal sync IPC and then GetSurfaceTextureSurface goes
> synchronously through binder?  Even if so, why not do both at the same time?

surface_texture_id generation (giving the renderer the permission to allocate a
surface texture backed gpumemorybuffer) goes through normal IPC.

The actual creation of a SurfaceTexture instance doesn't have to be done at the
time of normal IPC but I didn't see any reason not to do that unless we remove
the normal IPC all together but I think that's a bad idea for the following
reasons:

1. The browser process should be able to decide what type of gpumemorybuffer to
give the renderer. I don't think we should make that decision in different
places depending on the platform.
2. One entry point in the browser process where permission to allocate
gpumemorybuffers are handled to maximize code reuse and make the flow easy to
understand.

> 
> A comment about this flow (and why) would help (for posterity).

It's in the design doc but I'll figure out how to make this clear in the code as
well.

https://codereview.chromium.org/195583003/diff/20001/content/common/android/s...
File content/common/android/surface_texture_lookup.cc (right):

https://codereview.chromium.org/195583003/diff/20001/content/common/android/s...
content/common/android/surface_texture_lookup.cc:12: SurfaceTextureLookup*
g_instance_ = NULL;
On 2014/03/12 00:45:06, epennerAtGoogle wrote:
> It would be nice if someone could own this. Otherwise can you comment why
> initialization races are impossible (or change to LazyInstance).

It's not the responsibility of this code to prevent races. It's the
responsibility of the user. Same as GpuSurfaceLookup..

Can't be a LazyInstance as this is just an interface. The user is free to use a
LazyInstance to protect against races though.

https://codereview.chromium.org/195583003/diff/20001/ui/gfx/gpu_memory_buffer.h
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/195583003/diff/20001/ui/gfx/gpu_memory_buffer...
ui/gfx/gpu_memory_buffer.h:52: uint32 surface_texture_id;
On 2014/03/12 00:45:06, epennerAtGoogle wrote:
> Throughout this patch, I'd like it if we can make it very clear that these
ID's
> come from a certain 'name-space' of SurfaceTextures. They will have more and
> more uses of SurfaceTexture, and not all of them have an ID from this source
of
> IDs and you can't find the source of 'int' as easily in code-search. Possibly
> longer names like name_space_surface_texture_id, or even a real type with
> ::GetID(), might help?

Would a simple typedef int SurfaceTextureID help?

https://codereview.chromium.org/195583003/diff/20001/ui/gl/android/surface_te...
File ui/gl/android/surface_texture_tracker.cc (right):

https://codereview.chromium.org/195583003/diff/20001/ui/gl/android/surface_te...
ui/gl/android/surface_texture_tracker.cc:9: SurfaceTextureTracker* g_instance =
NULL;
On 2014/03/12 00:45:06, epennerAtGoogle wrote:
> Same on this one. Can you use LazyInstance or comment why initialization races
> are impossible?

Same answer.

https://codereview.chromium.org/195583003/diff/20001/ui/gl/android/surface_te...
ui/gl/android/surface_texture_tracker.cc:13: SurfaceTextureTracker*
SurfaceTextureTracker::GetInstance() {
On 2014/03/12 00:45:06, epennerAtGoogle wrote:
> This is a lot better than an 'extern' but not _huge_ amount better ;). What
I'm
> worried about is this becoming an extra item that prevents us from using
> out-of-proc GPU which otherwise just has one blocking issue IIUC.
> 
> I suppose we can just disable this feature if we do that, so I'm okay with it
> for now. Sievers@, what's your thoughts?

I'd like to get this working with out-of-proc GPU asap but thought it'd be
better to land this first and do that as a follow up.

The idea is that none of this would change in out-of-proc GPU mode. This
instance would just be set to a different implementation by the GPU service
code.

https://codereview.chromium.org/195583003/diff/20001/ui/gl/gl_image_surface_t...
File ui/gl/gl_image_surface_texture.cc (right):

https://codereview.chromium.org/195583003/diff/20001/ui/gl/gl_image_surface_t...
ui/gl/gl_image_surface_texture.cc:41: void DetachFromGLContext(SurfaceTexture*
surface_texture) {
On 2014/03/12 00:45:06, epennerAtGoogle wrote:
> I'm not sure this works 100% the way we want, at least for the use cases we
> discussed. The entire bit about 'early fencing' being caused by detach also
> won't happen if there is no context current. In fact I'm not sure this can
even
> be called at all with no context current.

Seems to be working OK but there might be some fencing issues as you mentioned.

> 
> In any case, without that first bit I don't see why we need it at all yet. Is
> there cases where we need to delete the GLImage but not service texture (along
> with it's ID)?

What do you mean by the first bit?

The GLImage instance never owns the texture id. Deleting it will mess up the
service, which should be allowed to reuse it.

[epennerAtGoogle, 2014-03-12 19:46:28]

LGTM % hashing out the pointer bit and % sievers@ review.

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:191:
base::LazyInstance<SurfaceTextureTrackerImpl> g_surface_texture_tracker =
> This doesn't have to be a lazy instance but I found it consistent with
> g_surface_map below, which is the other set of surfaces that can be acquired
by
> the renderer using Java IPC.

I think if this doesn't have to be a lazy-instance, it shouldn't be, or if it
should be then the parent global should do it all the time.

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:237:
g_surface_texture_tracker.Pointer()->GetSurface(surface_texture_id);
Ahh okay if we've already gone the global route a few times, all good with me
assuming Sievers thinks its okay. We've previously routed these things all the
way to their owners, but maybe it's not necessary.

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:248:
make_scoped_refptr(new gfx::SurfaceTexture(kDummyTextureId));
I think you might be right on the creation part. Unfortunately though, we'll be
relying very heavily on the implementation specifics. However most of the
specifics fall out of the API's constraints, so I don't think any of the
semantics are subject to change.

https://codereview.chromium.org/195583003/diff/20001/content/common/android/s...
File content/common/android/surface_texture_lookup.cc (right):

https://codereview.chromium.org/195583003/diff/20001/content/common/android/s...
content/common/android/surface_texture_lookup.cc:12: SurfaceTextureLookup*
g_instance_ = NULL;
g_instance_ isn't an interface though right? It's a real piece of data. It could
as easily be a lazy-instance no?

For example, if you use this interface as the getter, it seems like you could
have set the pointer already on another thread long ago in the past, but still
get a stale NULL in the getter, even though you had every expectation that it
was already set. This would be a DCHECK.

Excuse me if I'm wrong here, I just like err on the side of extreme caution when
it comes to thread safety issues. There was a long discussion on another thread
about this so if anything I'll learn something here if I'm wrong ;)

https://codereview.chromium.org/195583003/diff/20001/ui/gfx/gpu_memory_buffer.h
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/195583003/diff/20001/ui/gfx/gpu_memory_buffer...
ui/gfx/gpu_memory_buffer.h:52: uint32 surface_texture_id;
Yeppers!

https://codereview.chromium.org/195583003/diff/20001/ui/gl/android/surface_te...
File ui/gl/android/surface_texture_tracker.cc (right):

https://codereview.chromium.org/195583003/diff/20001/ui/gl/android/surface_te...
ui/gl/android/surface_texture_tracker.cc:13: SurfaceTextureTracker*
SurfaceTextureTracker::GetInstance() {
If sievers@ is okay with it then no worries here.

https://codereview.chromium.org/195583003/diff/20001/ui/gl/gl_image_surface_t...
File ui/gl/gl_image_surface_texture.cc (right):

https://codereview.chromium.org/195583003/diff/20001/ui/gl/gl_image_surface_t...
ui/gl/gl_image_surface_texture.cc:41: void DetachFromGLContext(SurfaceTexture*
surface_texture) {
What I meant here was, 'if we don't need the fencing, do we need this at all'.
Sounds like we do, such that the service can delete the texture rather than the
GLImage doing it on behalf of it etc.

I'm a bit confused what is 'left' inside the texture this way. Perhaps it keeps
a single ST buffer(s) alive until the texture is redefined, similar to EGL. SGTM
if that's the idea.

[epennerAtGoogle, 2014-03-12 20:52:33]

https://codereview.chromium.org/195583003/diff/20001/ui/gl/gl_image_surface_t...
File ui/gl/gl_image_surface_texture.cc (right):

https://codereview.chromium.org/195583003/diff/20001/ui/gl/gl_image_surface_t...
ui/gl/gl_image_surface_texture.cc:41: void DetachFromGLContext(SurfaceTexture*
surface_texture) {
Oh, one thought here would be: should we redefine the texture to free any
remaining reference to the ST data? Overall if this works it's cleaner than I
expected.

If we ever want the 'early fencing' part we'll need to revisit swapping out
service IDs, but for one-copy we just need to releaseTexImage after the FBO
blit, which doesn't have this problem.

[reveman, 2014-03-12 21:19:10]

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:191:
base::LazyInstance<SurfaceTextureTrackerImpl> g_surface_texture_tracker =
On 2014/03/12 19:46:28, epennerAtGoogle wrote:
> > This doesn't have to be a lazy instance but I found it consistent with
> > g_surface_map below, which is the other set of surfaces that can be acquired
> by
> > the renderer using Java IPC.
> 
> I think if this doesn't have to be a lazy-instance, it shouldn't be, or if it
> should be then the parent global should do it all the time.

LazyInstance seems like a good match as we should only have one instance
per-process. Mostly because of how GetSurface() and the Java IPC part works.
What do you mean by "parent global should do it all the time"?

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:248:
make_scoped_refptr(new gfx::SurfaceTexture(kDummyTextureId));
On 2014/03/12 19:46:28, epennerAtGoogle wrote:
> I think you might be right on the creation part. Unfortunately though, we'll
be
> relying very heavily on the implementation specifics. However most of the
> specifics fall out of the API's constraints, so I don't think any of the
> semantics are subject to change.

I'll probably switch this over to a real texture id when allocation is moved to
the GPU service side.

https://codereview.chromium.org/195583003/diff/20001/content/common/android/s...
File content/common/android/surface_texture_lookup.cc (right):

https://codereview.chromium.org/195583003/diff/20001/content/common/android/s...
content/common/android/surface_texture_lookup.cc:12: SurfaceTextureLookup*
g_instance_ = NULL;
On 2014/03/12 19:46:28, epennerAtGoogle wrote:
> g_instance_ isn't an interface though right? It's a real piece of data. It
could
> as easily be a lazy-instance no?

While g_instance will be a pointer to a real implementation,
SurfaceTextureLookup is not. So there's nothing here I can lazily instantiate.
It's up to the client to create an implementation and instantiate it.

> 
> For example, if you use this interface as the getter, it seems like you could
> have set the pointer already on another thread long ago in the past, but still
> get a stale NULL in the getter, even though you had every expectation that it
> was already set. This would be a DCHECK.
> 
> Excuse me if I'm wrong here, I just like err on the side of extreme caution
when
> it comes to thread safety issues. There was a long discussion on another
thread
> about this so if anything I'll learn something here if I'm wrong ;)

Usage of this API requires the client to set the instance in a thread safe
manner. It's actually preferred to not have this API thread safe as that would
prevent TSan from detecting incorrect usage by a client.

https://codereview.chromium.org/195583003/diff/20001/ui/gl/gl_image_surface_t...
File ui/gl/gl_image_surface_texture.cc (right):

https://codereview.chromium.org/195583003/diff/20001/ui/gl/gl_image_surface_t...
ui/gl/gl_image_surface_texture.cc:41: void DetachFromGLContext(SurfaceTexture*
surface_texture) {
On 2014/03/12 20:52:33, epennerAtGoogle wrote:
> Oh, one thought here would be: should we redefine the texture to free any
> remaining reference to the ST data? Overall if this works it's cleaner than I
> expected.

Yes, we might want to do that in GLImage::ReleaseTexImage later but not sure it
makes a difference with current usage by the compositor. I'm trying to keep
things minimal for now.

> 
> If we ever want the 'early fencing' part we'll need to revisit swapping out
> service IDs, but for one-copy we just need to releaseTexImage after the FBO
> blit, which doesn't have this problem.

Right.

[epennerAtGoogle, 2014-03-12 23:10:14]

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:191:
base::LazyInstance<SurfaceTextureTrackerImpl> g_surface_texture_tracker =
Ah, your other comment cleared up why we can't create the lazy instance in the
other class (since LazyInstance needs to know the type!). So forget this part.
See the last question in my other comment.

https://codereview.chromium.org/195583003/diff/20001/content/common/android/s...
File content/common/android/surface_texture_lookup.cc (right):

https://codereview.chromium.org/195583003/diff/20001/content/common/android/s...
content/common/android/surface_texture_lookup.cc:12: SurfaceTextureLookup*
g_instance_ = NULL;
I get now why we can't use LazyInstance (we don't know the type to create! ;) ).
Sorry about that. However I still don't see how Initialization is synchronized.

When lazy instance accesses it's pointer (the equivalent of GetIntance()) it
needs an Acquire_load or equivalent, on every accesses to the pointer. It's
considerably more expensive than NoBarrier_load IIUC, and in Singleton it was a
recent bug to use NoBarrier_Load. I don't see how we are achieving that
synchronization if we allow calls to GetInstance on another thread. If other
code is synchronizing it currently, can you point me at what mechanism makes it
synchronized?

From my understanding we have:

SurfaceTextureLookup::InitInstance() // Main thread very early.
SurfaceTextureLookup::GetInstance() // Compositor thread later on.

SurfaceTextureTracker::InitInstance(); // Binder (bad client), otherwise GPU
thread.
SurfaceTextureTracker::GetInstance(); // GPU thread.

In the first case, the 'later on' by itself doesn't seem enough to ensure the
pointer (and the object) are correct on the compositor thread.

In the second case we seem mostly okay because Init + GetInstance _should_
happen on the GPU thread if the client behaves correctly, and the binder thread
access will then go through the lazy instance.

The order of these things could also change, and I don't like the idea that Tsan
is required to verify that we are correct (if we were to change the locations of
Init, or allow changing the global pointer later on, etc.)

[epenner, 2014-03-13 19:59:39]

Also, note that this still SGTM (%sievers@ review) I just want to understand if
this is acceptable (in general in Chrome) or if we need to use the thread-safe
Singleton/LazyInstance (when possible - in this case that isn't easy). Given the
current access to these classes, it seems extraordinarily unlikely there would
ever be a problem.

[reveman, 2014-03-13 21:36:00]

PTAL

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/195583003/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:168: jobject
GetSurface(int surface_texture_id) const {
On 2014/03/11 22:59:36, reveman wrote:
> This should probably take a renderer_id to prevent a renderer from stealing a
> surface from another renderer.

Done.

https://codereview.chromium.org/195583003/diff/20001/content/common/android/s...
File content/common/android/surface_texture_lookup.cc (right):

https://codereview.chromium.org/195583003/diff/20001/content/common/android/s...
content/common/android/surface_texture_lookup.cc:12: SurfaceTextureLookup*
g_instance_ = NULL;
On 2014/03/12 23:10:14, epennerAtGoogle wrote:
> I get now why we can't use LazyInstance (we don't know the type to create! ;)
).
> Sorry about that. However I still don't see how Initialization is
synchronized.
> 
> When lazy instance accesses it's pointer (the equivalent of GetIntance()) it
> needs an Acquire_load or equivalent, on every accesses to the pointer. It's
> considerably more expensive than NoBarrier_load IIUC, and in Singleton it was
a
> recent bug to use NoBarrier_Load. I don't see how we are achieving that
> synchronization if we allow calls to GetInstance on another thread. If other
> code is synchronizing it currently, can you point me at what mechanism makes
it
> synchronized?

You're making a valid point. While the previous patch is unlikely to cause
issues in practice I think we should get this right in theory too. There was at
least one issue with the previous patch that I would have overlooked if you
wouldn't have pointed this out so thanks for doing that.

These type of global instances are usually set very early before any threads are
spawned to ensure thread safe usage and I think that's what we should be doing
here too.

Latest patch should be doing that in both cases.

> 
> From my understanding we have:
> 
> SurfaceTextureLookup::InitInstance() // Main thread very early.
> SurfaceTextureLookup::GetInstance() // Compositor thread later on.

This is safe as InitInstance() happens before we spawn the compositor thread.

> 
> SurfaceTextureTracker::InitInstance(); // Binder (bad client), otherwise GPU
> thread.

In previous patch, this was:
// Binder or GPU thread (bad client), otherwise Browser IO thread.

In latest patch, this happens on Browser UI thread very early (from
PreEarlyInitialization()) which should make it safe.

> SurfaceTextureTracker::GetInstance(); // GPU thread.
> 
> In the first case, the 'later on' by itself doesn't seem enough to ensure the
> pointer (and the object) are correct on the compositor thread.
> 
> In the second case we seem mostly okay because Init + GetInstance _should_
> happen on the GPU thread if the client behaves correctly, and the binder
thread
> access will then go through the lazy instance.
> 
> The order of these things could also change, and I don't like the idea that
Tsan
> is required to verify that we are correct (if we were to change the locations
of
> Init, or allow changing the global pointer later on, etc.)

I don't think we should allow the global pointer to change. There are DCHECKs in
place to prevent this. If someone moves InitInstance around they might mess up
the ordering and we rely on TSan to detect that. That's good enough imo.

https://codereview.chromium.org/195583003/diff/20001/ui/gfx/gpu_memory_buffer.h
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/195583003/diff/20001/ui/gfx/gpu_memory_buffer...
ui/gfx/gpu_memory_buffer.h:52: uint32 surface_texture_id;
On 2014/03/12 19:46:28, epennerAtGoogle wrote:
> Yeppers!

I started doing this but realized I need to put it in a separate file to do it
properly. ui/gfx/android/surface_texture_id.h or something like that. Not sure
that's worth it. At least I made it consistently "int" in latest patch but I
don't think that addresses your concern. I think "int" is fine but I'll go ahead
and add a new file for this if you feel strongly about it. Let me know.

[epenner, 2014-03-13 22:37:24]

> These type of global instances are usually set very early
> before any threads are spawned to ensure thread safe usage
> and I think that's what we should be doing here too.

Okay that sounds solid now and I like that as a method of
avoiding the Lazy-instance cost in other cases also! Can you
add a comment on Init to that effect (that init should happen
before other threads are spawned, to be absolutely safe).

I'm torn on the 'id type' needing a file etc. So up to you.

[reveman, 2014-03-13 23:04:10]

On 2014/03/13 22:37:24, epenner wrote:
> > These type of global instances are usually set very early
> > before any threads are spawned to ensure thread safe usage
> > and I think that's what we should be doing here too.
> 
> Okay that sounds solid now and I like that as a method of
> avoiding the Lazy-instance cost in other cases also! Can you
> add a comment on Init to that effect (that init should happen
> before other threads are spawned, to be absolutely safe).

Good idea. I added a comment next to each InitInstance call making it clear that
it needs to be called before a certain event. That should give us some
protection against someone moving this to the wrong place.

> 
> I'm torn on the 'id type' needing a file etc. So up to you.

I'm going to keep it as an 'int' for now. I find it a bit cleaner when it needs
to enter Java land.

[no sievers, 2014-03-14 01:24:51]

Very cool!

Can we move all the code from CompositorImpl to somewhere else? Maybe it can
just be under gpu/ or ui/gl somewhere and we just have to instantiate it in the
browser (from browser_main_loop.cc or so where we also create
SurfaceTexturePeerBrowserImpl), but would then also be able to instantiate it in
the GPU process.

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

https://codereview.chromium.org/195583003/diff/60001/content/app/android/chil...
content/app/android/child_process_service.cc:88: const
base::android::ScopedJavaLocalRef<jobject>& service)
Do you maybe just fold this into the existing SurfaceTexturePeerChildImpl above
instead? Then we need one less new interface.

It should really be renamed, but it's already used for similar things happening
both from renderer and gpu child process.

https://codereview.chromium.org/195583003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/195583003/diff/60001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:159: int
AddSurfaceTexture(gfx::SurfaceTexture* surface_texture,
How do they ever get deleted? :)

https://codereview.chromium.org/195583003/diff/60001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:165: base::AutoLock
lock(surface_textures_lock_);
You should hold the lock when modifying next_surface_id_ above.

https://codereview.chromium.org/195583003/diff/60001/ui/gl/gl_image_surface_t...
File ui/gl/gl_image_surface_texture.cc (right):

https://codereview.chromium.org/195583003/diff/60001/ui/gl/gl_image_surface_t...
ui/gl/gl_image_surface_texture.cc:27: eglMakeCurrent(display_, EGL_NO_SURFACE,
EGL_NO_SURFACE, EGL_NO_CONTEXT);
I was going to suggest using ui::ScopedMakeCurrent, but looks like you'd have to
add support in there for calling with NULL/NULL for context/surface and then
have it call this in the constructor:
previous_context_->ReleaseCurrent(NULL);

https://codereview.chromium.org/195583003/diff/60001/ui/gl/gl_image_surface_t...
ui/gl/gl_image_surface_texture.cc:64:
DetachFromGLContext(surface_texture_.get());
Is this necessary? I *thought* it does not try to delete the texture if you
don't call Detach, so we should be ok in doing nothing here.

[reveman, 2014-03-14 19:44:52]

SurfaceTextureTrackerImpl doesn't have to be in CompositorImpl but I think it
belongs in content/ as it needs to be aware of the notion of a renderer and a
render_process_id. I put the code in CompositorImpl for now as we handle similar
Java IPC there.

There's really two interfaces here.

1. The CompositorImpl functions:
::AllocateSurfaceTexture
::GetSurfaceTextureSurface
these are implemented by the browser and access SurfaceTextureTrackerImpl
directly rather than using GPU process IPC. The implementation of these will
change when we add out-of-proc GPU support. Maybe I should add an explicit
interface for these...

2. gfx::SurfaceTextureTracker with AcquireSurfaceTexture function. This is used
by the GPU service and would be instantiated in the GPU process with out-of-proc
GPU.

SurfaceTextureTrackerImpl just happens to implement both of these interfaces
right now when using in-proc GPU.

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

https://codereview.chromium.org/195583003/diff/60001/content/app/android/chil...
content/app/android/child_process_service.cc:88: const
base::android::ScopedJavaLocalRef<jobject>& service)
On 2014/03/14 01:24:51, sievers wrote:
> Do you maybe just fold this into the existing SurfaceTexturePeerChildImpl
above
> instead? Then we need one less new interface.
> 
> It should really be renamed, but it's already used for similar things
happening
> both from renderer and gpu child process.

To fold this into the above class I'd like to merge
content::SurfaceTextureLookup with content::GpuSurfaceLookup and not have two
different AcquireNativeWidget functions somehow. Any suggestions for how to do
that? Maybe ChildProcessService_getSurfaceTextureSurface can be used in both
cases and establishSurfaceTexturePeer will register the surface with the
SurfaceTextureTrackerImpl on the browser side.

I'm very interested unifying as much as possible of this code but maybe better
to do this as a follow up? I'm reluctant to making too many changes to existing
code paths in this initial patch.

https://codereview.chromium.org/195583003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/195583003/diff/60001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:159: int
AddSurfaceTexture(gfx::SurfaceTexture* surface_texture,
On 2014/03/14 01:24:51, sievers wrote:
> How do they ever get deleted? :)

Removed using AcquireSurfaceTexture() above, which will be called from the GPU
thread.

https://codereview.chromium.org/195583003/diff/60001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:165: base::AutoLock
lock(surface_textures_lock_);
On 2014/03/14 01:24:51, sievers wrote:
> You should hold the lock when modifying next_surface_id_ above.

This should always be called from browser IO thread so it's only
surface_textures_ that needs to be guarded by a lock, hence the name
|surface_textures_lock_|

I added a CalledOnValidThread() check here to make sure this is used correctly.

https://codereview.chromium.org/195583003/diff/60001/ui/gl/gl_image_surface_t...
File ui/gl/gl_image_surface_texture.cc (right):

https://codereview.chromium.org/195583003/diff/60001/ui/gl/gl_image_surface_t...
ui/gl/gl_image_surface_texture.cc:27: eglMakeCurrent(display_, EGL_NO_SURFACE,
EGL_NO_SURFACE, EGL_NO_CONTEXT);
On 2014/03/14 01:24:51, sievers wrote:
> I was going to suggest using ui::ScopedMakeCurrent, but looks like you'd have
to
> add support in there for calling with NULL/NULL for context/surface and then
> have it call this in the constructor:
> previous_context_->ReleaseCurrent(NULL);

I was worried that this might have some other side effects. ie.
unbind_fbo_on_makecurrent_ property on the GLContextEGL causing trouble.

https://codereview.chromium.org/195583003/diff/60001/ui/gl/gl_image_surface_t...
ui/gl/gl_image_surface_texture.cc:64:
DetachFromGLContext(surface_texture_.get());
On 2014/03/14 01:24:51, sievers wrote:
> Is this necessary? I *thought* it does not try to delete the texture if you
> don't call Detach, so we should be ok in doing nothing here.

Yes, I think you're right. This also allows me to make BindTexImage more
efficient. This is one weird API..

[no sievers, 2014-03-18 20:58:02]

On 2014/03/14 19:44:52, reveman wrote:
> SurfaceTextureTrackerImpl doesn't have to be in CompositorImpl but I think it
> belongs in content/ as it needs to be aware of the notion of a renderer and a
> render_process_id. I put the code in CompositorImpl for now as we handle
similar
> Java IPC there.
> 
> There's really two interfaces here.
> 
> 1. The CompositorImpl functions:
> ::AllocateSurfaceTexture
> ::GetSurfaceTextureSurface
> these are implemented by the browser and access SurfaceTextureTrackerImpl
> directly rather than using GPU process IPC. The implementation of these will
> change when we add out-of-proc GPU support. Maybe I should add an explicit
> interface for these...
> 
> 2. gfx::SurfaceTextureTracker with AcquireSurfaceTexture function. This is
used
> by the GPU service and would be instantiated in the GPU process with
out-of-proc
> GPU.
> 


So if we implemented this for out-of-process gpu, can't this really do entirely
the same thing except for that we forward getSurfaceTextureSurface() from
ChildProcessLauncher.java to the GPU child process with another binder call and
have it all synchronously return the Java Surface.

If the impl for 1. and 2. lived in content/common/gpu or somewhere, it'd work
for both cases with the exception of AllocateSurfaceTexture() which we would
somehow have to figure out (either forward from browser to gpu process for
Android, or make the call from renderer to gpu process directly on Android).



> SurfaceTextureTrackerImpl just happens to implement both of these interfaces
> right now when using in-proc GPU.
> 
>
https://codereview.chromium.org/195583003/diff/60001/content/app/android/chil...
> File content/app/android/child_process_service.cc (right):
> 
>
https://codereview.chromium.org/195583003/diff/60001/content/app/android/chil...
> content/app/android/child_process_service.cc:88: const
> base::android::ScopedJavaLocalRef<jobject>& service)
> On 2014/03/14 01:24:51, sievers wrote:
> > Do you maybe just fold this into the existing SurfaceTexturePeerChildImpl
> above
> > instead? Then we need one less new interface.
> > 
> > It should really be renamed, but it's already used for similar things
> happening
> > both from renderer and gpu child process.
> 
> To fold this into the above class I'd like to merge
> content::SurfaceTextureLookup with content::GpuSurfaceLookup and not have two
> different AcquireNativeWidget functions somehow. Any suggestions for how to do
> that? Maybe ChildProcessService_getSurfaceTextureSurface can be used in both
> cases and establishSurfaceTexturePeer will register the surface with the
> SurfaceTextureTrackerImpl on the browser side.
> 
> I'm very interested unifying as much as possible of this code but maybe better
> to do this as a follow up? I'm reluctant to making too many changes to
existing
> code paths in this initial patch.
> 
>
https://codereview.chromium.org/195583003/diff/60001/content/browser/renderer...
> File content/browser/renderer_host/compositor_impl_android.cc (right):
> 
>
https://codereview.chromium.org/195583003/diff/60001/content/browser/renderer...
> content/browser/renderer_host/compositor_impl_android.cc:159: int
> AddSurfaceTexture(gfx::SurfaceTexture* surface_texture,
> On 2014/03/14 01:24:51, sievers wrote:
> > How do they ever get deleted? :)
> 
> Removed using AcquireSurfaceTexture() above, which will be called from the GPU
> thread.
> 
>
https://codereview.chromium.org/195583003/diff/60001/content/browser/renderer...
> content/browser/renderer_host/compositor_impl_android.cc:165: base::AutoLock
> lock(surface_textures_lock_);
> On 2014/03/14 01:24:51, sievers wrote:
> > You should hold the lock when modifying next_surface_id_ above.
> 
> This should always be called from browser IO thread so it's only
> surface_textures_ that needs to be guarded by a lock, hence the name
> |surface_textures_lock_|
> 
> I added a CalledOnValidThread() check here to make sure this is used
correctly.
> 
>
https://codereview.chromium.org/195583003/diff/60001/ui/gl/gl_image_surface_t...
> File ui/gl/gl_image_surface_texture.cc (right):
> 
>
https://codereview.chromium.org/195583003/diff/60001/ui/gl/gl_image_surface_t...
> ui/gl/gl_image_surface_texture.cc:27: eglMakeCurrent(display_, EGL_NO_SURFACE,
> EGL_NO_SURFACE, EGL_NO_CONTEXT);
> On 2014/03/14 01:24:51, sievers wrote:
> > I was going to suggest using ui::ScopedMakeCurrent, but looks like you'd
have
> to
> > add support in there for calling with NULL/NULL for context/surface and then
> > have it call this in the constructor:
> > previous_context_->ReleaseCurrent(NULL);
> 
> I was worried that this might have some other side effects. ie.
> unbind_fbo_on_makecurrent_ property on the GLContextEGL causing trouble.
> 
>
https://codereview.chromium.org/195583003/diff/60001/ui/gl/gl_image_surface_t...
> ui/gl/gl_image_surface_texture.cc:64:
> DetachFromGLContext(surface_texture_.get());
> On 2014/03/14 01:24:51, sievers wrote:
> > Is this necessary? I *thought* it does not try to delete the texture if you
> > don't call Detach, so we should be ok in doing nothing here.
> 
> Yes, I think you're right. This also allows me to make BindTexImage more
> efficient. This is one weird API..

[no sievers, 2014-03-18 20:58:12]

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

https://codereview.chromium.org/195583003/diff/60001/content/app/android/chil...
content/app/android/child_process_service.cc:88: const
base::android::ScopedJavaLocalRef<jobject>& service)
On 2014/03/14 19:44:53, reveman wrote:
> On 2014/03/14 01:24:51, sievers wrote:
> > Do you maybe just fold this into the existing SurfaceTexturePeerChildImpl
> above
> > instead? Then we need one less new interface.
> > 
> > It should really be renamed, but it's already used for similar things
> happening
> > both from renderer and gpu child process.
> 
> To fold this into the above class I'd like to merge
> content::SurfaceTextureLookup with content::GpuSurfaceLookup and not have two
> different AcquireNativeWidget functions somehow. Any suggestions for how to do
> that? Maybe ChildProcessService_getSurfaceTextureSurface can be used in both
> cases and establishSurfaceTexturePeer will register the surface with the
> SurfaceTextureTrackerImpl on the browser side.
> 
> I'm very interested unifying as much as possible of this code but maybe better
> to do this as a follow up? I'm reluctant to making too many changes to
existing
> code paths in this initial patch.

For the view surface lookup, it's slightly different: GpuSurfaceTracker lives in
the browser and implements GpuSurfaceLookup for in-process-gpu. For
out-of-process-gpu it uses  SurfaceTexturePeerChildImp to implement
GpuSurfaceLookup which then goes to the browser. It would be nice to refactor it
so that it also ends up talking to GpuSurfaceTracker in the browser in the
second case, instead of talking to CompositorImpl there. But it would need some
opaque type to be returned from GpuSurfaceTracker so that we can get to the Java
Surface.

Still not fully sure if it's feasible to merge it with this usage case, because
in this case we then would really need to go from the browser and forward the
lookup to the GPU process, while for the view surface the lookup is in the
reverse direction.

Overall definitely sounds more like a follow-up thing. But still seems find to
merge the interface here into the above on the client side here, since you also
merge it into the existing .aidl binder interface anyway.

https://codereview.chromium.org/195583003/diff/120001/ui/gl/gl_image_surface_...
File ui/gl/gl_image_surface_texture.cc (right):

https://codereview.chromium.org/195583003/diff/120001/ui/gl/gl_image_surface_...
ui/gl/gl_image_surface_texture.cc:58:
SurfaceTextureTracker::GetInstance()->AcquireSurfaceTexture(
This handle comes from the client, so we also need to avoid somehow that a
malicious renderer can get access to another renderer's texture and modify it.
(Even if cannot get the Surface handle, because there we do check the child
process id, it could still modify the texture through other means.)

You can get the renderer_pid() from GpuChannel.

Or we could also avoid the problem similar to how we do it on other platforms by
not having a global surface tracker, but instead not making lookup possible
outside of the context group here.

https://codereview.chromium.org/195583003/diff/120001/ui/gl/gl_image_surface_...
ui/gl/gl_image_surface_texture.cc:95: surface_texture_->UpdateTexImage();
So whenever we updated the contents from the client side (releaseTexImage, lock,
unlockAndPost), we'd end up calling glBindTexImage2D again, right?

Maybe nothing to worry about now, but note that unfortunately SurfaceTexture
does not support the idea of GL share groups. It checks and remembers the
context handle whenever you call a SurfaceTexture API that needs a context (such
as updateTexImage()) and bails out if you ever call such again with a different
context current.

[reveman, 2014-03-24 18:35:06]

PTAL

https://codereview.chromium.org/195583003/diff/120001/ui/gl/gl_image_surface_...
File ui/gl/gl_image_surface_texture.cc (right):

https://codereview.chromium.org/195583003/diff/120001/ui/gl/gl_image_surface_...
ui/gl/gl_image_surface_texture.cc:58:
SurfaceTextureTracker::GetInstance()->AcquireSurfaceTexture(
On 2014/03/18 20:58:13, sievers wrote:
> This handle comes from the client, so we also need to avoid somehow that a
> malicious renderer can get access to another renderer's texture and modify it.
> (Even if cannot get the Surface handle, because there we do check the child
> process id, it could still modify the texture through other means.)
> 
> You can get the renderer_pid() from GpuChannel.
> 
> Or we could also avoid the problem similar to how we do it on other platforms
by
> not having a global surface tracker, but instead not making lookup possible
> outside of the context group here.

Gpu memory buffers are global on other platforms. Once you've created a gpu
memory buffer, you can register it with any command buffer instance you like.
Security comes from ownership being passed by the IPC system, which is of course
not the case with surface textures on Android.

Latest patch should address this. The gpu service is now validating the surface
texture id before registering the gpu memory buffer.

https://codereview.chromium.org/195583003/diff/120001/ui/gl/gl_image_surface_...
ui/gl/gl_image_surface_texture.cc:95: surface_texture_->UpdateTexImage();
On 2014/03/18 20:58:13, sievers wrote:
> So whenever we updated the contents from the client side (releaseTexImage,
lock,
> unlockAndPost), we'd end up calling glBindTexImage2D again, right?

Yes, no guarantee that updated contents will be used unless BindTexImage2D is
called. 

> 
> Maybe nothing to worry about now, but note that unfortunately SurfaceTexture
> does not support the idea of GL share groups. It checks and remembers the
> context handle whenever you call a SurfaceTexture API that needs a context
(such
> as updateTexImage()) and bails out if you ever call such again with a
different
> context current.

Ok, good to know.

https://codereview.chromium.org/195583003/diff/140001/content/app/android/chi...
File content/app/android/child_process_service.cc (right):

https://codereview.chromium.org/195583003/diff/140001/content/app/android/chi...
content/app/android/child_process_service.cc:35: public SurfaceTextureLookup {
I merged the implementation but not the interfaces. Did you want me to also
merge the interfaces? I can do that but need some advice on the naming.

GpuSurfaceLookup+SurfaceTextureLookup=?

We'll need to rename the functions too in that case.
AcquireNativeWidgetForViewSurface and AcquireNativeWidgetForSurfaceTexture?

[no sievers, 2014-03-24 19:26:07]

LGTM with nits.

Still think the tracker impl should be somewhere other than inside
CompositorImpl. For example, WebView doesn't even instantiate a browser
compositor and wouldn't be able to use this. Also we need to think if this will
work right with out-of-process GPU. But seems ok to do as a follow-up, as long
as we don't end up having to rewire everything :)

https://codereview.chromium.org/195583003/diff/140001/content/app/android/chi...
File content/app/android/child_process_service.cc (right):

https://codereview.chromium.org/195583003/diff/140001/content/app/android/chi...
content/app/android/child_process_service.cc:35: public SurfaceTextureLookup {
On 2014/03/24 18:35:07, reveman wrote:
> I merged the implementation but not the interfaces. Did you want me to also
> merge the interfaces? I can do that but need some advice on the naming.
> 
> GpuSurfaceLookup+SurfaceTextureLookup=?
> 

Maybe just leave it as GpuSurfaceLookup?

> We'll need to rename the functions too in that case.
> AcquireNativeWidgetForViewSurface and AcquireNativeWidgetForSurfaceTexture?

Sounds good.

https://codereview.chromium.org/195583003/diff/140001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/195583003/diff/140001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.h:53: static jobject
GetSurfaceTextureSurface(int primary_id, int secondary_id);
Why did you follow the bad example from the existing EstablishPeer code and
changed the variable names to be so generic? :)


Could it just be |surface_texture_id| and |child_process_id| or something like
that?

[reveman, 2014-03-25 01:31:23]

It's easy to move the SurfaceTrackerImpl as it's a global instance. Just let me
know what would be a better place. Maybe content/common/gpu somewhere?

I'm going to work on multi-proc support next. The idea is to replace this
SurfaceTrackerImpl with some GPU process IPC on the browser side and instead
instantiate the SurfaceTrackerImpl on the GPU service side. I don't think it
needs to change much from it's current implementation.

I'd prefer to land this patch before the multi-proc support is ready to land but
don't mind holding off a bit if you prefer that.

https://codereview.chromium.org/195583003/diff/140001/content/app/android/chi...
File content/app/android/child_process_service.cc (right):

https://codereview.chromium.org/195583003/diff/140001/content/app/android/chi...
content/app/android/child_process_service.cc:35: public SurfaceTextureLookup {
On 2014/03/24 19:26:08, sievers wrote:
> On 2014/03/24 18:35:07, reveman wrote:
> > I merged the implementation but not the interfaces. Did you want me to also
> > merge the interfaces? I can do that but need some advice on the naming.
> > 
> > GpuSurfaceLookup+SurfaceTextureLookup=?
> > 
> 
> Maybe just leave it as GpuSurfaceLookup?
> 
> > We'll need to rename the functions too in that case.
> > AcquireNativeWidgetForViewSurface and AcquireNativeWidgetForSurfaceTexture?
> 
> Sounds good.

Oh, I forgot to mention one reason why I kept these interfaces separate.
GpuSurfaceLookup is not android specific. It lives in content/common/gpu/, while
surface textures are very android specific and content/common/android/ seemed
like a better home for an interface that explicitly relate to surface textures.

Still want me to merge these interfaces?

https://codereview.chromium.org/195583003/diff/140001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/195583003/diff/140001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.h:53: static jobject
GetSurfaceTextureSurface(int primary_id, int secondary_id);
On 2014/03/24 19:26:08, sievers wrote:
> Why did you follow the bad example from the existing EstablishPeer code and
> changed the variable names to be so generic? :)
> 
> 
> Could it just be |surface_texture_id| and |child_process_id| or something like
> that?

Changed it to an explicit base::ProcessHandle where I think is appropriate in
latest patch. I kept the detail that we use a process handle as part of the id
hidden from the renderer side and from the ui/gl/ code. I prefer if this is only
exposed to the code that depends on it. That's content/browser,
content/common/gpu/gpu_command_buffer_stub.cc and
content/public/android/java/src/org/chromium/content/browser.

Does that make sense?

[no sievers, 2014-03-25 01:43:01]

On 2014/03/25 01:31:23, reveman wrote:
> It's easy to move the SurfaceTrackerImpl as it's a global instance. Just let
me
> know what would be a better place. Maybe content/common/gpu somewhere?
> 
> I'm going to work on multi-proc support next. The idea is to replace this
> SurfaceTrackerImpl with some GPU process IPC on the browser side and instead
> instantiate the SurfaceTrackerImpl on the GPU service side. I don't think it
> needs to change much from it's current implementation.
> 
> I'd prefer to land this patch before the multi-proc support is ready to land
but
> don't mind holding off a bit if you prefer that.
> 

Nah, ship it!


>
https://codereview.chromium.org/195583003/diff/140001/content/app/android/chi...
> File content/app/android/child_process_service.cc (right):
> 
>
https://codereview.chromium.org/195583003/diff/140001/content/app/android/chi...
> content/app/android/child_process_service.cc:35: public SurfaceTextureLookup {
> On 2014/03/24 19:26:08, sievers wrote:
> > On 2014/03/24 18:35:07, reveman wrote:
> > > I merged the implementation but not the interfaces. Did you want me to
also
> > > merge the interfaces? I can do that but need some advice on the naming.
> > > 
> > > GpuSurfaceLookup+SurfaceTextureLookup=?
> > > 
> > 
> > Maybe just leave it as GpuSurfaceLookup?
> > 
> > > We'll need to rename the functions too in that case.
> > > AcquireNativeWidgetForViewSurface and
AcquireNativeWidgetForSurfaceTexture?
> > 
> > Sounds good.
> 
> Oh, I forgot to mention one reason why I kept these interfaces separate.
> GpuSurfaceLookup is not android specific. It lives in content/common/gpu/,
while
> surface textures are very android specific and content/common/android/ seemed
> like a better home for an interface that explicitly relate to surface
textures.
> 
> Still want me to merge these interfaces?
> 

Oh, missed that. Seems fine then.

>
https://codereview.chromium.org/195583003/diff/140001/content/browser/rendere...
> File content/browser/renderer_host/compositor_impl_android.h (right):
> 
>
https://codereview.chromium.org/195583003/diff/140001/content/browser/rendere...
> content/browser/renderer_host/compositor_impl_android.h:53: static jobject
> GetSurfaceTextureSurface(int primary_id, int secondary_id);
> On 2014/03/24 19:26:08, sievers wrote:
> > Why did you follow the bad example from the existing EstablishPeer code and
> > changed the variable names to be so generic? :)
> > 
> > 
> > Could it just be |surface_texture_id| and |child_process_id| or something
like
> > that?
> 
> Changed it to an explicit base::ProcessHandle where I think is appropriate in
> latest patch. I kept the detail that we use a process handle as part of the id
> hidden from the renderer side and from the ui/gl/ code. I prefer if this is
only
> exposed to the code that depends on it. That's content/browser,
> content/common/gpu/gpu_command_buffer_stub.cc and
> content/public/android/java/src/org/chromium/content/browser.
> 
> Does that make sense?

Yea, that sounds even better. I assume now it's also impossible to mix them up,
because the one would need an explicit cast to int.

[reveman, 2014-03-26 21:00:58]

+piman

I started working on out-of-process GPU service support to make sure the current
design fits well. The design doc has some more details related to this but the
general idea is:

1. Make GpuMemoryBuffer allocation IPC async. We need this for performance
anyhow. Also makes it much easier to handle the necessary Java IPC in an
efficient manner.
2. Refactor current GpuChannelHostFactory::CreateImage/DeleteImage interface to
be a more generic interface that can be used for surface textures and future
GpuMemoryBuffer types that need to be allocated on the GPU service side.
3. Replace current in-process-GPU code with some code that uses the new
GpuChannelHostFactory:: interface to allocate surface textures on the GPU
service side.

The final patch will look very different but I uploaded this as it will give you
an idea of where some of the code will be moved in an out-of-proc GPU world:
https://codereview.chromium.org/213493002/

I'd like to land this in-proc GPU version first so we can start working on
necessary compositor changes and get an idea of the performance we can expect
from using surface textures.

Antoine, please take a look.

Design doc:
https://docs.google.com/a/chromium.org/document/d/1jHmGdxwCZdo9Ypeeh-u8DPsXgt...

[no sievers, 2014-03-27 00:22:17]

On 2014/03/26 21:00:58, reveman wrote:
> +piman
> 
> I started working on out-of-process GPU service support to make sure the
current
> design fits well. The design doc has some more details related to this but the
> general idea is:
> 
> 1. Make GpuMemoryBuffer allocation IPC async. We need this for performance
> anyhow. Also makes it much easier to handle the necessary Java IPC in an
> efficient manner.
> 2. Refactor current GpuChannelHostFactory::CreateImage/DeleteImage interface
to
> be a more generic interface that can be used for surface textures and future
> GpuMemoryBuffer types that need to be allocated on the GPU service side.
> 3. Replace current in-process-GPU code with some code that uses the new
> GpuChannelHostFactory:: interface to allocate surface textures on the GPU
> service side.
> 

That sounds great!

> The final patch will look very different but I uploaded this as it will give
you
> an idea of where some of the code will be moved in an out-of-proc GPU world:
> https://codereview.chromium.org/213493002/
> 
> I'd like to land this in-proc GPU version first so we can start working on
> necessary compositor changes and get an idea of the performance we can expect
> from using surface textures.
> 
> Antoine, please take a look.
> 
> Design doc:
>
https://docs.google.com/a/chromium.org/document/d/1jHmGdxwCZdo9Ypeeh-u8DPsXgt...

[piman, 2014-03-27 01:07:25]

https://codereview.chromium.org/195583003/diff/160001/content/browser/android...
File content/browser/android/child_process_launcher_android.cc (right):

https://codereview.chromium.org/195583003/diff/160001/content/browser/android...
content/browser/android/child_process_launcher_android.cc:172: return
CompositorImpl::GetSurfaceTextureSurface(surface_texture_id, pid);
Should there be a check somewhere to ensure a renderer can't access a ST from
another renderer?

https://codereview.chromium.org/195583003/diff/160001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/195583003/diff/160001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:190: :
it->second->surface.j_surface().obj();
Is the jobject returned thread safe? What if RemoveAllSurfaceTextures is called
on one thread while the jobject is used on another thread?

https://codereview.chromium.org/195583003/diff/160001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/195583003/diff/160001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:877: channel()->renderer_pid()) {
nit: rather than the PID, use the channel's client_id_ (which is the renderer
process id, render_process_id_ in RenderMessageFilter or GpuMessageFilter), it's
more portable and keeps the separation independent of how renderer processes are
used by various compositors.
Also, PIDs get reused (small namespace), whereas render_process_id_ don't.

https://codereview.chromium.org/195583003/diff/160001/ui/gfx/gpu_memory_buffer.h
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/195583003/diff/160001/ui/gfx/gpu_memory_buffe...
ui/gfx/gpu_memory_buffer.h:59: SurfaceTextureId surface_texture_id;
nit: can we factor that into the #if in line 53?

https://codereview.chromium.org/195583003/diff/160001/ui/gl/gl_image_surface_...
File ui/gl/gl_image_surface_texture.cc (right):

https://codereview.chromium.org/195583003/diff/160001/ui/gl/gl_image_surface_...
ui/gl/gl_image_surface_texture.cc:91:
DetachFromGLContext(surface_texture_.get());
Do we need this (including the "ugly hack") given the test above?
We'd only go through this code if texture_id_ is 0. Doesn't that mean we never
attached to a context?

[reveman, 2014-03-27 14:48:10]

The jobject ref problem still needs to be fixed but PTAL while I figure out the
best way to do that.

https://codereview.chromium.org/195583003/diff/160001/content/browser/android...
File content/browser/android/child_process_launcher_android.cc (right):

https://codereview.chromium.org/195583003/diff/160001/content/browser/android...
content/browser/android/child_process_launcher_android.cc:172: return
CompositorImpl::GetSurfaceTextureSurface(surface_texture_id, pid);
On 2014/03/27 01:07:25, piman wrote:
> Should there be a check somewhere to ensure a renderer can't access a ST from
> another renderer?

This is done in the Java code. See line 436 in ChildProcessLauncher.java of
latest patch.

https://codereview.chromium.org/195583003/diff/160001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/195583003/diff/160001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:190: :
it->second->surface.j_surface().obj();
On 2014/03/27 01:07:25, piman wrote:
> Is the jobject returned thread safe? What if RemoveAllSurfaceTextures is
called
> on one thread while the jobject is used on another thread?

Good call. We probably have to take a ref here and hold on to it until the
binder thread is done with the current IPC call. I'll look into this.

@sievers, this is a problem with the existing getViewSurface IPC as well, right?

https://codereview.chromium.org/195583003/diff/160001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/195583003/diff/160001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:877: channel()->renderer_pid()) {
On 2014/03/27 01:07:25, piman wrote:
> nit: rather than the PID, use the channel's client_id_ (which is the renderer
> process id, render_process_id_ in RenderMessageFilter or GpuMessageFilter),
it's
> more portable and keeps the separation independent of how renderer processes
are
> used by various compositors.
> Also, PIDs get reused (small namespace), whereas render_process_id_ don't.

Oh, I didn't know that we shared this id the GPU service. That's great! Cleans
up a lot of the Java code and will make out-of-process GPU support easier to
implement as we don't have to worry about id reuse.

https://codereview.chromium.org/195583003/diff/160001/ui/gfx/gpu_memory_buffer.h
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/195583003/diff/160001/ui/gfx/gpu_memory_buffe...
ui/gfx/gpu_memory_buffer.h:59: SurfaceTextureId surface_texture_id;
On 2014/03/27 01:07:25, piman wrote:
> nit: can we factor that into the #if in line 53?

Done.

https://codereview.chromium.org/195583003/diff/160001/ui/gl/gl_image_surface_...
File ui/gl/gl_image_surface_texture.cc (right):

https://codereview.chromium.org/195583003/diff/160001/ui/gl/gl_image_surface_...
ui/gl/gl_image_surface_texture.cc:91:
DetachFromGLContext(surface_texture_.get());
On 2014/03/27 01:07:25, piman wrote:
> Do we need this (including the "ugly hack") given the test above?
> We'd only go through this code if texture_id_ is 0. Doesn't that mean we never
> attached to a context?

The SurfaceTexture implementation also takes ownership of the texture passed to
the ctor. We currently pass a dummy texture id to the ctor as it's done on the
IO thread in the browser where we can't easily generate a real texture id.

This hack would be to prevent the surface texture implementation from calling
glDeleteTextures with the dummy texture id. However, looking at the spec it
seems like the GL is supposed to silently ignore 0 when passed to
glDeleteTextures so it should be safe to just call DetachFromGLContext directly
here and not do the nasty context binding trick.

Removed the hack and added some comments to better explain what's happening
here.

[piman, 2014-03-27 17:51:15]

thanks for the comments. LGTM % figuring out the jobject thing.

[no sievers, 2014-03-27 18:25:25]

https://codereview.chromium.org/195583003/diff/160001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/195583003/diff/160001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:190: :
it->second->surface.j_surface().obj();
On 2014/03/27 14:48:11, reveman wrote:
> On 2014/03/27 01:07:25, piman wrote:
> > Is the jobject returned thread safe? What if RemoveAllSurfaceTextures is
> called
> > on one thread while the jobject is used on another thread?
> 
> Good call. We probably have to take a ref here and hold on to it until the
> binder thread is done with the current IPC call. I'll look into this.
> 
> @sievers, this is a problem with the existing getViewSurface IPC as well,
right?

There are a few things:

- The SurfaceTexture native object, which is RefCountedThreadSafe, which overall
holds one global strong ref to a Java SurfaceTexture. (and otherwise just
forwards calls through JNI to the Java object.)

- The SurfaceTexture Java object which is thread-safe... well.. some APIs can be
called from any thread, others need to be called with a certain context current
(the first one you start using any of those APIs with, such as updateTexImage).

- The Surface Java object which is created from the SurfaceTexture and provides
the producer interface.

- The ScopedJavaSurface which creates such a Java Surface and then holds a
global ref to it.

Now, I *think* there is a small race possible with just returning the naked
obj() handle here, because dalvik will have to go an acquire a ref to it also
(while our mutex might be already released). It's not clear to me from the
documentation whether you have to leak a LocalRef here, so that it only gets
released when you are entirely returning to Java, or if it's maybe good enough
to just create a ScopedLocalRef here (and assume it does the right thing by
deferring the release).
(Or if JNI is entirely protected with locks to prevent such scenarios - I doubt
it.)

I think I'd just create a ScopedLocalRef for the surface obj here to show your
good intention (and avoid a leak) :)

[reveman, 2014-03-28 01:19:17]

+cdn for content/common/child_process_messages.h
+sky for ui/gfx

[sky, 2014-03-28 15:38:26]

ui/gfx/gpu_memory_buffer.h LGTM

[Cris Neckar, 2014-03-28 17:00:24]

IPC changes LGTM

[reveman, 2014-03-28 18:44:22]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-03-28 18:45:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/195583003/200001

[commit-bot: I haz the power, 2014-03-28 20:28:52]

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

[commit-bot: I haz the power, 2014-03-28 20:28:52]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[reveman, 2014-03-28 20:37:18]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-03-28 20:38:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/195583003/200001

[commit-bot: I haz the power, 2014-03-28 21:38:19]

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

[commit-bot: I haz the power, 2014-03-28 21:38:20]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[reveman, 2014-03-28 21:41:54]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-03-28 21:50:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/195583003/200001

[commit-bot: I haz the power, 2014-03-28 23:45:13]

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

[commit-bot: I haz the power, 2014-03-28 23:45:14]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[reveman, 2014-03-28 23:51:10]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-03-28 23:56:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/195583003/200001

[commit-bot: I haz the power, 2014-03-29 01:37:12]

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

[commit-bot: I haz the power, 2014-03-29 01:37:13]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[reveman, 2014-03-29 01:49:14]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-03-29 01:51:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/195583003/200001

[commit-bot: I haz the power, 2014-03-29 03:25:16]

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

[commit-bot: I haz the power, 2014-03-29 03:25:17]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[reveman, 2014-03-29 04:10:17]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-03-29 04:11:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/195583003/200001

[commit-bot: I haz the power, 2014-03-29 05:42:40]

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

[commit-bot: I haz the power, 2014-03-29 05:42:41]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[reveman, 2014-03-30 04:32:03]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-03-30 04:32:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/195583003/200001

[commit-bot: I haz the power, 2014-03-30 06:07:12]

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

[commit-bot: I haz the power, 2014-03-30 06:07:13]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[reveman, 2014-03-31 05:10:21]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-03-31 05:10:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/195583003/200001

[commit-bot: I haz the power, 2014-03-31 06:45:04]

Change committed as 260498