Command to mark surface inactive, so gpu process can release resources.

Adding gl command to change surface visibility, so that the gpu process can appropriately release/reacquire resources.

BUG=5175544
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105563

[jonathan.backer, 2011-09-27 16:53:14]

I don't see where the ImageTransportSurface's use the framework on TOUCH_UI.

Separate CL?

http://codereview.chromium.org/7890046/diff/8001/content/renderer/gpu/webgrap...
File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right):

http://codereview.chromium.org/7890046/diff/8001/content/renderer/gpu/webgrap...
content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:282:
gl_->Flush();
Don't think that the flush is necessary if you're using a separate
glSetSurfaceVisibleCHROMIUM command to communicate with the GPU process.

http://codereview.chromium.org/7890046/diff/8001/gpu/command_buffer/client/gl...
File gpu/command_buffer/client/gles2_c_lib_autogen.h (right):

http://codereview.chromium.org/7890046/diff/8001/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_c_lib_autogen.h:579:
gles2::GetGLContext()->SetSurfaceVisibleCHROMIUM(visible);
If this is autogen does it checked in? Kinda confusing.

http://codereview.chromium.org/7890046/diff/8001/webkit/gpu/webgraphicscontex...
File webkit/gpu/webgraphicscontext3d_in_process_impl.cc (right):

http://codereview.chromium.org/7890046/diff/8001/webkit/gpu/webgraphicscontex...
webkit/gpu/webgraphicscontext3d_in_process_impl.cc:569: NOTIMPLEMENTED();
NOTREACHED()?

I'm not sure which is more appropriate. NOTREACHED reflects that this is only
used in DRT. I think that this is true (not 100% sure).

[apatrick_chromium, 2011-09-28 17:33:45]

http://codereview.chromium.org/7890046/diff/8001/gpu/command_buffer/build_gle...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

http://codereview.chromium.org/7890046/diff/8001/gpu/command_buffer/build_gle...
gpu/command_buffer/build_gles2_cmd_buffer.py:226: GL_APICALL void        
GL_APIENTRY glSetSurfaceVisibleCHROMIUM (GLboolean visible);
Is there any reason why changing surface visibility can't be requested using an
IPC message. If the IPC message follows a call to flush() it will be effectively
"in-band" with the GL command stream.

[mmocny, 2011-09-28 21:37:33]

http://codereview.chromium.org/7890046/diff/8001/content/renderer/gpu/webgrap...
File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right):

http://codereview.chromium.org/7890046/diff/8001/content/renderer/gpu/webgrap...
content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:282:
gl_->Flush();
See discussion in other comment.

On 2011/09/27 16:53:14, jonathan.backer wrote:
> Don't think that the flush is necessary if you're using a separate
> glSetSurfaceVisibleCHROMIUM command to communicate with the GPU process.

http://codereview.chromium.org/7890046/diff/8001/gpu/command_buffer/build_gle...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

http://codereview.chromium.org/7890046/diff/8001/gpu/command_buffer/build_gle...
gpu/command_buffer/build_gles2_cmd_buffer.py:226: GL_APICALL void        
GL_APIENTRY glSetSurfaceVisibleCHROMIUM (GLboolean visible);
I originally assumed we would not need the flush, and so I had to use a gl
command.
However, I found that this glSetSurfaceVisibleCHROMIUM command was not always
appropriately sent to the gpu process without a flush.  It wasn't just that it
was delayed by some #of ms, rather it wasn't sent at all until further gl
commands are issued.

I am not sure how the command stream decides when to push, but if I do end up
relying on the flush, then you are right, that same flush allows me to just use
an IPC message anyway.

So, a few questions for you:
1) Is there perhaps a less heavy handed way to make sure the message is
delivered?
2) If I switch to using an IPC message, the glflush guarantees it will be
processed after outstanding gl commands, but how do I guarantee it will be
processed before any subsequent ones? (this needs to be true in the
setVisibility(true) case.)

Thanks.

On 2011/09/28 17:33:45, apatrick_chromium wrote:
> Is there any reason why changing surface visibility can't be requested using
an
> IPC message. If the IPC message follows a call to flush() it will be
effectively
> "in-band" with the GL command stream.

http://codereview.chromium.org/7890046/diff/8001/gpu/command_buffer/client/gl...
File gpu/command_buffer/client/gles2_c_lib_autogen.h (right):

