Make SkDeferredCanvas query SkGPipeWriter for space allocated for bitmaps.

Make SkDeferredCanvas query SkGPipeWriter for space allocated for bitmaps.

SkGPipe now has a method to report how much memory is used for its shared heap.

BUG=http://code.google.com/p/skia/issues/detail?id=738
TEST=DeferredCanvasTest

Committed: https://code.google.com/p/skia/source/detail?r=4791

[Leon, 2012-07-25 21:24:53]

I noticed that running the GM never triggers a flush due to hitting the limit.
Should I test in Chrome? I did run the DeferredCanvasTest, and we still flush
them (though I had to change the number).

[junov1, 2012-07-26 13:40:57]

That it OK. The flush should only be triggered in extreme or degenerate cases,
even in chrome.

[reed1, 2012-07-26 14:01:17]

lgtm

[junov1, 2012-07-26 14:03:19]

https://codereview.appspot.com/6445046/diff/1/include/utils/SkDeferredCanvas.h
File include/utils/SkDeferredCanvas.h (right):

https://codereview.appspot.com/6445046/diff/1/include/utils/SkDeferredCanvas....
include/utils/SkDeferredCanvas.h:354: friend class AutoImmediateDrawIfNeeded;
You can delete this line.  It was so that AutoImmediateDrawIfNeeded could call
accountForTempBitmapStorage.

https://codereview.appspot.com/6445046/diff/1/src/pipe/SkGPipePriv.h
File src/pipe/SkGPipePriv.h (right):

https://codereview.appspot.com/6445046/diff/1/src/pipe/SkGPipePriv.h#newcode181
src/pipe/SkGPipePriv.h:181: // FIXME: Is it reasonable to make copying a
bitmap/pixelref copy the
I think that is fine.  Why does this trouble you?

https://codereview.appspot.com/6445046/diff/1/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.appspot.com/6445046/diff/1/src/pipe/SkGPipeWrite.cpp#newco...
src/pipe/SkGPipeWrite.cpp:169: fBytesAllocated -= (iter->fBytesAllocated +
sizeof(BitmapInfo));
I think this line is only needed in debug builds.  Same for fBitmapCount--
below.

https://codereview.appspot.com/6445046/diff/1/src/pipe/SkGPipeWrite.cpp#newco...
src/pipe/SkGPipeWrite.cpp:185: * been removed, in which case we will no longer
be counting those bytes.
Interesting... The artifact you describe will not be an issue for deferred
canvas since it flushes synchronously and it flushes everything when it flushes.
 So this is fine by me.

https://codereview.appspot.com/6445046/diff/1/src/pipe/SkGPipeWrite.cpp#newco...
src/pipe/SkGPipeWrite.cpp:280: info->fBytesAllocated += orig.getSize();
I don't think you are don't think this is right.  In the case where the bitmap
is immutable, no new pixelRef was allocated, right?

[Leon, 2012-07-26 16:44:03]

New patch uploaded.

https://codereview.appspot.com/6445046/diff/1/include/utils/SkDeferredCanvas.h
File include/utils/SkDeferredCanvas.h (right):

https://codereview.appspot.com/6445046/diff/1/include/utils/SkDeferredCanvas....
include/utils/SkDeferredCanvas.h:354: friend class AutoImmediateDrawIfNeeded;
On 2012/07/26 14:03:19, junov1 wrote:
> You can delete this line.  It was so that AutoImmediateDrawIfNeeded could call
> accountForTempBitmapStorage.

Done.

https://codereview.appspot.com/6445046/diff/1/src/pipe/SkGPipePriv.h
File src/pipe/SkGPipePriv.h (right):

https://codereview.appspot.com/6445046/diff/1/src/pipe/SkGPipePriv.h#newcode181
src/pipe/SkGPipePriv.h:181: // FIXME: Is it reasonable to make copying a
bitmap/pixelref copy the
On 2012/07/26 14:03:19, junov1 wrote:
> I think that is fine.  Why does this trouble you?

Yeah, I think so, too. Generation ID was not always a part of SkBitmap, and I'm
not sure whether it was a deliberate decision to not copy it when the code was
added.

https://codereview.appspot.com/6445046/diff/1/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.appspot.com/6445046/diff/1/src/pipe/SkGPipeWrite.cpp#newco...
src/pipe/SkGPipeWrite.cpp:169: fBytesAllocated -= (iter->fBytesAllocated +
sizeof(BitmapInfo));
On 2012/07/26 14:03:19, junov1 wrote:
> I think this line is only needed in debug builds.  Same for fBitmapCount--
> below.

Done.

https://codereview.appspot.com/6445046/diff/1/src/pipe/SkGPipeWrite.cpp#newco...
src/pipe/SkGPipeWrite.cpp:185: * been removed, in which case we will no longer
be counting those bytes.
On 2012/07/26 14:03:19, junov1 wrote:
> Interesting... The artifact you describe will not be an issue for deferred
> canvas since it flushes synchronously and it flushes everything when it
flushes.
>  So this is fine by me.

Great! I just wanted to document it in case some other client does care.

https://codereview.appspot.com/6445046/diff/1/src/pipe/SkGPipeWrite.cpp#newco...
src/pipe/SkGPipeWrite.cpp:280: info->fBytesAllocated += orig.getSize();
On 2012/07/26 14:03:19, junov1 wrote:
> I don't think you are don't think this is right.  In the case where the bitmap
> is immutable, no new pixelRef was allocated, right? 

It is true that no new pixelRef was allocated, but it is possible that the
creator of the pixelRef has unreffed the pixelRef, so the SharedHeap has forced
that allocation to stick around.

I considered using a different name for the APIs to make it clearer what exactly
is happening, but could not decide on a better one.

[junov1, 2012-07-26 17:56:12]

> It is true that no new pixelRef was allocated, but it is possible that the
> creator of the pixelRef has unreffed the pixelRef, so the SharedHeap has
forced
> that allocation to stick around.

Hmmm... that make sense, but I am not convinced it serves the purpose well...  I
guess it is theoretically possible for immutable bitmaps to be collected without
bounds in the gpipe.  It is a rather extreme edge case for 2d canvas. I guess
preventing crashes trumps premature flushing.

Ok, I just convinced myself that you did the right thing. :-)

lgtm

[reed1, 2012-07-26 18:08:39]

lgtm

possibly we could rename the new getters at each level, if we think they can be
more clearerer about what is being computed/tracked, but I have no specific
suggestion.