Fix unbounded memory consumption problem with run away deferred canvases

Fix unbound memory consumption problem with run away deferred canvases.

With this CL, deferred canvases will trigger a flush when then the 
memory allocated for recording commands (including flattened objects)
exceeds 64MB.

TEST=DeferredCanvas skia unit test, test step TestDeferredCanvasMemoryLimit
BUG=http://code.google.com/p/chromium/issues/detail?id=137884
Committed: https://code.google.com/p/skia/source/detail?r=4714

[junov1, 2012-07-19 19:46:25]

PTAL

[reed1, 2012-07-20 11:58:53]

As I am in a strange land called Canada today, I won't be able to fully review
this until Monday.

1. What is the status of switching DeferredCanvas to using Pipe?
2. Does Pipe need these changes, or is there a less invasive way?

[junov1, 2012-07-20 13:16:27]

DeferredCanvas using pipe is functional, and it appears to be in good health
(passes all skia tests), but I have not yet fully evaluated performance impact. 
Today I will try running webkit layout tests through it to see if anything
explodes.  Then, maybe I'll flip the switch in Chrome, and see what the GPU
performance test bots tell us.

Both the pipe and the picture paths have this problem.

In patch Set 1, I am fixing both the SkPicture and the pipe paths.  Fixing the
pipe path is less intrusive because the fix is all contained within the pipe
controller derived class (all in SkDeferredCanvas.h/cpp)

FYI: Keeping deferred canvas turned on by default is critical for M22 because
other work depends on it.  To be safe, I wanted to keep the picture path alive
in case we run out of time for getting the pipe path up to speed.

On 2012/07/20 11:58:53, reed1 wrote:
> As I am in a strange land called Canada today, I won't be able to fully review
> this until Monday.
> 
> 1. What is the status of switching DeferredCanvas to using Pipe?
> 2. Does Pipe need these changes, or is there a less invasive way?

[Leon, 2012-07-20 14:20:24]

https://codereview.appspot.com/6425053/diff/1/src/utils/SkDeferredCanvas.cpp
File src/utils/SkDeferredCanvas.cpp (right):

https://codereview.appspot.com/6425053/diff/1/src/utils/SkDeferredCanvas.cpp#...
src/utils/SkDeferredCanvas.cpp:628: flushPending();
this->flushPending()

https://codereview.appspot.com/6425053/diff/1/src/utils/SkDeferredCanvas.cpp#...
src/utils/SkDeferredCanvas.cpp:632: flushPending();
this->flushPending()

https://codereview.appspot.com/6425053/diff/1/tests/DeferredCanvasTest.cpp
File tests/DeferredCanvasTest.cpp (right):

https://codereview.appspot.com/6425053/diff/1/tests/DeferredCanvasTest.cpp#ne...
tests/DeferredCanvasTest.cpp:211: // The following expected result is subject to
change depending on the
Should we put a note in SkDeferredCanvas.cpp by kMaxRecordingStorageBytes
stating that the test depends on the current value? That way if someone does
change it they'll immediately know to modify the test, rather than after they
check in the new code, see the bot break, and look up the test to understand
why.

[junov1, 2012-07-20 19:11:21]

So after all the testing I conducted today, I can say that the state of the
SkGPipe implementation looks very promising.  Therefore, I scrapped all the
SkPicture changes that were in patch set 1, to just focus on the gpipe path. 
New patch also has the changes suggested by scroggo.

[Leon, 2012-07-20 19:21:34]

https://codereview.appspot.com/6425053/diff/7001/tests/DeferredCanvasTest.cpp
File tests/DeferredCanvasTest.cpp (right):

https://codereview.appspot.com/6425053/diff/7001/tests/DeferredCanvasTest.cpp...
tests/DeferredCanvasTest.cpp:217: REPORTER_ASSERT(reporter,
mockDevice.fDrawBitmapCallCount == 4);
Now that the loop above only goes to 1, won't this fail?

Also, since the SkPicture path no longer flushes based on memory, will this test
be meaningful?

[junov1, 2012-07-20 19:30:32]

D'oh! that was a temp edit.  Good catch.

On Fri, Jul 20, 2012 at 3:21 PM, <scroggo@google.com> wrote:

>
> https://codereview.appspot.**com/6425053/diff/7001/tests/**
>
DeferredCanvasTest.cpp<https://codereview.appspot.com/6425053/diff/7001/tests/DeferredCanvasTest.cpp>
> File tests/DeferredCanvasTest.cpp (right):
>
> https://codereview.appspot.**com/6425053/diff/7001/tests/**
>
DeferredCanvasTest.cpp#**newcode217<https://codereview.appspot.com/6425053/diff/7001/tests/DeferredCanvasTest.cpp#newcode217>
> tests/DeferredCanvasTest.cpp:**217: REPORTER_ASSERT(reporter,
> mockDevice.**fDrawBitmapCallCount == 4);
> Now that the loop above only goes to 1, won't this fail?
>
> Also, since the SkPicture path no longer flushes based on memory, will
> this test be meaningful?
>
>
https://codereview.appspot.**com/6425053/<https://codereview.appspot.com/6425...
>

[junov1, 2012-07-20 19:38:18]

New patch.
Test will be disabled until The SkGPipe path in SkDeferredCanvas starts
conditionally flattening bitmaps.

[Leon, 2012-07-20 19:48:16]

lgtm

[junov1, 2012-07-20 20:06:37]

On 2012/07/20 19:48:16, scroggo-work wrote:
> lgtm

Thx. Will wait to commit in case Mike has something to say.

[reed1, 2012-07-20 20:29:51]

much cleaner!

Sure feels like that giant budget should be available as a setter to the caller,
so that Chrome can decide if 64meg is right (what if they have N deferred
canvases?)

[junov1, 2012-07-20 20:38:30]

Hah! That will also allow me to make the unit test independent from the
hard-coded constant.

On Fri, Jul 20, 2012 at 4:29 PM, <reed@google.com> wrote:

> much cleaner!
>
> Sure feels like that giant budget should be available as a setter to the
> caller, so that Chrome can decide if 64meg is right (what if they have N
> deferred canvases?)
>
>
https://codereview.appspot.**com/6425053/<https://codereview.appspot.com/6425...
>

[junov1, 2012-07-20 21:07:47]

New patch.

[reed1, 2012-07-21 16:48:08]

lgtm

how much of what is in the public header can be made private to the impl?

https://codereview.appspot.com/6425053/diff/7002/include/utils/SkDeferredCanv...
File include/utils/SkDeferredCanvas.h (right):

https://codereview.appspot.com/6425053/diff/7002/include/utils/SkDeferredCanv...
include/utils/SkDeferredCanvas.h:172: class DeferredPipeController : public
SkGPipeController {
Does this class need to be in the public header, or can it be private to the
impl?

[junov1, 2012-07-23 13:32:06]

On 2012/07/21 16:48:08, reed1 wrote:
> lgtm
> 
> how much of what is in the public header can be made private to the impl?

That went through my mind when I was looking at the SkGPipe implementation
(which has a lot of private stuff).  There is a whole lot I could hide.  In a
few places I am directly accessing the deferred device, but that could be hidden
with a couple additions to the SkDeferredCanvas API.  I'll get on that as soon
as I am done with M22 stuff.  

http://code.google.com/p/skia/issues/detail?id=728