Refactoring how SkDeferredCanvas manages mutable bitmaps

Refactoring how SkDeferredCanvas manages mutable bitmaps

This CL makes the SkGPipe flavor of SkDeferredCanvas properly
decide whether to flush or record mutable bitmaps.  The flushing
is now managed by conditionally switching the canvas to non-deferred
mode, which avoids an unnecessary transient copy of the bitmap.

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

[junov1, 2012-07-23 20:28:26]

PTAL

[junov1, 2012-07-23 20:41:50]

On 2012/07/23 20:28:26, junov1 wrote:
> PTAL

Patch set 3+4 fix the case of a source bitmap with no pixelRef, and remove
unnecessary changes to pixelRef classes

[reed1, 2012-07-23 20:58:48]

Lets have a VC tomorrow. I need some hand-holding understanding the issue/fix.

[junov1, 2012-07-23 21:10:00]

On 2012/07/23 20:58:48, reed1 wrote:
> Lets have a VC tomorrow. I need some hand-holding understanding the issue/fix.

Sure.  In a nutshell, there is a class called SharedHeap in SkGPipeWrite.cpp
that is used keep a handle on SkBitmap objects that are referenced by draw
command that are in the pipe.  SharedHeap is uses in the case where the reader
and writer share the same address space (same process).  Some bitmaps in the
shared heap just reference pixelref objects from the outside world (e.g. WebKit)
if they are marked as immutable.  In the mutable case, the bitmap gets copied. 
These copies use memory allocated from the global heap (SkNEW). At the present
time, SharedHeap does not track memory usage for bitmap copies.

[Leon, 2012-07-23 21:23:22]

I think that if we can, we should have the SharedHeap report how much space it
is using (though I haven't yet checked to see if that's more complicated).

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

https://codereview.appspot.com/6421060/diff/1012/include/utils/SkDeferredCanv...
include/utils/SkDeferredCanvas.h:12: #define SK_DEFERRED_CANVAS_USES_GPIPE 1
So we're turning Pipe mode on? Just want to make sure bc that was not mentioned
in the CL description.

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

https://codereview.appspot.com/6421060/diff/1012/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:579: // the controller, so we do as best we can
to track consumption here
Would it be better to have the writer or the controller keep track of this?

[junov1, 2012-07-24 15:00:43]

On Mon, Jul 23, 2012 at 5:23 PM, <scroggo@google.com> wrote:

> I think that if we can, we should have the SharedHeap report how much
> space it is using (though I haven't yet checked to see if that's more
> complicated).
>

Yes, that would be right solution. But it is more complicated. This CL is a
temporary band-aid to prevent Chrome from going OOM in the short term
(M22).