http://codereview.chromium.org/7890046/diff/8001/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_c_lib_autogen.h:579:
gles2::GetGLContext()->SetSurfaceVisibleCHROMIUM(visible);
I am not 100% sure here.  I assumed it would need to be checked in.
On 2011/09/27 16:53:14, jonathan.backer wrote:
> If this is autogen does it checked in? Kinda confusing.

http://codereview.chromium.org/7890046/diff/8001/webkit/gpu/webgraphicscontex...
File webkit/gpu/webgraphicscontext3d_in_process_impl.cc (right):

http://codereview.chromium.org/7890046/diff/8001/webkit/gpu/webgraphicscontex...
webkit/gpu/webgraphicscontext3d_in_process_impl.cc:569: NOTIMPLEMENTED();
I believe you are correct. Changed.

On 2011/09/27 16:53:14, jonathan.backer wrote:
> NOTREACHED()?
> 
> I'm not sure which is more appropriate. NOTREACHED reflects that this is only
> used in DRT. I think that this is true (not 100% sure).

[apatrick_chromium, 2011-09-28 22:29:45]

http://codereview.chromium.org/7890046/diff/8001/gpu/command_buffer/build_gle...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

http://codereview.chromium.org/7890046/diff/8001/gpu/command_buffer/build_gle...
gpu/command_buffer/build_gles2_cmd_buffer.py:226: GL_APICALL void        
GL_APIENTRY glSetSurfaceVisibleCHROMIUM (GLboolean visible);
On 2011/09/28 21:37:33, mmocny wrote:
> I originally assumed we would not need the flush, and so I had to use a gl
> command.
> However, I found that this glSetSurfaceVisibleCHROMIUM command was not always
> appropriately sent to the gpu process without a flush.  It wasn't just that it
> was delayed by some #of ms, rather it wasn't sent at all until further gl
> commands are issued.
> 

OpenGL does not guarantee that a GL command will be executed until after glFlush
or some other synchronizing event happens. It can be held up indefinitely.

> I am not sure how the command stream decides when to push, but if I do end up
> relying on the flush, then you are right, that same flush allows me to just
use
> an IPC message anyway.
> 
> So, a few questions for you:
> 1) Is there perhaps a less heavy handed way to make sure the message is
> delivered?

glFlush will work. It is stronger than you need because it will glFlush the
underlying GL backend unnecessarily. GLES2CmdHelper::Flush is cheaper and all
you need.

> 2) If I switch to using an IPC message, the glflush guarantees it will be
> processed after outstanding gl commands, but how do I guarantee it will be
> processed before any subsequent ones? (this needs to be true in the
> setVisibility(true) case.)

IPCs, including the async IPC that signals the flush, are not reordered on the
channel so the the flush before the set visibility IPC is a barrier and the set
visibility IPC itself is a barrier wrt to ordering of GL commands.

[mmocny, 2011-10-07 14:26:13]

I've updated to remove my debug code and change the flush to something less
heavy handed.

http://codereview.chromium.org/7890046/diff/8001/gpu/command_buffer/build_gle...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

http://codereview.chromium.org/7890046/diff/8001/gpu/command_buffer/build_gle...
gpu/command_buffer/build_gles2_cmd_buffer.py:226: GL_APICALL void        
GL_APIENTRY glSetSurfaceVisibleCHROMIUM (GLboolean visible);
GLES2CmdHelper::Flush seems to only add a glFlush into the command stream, but
that led me to find CommandBufferHelper::Flush which is I think what you
intended.

On 2011/09/28 22:29:45, apatrick_chromium wrote:
> On 2011/09/28 21:37:33, mmocny wrote:
> > I originally assumed we would not need the flush, and so I had to use a gl
> > command.
> > However, I found that this glSetSurfaceVisibleCHROMIUM command was not
always
> > appropriately sent to the gpu process without a flush.  It wasn't just that
it
> > was delayed by some #of ms, rather it wasn't sent at all until further gl
> > commands are issued.
> > 
> 
> OpenGL does not guarantee that a GL command will be executed until after
glFlush
> or some other synchronizing event happens. It can be held up indefinitely.
> 
> > I am not sure how the command stream decides when to push, but if I do end
up
> > relying on the flush, then you are right, that same flush allows me to just
> use
> > an IPC message anyway.
> > 
> > So, a few questions for you:
> > 1) Is there perhaps a less heavy handed way to make sure the message is
> > delivered?
> 
> glFlush will work. It is stronger than you need because it will glFlush the
> underlying GL backend unnecessarily. GLES2CmdHelper::Flush is cheaper and all
> you need.
> 
> > 2) If I switch to using an IPC message, the glflush guarantees it will be
> > processed after outstanding gl commands, but how do I guarantee it will be
> > processed before any subsequent ones? (this needs to be true in the
> > setVisibility(true) case.)
> 
> IPCs, including the async IPC that signals the flush, are not reordered on the
> channel so the the flush before the set visibility IPC is a barrier and the
set
> visibility IPC itself is a barrier wrt to ordering of GL commands.

