Deferred canvas can now be flushed if an image is beyond a certain size to avoid a costly image cop…

Deferred canvas can now be flushed if an image is beyond a certain size to avoid a costly image copy.

BUG=http://code.google.com/p/chromium/issues/detail?id=137924
TEST=TestDeferredCanvasBitmapSizeThreshold unit test
Committed: https://code.google.com/p/skia/source/detail?r=6527

[junov, 2012-11-20 18:34:13]

https://codereview.appspot.com/6852071/diff/1/include/config/SkUserConfig.h
File include/config/SkUserConfig.h (right):

https://codereview.appspot.com/6852071/diff/1/include/config/SkUserConfig.h#n...
include/config/SkUserConfig.h:205: is considered too large and the copy done by
the deferred canvas too expensive,
This file is 80 col

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

https://codereview.appspot.com/6852071/diff/1/include/utils/SkDeferredCanvas....
include/utils/SkDeferredCanvas.h:122: size_t getBitmapSizeThreshold() const;
I don't see a use case for this method.

https://codereview.appspot.com/6852071/diff/1/include/utils/SkDeferredCanvas....
include/utils/SkDeferredCanvas.h:126: the deferred canvas.
Missing *

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

https://codereview.appspot.com/6852071/diff/1/src/utils/SkDeferredCanvas.cpp#...
src/utils/SkDeferredCanvas.cpp:35: size_t bitmapSizeThreshold) {
No tabs please.

https://codereview.appspot.com/6852071/diff/1/src/utils/SkDeferredCanvas.cpp#...
src/utils/SkDeferredCanvas.cpp:38: // a costly copy of the image by drawing it
immediately.
Comment not necessary. this code is self-explanatory.

https://codereview.appspot.com/6852071/diff/1/src/utils/SkDeferredCanvas.cpp#...
src/utils/SkDeferredCanvas.cpp:39: (bitmap->getSize() > bitmapSizeThreshold))) {
getSize in bytes.  In the description of setBitmapSizeThreshold, it was not
clear that the argument value was to be expressed in bytes (could have been
pixels).

https://codereview.appspot.com/6852071/diff/1/src/utils/SkDeferredCanvas.cpp#...
src/utils/SkDeferredCanvas.cpp:450: size_t
DeferredDevice::GetDefaultBitmapSizeThreshold() {
This method is not necessary

https://codereview.appspot.com/6852071/diff/1/src/utils/SkDeferredCanvas.cpp#...
src/utils/SkDeferredCanvas.cpp:584: size_t
SkDeferredCanvas::getBitmapSizeThreshold() const {
Method not necessary

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

https://codereview.appspot.com/6852071/diff/1/tests/DeferredCanvasTest.cpp#ne...
tests/DeferredCanvasTest.cpp:398: MockDevice mockDevice(store);
You are not mocking, just use SkDevice instead of MockDevice.

https://codereview.appspot.com/6852071/diff/1/tests/DeferredCanvasTest.cpp#ne...
tests/DeferredCanvasTest.cpp:406: for (int i = 0; i < 6; i++) {
You really just need two iterations to test what you want to test.  For clarity,
you might as well unroll.

[sugoi, 2012-11-21 13:57:27]

On 2012/11/20 18:34:13, junov wrote:
>
https://codereview.appspot.com/6852071/diff/1/src/utils/SkDeferredCanvas.cpp#...
> src/utils/SkDeferredCanvas.cpp:584: size_t
> SkDeferredCanvas::getBitmapSizeThreshold() const {
> Method not necessary

I fixed all other comments except this one. I do need this function at line 79
of SkDeferredCanvas.cpp because I need to access this value and the
DeferredDevice class, which is local to this file, is not yet defined at this
point, so I need to access this value through the canvas. If you prefer, I could
reorder code in this file instead of implementing this function. Your choice.

[junov, 2012-11-21 14:50:38]

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

https://codereview.appspot.com/6852071/diff/8001/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:539: device ? device->getBitmapSizeThreshold() :
Ternary operator not necessary.  If there is no device, no need to turn off
deferred drawing. Condition could be:
if (canvas.isDeferredDrawing() && NULL != device &&
shouldDrawImmediately(bitmap, paint, device->getBitmapSizeThreshold())

https://codereview.appspot.com/6852071/diff/8001/tests/DeferredCanvasTest.cpp
File tests/DeferredCanvasTest.cpp (right):

https://codereview.appspot.com/6852071/diff/8001/tests/DeferredCanvasTest.cpp...
tests/DeferredCanvasTest.cpp:421: newBytesAllocated =
canvas.storageAllocatedForRecording();
This statistic is cumulative, so it is affected by the storage that was
allocated for the previous test step.  You could scope each of the test steps to
isolate their canvas and device objects.

[sugoi, 2012-11-21 15:36:27]

done

[junov, 2012-11-21 15:39:07]

On 2012/11/21 15:36:27, sugoi wrote:
> done

lgtm

[reed1, 2012-11-21 15:57:24]

What is the need for the #define, since there is a runtime API for the
threshold?

[junov, 2012-11-21 16:16:39]

You're right, let's just have a local constant in the .cpp file with the
default (~0)

On Wed, Nov 21, 2012 at 10:57 AM, <reed@google.com> wrote:

> What is the need for the #define, since there is a runtime API for the
> threshold?
>
>
https://codereview.appspot.**com/6852071/<https://codereview.appspot.com/6852...
>

[sugoi1, 2012-11-21 16:18:03]

Ok, will do.

On Wed, Nov 21, 2012 at 11:16 AM, Justin Novosad <junov@google.com> wrote:

> You're right, let's just have a local constant in the .cpp file with the
> default (~0)
>
>
> On Wed, Nov 21, 2012 at 10:57 AM, <reed@google.com> wrote:
>
>> What is the need for the #define, since there is a runtime API for the
>> threshold?
>>
>>
https://codereview.appspot.**com/6852071/<https://codereview.appspot.com/6852...
>>
>
>