>
>
> https://codereview.appspot.**com/6421060/diff/1012/include/**
>
utils/SkDeferredCanvas.h<https://codereview.appspot.com/6421060/diff/1012/include/utils/SkDeferredCanvas.h>
> File include/utils/**SkDeferredCanvas.h (right):
>
> https://codereview.appspot.**com/6421060/diff/1012/include/**
>
utils/SkDeferredCanvas.h#**newcode12<https://codereview.appspot.com/6421060/diff/1012/include/utils/SkDeferredCanvas.h#newcode12>
> include/utils/**SkDeferredCanvas.h:12: #define
> SK_DEFERRED_CANVAS_USES_GPIPE 1
> So we're turning Pipe mode on? Just want to make sure bc that was not
> mentioned in the CL description.
>

Oops

>
> https://codereview.appspot.**com/6421060/diff/1012/src/**
>
utils/SkDeferredCanvas.cpp<https://codereview.appspot.com/6421060/diff/1012/src/utils/SkDeferredCanvas.cpp>
> File src/utils/SkDeferredCanvas.cpp (right):
>
> https://codereview.appspot.**com/6421060/diff/1012/src/**
>
utils/SkDeferredCanvas.cpp#**newcode579<https://codereview.appspot.com/6421060/diff/1012/src/utils/SkDeferredCanvas.cpp#newcode579>
> src/utils/SkDeferredCanvas.**cpp:579: // the controller, so we do as best
> we can to track consumption here
> Would it be better to have the writer or the controller keep track of
> this?
>

That would be nice, but much more work.  If the pixel got flattened instead
of copied (as is the case with SkPicture), then the controller would easily
keep track of this.  However flattening to the stream may not be a good
strategy because the playback counting smarts in SharedHeap would not be
possible.


>
>
https://codereview.appspot.**com/6421060/<https://codereview.appspot.com/6421...
>

[Leon, 2012-07-24 15:43:58]

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

https://codereview.appspot.com/6421060/diff/1013/include/utils/SkDeferredCanv...
include/utils/SkDeferredCanvas.h:89: *  Returns true if deferred drawing is
currenlty enabled.
currently*

https://codereview.appspot.com/6421060/diff/1013/include/utils/SkDeferredCanv...
include/utils/SkDeferredCanvas.h:247: void accountForTempBitmapStorage(const
SkBitmap& bitmap);
Perhaps this should be inside #if SK_DEFERRED_CANVAS_USES_GPIPE.

Also, could you make a comment that this is temporary?

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

https://codereview.appspot.com/6421060/diff/1013/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:366: this->accountForTempBitmapStorage(bitmap);
Since this is required each time we call autoDraw (and it shares a parameter),
maybe this should go inside autoDraw's constructor (that also means we can
remove less code when we make Pipe do the right thing).

https://codereview.appspot.com/6421060/diff/1013/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:575: void
SkDeferredCanvas::DeferredDevice::accountForTempBitmapStorage(const SkBitmap&
bitmap) {
I wonder if a better temporary fix would be to put this on
DeferredPipeController.  Then
DeferredPipeController.storageAllocatedForRecording() could include
fTempBitmapStorage.

[Leon, 2012-07-24 19:19:04]

utils/SkDeferredCanvas.cpp#**newcode579<https://codereview.appspot.com/6421060/diff/1012/src/utils/SkDeferredCanvas.cpp#newcode579>
> > src/utils/SkDeferredCanvas.**cpp:579: // the controller, so we do as best
> > we can to track consumption here
> > Would it be better to have the writer or the controller keep track of
> > this?
> >
> 
> That would be nice, but much more work.  If the pixel got flattened instead
> of copied (as is the case with SkPicture), then the controller would easily
> keep track of this.  However flattening to the stream may not be a good
> strategy because the playback counting smarts in SharedHeap would not be
> possible.

Using the SharedHeap may still be the best strategy, but when flattening, Pipe
ensures that bitmaps do not get thrown away before being used (and it keeps some
flat versions on the writer side so that hopefully it does not need to copy and
unflatten the same bitmap multiple times).

[Leon, 2012-07-24 22:01:57]

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

https://codereview.appspot.com/6421060/diff/1013/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:673: fTempBitmapStorage = 0;
This does not reflect the fact that the SharedHeap may still have stored
bitmaps. The SharedHeap only removes old ones when a new one is inserted (it
keeps the old ones around in case they are used again - that said, we could
consider a call to flush all the existing ones). I assume that after the
playback, the old ones will be safe for the SharedHeap to throw out, so we won't
get into a situation where the actual bitmap storage increases faster than
fTempBitmapStorage.

This just highlights that I need to keep track of bytes allocated by the Pipe so
you can do a query (probably on the PipeController) rather than attempting to
keep track on your own. I'll get to work on a CL for that.

[junov1, 2012-07-25 14:53:30]

On 2012/07/24 22:01:57, scroggo-work wrote:
>
https://codereview.appspot.com/6421060/diff/1013/src/utils/SkDeferredCanvas.cpp
> File src/utils/SkDeferredCanvas.cpp (right):
> 
>
https://codereview.appspot.com/6421060/diff/1013/src/utils/SkDeferredCanvas.c...
> src/utils/SkDeferredCanvas.cpp:673: fTempBitmapStorage = 0;
> This does not reflect the fact that the SharedHeap may still have stored
> bitmaps. The SharedHeap only removes old ones when a new one is inserted (it
> keeps the old ones around in case they are used again - that said, we could
> consider a call to flush all the existing ones). I assume that after the
> playback, the old ones will be safe for the SharedHeap to throw out, so we
won't
> get into a situation where the actual bitmap storage increases faster than
> fTempBitmapStorage.
> 

I think we can live with that. These are bitmap that are in an "unlocked" state
with respect to SharedHeap, right?  I think it is best to NOT take those into
account. And it is useful to not purge them from a performance point of view.

> This just highlights that I need to keep track of bytes allocated by the Pipe
so
> you can do a query (probably on the PipeController) rather than attempting to
> keep track on your own. I'll get to work on a CL for that.

Thanks.

[junov1, 2012-07-25 14:55:33]

https://codereview.appspot.com/6421060/diff/1013/src/utils/SkDeferredCanvas.c...
> src/utils/SkDeferredCanvas.cpp:575: void
> SkDeferredCanvas::DeferredDevice::accountForTempBitmapStorage(const SkBitmap&
> bitmap) {
> I wonder if a better temporary fix would be to put this on
> DeferredPipeController.  Then
> DeferredPipeController.storageAllocatedForRecording() could include
> fTempBitmapStorage.

But the controller does not know the writer.

[junov1, 2012-07-25 14:55:47]

new patch

[Leon, 2012-07-25 17:07:26]

lgtm

[junov1, 2012-07-25 17:20:57]

On 2012/07/25 17:07:26, scroggo-work wrote:
> lgtm

Mike: want to take a last peek before I throw it down the hatch?

[reed1, 2012-07-25 17:26:45]

lgtm