[jonathan.backer, 2011-10-07 15:48:36]

I removed image_transport_surface_linux.h in an earlier CL. Forgot to delete it.

http://codereview.chromium.org/7890046/diff/17001/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface_linux.cc (right):

http://codereview.chromium.org/7890046/diff/17001/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_linux.cc:405: NOTREACHED();
Probably will be reached?

You'll need to add this stub to the OSX image transport surface as well.

http://codereview.chromium.org/7890046/diff/17001/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/7890046/diff/17001/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2635: TRACE_EVENT1("gpu",
"glSetSurfaceVisibleChromium", "visible", visible);
I'm not sure if this should be upstreamed. Not all HandleXXX are logged.

http://codereview.chromium.org/7890046/diff/17001/webkit/gpu/webgraphicsconte...
File webkit/gpu/webgraphicscontext3d_in_process_impl.cc (right):

http://codereview.chromium.org/7890046/diff/17001/webkit/gpu/webgraphicsconte...
webkit/gpu/webgraphicscontext3d_in_process_impl.cc:569: NOTREACHED();
Probably will be reached on DumpRenderTree.

[apatrick_chromium, 2011-10-07 22:45:49]

http://codereview.chromium.org/7890046/diff/8001/gpu/command_buffer/build_gle...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

http://codereview.chromium.org/7890046/diff/8001/gpu/command_buffer/build_gle...
gpu/command_buffer/build_gles2_cmd_buffer.py:226: GL_APICALL void        
GL_APIENTRY glSetSurfaceVisibleCHROMIUM (GLboolean visible);
Yeah that's the one. Sorry about that.

http://codereview.chromium.org/7890046/diff/19022/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface.h (right):

http://codereview.chromium.org/7890046/diff/19022/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface.h:110: void SurfaceVisible(bool
visible);
I'm still not clear why you want to add a new GL command? It seems you could
sidestep a lot of plumbing via the decoder by just sending an async IPC to this
object and having this function handle it? Is the sender not able to determine
the routing routing ID? Preceding it with an async flush will ensure that it is
not reordered relative to prior and subsequent GL commands.

[mmocny, 2011-10-11 13:58:43]

http://codereview.chromium.org/7890046/diff/19022/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface.h (right):

http://codereview.chromium.org/7890046/diff/19022/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface.h:110: void SurfaceVisible(bool
visible);
You are correct.
First I thought it would not be possible with IPC, and then that it might
slightly less efficient (glflush+IPC+(possibly)command stream flush).
Now I am wondering if it means more WebKit plumbing, and I am investigating.
I will update this CL with either a new implementation, or a reason why not.

On 2011/10/07 22:45:49, apatrick_chromium wrote:
> I'm still not clear why you want to add a new GL command? It seems you could
> sidestep a lot of plumbing via the decoder by just sending an async IPC to
this
> object and having this function handle it? Is the sender not able to determine
> the routing routing ID? Preceding it with an async flush will ensure that it
is
> not reordered relative to prior and subsequent GL commands.

[mmocny, 2011-10-13 18:54:17]

Replaced dropping inactive backbuffer with IPC implementation.
I had a few choices as to how to send the IPC, but this way seemed most correct.
 Hope to confirm I was correct :)

http://codereview.chromium.org/7890046/diff/31001/content/renderer/gpu/webgra...
File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right):

http://codereview.chromium.org/7890046/diff/31001/content/renderer/gpu/webgra...
content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:275:
context_->GetCommandBufferProxy()->SetSurfaceVisible(visible);
RE TODO: I'm leaning towards having RendererGLContext have a forwarding
function, and will change this unless advised otherwise.

http://codereview.chromium.org/7890046/diff/31001/webkit/gpu/webgraphicsconte...
File webkit/gpu/webgraphicscontext3d_in_process_impl.h (right):

