Add gfx::SurfaceFactoryWebview

Add AwGLSurface and use it in WGC3D

Android can put the webview into an FBO, and compositor should draw into 
the FBO instead of directly on screen.  Use AwGLSurface and explicitly
set the FBO at the beginning of each hardware draw.

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

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

[boliu, 2013-08-06 01:35:56]

fbo attempt #3, probably in pretty rough shape right now, conceptually much
simpler though

[no sievers, 2013-08-06 01:50:06]

LGTM with nits.

https://codereview.chromium.org/22277004/diff/5001/android_webview/browser/gl...
File android_webview/browser/gl_surface_factory.cc (right):

https://codereview.chromium.org/22277004/diff/5001/android_webview/browser/gl...
android_webview/browser/gl_surface_factory.cc:69:
gfx::SurfaceFactoryWebView::SetInstance(g_gl_surface_factory.Pointer());
do you need the LazyInstance (esp. since you are leaking it)? Or can it just be:
DCHECK(!gfx::SurfaceFactoryWebView::GetInstance());
gfx::SurfaceFactoryWebView::SetInstance(new GLSurfaceFactory());

https://codereview.chromium.org/22277004/diff/5001/ui/gl/android/surface_fact...
File ui/gl/android/surface_factory_webview.h (right):

https://codereview.chromium.org/22277004/diff/5001/ui/gl/android/surface_fact...
ui/gl/android/surface_factory_webview.h:12: 
nit: fwd declare GLSurface

https://codereview.chromium.org/22277004/diff/5001/ui/gl/android/surface_fact...
ui/gl/android/surface_factory_webview.h:15: virtual ~SurfaceFactoryWebView();
nit: protected destructor

https://codereview.chromium.org/22277004/diff/5001/ui/gl/gl.gyp
File ui/gl/gl.gyp (right):

https://codereview.chromium.org/22277004/diff/5001/ui/gl/gl.gyp#newcode52
ui/gl/gl.gyp:52: 'android/surface_factory_webview.h',
nit: order

[boliu, 2013-08-06 02:14:26]

s/SurfaceFactoryWebview/SurfaceFactoryAndroid/ as well

https://codereview.chromium.org/22277004/diff/5001/android_webview/browser/gl...
File android_webview/browser/gl_surface_factory.cc (right):

https://codereview.chromium.org/22277004/diff/5001/android_webview/browser/gl...
android_webview/browser/gl_surface_factory.cc:69:
gfx::SurfaceFactoryWebView::SetInstance(g_gl_surface_factory.Pointer());
On 2013/08/06 01:50:06, sievers wrote:
> do you need the LazyInstance (esp. since you are leaking it)? Or can it just
be:
> DCHECK(!gfx::SurfaceFactoryWebView::GetInstance());
> gfx::SurfaceFactoryWebView::SetInstance(new GLSurfaceFactory());

For no particular reason other than it's a pattern with android_webview code. I
guess it makes the leaking part explicit.

Added the DCHECK though.

https://codereview.chromium.org/22277004/diff/5001/ui/gl/android/surface_fact...
File ui/gl/android/surface_factory_webview.h (right):

https://codereview.chromium.org/22277004/diff/5001/ui/gl/android/surface_fact...
ui/gl/android/surface_factory_webview.h:12: 
On 2013/08/06 01:50:06, sievers wrote:
> nit: fwd declare GLSurface

That works?...wow, that works. Done.

https://codereview.chromium.org/22277004/diff/5001/ui/gl/android/surface_fact...
ui/gl/android/surface_factory_webview.h:15: virtual ~SurfaceFactoryWebView();
On 2013/08/06 01:50:06, sievers wrote:
> nit: protected destructor

Done.

https://codereview.chromium.org/22277004/diff/5001/ui/gl/gl.gyp
File ui/gl/gl.gyp (right):

https://codereview.chromium.org/22277004/diff/5001/ui/gl/gl.gyp#newcode52
ui/gl/gl.gyp:52: 'android/surface_factory_webview.h',
On 2013/08/06 01:50:06, sievers wrote:
> nit: order

Done.

[boliu, 2013-08-06 02:24:02]

Antoine, could you take a look changes in ui/gl please. Thanks.

