Fixing a deferred canvas optimization that purges pending draws when the canvas is cleared

Fixing a deferred canvas optimization that purges pending draws when the canvas is cleared

It appears that the recording canvas returns a save count of 1 when the save stack is empty.
In order to pass Canvas unit tests when a clear occurs, changes to SkGPipe were necessary
to allow SkDeferredCanvas to set the device bounds on the SkGPipeCanvas.  A positive
side effect of this change is that graphics primitives that fall outside of the device
bounds will now always be culled at the recording stage (as opposed playback).

BUG=http://code.google.com/p/skia/issues/detail?id=782
TEST=deferred_canvas_record bench test
Committed: https://code.google.com/p/skia/source/detail?r=5117

[Leon, 2012-08-15 18:39:49]

On 2012/08/15 18:09:36, junov1 wrote:

lgtm, with some thoughts:

SkGPipe does not do anything to muck with the save count, so I do not see how
this would have changed when you switched from SkPicture. Looking at SkCanvas,
it appears that the save count of any SkCanvas immediately after init would be
1.

Mike wondered if it might be better to call getSaveCount earlier on, and compare
to the previous value, which is generally how save counts are used. If not, this
code is fine.

[junov1, 2012-08-15 18:52:49]

Hmmm... this code has been broken for a long time then...

Since SkDeferredCanvas fully controls the recording canvas, we're sure
there won't be any bracketting save/restore calls, so I don't think it is
necessary to capture the initial save count.

This CL makes a unit test fail, so I won't submit as is. new patch is on
the way...

On Wed, Aug 15, 2012 at 2:39 PM, <scroggo@google.com> wrote:

> On 2012/08/15 18:09:36, junov1 wrote:
>
> lgtm, with some thoughts:
>
> SkGPipe does not do anything to muck with the save count, so I do not
> see how this would have changed when you switched from SkPicture.
> Looking at SkCanvas, it appears that the save count of any SkCanvas
> immediately after init would be 1.
>
> Mike wondered if it might be better to call getSaveCount earlier on, and
> compare to the previous value, which is generally how save counts are
> used. If not, this code is fine.
>
>
https://codereview.appspot.**com/6454157/<https://codereview.appspot.com/6454...
>

[junov1, 2012-08-15 19:19:03]

New patch.

In order to pass Canvas unit tests when a clear occurs, changes to SkGPipe were
necessary to allow SkDeferredCanvas to set the device bounds on the
SkGPipeCanvas.  A positive side effect of this change is that graphics
primitives that fall outside of the device bounds will now always be culled at
the recording stage (as opposed playback).

[Leon, 2012-08-15 19:29:12]

https://codereview.appspot.com/6454157/diff/4001/include/pipe/SkGPipe.h
File include/pipe/SkGPipe.h (right):

https://codereview.appspot.com/6454157/diff/4001/include/pipe/SkGPipe.h#newco...
include/pipe/SkGPipe.h:93: kDefaultRecordingCanvasSize = 32768,
Any particular reason you changed this from 32767 to 32768?

Also, does this need to be public?

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

https://codereview.appspot.com/6454157/diff/4001/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:419: // should the caller give us the bounds?
This comment should be removed.

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

https://codereview.appspot.com/6454157/diff/4001/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:398: // with an empty stack.
Maybe there should be a comment explaining that a saveCount of 1 means an empty
stack?

[junov1, 2012-08-15 19:40:21]

Done.  New Patch.

[Leon, 2012-08-15 19:47:40]

On 2012/08/15 19:40:21, junov1 wrote:
> Done.  New Patch.

lgtm

[reed1, 2012-08-16 15:57:48]

http://codereview.appspot.com/6454157/diff/2002/include/pipe/SkGPipe.h
File include/pipe/SkGPipe.h (right):

http://codereview.appspot.com/6454157/diff/2002/include/pipe/SkGPipe.h#newcod...
include/pipe/SkGPipe.h:106: SkCanvas* startRecording(SkGPipeController*,
uint32_t flags = 0,
skia tabs these to align with the first argument