Splitting GpuMemoryAllocation into two parts: for renderer and browser

Splitting GpuMemoryAllocation into two parts: for renderer and browser.

Sets the suggest_have_backbuffer value of WebKit::WebGraphicsMemoryAllocation as part of GpuMemoryManager work.

BUG=116049
TEST=content_unittests GpuMemoryManagerTest.* and Manual testing

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

[mmocny, 2012-03-15 19:11:42]

This is an intermediate CL to move drop backbuffer hooks over to renderer.

I am looking for feedback on some naming and just for an early glance.

https://chromiumcodereview.appspot.com/9702081/diff/1/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy.cc (right):

https://chromiumcodereview.appspot.com/9702081/diff/1/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy.cc:111: if
(allocation.suggest_have_backbuffer)
Send right away for now, but should be done via GraphicsContext extension, and
initiated by the compositor.

https://chromiumcodereview.appspot.com/9702081/diff/1/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://chromiumcodereview.appspot.com/9702081/diff/1/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:577: printf("   [%i - %p] %s\n",
getpid(), this, __PRETTY_FUNCTION__);
printfs will be removed..

https://chromiumcodereview.appspot.com/9702081/diff/1/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:580:
gfx::GLSurface::BUFFER_ALLOCATION_FRONT_AND_BACK);
I will decouple front/back buffer management, but for now I'm using the existing
code as is.

https://chromiumcodereview.appspot.com/9702081/diff/1/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:588:
(allocation_.suggest_have_frontbuffer) ?
Ditto here for decoupling.  As is, we wait for renderer to request backbuffer
drop to initiate dropping frontbuffer. This will change.

https://chromiumcodereview.appspot.com/9702081/diff/1/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_allocation.h (right):

https://chromiumcodereview.appspot.com/9702081/diff/1/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_allocation.h:4: 
I've separated out the two different "Allocations", one intended for the
renderer/client (manages allocation bytes, backbuffer), and one for the
browser/host (manages frontbuffer).

The GpuMemoryManager manages and assigns a single unified allocation, and the
GpuCommandBufferStub splits it accordingly.

https://chromiumcodereview.appspot.com/9702081/diff/1/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_allocation.h:10: #include "ui/gfx/gl/gl_surface.h"
I dont need this, will remove..