[piman, 2013-08-06 03:09:45]

globals, globals... what happens if one app has 2 webviews in it?

https://codereview.chromium.org/22277004/diff/12001/android_webview/browser/g...
File android_webview/browser/gl_surface_factory.cc (right):

https://codereview.chromium.org/22277004/diff/12001/android_webview/browser/g...
android_webview/browser/gl_surface_factory.cc:15: unsigned int g_current_fbo =
0;
Can we make this a member of the GLSurfaceFactory?

https://codereview.chromium.org/22277004/diff/12001/android_webview/browser/g...
android_webview/browser/gl_surface_factory.cc:64:
base::LazyInstance<GLSurfaceFactory>::Leaky g_gl_surface_factory;
Can we make this non-leaky?

[boliu, 2013-08-06 03:33:40]

On 2013/08/06 03:09:45, piman wrote:
> globals, globals... what happens if one app has 2 webviews in it?

All webviews in one process share the same underlying eglContext when drawing,
so at least there are no weird things there.

Individual webviews could still be put into different FBOs. But it's also ok
here to since only one webview can be drawn at a time, and the FBO binding is
read at the beginning of each draw. (I forgot to reset it back to 0 after draw,
fixed now).

Ideally we would not rely on this synchronous draw one at a time behavior, but
that patch got pretty hairy too: https://codereview.chromium.org/22066002/  So
going with this conceptually simpler one, that also allows webview greater
control the surface.

https://codereview.chromium.org/22277004/diff/12001/android_webview/browser/g...
File android_webview/browser/gl_surface_factory.cc (right):

https://codereview.chromium.org/22277004/diff/12001/android_webview/browser/g...
android_webview/browser/gl_surface_factory.cc:15: unsigned int g_current_fbo =
0;
On 2013/08/06 03:09:45, piman wrote:
> Can we make this a member of the GLSurfaceFactory?

Made this a static member var. (You didn't mean non-static member var, right?)

https://codereview.chromium.org/22277004/diff/12001/android_webview/browser/g...
android_webview/browser/gl_surface_factory.cc:64:
base::LazyInstance<GLSurfaceFactory>::Leaky g_gl_surface_factory;
On 2013/08/06 03:09:45, piman wrote:
> Can we make this non-leaky?

Actually this is a pattern in android_webview/. Like everything else on android,
OnExit is never called. And in this regard, webview is actually in less control
of the situation than chrome on android.

So we use Leaky to explicitly state that global vars will never be destroyed in
android webivew.

[joth, 2013-08-06 03:55:41]

lgtm if piman is OK with it

https://codereview.chromium.org/22277004/diff/5001/android_webview/browser/gl...
File android_webview/browser/gl_surface_factory.cc (right):

https://codereview.chromium.org/22277004/diff/5001/android_webview/browser/gl...
android_webview/browser/gl_surface_factory.cc:69:
gfx::SurfaceFactoryWebView::SetInstance(g_gl_surface_factory.Pointer());
On 2013/08/06 02:14:26, boliu wrote:
> On 2013/08/06 01:50:06, sievers wrote:
> > do you need the LazyInstance (esp. since you are leaking it)? Or can it just
> be:
> > DCHECK(!gfx::SurfaceFactoryWebView::GetInstance());
> > gfx::SurfaceFactoryWebView::SetInstance(new GLSurfaceFactory());
> 
> For no particular reason other than it's a pattern with android_webview code.
I
> guess it makes the leaking part explicit.
> 
> Added the DCHECK though.

we use Daniel's pattern too... see content::SetContentMainDelegate(new
android_webview::AwMainDelegate()); for example.

As AtExit manager is never run it's fairly moot which of the three patterns are
used.  I'm personally for the simplicity of what Daniel suggests for places like
this that we know will be called in early initialization when we're still single
threaded, and given we can assert the sington-ness of it easily.

https://codereview.chromium.org/22277004/diff/15002/android_webview/browser/g...
File android_webview/browser/gl_surface_factory.cc (right):

