Add calls to EnsureGPUMemoryAvailable into GLES2DecoderImpl

Add calls to EnsureGPUMemoryAvailable into GLES2DecoderImpl

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

[greggman, 2013-01-19 01:18:40]

Hey Chris, can you PTAL and give me your feedback? 

I still need to add tests and a few more checks.

[ccameron, 2013-01-19 01:38:31]

On 2013/01/19 01:18:40, greggman wrote:
> Hey Chris, can you PTAL and give me your feedback? 
> 
> I still need to add tests and a few more checks.

This is coming along well -- the general structure of getting to the
GpuMemoryManager is good.

The situation where one grows or shrinks the size of a resource has some issue. 
In particular, we're verifying that we have enough space to allocate the new
size, disregarding the fact that we are going to free the old size.  So if we
allocate something 1024x1024, then do a glTexImage2D to make it 128x128, that
operation (which should only decrease memory usage) would fail with OOM.

It may be better to have something where we make a call that says
"EnsureGpuMemoryAdjustment(old_size, new_size)".

Also, it may be simpler to have the Ensure function return a promise, which you
can cash in at the SetLevelInfo (and then just get rid of the direct adjustment
change).  I outlined it inline.  Too over-engineered?

[ccameron, 2013-01-19 01:38:46]

(drr... and including inline comments)

https://codereview.chromium.org/12017032/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/12017032/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7965: if
(!memory_tracker()->EnsureGPUMemoryAvailable(estimated_size)) {
So that we can take into account the current usage:
memory_tracker()->EnsureGPUMemoryAvailable(estimatedSize(), pixels_size)

We could also have it return a "promise", to the effect of
promise = memory_tracker()->EnsureGPUMemoryAvailable(estimatedSize(),
pixels_size);
if (promise.isValid()) { ... }

and then cash in the promis later...

https://codereview.chromium.org/12017032/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8022: border, internal_format,
GL_UNSIGNED_BYTE, true);
... we would cash in here (and otherwise in the promise's dtor).  By doing this,
we could remove the memory tracking from the SetLevelInfo (and maybe just assert
that estimatedSize()==pixel_size or <= pixel_size), so, something like

promise.cashIn(estimatedSize)

[greggman, 2013-01-19 01:47:46]

On 2013/01/19 01:38:31, ccameron1 wrote:
> On 2013/01/19 01:18:40, greggman wrote:
> > Hey Chris, can you PTAL and give me your feedback? 
> > 
> > I still need to add tests and a few more checks.
> 
> This is coming along well -- the general structure of getting to the
> GpuMemoryManager is good.
> 
> The situation where one grows or shrinks the size of a resource has some
issue. 
> In particular, we're verifying that we have enough space to allocate the new
> size, disregarding the fact that we are going to free the old size.  So if we
> allocate something 1024x1024, then do a glTexImage2D to make it 128x128, that
> operation (which should only decrease memory usage) would fail with OOM.

We know that some drivers will use double the memory. The textures are lazily
freed as in

  glTexImage2D
  glDrawXXX
  glTexImage2D <- does not free the texture but instead allocates a
        new one because the old one is still being rendered with :-(

So for now it seems like we should not worry about the old size?

> 
> It may be better to have something where we make a call that says
> "EnsureGpuMemoryAdjustment(old_size, new_size)".
> 
> Also, it may be simpler to have the Ensure function return a promise, which
you
> can cash in at the SetLevelInfo (and then just get rid of the direct
adjustment
> change).  I outlined it inline.  Too over-engineered?

That sounds over engineered to me but then maybe I'm not seeing the benefits.
How long can I hold on to a promise? Forever? The memory tracker is already
plummed into TextureManager, BufferManager and RenderbufferManager. I'd prefer
not to refactor those at this point.

[ccameron, 2013-01-22 10:22:47]

On 2013/01/19 01:47:46, greggman wrote:
> We know that some drivers will use double the memory.

Okay, as long as that is a conscious choice.  Exact behavior is probably very
hard to predict -- sometimes requiring both copies in memory, sometimes not.

> That sounds over engineered to me but then maybe I'm not seeing the benefits.
> How long can I hold on to a promise? Forever? The memory tracker is already
> plummed into TextureManager, BufferManager and RenderbufferManager. I'd prefer
> not to refactor those at this point.

If we're consciously choosing to say "you must be able to allocate the old size
plus the new size", then it adds nothing.  Let's go ahead without it.

For completeness' sake, the promise would be an object that would fall out of
scope (and would involve lots of code motion).

[greggman, 2013-01-22 18:01:12]

On 2013/01/22 10:22:47, ccameron1 wrote:
> On 2013/01/19 01:47:46, greggman wrote:
> > We know that some drivers will use double the memory.
> 
> Okay, as long as that is a conscious choice.  Exact behavior is probably very
> hard to predict -- sometimes requiring both copies in memory, sometimes not.
> 
> > That sounds over engineered to me but then maybe I'm not seeing the
benefits.
> > How long can I hold on to a promise? Forever? The memory tracker is already
> > plummed into TextureManager, BufferManager and RenderbufferManager. I'd
prefer
> > not to refactor those at this point.
> 
> If we're consciously choosing to say "you must be able to allocate the old
size
> plus the new size", then it adds nothing.  Let's go ahead without it.
> 
> For completeness' sake, the promise would be an object that would fall out of
> scope (and would involve lots of code motion).

So is there something you're asking me to change at this point or can I get an
lgtm?

[ccameron, 2013-01-22 18:11:27]

lgtm (sorry, thought I clicked this earlier).

[commit-bot: I haz the power, 2013-01-22 18:52:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gman@chromium.org/12017032/3001

[commit-bot: I haz the power, 2013-01-23 04:09:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gman@chromium.org/12017032/19001

[commit-bot: I haz the power, 2013-01-23 04:31:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gman@chromium.org/12017032/17003

[commit-bot: I haz the power, 2013-01-23 06:39:28]

Change committed as 178265