Adding API and unit testing for deferred canvas clearing/purging

[junov1, 2012-02-08 22:07:53]

http://codereview.appspot.com/5646057/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

http://codereview.appspot.com/5646057/diff/1/include/core/SkCanvas.h#newcode298
include/core/SkCanvas.h:298: virtual bool isDrawingToLayer() const;
Incidental discovery thanks to new unit test: isDrawingToLayer was not working
with SkPicture. SkPictureRecord does not maintain fLayerCount.

http://codereview.appspot.com/5646057/diff/1/include/utils/SkDeferredCanvas.h
File include/utils/SkDeferredCanvas.h (left):

http://codereview.appspot.com/5646057/diff/1/include/utils/SkDeferredCanvas.h...
include/utils/SkDeferredCanvas.h:278: bool fBitmapInitialized;
fBitmapInitialized is unreferenced, comitted by mistake.

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

http://codereview.appspot.com/5646057/diff/1/include/utils/SkDeferredCanvas.h...
include/utils/SkDeferredCanvas.h:188: bool isFreshFrame();
This new method will make it possible for the chromium compositor to detect
conditions under which it is safe to disable the rate limiter.

http://codereview.appspot.com/5646057/diff/1/include/utils/SkDeferredCanvas.h...
include/utils/SkDeferredCanvas.h:191: void contentsCleared();
renaming for clarity

http://codereview.appspot.com/5646057/diff/1/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

http://codereview.appspot.com/5646057/diff/1/src/core/SkPictureRecord.cpp#new...
src/core/SkPictureRecord.cpp:54: */
I will change to "//" in next patch

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

http://codereview.appspot.com/5646057/diff/1/src/utils/SkDeferredCanvas.cpp#n...
src/utils/SkDeferredCanvas.cpp:329: SkRect bitmapRect = SkRect::MakeXYWH(left,
top, SkIntToScalar(bitmap.width()),
Will fix 80col in next patch

http://codereview.appspot.com/5646057/diff/1/src/utils/SkDeferredCanvas.cpp#n...
src/utils/SkDeferredCanvas.cpp:511: // beginRecording creates a new recording
canvas and discards the old one,
80col

[reed1, 2012-02-09 13:16:17]

Can you just save the index (getsavecount) of the first saveLayer, and in
restore, just notice when you pop of the last layer (i.e. the first layer you
saved)? Then you wouldn't need a stack.

[junov1, 2012-02-09 14:40:07]

On 2012/02/09 13:16:17, reed1 wrote:
> Can you just save the index (getsavecount) of the first saveLayer, and in
> restore, just notice when you pop of the last layer (i.e. the first layer you
> saved)? Then you wouldn't need a stack.

Of course...  what was I thinking? We don't need a layer count.

Done.

[reed1, 2012-02-09 15:15:44]

Don't know if we already have unittests for picture recording, but should be
easy to create a test for this behavior.

http://codereview.appspot.com/5646057/diff/7007/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

http://codereview.appspot.com/5646057/diff/7007/src/core/SkPictureRecord.cpp#...
src/core/SkPictureRecord.cpp:53: 
nit: use { } around all "if"s

http://codereview.appspot.com/5646057/diff/7007/src/core/SkPictureRecord.cpp#...
src/core/SkPictureRecord.cpp:85: 
nit: use { } around all "if"s

http://codereview.appspot.com/5646057/diff/7007/src/core/SkPictureRecord.h
File src/core/SkPictureRecord.h (right):

http://codereview.appspot.com/5646057/diff/7007/src/core/SkPictureRecord.h#ne...
src/core/SkPictureRecord.h:97: enum {
kNoSavedLayerIndex = -1

[junov1, 2012-02-09 15:44:12]

On 2012/02/09 15:15:44, reed1 wrote:
> Don't know if we already have unittests for picture recording, but should be
> easy to create a test for this behavior.
> 
> http://codereview.appspot.com/5646057/diff/7007/src/core/SkPictureRecord.cpp
> File src/core/SkPictureRecord.cpp (right):
> 
>
http://codereview.appspot.com/5646057/diff/7007/src/core/SkPictureRecord.cpp#...
> src/core/SkPictureRecord.cpp:53: 
> nit: use { } around all "if"s
> 
>
http://codereview.appspot.com/5646057/diff/7007/src/core/SkPictureRecord.cpp#...
> src/core/SkPictureRecord.cpp:85: 
> nit: use { } around all "if"s
> 
> http://codereview.appspot.com/5646057/diff/7007/src/core/SkPictureRecord.h
> File src/core/SkPictureRecord.h (right):
> 
>
http://codereview.appspot.com/5646057/diff/7007/src/core/SkPictureRecord.h#ne...
> src/core/SkPictureRecord.h:97: enum {
> kNoSavedLayerIndex = -1

Done.

In the tests/ subdirectory the string SkPicture is not found in any file.  We
are still getting a reasonable amount of coverage from gm, which runs every test
case through SkPicture, and now also SkDeferredCanvas.  I still think it would
be a good idea to have some unit test coverage for stuff like the add* methods
in SkPictureRecord and also to validate symmetry between serialization and
deserialization.  I logged a bug for it: 
http://code.google.com/p/skia/issues/detail?id=481

[reed1, 2012-02-10 13:20:48]

my specific ask for a unittest is around (the now virtual) isDrawingToLayer,
since that will not normally get exercised by gm and other tests that just draw.

[junov1, 2012-02-13 15:53:28]

On 2012/02/10 13:20:48, reed1 wrote:
> my specific ask for a unittest is around (the now virtual) isDrawingToLayer,
> since that will not normally get exercised by gm and other tests that just
draw.

The test added in this CL exercises that code path, though it does not call the
method directly.  Is that good enough for now?  I started working on issue 481,
and I will be exercising it explicitly there.

[reed1, 2012-02-13 18:04:55]

sure lgtm

I think the explicit test is pretty trivial.

SkCanvas* canvas = picture.beginRecording(...);
assert(!canvas->isDrawingToLayer());
canvas->save();
canvas->save();
assert(!canvas->isDrawingToLayer());
canvas->saveLayer(...);
canvas->save();
assert(canvas->isDrawingToLayer());
canvas->restore();
assert(canvas->isDrawingToLayer());
canvas->restore();
assert(!canvas->isDrawingToLayer());

[junov1, 2012-02-14 15:23:25]

Closed at r3181