Add FBO support in Android WebView

Add FBO support in Android WebView

Android can put the webview into an FBO, and compositor should draw into
the FBO instead of directly on screen.

Idea is to ask the GLSurface to get the FBO binding at the beginning of
each draw return it to GLContextVirtual to be restored.

Add new APIs in GLInProcessContext and WGC3DIPCBI to allow the
underlying GLSurface to be created separately.

BUG=251501

[boliu, 2013-08-04 00:20:47]

joth and daniel for first pass.

So much code, for something so simple...

[joth, 2013-08-04 00:45:16]

plumb plumb plumb.

I don't have a lot of value to add..., basically lg2m if it works.

Thanks!

https://codereview.chromium.org/22066002/diff/2001/content/browser/android/in...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/22066002/diff/2001/content/browser/android/in...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:39:
gfx::Size size(1, 1);
const all these?

comment where the values came from? (esp 1x1 is ok because.....)

https://codereview.chromium.org/22066002/diff/2001/content/browser/android/in...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:59:
};
this chunk smells like is was c&p from somewhere. if so comment where.

https://codereview.chromium.org/22066002/diff/2001/content/browser/android/in...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:75:
context.Pass(), attributes));
nit: tend to line these up like line 62-68

https://codereview.chromium.org/22066002/diff/2001/content/browser/android/in...
File content/browser/android/in_process/synchronous_compositor_output_surface.h
(right):

https://codereview.chromium.org/22066002/diff/2001/content/browser/android/in...
content/browser/android/in_process/synchronous_compositor_output_surface.h:92:
// Non-NULL when hardware draw has been initialized.
nit: and returns to NULL when HW draw is released

https://codereview.chromium.org/22066002/diff/2001/gpu/command_buffer/client/...
File gpu/command_buffer/client/gl_in_process_context.h (right):

https://codereview.chromium.org/22066002/diff/2001/gpu/command_buffer/client/...
gpu/command_buffer/client/gl_in_process_context.h:60: static GLInProcessContext*
CreateWithSurface(
comment?

https://codereview.chromium.org/22066002/diff/2001/gpu/command_buffer/service...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/22066002/diff/2001/gpu/command_buffer/service...
gpu/command_buffer/service/in_process_command_buffer.h:61: bool
Initialize(scoped_refptr<gfx::GLSurface> surface,
|surface| may be NULL ?

https://codereview.chromium.org/22066002/diff/2001/ui/gl/gl_surface_stub.h
File ui/gl/gl_surface_stub.h (right):

https://codereview.chromium.org/22066002/diff/2001/ui/gl/gl_surface_stub.h#ne...
ui/gl/gl_surface_stub.h:18: void UpdateFBO();
may need a comment, or more verbose name
UpdateFboFromContext()
UpdateFrameBufferObjectFromContext()

https://codereview.chromium.org/22066002/diff/2001/webkit/common/gpu/webgraph...
File webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc
(right):

https://codereview.chromium.org/22066002/diff/2001/webkit/common/gpu/webgraph...
webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:98:
context.Pass(),
nit: make indent consistent with line 86 (I think this one is preferred)

https://codereview.chromium.org/22066002/diff/2001/webkit/common/gpu/webgraph...
webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:140: //
size requests.
ah ha, here's where it was copied from. I have no suggestion if there is some
way we can share this blob of stuff. Static method on this class? Anyway, unless
it's simple I'm OK with a TODO in the other place to do this as follow up ,

[boliu, 2013-08-04 00:53:32]

https://codereview.chromium.org/22066002/diff/2001/webkit/common/gpu/webgraph...
File webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc
(right):

https://codereview.chromium.org/22066002/diff/2001/webkit/common/gpu/webgraph...
webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:140: //
size requests.
On 2013/08/04 00:45:16, joth wrote:
> ah ha, here's where it was copied from. I have no suggestion if there is some
> way we can share this blob of stuff. Static method on this class? Anyway,
unless
> it's simple I'm OK with a TODO in the other place to do this as follow up ,

Oh I tried static method. But didn't figure out a way to return a fixed size int
array. I could pass one in but can't use the initializer syntax and has to use
[] to access members which is error prone, so I got mad with c++ and just copied
it...

[joth, 2013-08-04 01:15:50]

https://codereview.chromium.org/22066002/diff/2001/webkit/common/gpu/webgraph...
File webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc
(right):

https://codereview.chromium.org/22066002/diff/2001/webkit/common/gpu/webgraph...
webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:140: //
size requests.
On 2013/08/04 00:53:32, boliu wrote:
> On 2013/08/04 00:45:16, joth wrote:
> > ah ha, here's where it was copied from. I have no suggestion if there is
some
> > way we can share this blob of stuff. Static method on this class? Anyway,
> unless
> > it's simple I'm OK with a TODO in the other place to do this as follow up ,
> 
> Oh I tried static method. But didn't figure out a way to return a fixed size
int
> array. I could pass one in but can't use the initializer syntax and has to use
> [] to access members which is error prone, so I got mad with c++ and just
copied
> it...

yeah it's mucky if you want to carry the size info. we sort-of don't need them
due to the nul-terminated approach. So as long as we're happy is is living in
static const data, we can just do

static const int32* GetDefaultSurfaceAttribs() {
  const int alpha_size = attributes_.alpha ? 8 : 0;
  ...
  const static int32 attribs[] = { ... };
  return attribs;
}

[boliu, 2013-08-05 16:58:23]

https://codereview.chromium.org/22066002/diff/2001/content/browser/android/in...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/22066002/diff/2001/content/browser/android/in...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:39:
gfx::Size size(1, 1);
On 2013/08/04 00:45:16, joth wrote:
> const all these?
> 
> comment where the values came from? (esp 1x1 is ok because.....)

Actually I'm not sure. I'm inclined to say it's not used, but the value is
passed into decoder and saved in the context state...added comment that this is
copied code though

https://codereview.chromium.org/22066002/diff/2001/content/browser/android/in...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:75:
context.Pass(), attributes));
On 2013/08/04 00:45:16, joth wrote:
> nit: tend to line these up like line 62-68