http://codereview.chromium.org/7890046/diff/31001/webkit/gpu/webgraphicsconte...
webkit/gpu/webgraphicscontext3d_in_process_impl.h:75: virtual void
setVisibility(bool visible);
I was hesitant to add this here, as I am not sure if maybe GLSurface should have
limited functionality (imitating an actual gl surface)?

However, due to the multiple inheritance structure of the various
ImageTransportSurface implementations, the cleanest solution by far was to
migrate this function from ImageTransportSurface to GLSurface (here).
(Background: GpuCommandBufferStub owns a GLSurface surface_, and
ImageTransportSurface does not extend GLSurface, nor could it be made to)

[jonathan.backer, 2011-10-13 19:25:07]

SGTM

http://codereview.chromium.org/7890046/diff/31001/content/renderer/gpu/webgra...
File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right):

http://codereview.chromium.org/7890046/diff/31001/content/renderer/gpu/webgra...
content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:275:
context_->GetCommandBufferProxy()->SetSurfaceVisible(visible);
I'm OK with it as is. There's one surface per command buffer stub.

http://codereview.chromium.org/7890046/diff/31001/webkit/gpu/webgraphicsconte...
File webkit/gpu/webgraphicscontext3d_in_process_impl.h (right):

http://codereview.chromium.org/7890046/diff/31001/webkit/gpu/webgraphicsconte...
webkit/gpu/webgraphicscontext3d_in_process_impl.h:75: virtual void
setVisibility(bool visible);
On 2011/10/13 18:54:17, mmocny wrote:
> I was hesitant to add this here, as I am not sure if maybe GLSurface should
have
> limited functionality (imitating an actual gl surface)?
> 
> However, due to the multiple inheritance structure of the various
> ImageTransportSurface implementations, the cleanest solution by far was to
> migrate this function from ImageTransportSurface to GLSurface (here).
> (Background: GpuCommandBufferStub owns a GLSurface surface_, and
> ImageTransportSurface does not extend GLSurface, nor could it be made to)

I think that this comment is in the wrong place. I think that you're referring
to adding another method to GLSurface. And yes, that class is becoming more than
a thin wrapper around a GL surface. I think that the alternatives are uglier
though.

[apatrick_chromium, 2011-10-13 19:51:31]

Thanks, this is much simpler. A few comments but no major objections.

http://codereview.chromium.org/7890046/diff/31001/content/renderer/gpu/webgra...
File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right):

http://codereview.chromium.org/7890046/diff/31001/content/renderer/gpu/webgra...
content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:275:
context_->GetCommandBufferProxy()->SetSurfaceVisible(visible);
I lean towards a forwarding function. Law of demeter. I think there is only one
caller of that function at the moment and I'm getting closer to being able to
remove it.

http://codereview.chromium.org/7890046/diff/31001/ui/gfx/gl/gl_surface.h
File ui/gfx/gl/gl_surface.h (right):

http://codereview.chromium.org/7890046/diff/31001/ui/gfx/gl/gl_surface.h#newc...
ui/gfx/gl/gl_surface.h:56: virtual void OnSetSurfaceVisible(bool visible);
I think it's okay to add this to GLSurface. I would be tempted to call it just
SetVisible or SetVisibility though since it is implicit that the operation is on
a surface and it is more of a command to change the visibility than an event
handler.

[mmocny, 2011-10-13 21:48:43]

Updated with the changes requested.

http://codereview.chromium.org/7890046/diff/35002/content/renderer/gpu/comman...
File content/renderer/gpu/command_buffer_proxy.h (right):

http://codereview.chromium.org/7890046/diff/35002/content/renderer/gpu/comman...
content/renderer/gpu/command_buffer_proxy.h:73: bool SetSurfaceVisible(bool
visible);
TODO: add a comment here

http://codereview.chromium.org/7890046/diff/35002/content/renderer/gpu/render...
File content/renderer/gpu/renderer_gl_context.h (right):

http://codereview.chromium.org/7890046/diff/35002/content/renderer/gpu/render...
content/renderer/gpu/renderer_gl_context.h:160: bool SetSurfaceVisible(bool
visibility);
TODO: add a comment here

[apatrick_chromium, 2011-10-13 21:53:05]

Looks fine. Just add the noted comments and then LGTM for my part.

[commit-bot: I haz the power, 2011-10-14 17:57:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/7890046/41001

[commit-bot: I haz the power, 2011-10-14 20:35:55]

Change committed as 105563