https://codereview.chromium.org/22277004/diff/15002/android_webview/browser/g...
android_webview/browser/gl_surface_factory.cc:75: void
GLSurfaceFactory::SetCurrentFBO(unsigned int fbo) {
To make it clear at any moment only one surface can be drawing, how about

DCHECK(!current_fbo_);

and then add a ResetFbo() to call from state-restorer's d'tor. (We can't have
any useful DCHECK in that one (unless we make -1 be a sentinel value. May an FBO
id be negative ?)

[piman, 2013-08-06 04:06:30]

On Mon, Aug 5, 2013 at 8:33 PM, <boliu@chromium.org> wrote:

> On 2013/08/06 03:09:45, piman wrote:
>
>> globals, globals... what happens if one app has 2 webviews in it?
>>
>
> All webviews in one process share the same underlying eglContext when
> drawing,
> so at least there are no weird things there.
>
> Individual webviews could still be put into different FBOs. But it's also
> ok
> here to since only one webview can be drawn at a time, and the FBO binding
> is
> read at the beginning of each draw. (I forgot to reset it back to 0 after
> draw,
> fixed now).
>

This makes me very uncomfortable.
We're relying on a lot of logic that's distributed in many places (command
buffer infrastructure, ui/gl, android_webview) for correctness ("we know
the command buffer layer will access this value only in very specific
places, and so we're going to set this global value just in time for it,
and not worry about it possibly accessing it later").
This logic is only exercised by code out of the chromium tree, with no
test, and impossible to replicate on other platforms. You may be surprised
at the places GetBackingFrameBufferObject ends up affecting.
I don't want my next command buffer change to break webview with no simple
way for me to debug/fix, so allow me to be skeptical of hacks. So far just
about every bit of global state has ended up burning us in one way or
another.

Maybe with just a bit more effort, we can make this work without involving
a global state for the fbo? Can we keep a map in the GLSurfaceFactory of
the webview instance -> fbo, that the AwGLSurface would use
for GetBackingFrameBufferObject, that you would update in the same places
(but associating it with the right instance). Would that be possible?


> Ideally we would not rely on this synchronous draw one at a time behavior,
> but
> that patch got pretty hairy too: https://codereview.chromium.**
> org/22066002/ <https://codereview.chromium.org/22066002/>  So
> going with this conceptually simpler one, that also allows webview greater
> control the surface.
>
>
>
> https://codereview.chromium.**org/22277004/diff/12001/**
>
android_webview/browser/gl_**surface_factory.cc<https://codereview.chromium.org/22277004/diff/12001/android_webview/browser/gl_surface_factory.cc>
> File android_webview/browser/gl_**surface_factory.cc (right):
>
> https://codereview.chromium.**org/22277004/diff/12001/**
>
android_webview/browser/gl_**surface_factory.cc#newcode15<https://codereview.chromium.org/22277004/diff/12001/android_webview/browser/gl_surface_factory.cc#newcode15>
> android_webview/browser/gl_**surface_factory.cc:15: unsigned int
> g_current_fbo = 0;
> On 2013/08/06 03:09:45, piman wrote:
>
>> Can we make this a member of the GLSurfaceFactory?
>>
>
> Made this a static member var. (You didn't mean non-static member var,
> right?)


I did mean that. If GLSurfaceFactory is a global, you can set this by using
GLSurfaceFactory::GetInstance()->SetFBO, etc. But then again, see above
about the fbo being a global and possibly better alternatives.

>
>
> https://codereview.chromium.**org/22277004/diff/12001/**
>
android_webview/browser/gl_**surface_factory.cc#newcode64<https://codereview.chromium.org/22277004/diff/12001/android_webview/browser/gl_surface_factory.cc#newcode64>
> android_webview/browser/gl_**surface_factory.cc:64:
> base::LazyInstance<**GLSurfaceFactory>::Leaky g_gl_surface_factory;
> On 2013/08/06 03:09:45, piman wrote:
>
>> Can we make this non-leaky?
>>
>
> Actually this is a pattern in android_webview/. Like everything else on
> android, OnExit is never called. And in this regard, webview is actually
> in less control of the situation than chrome on android.
>
> So we use Leaky to explicitly state that global vars will never be
> destroyed in android webivew.
>

But who said it has to be a LazyInstance.... Can you create it at
initialization (e.g. when the first webview is created?), and reset/free it
later once it's not in use any more (e.g. when we released the last
webview) ?


>
https://codereview.chromium.**org/22277004/<https://codereview.chromium.org/2...
>

[boliu, 2013-08-06 04:24:33]

On 2013/08/06 04:06:30, piman wrote:
> Maybe with just a bit more effort, we can make this work without involving
> a global state for the fbo? Can we keep a map in the GLSurfaceFactory of
> the webview instance -> fbo, that the AwGLSurface would use
> for GetBackingFrameBufferObject, that you would update in the same places
> (but associating it with the right instance). Would that be possible?

It's a bit weird at least using the implicit factory pattern. When GL is
initialized, webview has to assume the one on-screen surface is created
synchronously, so it can be mapped back to one webview. Hmm, I guess that's
actually a pretty safe assumption since even the cross-process context has this
behavior, and that we don't care about the global offscreen context at this
point. Alright, coding it up.

[boliu, 2013-08-06 05:37:24]

AwGLSurface now owned by InProcessViewRenderer.

Using raw pointer for the global factory instance. Re-implementing OnExit for
when the last webview is destroyed is out of scope of course, but it is an
interesting idea. although not sure how much savings there can be.

[no sievers, 2013-08-06 17:30:07]

https://codereview.chromium.org/22277004/diff/38001/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/22277004/diff/38001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:306:
TRACE_EVENT0("android_webview", "InitializeHwDraw");
Do you need GLSurfaceFactory if you are doing this now?
Or would it be equivalent if the InProcessViewRenderer just implemented the
factory (and you use SetInstance() instead of setting the callback)?

[boliu, 2013-08-06 18:11:16]

https://codereview.chromium.org/22277004/diff/38001/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/22277004/diff/38001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:306:
TRACE_EVENT0("android_webview", "InitializeHwDraw");
On 2013/08/06 17:30:07, sievers wrote:
> Do you need GLSurfaceFactory if you are doing this now?
> Or would it be equivalent if the InProcessViewRenderer just implemented the
> factory (and you use SetInstance() instead of setting the callback)?

This brought up something else. We should always have the factory installed and
create AwGLSurface, even if it is not associated with a webview, so we need a
factory always installed. I don't think it can happen now, but things may change
in the future and should be safer this way.

Also calling SetInstance like that is abusing the api a bit.

[no sievers, 2013-08-06 18:49:56]

On 2013/08/06 18:11:16, boliu wrote:
>
https://codereview.chromium.org/22277004/diff/38001/android_webview/browser/i...
> File android_webview/browser/in_process_view_renderer.cc (right):
> 
>
https://codereview.chromium.org/22277004/diff/38001/android_webview/browser/i...
> android_webview/browser/in_process_view_renderer.cc:306:
> TRACE_EVENT0("android_webview", "InitializeHwDraw");
> On 2013/08/06 17:30:07, sievers wrote:
> > Do you need GLSurfaceFactory if you are doing this now?
> > Or would it be equivalent if the InProcessViewRenderer just implemented the
> > factory (and you use SetInstance() instead of setting the callback)?
> 
> This brought up something else. We should always have the factory installed
and
> create AwGLSurface, even if it is not associated with a webview, so we need a
> factory always installed. I don't think it can happen now, but things may
change
> in the future and should be safer this way.
> 

We know that creating the underlying context is synchronous and can happen only
during Draw when we have a context.
Why do we need to support the ability to create a surface that cannot be
associated with a WebView/context?

> Also calling SetInstance like that is abusing the api a bit.

[boliu, 2013-08-06 20:16:02]

On 2013/08/06 18:49:56, sievers wrote:
> We know that creating the underlying context is synchronous and can happen
only
> during Draw when we have a context.
> Why do we need to support the ability to create a surface that cannot be
> associated with a WebView/context?

Ok, done.

[no sievers, 2013-08-06 20:31:29]

And since you pointed out iffy semantics - I assume you meant Get/SetInstance()
maybe suggesting a singleton - that could probably be fixed by some renaming
(Get/SetInstance() -> Get/SetCurrent(), wdyt? It could also be a callback which
for some reason sounds less static...). But just in case someone feels strongly
about it, not sure I do :)

https://codereview.chromium.org/22277004/diff/45001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/22277004/diff/45001/ui/gl/gl_surface_egl.cc#n...
ui/gl/gl_surface_egl.cc:658: surface = factory->CreateNonOwnedViewSurface();
null check factory or tests will crash

[no sievers, 2013-08-06 20:36:00]

Another thought:

gl_context_android.cc has the counterpart for this webview path:

if (compatible_surface->GetHandle()) {
...
} else {
...
 context = new GLNonOwnedContext(share_group);


Should we consolidate things while you're at it with the factory interface and
also move that class up to in_process_view_renderer.cc?

[boliu, 2013-08-06 21:10:05]

On 2013/08/06 20:31:29, sievers wrote:
> And since you pointed out iffy semantics - I assume you meant
Get/SetInstance()
> maybe suggesting a singleton - that could probably be fixed by some renaming
> (Get/SetInstance() -> Get/SetCurrent(), wdyt? It could also be a callback
which
> for some reason sounds less static...). But just in case someone feels
strongly
> about it, not sure I do :)

I think that's a good idea. Renamed to Get/SetCurrent instead.

> Another thought:
> 
> gl_context_android.cc has the counterpart for this webview path:
> 
> if (compatible_surface->GetHandle()) {
> ...
> } else {
> ...
>  context = new GLNonOwnedContext(share_group);
> 
> 
> Should we consolidate things while you're at it with the factory interface and
> also move that class up to in_process_view_renderer.cc?

Do this in a follow up to this?

https://codereview.chromium.org/22277004/diff/45001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/22277004/diff/45001/ui/gl/gl_surface_egl.cc#n...
ui/gl/gl_surface_egl.cc:658: surface = factory->CreateNonOwnedViewSurface();
On 2013/08/06 20:31:29, sievers wrote:
> null check factory or tests will crash

That's what the if is for :)

[no sievers, 2013-08-06 21:13:17]

On 2013/08/06 21:10:05, boliu wrote:
> On 2013/08/06 20:31:29, sievers wrote:
> > And since you pointed out iffy semantics - I assume you meant
> Get/SetInstance()
> > maybe suggesting a singleton - that could probably be fixed by some renaming
> > (Get/SetInstance() -> Get/SetCurrent(), wdyt? It could also be a callback
> which
> > for some reason sounds less static...). But just in case someone feels
> strongly
> > about it, not sure I do :)
> 
> I think that's a good idea. Renamed to Get/SetCurrent instead.
> 
> > Another thought:
> > 
> > gl_context_android.cc has the counterpart for this webview path:
> > 
> > if (compatible_surface->GetHandle()) {
> > ...
> > } else {
> > ...
> >  context = new GLNonOwnedContext(share_group);
> > 
> > 
> > Should we consolidate things while you're at it with the factory interface
and
> > also move that class up to in_process_view_renderer.cc?
> 
> Do this in a follow up to this?
> 

Sure, but then you might be renaming all over again.


> https://codereview.chromium.org/22277004/diff/45001/ui/gl/gl_surface_egl.cc
> File ui/gl/gl_surface_egl.cc (right):
> 
>
https://codereview.chromium.org/22277004/diff/45001/ui/gl/gl_surface_egl.cc#n...
> ui/gl/gl_surface_egl.cc:658: surface = factory->CreateNonOwnedViewSurface();
> On 2013/08/06 20:31:29, sievers wrote:
> > null check factory or tests will crash
> 
> That's what the if is for :)

oops

[boliu, 2013-08-06 21:24:33]

On 2013/08/06 21:13:17, sievers wrote:
> Sure, but then you might be renaming all over again.

Ok, ok, renamed to GLFactoryAndroid here. No need to rename things for creating
GLContext in follow up...

[piman, 2013-08-06 21:41:55]

https://codereview.chromium.org/22277004/diff/20002/android_webview/browser/a...
File android_webview/browser/aw_gl_surface.h (right):

https://codereview.chromium.org/22277004/diff/20002/android_webview/browser/a...
android_webview/browser/aw_gl_surface.h:34: unsigned int fbo_;
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/22277004/diff/20002/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/22277004/diff/20002/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:166:
ScopedSetSurfaceFactory(InProcessViewRenderer* factory);
nit: explicit

https://codereview.chromium.org/22277004/diff/20002/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:322: ScopedSetSurfaceFactory
setter(this);
So, essentially, you're passing |this| all the way to CreateNonOwnedViewSurface
through a global variable.

Can we fix this? E.g. by creating the GLSurface here, and passing it to
InitializeHwDraw that can then give it to the
WebGraphicsContext3DInProcessCommandBufferImpl?

[no sievers, 2013-08-06 21:44:49]

https://codereview.chromium.org/22277004/diff/20002/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/22277004/diff/20002/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:322: ScopedSetSurfaceFactory
setter(this);
On 2013/08/06 21:41:55, piman wrote:
> So, essentially, you're passing |this| all the way to
CreateNonOwnedViewSurface
> through a global variable.
> 
> Can we fix this? E.g. by creating the GLSurface here, and passing it to
> InitializeHwDraw that can then give it to the
> WebGraphicsContext3DInProcessCommandBufferImpl?

Just chatted with Bo and realized (a little too late) that having a clean
failure path that does not create a GLSurfaceStub for WebView might be worth the
extra layer of indirection he had earlier. (Esp. since a GLSurfaceStub will
somewhat work except for not binding the right FBO which is only sometimes
needed.)

[boliu, 2013-08-06 21:51:16]

https://codereview.chromium.org/22277004/diff/20002/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/22277004/diff/20002/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:322: ScopedSetSurfaceFactory
setter(this);
On 2013/08/06 21:41:55, piman wrote:
> So, essentially, you're passing |this| all the way to
CreateNonOwnedViewSurface
> through a global variable.
> 
> Can we fix this? E.g. by creating the GLSurface here, and passing it to
> InitializeHwDraw that can then give it to the
> WebGraphicsContext3DInProcessCommandBufferImpl?

That's actually attempt 1: https://codereview.chromium.org/22066002/

Could you take a look at the gpu and ui changes there and see if you prefer that
over this factory approach.

Concern there was GLSurface is not thread safe so passing it from the client to
server side using scoped_refptr is wrong.

[piman, 2013-08-06 22:01:18]

On Tue, Aug 6, 2013 at 2:51 PM, <boliu@chromium.org> wrote:

>
> https://codereview.chromium.**org/22277004/diff/20002/**
>
android_webview/browser/in_**process_view_renderer.cc<https://codereview.chromium.org/22277004/diff/20002/android_webview/browser/in_process_view_renderer.cc>
> File android_webview/browser/in_**process_view_renderer.cc (right):
>
> https://codereview.chromium.**org/22277004/diff/20002/**
>
android_webview/browser/in_**process_view_renderer.cc#**newcode322<https://codereview.chromium.org/22277004/diff/20002/android_webview/browser/in_process_view_renderer.cc#newcode322>
> android_webview/browser/in_**process_view_renderer.cc:322:
> ScopedSetSurfaceFactory setter(this);
> On 2013/08/06 21:41:55, piman wrote:
>
>> So, essentially, you're passing |this| all the way to
>>
> CreateNonOwnedViewSurface
>
>> through a global variable.
>>
>
>  Can we fix this? E.g. by creating the GLSurface here, and passing it
>>
> to
>
>> InitializeHwDraw that can then give it to the
>> WebGraphicsContext3DInProcessC**ommandBufferImpl?
>>
>
> That's actually attempt 1:
https://codereview.chromium.**org/22066002/<https://codereview.chromium.org/2...
>
> Could you take a look at the gpu and ui changes there and see if you
> prefer that over this factory approach.
>

I like it better.


>
> Concern there was GLSurface is not thread safe so passing it from the
> client to server side using scoped_refptr is wrong.
>

In your case, the initialization/wgc3d creation happens on the same thread
as where the GLSurface is used, so it should be safe. Maybe we can document
the behavior (e.g "if passing a surface through Initialize, it has to be on
the GPU thread"), maybe with asserts to guard against incorrect usage?


>
https://codereview.chromium.**org/22277004/<https://codereview.chromium.org/2...
>

[boliu, 2013-08-06 23:38:21]

https://codereview.chromium.org/22277004/diff/58001/content/browser/android/i...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/22277004/diff/58001/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:38:
// TODO(boliu): Much of this code is copied and adapted from
Umm, maybe make GLInProcessContext take a struct of bools similar to
WGC3D::Attributes, then no need to copy this code.

Pls ignore this bit and review the rest of this patch.

[piman, 2013-08-07 00:03:41]

Looks reasonable.

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

https://codereview.chromium.org/22277004/diff/58001/gpu/command_buffer/servic...
gpu/command_buffer/service/in_process_command_buffer.cc:500:
DCHECK(sequence_checker_->CalledOnValidSequencedThread());
maybe DCHECK(!sequence_checker_ ||
sequence_checker_->CalledOnValidSequencedThread()), that way you don't pay for
the extra test in release.

Note that the current code is wrong: you dereference a pointer that you just
tested is NULL.

[boliu, 2013-08-07 00:24:56]

Made GLInProcessContext take a struct of int32s instead of a null terminated int
array. The add a static method in WGC3D to convert it from WGC3D::Attributes.

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

https://codereview.chromium.org/22277004/diff/58001/gpu/command_buffer/servic...
gpu/command_buffer/service/in_process_command_buffer.cc:500:
DCHECK(sequence_checker_->CalledOnValidSequencedThread());
On 2013/08/07 00:03:41, piman wrote:
> maybe DCHECK(!sequence_checker_ ||
> sequence_checker_->CalledOnValidSequencedThread()), that way you don't pay for
> the extra test in release.
> 
> Note that the current code is wrong: you dereference a pointer that you just
> tested is NULL.

Oop!! Fixed.

[piman, 2013-08-07 00:35:29]

LGTM if Daniel is happy too.

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

https://codereview.chromium.org/22277004/diff/22004/gpu/command_buffer/client...
gpu/command_buffer/client/gl_in_process_context.h:27: // The default
uninitialize value is -1.
typo: "uninitialized"

[boliu, 2013-08-07 01:14:33]

+Ted for content/public/browser/android

Daniel/Joth, should look over this again since it has changed a lot.

[no sievers, 2013-08-07 01:52:38]

lgtm with small comments

https://codereview.chromium.org/22277004/diff/22004/android_webview/browser/a...
File android_webview/browser/aw_gl_surface.cc (right):

https://codereview.chromium.org/22277004/diff/22004/android_webview/browser/a...
android_webview/browser/aw_gl_surface.cc:44: void
AwGLSurface::ResetBackingFrameBufferObject() {
nit: Do we need this? :)

https://codereview.chromium.org/22277004/diff/22004/content/browser/android/i...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/22277004/diff/22004/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:36:
scoped_ptr<WebKit::WebGraphicsContext3D>();
return

https://codereview.chromium.org/22277004/diff/22004/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:61:
scoped_ptr<WebKit::WebGraphicsContext3D>();
return

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

https://codereview.chromium.org/22277004/diff/22004/gpu/command_buffer/client...
gpu/command_buffer/client/gl_in_process_context.h:66: bool is_offscreen,
is_offscreen and size should now be derived from GLSurface::IsOffscreen() and
GetSize()

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

https://codereview.chromium.org/22277004/diff/22004/gpu/command_buffer/servic...
gpu/command_buffer/service/in_process_command_buffer.cc:342:
CheckSequencedThread();
Does it make sense to always have a sequence checker? It seems useful for
verifying that things happen on the right thread. You could have one for the
client entry points and one for the service. And if you pass in a surface, both
would point to the same thread checker instance.

Then you'd also not need the function indirection, since it would stay a
one-liner.

https://codereview.chromium.org/22277004/diff/22004/webkit/common/gpu/webgrap...
File webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc
(left):

https://codereview.chromium.org/22277004/diff/22004/webkit/common/gpu/webgrap...
webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:153:
gfx::Size(1, 1),
interesting that we pass in 1,1 here

https://codereview.chromium.org/22277004/diff/22004/webkit/common/gpu/webgrap...
File webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h
(right):

https://codereview.chromium.org/22277004/diff/22004/webkit/common/gpu/webgrap...
webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h:60:
const WebKit::WebGraphicsContext3D::Attributes& attributes);
Wouldn't it make more sense if WGC3D got the attributes from the context?

[boliu, 2013-08-07 02:58:48]

https://codereview.chromium.org/22277004/diff/22004/android_webview/browser/a...
File android_webview/browser/aw_gl_surface.cc (right):

https://codereview.chromium.org/22277004/diff/22004/android_webview/browser/a...
android_webview/browser/aw_gl_surface.cc:44: void
AwGLSurface::ResetBackingFrameBufferObject() {
On 2013/08/07 01:52:38, sievers wrote:
> nit: Do we need this? :)

I think it should be set back to 0, or maybe -1 in debug builds just to be safe.

But having this method is purely style, so ehh..keeping it as is.

https://codereview.chromium.org/22277004/diff/22004/content/browser/android/i...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/22277004/diff/22004/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:36:
scoped_ptr<WebKit::WebGraphicsContext3D>();
On 2013/08/07 01:52:38, sievers wrote:
> return

Done.

https://codereview.chromium.org/22277004/diff/22004/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:61:
scoped_ptr<WebKit::WebGraphicsContext3D>();
On 2013/08/07 01:52:38, sievers wrote:
> return

Done.

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

https://codereview.chromium.org/22277004/diff/22004/gpu/command_buffer/client...
gpu/command_buffer/client/gl_in_process_context.h:27: // The default
uninitialize value is -1.
On 2013/08/07 00:35:29, piman wrote:
> typo: "uninitialized"

Done.

https://codereview.chromium.org/22277004/diff/22004/gpu/command_buffer/client...
gpu/command_buffer/client/gl_in_process_context.h:66: bool is_offscreen,
On 2013/08/07 01:52:38, sievers wrote:
> is_offscreen and size should now be derived from GLSurface::IsOffscreen() and
> GetSize()

Done.

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

https://codereview.chromium.org/22277004/diff/22004/gpu/command_buffer/servic...
gpu/command_buffer/service/in_process_command_buffer.cc:342:
CheckSequencedThread();
On 2013/08/07 01:52:38, sievers wrote:
> Does it make sense to always have a sequence checker? It seems useful for
> verifying that things happen on the right thread. You could have one for the
> client entry points and one for the service. And if you pass in a surface,
both
> would point to the same thread checker instance.
> 
> Then you'd also not need the function indirection, since it would stay a
> one-liner.

Can't use scoped_ptr anymore since they could both point to the same object, and
can't refcount it directly. Going to keep this as is now.

https://codereview.chromium.org/22277004/diff/22004/webkit/common/gpu/webgrap...
File webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h
(right):

https://codereview.chromium.org/22277004/diff/22004/webkit/common/gpu/webgrap...
webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h:60:
const WebKit::WebGraphicsContext3D::Attributes& attributes);
On 2013/08/07 01:52:38, sievers wrote:
> Wouldn't it make more sense if WGC3D got the attributes from the context?

I think that's a layering violation that gpu code cannot include blink code.

[joth, 2013-08-07 02:59:44]

aw/ and content/ lgtm % 2 nits

https://codereview.chromium.org/22277004/diff/71001/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/22277004/diff/71001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:326: return;
nit: use { } when there's an else.
(I know cc and gpu code doesn't, but rest of chromium does and it's in The
Rules)

https://codereview.chromium.org/22277004/diff/71001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:567: gl_surface_ = NULL;
nit: do this outside the if {} block. (as you initialize it regardless of
whether the HW init succeeded or not.

[boliu, 2013-08-07 03:34:45]

https://codereview.chromium.org/22277004/diff/71001/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/22277004/diff/71001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:326: return;
On 2013/08/07 02:59:44, joth wrote:
> nit: use { } when there's an else.
> (I know cc and gpu code doesn't, but rest of chromium does and it's in The
> Rules)

Done.

https://codereview.chromium.org/22277004/diff/71001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:567: gl_surface_ = NULL;
On 2013/08/07 02:59:44, joth wrote:
> nit: do this outside the if {} block.

Done

> (as you initialize it regardless of
> whether the HW init succeeded or not.

That's a bug. Clearing it if init fails

[commit-bot: I haz the power, 2013-08-07 05:25:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/22277004/35002

[commit-bot: I haz the power, 2013-08-07 05:45:15]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-07 05:50:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/22277004/86003

[commit-bot: I haz the power, 2013-08-07 06:19:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/22277004/108001

[commit-bot: I haz the power, 2013-08-07 09:46:13]

Change committed as 216136