It doesn't fit on 80 characters. Also this is not against the style guide (since
it was formatted with clang-format) since all arguments fit on a signal line. No
change made.

https://codereview.chromium.org/22066002/diff/2001/ui/gl/gl_surface_stub.h
File ui/gl/gl_surface_stub.h (right):

https://codereview.chromium.org/22066002/diff/2001/ui/gl/gl_surface_stub.h#ne...
ui/gl/gl_surface_stub.h:18: void UpdateFBO();
On 2013/08/04 00:45:16, joth wrote:
> may need a comment, or more verbose name
> UpdateFboFromContext()
> UpdateFrameBufferObjectFromContext()

Changed to SaveCurrentFBO. The FromContext bit is confusing so didn't add it.

https://codereview.chromium.org/22066002/diff/2001/webkit/common/gpu/webgraph...
File webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc
(right):

https://codereview.chromium.org/22066002/diff/2001/webkit/common/gpu/webgraph...
webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:98:
context.Pass(),
On 2013/08/04 00:45:16, joth wrote:
> nit: make indent consistent with line 86 (I think this one is preferred)

clang-formatted both. Not exactly as you suggested.

[joth, 2013-08-05 17:11:19]

lgtm for synchronous_compositor_output_surface.* and re. my other comments

[no sievers, 2013-08-05 20:41:09]

https://codereview.chromium.org/22066002/diff/11001/gpu/command_buffer/servic...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/22066002/diff/11001/gpu/command_buffer/servic...
gpu/command_buffer/service/in_process_command_buffer.cc:289:
scoped_refptr<gfx::GLSurface> surface,
Hmm, this is dangerous. GLSurface is not RefCountedThreadSafe, so we cannot be
modifying the reference on the client thread (even if this is all on the same
thread for WebView, the code will be wrong).

I think we need to expose this is a factory that can be run on the gpu thread.
Maybe similar to SurfaceFactoryOzone, see gl_surface_egl.cc

[boliu, 2013-08-05 20:42:52]

https://codereview.chromium.org/22066002/diff/11001/gpu/command_buffer/servic...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/22066002/diff/11001/gpu/command_buffer/servic...
gpu/command_buffer/service/in_process_command_buffer.cc:289:
scoped_refptr<gfx::GLSurface> surface,
On 2013/08/05 20:41:09, sievers wrote:
> Hmm, this is dangerous. GLSurface is not RefCountedThreadSafe, so we cannot be
> modifying the reference on the client thread (even if this is all on the same
> thread for WebView, the code will be wrong).
> 
> I think we need to expose this is a factory that can be run on the gpu thread.
> Maybe similar to SurfaceFactoryOzone, see gl_surface_egl.cc

Bah...good point.

[boliu, 2013-08-06 02:25:31]

closing in favour of https://codereview.chromium.org/22277004/