https://chromiumcodereview.appspot.com/9702081/diff/1/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_allocation.h:18: class
GpuMemoryAllocationForRenderer {
This name will need to change to something more general, but I couldn't come up
with something sufficiently general yet appropriate.  Producer/consumer
client/host etc seem not to apply well.

Suggestions welcome.

https://chromiumcodereview.appspot.com/9702081/diff/1/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_allocation.h:53: class
GpuMemoryAllocationForBrowser {
ditto on naming here.

https://chromiumcodereview.appspot.com/9702081/diff/1/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_allocation.h:78: class GpuMemoryAllocation :
public GpuMemoryAllocationForRenderer,
This could have been std::pair<..ForRenderer, ..ForBrowser>, but I figured this
is nicer for the GpuMemoryManager.

https://chromiumcodereview.appspot.com/9702081/diff/1/content/common/gpu/gpu_...
File content/common/gpu/gpu_messages.h (right):

https://chromiumcodereview.appspot.com/9702081/diff/1/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:22: #include "ui/gfx/gl/gl_surface.h"
will remove.

[piman, 2012-03-15 20:14:31]

https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/c...
File content/common/gpu/client/command_buffer_proxy.cc (right):

https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/c...
content/common/gpu/client/command_buffer_proxy.cc:112: Send(new
GpuCommandBufferMsg_CreateBackbuffer(route_id_));
The problem is that here you have no ordering wrt pending commands.
You really want to have this go all the way to the compositor and let it decide
to actually drop the back buffer (it may want to finish the current frame, it
will need to to reset the damage rect after dropping the back buffer).

https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
File content/common/gpu/gpu_memory_allocation.h (right):

https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
content/common/gpu/gpu_memory_allocation.h:17: class
GpuMemoryAllocationForRenderer {
style nit: if it has public fields, and essentially behaves like a struct, then
we make it a struct. If it's a class, then it should have no public fields. See
style guide.

https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
content/common/gpu/gpu_memory_allocation.h:39: friend inline bool
operator==(const GpuMemoryAllocationForRenderer& lhs,
No need for friend or inline. Also, if it's a non-static member, it should use
the syntax for the member operator (i.e. 'this' is the lhs, and it takes only
one param).

https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
content/common/gpu/gpu_memory_allocation.h:52: class
GpuMemoryAllocationForBrowser {
class->struct.

https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
File content/common/gpu/gpu_messages.h (right):

https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
content/common/gpu/gpu_messages.h:444:
IPC_MESSAGE_ROUTED0(GpuCommandBufferMsg_CreateBackbuffer)
Doing these as commands into the command buffer (like SetVisibility is done now)
would make it easier to track state in the GLES2CmdDecoder, as to either
generate errors in case of misuse or to implicitly recreate the back buffer on
draw/clear/readback commands.

Similarly, client-side, making it go through the GLES2Implementation would let
you add asserts.

[mmocny, 2012-03-15 20:39:27]

https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/c...
File content/common/gpu/client/command_buffer_proxy.cc (right):

https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/c...
content/common/gpu/client/command_buffer_proxy.cc:112: Send(new
GpuCommandBufferMsg_CreateBackbuffer(route_id_));
You are correct, this will be moved to the extension once that is added.  As it
stands it is slightly better then we have now, because the compositor will
receive a callback before the backbuffer is dropped, and it can do things like
reset damage tracker.
Right now the callback doesn't plumb the bool suggest_drop_backbuffer but I am
working on adding that now.
Finally, I'll add the extension to GraphicsContext and move this code there.

On 2012/03/15 20:14:32, piman wrote:
> The problem is that here you have no ordering wrt pending commands.
> You really want to have this go all the way to the compositor and let it
decide
> to actually drop the back buffer (it may want to finish the current frame, it
> will need to to reset the damage rect after dropping the back buffer).

https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
File content/common/gpu/gpu_memory_allocation.h (right):

https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
content/common/gpu/gpu_memory_allocation.h:17: class
GpuMemoryAllocationForRenderer {
Fair, this is meant to act as a struct.

On 2012/03/15 20:14:32, piman wrote:
> style nit: if it has public fields, and essentially behaves like a struct,
then
> we make it a struct. If it's a class, then it should have no public fields.
See
> style guide.

https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
content/common/gpu/gpu_memory_allocation.h:39: friend inline bool
operator==(const GpuMemoryAllocationForRenderer& lhs,
Since this is the global non-member version of op==, it does need the friend
keyword to allow defining it here (though it doesn't actually need friendship). 
I am not actually sure if it strictly needs inline, it would if it were static
or defined out-of-class.

I'll concede that this is a C++ dark corner and may be awkward, and since I
prefer to leave it defined inside the class I'll change to use the member
version.

On 2012/03/15 20:14:32, piman wrote:
> No need for friend or inline. Also, if it's a non-static member, it should use
> the syntax for the member operator (i.e. 'this' is the lhs, and it takes only
> one param).

https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
File content/common/gpu/gpu_messages.h (right):

https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
content/common/gpu/gpu_messages.h:444:
IPC_MESSAGE_ROUTED0(GpuCommandBufferMsg_CreateBackbuffer)
Okay, I was undecided here, and I think you are correct.  It may make it harder
for me to temporarily hook dropping frontbuffers onto dropping backbuffers, but
that needs decoupling anyway.

On 2012/03/15 20:14:32, piman wrote:
> Doing these as commands into the command buffer (like SetVisibility is done
now)
> would make it easier to track state in the GLES2CmdDecoder, as to either
> generate errors in case of misuse or to implicitly recreate the back buffer on
> draw/clear/readback commands.
> 
> Similarly, client-side, making it go through the GLES2Implementation would let
> you add asserts.

[mmocny, 2012-03-15 23:02:45]

Latest patch relies on https://bugs.webkit.org/show_bug.cgi?id=81279 landing.

On 2012/03/15 20:39:27, mmocny wrote:
>
https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/c...
> File content/common/gpu/client/command_buffer_proxy.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/c...
> content/common/gpu/client/command_buffer_proxy.cc:112: Send(new
> GpuCommandBufferMsg_CreateBackbuffer(route_id_));
> You are correct, this will be moved to the extension once that is added.  As
it
> stands it is slightly better then we have now, because the compositor will
> receive a callback before the backbuffer is dropped, and it can do things like
> reset damage tracker.
> Right now the callback doesn't plumb the bool suggest_drop_backbuffer but I am
> working on adding that now.
> Finally, I'll add the extension to GraphicsContext and move this code there.
> 
> On 2012/03/15 20:14:32, piman wrote:
> > The problem is that here you have no ordering wrt pending commands.
> > You really want to have this go all the way to the compositor and let it
> decide
> > to actually drop the back buffer (it may want to finish the current frame,
it
> > will need to to reset the damage rect after dropping the back buffer).
> 
>
https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
> File content/common/gpu/gpu_memory_allocation.h (right):
> 
>
https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
> content/common/gpu/gpu_memory_allocation.h:17: class
> GpuMemoryAllocationForRenderer {
> Fair, this is meant to act as a struct.
> 
> On 2012/03/15 20:14:32, piman wrote:
> > style nit: if it has public fields, and essentially behaves like a struct,
> then
> > we make it a struct. If it's a class, then it should have no public fields.
> See
> > style guide.
> 
>
https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
> content/common/gpu/gpu_memory_allocation.h:39: friend inline bool
> operator==(const GpuMemoryAllocationForRenderer& lhs,
> Since this is the global non-member version of op==, it does need the friend
> keyword to allow defining it here (though it doesn't actually need
friendship). 
> I am not actually sure if it strictly needs inline, it would if it were static
> or defined out-of-class.
> 
> I'll concede that this is a C++ dark corner and may be awkward, and since I
> prefer to leave it defined inside the class I'll change to use the member
> version.
> 
> On 2012/03/15 20:14:32, piman wrote:
> > No need for friend or inline. Also, if it's a non-static member, it should
use
> > the syntax for the member operator (i.e. 'this' is the lhs, and it takes
only
> > one param).
> 
>
https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
> File content/common/gpu/gpu_messages.h (right):
> 
>
https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
> content/common/gpu/gpu_messages.h:444:
> IPC_MESSAGE_ROUTED0(GpuCommandBufferMsg_CreateBackbuffer)
> Okay, I was undecided here, and I think you are correct.  It may make it
harder
> for me to temporarily hook dropping frontbuffers onto dropping backbuffers,
but
> that needs decoupling anyway.
> 
> On 2012/03/15 20:14:32, piman wrote:
> > Doing these as commands into the command buffer (like SetVisibility is done
> now)
> > would make it easier to track state in the GLES2CmdDecoder, as to either
> > generate errors in case of misuse or to implicitly recreate the back buffer
on
> > draw/clear/readback commands.
> > 
> > Similarly, client-side, making it go through the GLES2Implementation would
let
> > you add asserts.

[mmocny, 2012-03-19 19:27:29]

I am going to break this patch up into smaller pieces, since the gles2 extension
work will take a little while longer, while the suggest_have_backbuffer work has
already landed WebKit side (https://bugs.webkit.org/show_bug.cgi?id=81279), and
while we need the les2 extension to support wk control of backbuffer, the memory
allocation changes are fine as they are and would be nice to have them for next
deps roll.

On 2012/03/15 23:02:45, mmocny wrote:
> Latest patch relies on https://bugs.webkit.org/show_bug.cgi?id=81279 landing.
> 
> On 2012/03/15 20:39:27, mmocny wrote:
> >
>
https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/c...
> > File content/common/gpu/client/command_buffer_proxy.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/c...
> > content/common/gpu/client/command_buffer_proxy.cc:112: Send(new
> > GpuCommandBufferMsg_CreateBackbuffer(route_id_));
> > You are correct, this will be moved to the extension once that is added.  As
> it
> > stands it is slightly better then we have now, because the compositor will
> > receive a callback before the backbuffer is dropped, and it can do things
like
> > reset damage tracker.
> > Right now the callback doesn't plumb the bool suggest_drop_backbuffer but I
am
> > working on adding that now.
> > Finally, I'll add the extension to GraphicsContext and move this code there.
> > 
> > On 2012/03/15 20:14:32, piman wrote:
> > > The problem is that here you have no ordering wrt pending commands.
> > > You really want to have this go all the way to the compositor and let it
> > decide
> > > to actually drop the back buffer (it may want to finish the current frame,
> it
> > > will need to to reset the damage rect after dropping the back buffer).
> > 
> >
>
https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
> > File content/common/gpu/gpu_memory_allocation.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
> > content/common/gpu/gpu_memory_allocation.h:17: class
> > GpuMemoryAllocationForRenderer {
> > Fair, this is meant to act as a struct.
> > 
> > On 2012/03/15 20:14:32, piman wrote:
> > > style nit: if it has public fields, and essentially behaves like a struct,
> > then
> > > we make it a struct. If it's a class, then it should have no public
fields.
> > See
> > > style guide.
> > 
> >
>
https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
> > content/common/gpu/gpu_memory_allocation.h:39: friend inline bool
> > operator==(const GpuMemoryAllocationForRenderer& lhs,
> > Since this is the global non-member version of op==, it does need the friend
> > keyword to allow defining it here (though it doesn't actually need
> friendship). 
> > I am not actually sure if it strictly needs inline, it would if it were
static
> > or defined out-of-class.
> > 
> > I'll concede that this is a C++ dark corner and may be awkward, and since I
> > prefer to leave it defined inside the class I'll change to use the member
> > version.
> > 
> > On 2012/03/15 20:14:32, piman wrote:
> > > No need for friend or inline. Also, if it's a non-static member, it should
> use
> > > the syntax for the member operator (i.e. 'this' is the lhs, and it takes
> only
> > > one param).
> > 
> >
>
https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
> > File content/common/gpu/gpu_messages.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/9702081/diff/2001/content/common/gpu/g...
> > content/common/gpu/gpu_messages.h:444:
> > IPC_MESSAGE_ROUTED0(GpuCommandBufferMsg_CreateBackbuffer)
> > Okay, I was undecided here, and I think you are correct.  It may make it
> harder
> > for me to temporarily hook dropping frontbuffers onto dropping backbuffers,
> but
> > that needs decoupling anyway.
> > 
> > On 2012/03/15 20:14:32, piman wrote:
> > > Doing these as commands into the command buffer (like SetVisibility is
done
> > now)
> > > would make it easier to track state in the GLES2CmdDecoder, as to either
> > > generate errors in case of misuse or to implicitly recreate the back
buffer
> on
> > > draw/clear/readback commands.
> > > 
> > > Similarly, client-side, making it go through the GLES2Implementation would
> let
> > > you add asserts.

[mmocny, 2012-03-19 20:31:40]

Alright, I've split the CL and this is now just the subset which will suggest to
the renderer when to drop backbuffer.

The WebKit portion has already landed, and now this patch is needed to send the
right value for the wk compositor allocation callback to make use of.

[piman, 2012-03-19 20:34:38]

lgtm

[commit-bot: I haz the power, 2012-03-19 21:55:20]

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

[commit-bot: I haz the power, 2012-03-19 23:39:45]

Change committed as 127572