Adding a silent playback option to SkGPipeRead

Adding a silent playback option to SkGPipeRead
Testing state consistency after silent playback in CanvasTest indirectly
through SkDeferredCanvas.

BUG=http://code.google.com/p/chromium/issues/detail?id=146178
TEST=CanvasTest unit test, and bench with --mode deferredSilent
Committed: https://code.google.com/p/skia/source/detail?r=5619

[junov1, 2012-09-20 15:04:57]

PTAL

[reed1, 2012-09-20 15:14:53]

is there an interesting bench here? just asking.

[Leon, 2012-09-20 15:28:14]

lgtm

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

https://codereview.appspot.com/6542047/diff/1/include/utils/SkDeferredCanvas....
include/utils/SkDeferredCanvas.h:204: void silentFlush(); // for unit testing
Should code for testing be #ifdef'ed out when not testing? It might be nice to
at least group this stuff together.

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

https://codereview.appspot.com/6542047/diff/1/src/pipe/SkGPipeRead.cpp#newcod...
src/pipe/SkGPipeRead.cpp:194: bool                      fSilent;
I know we call setSilent() before calling shouldDraw(), but should initialize it
just in case something changes?

[Leon, 2012-09-20 16:07:21]

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

https://codereview.appspot.com/6542047/diff/1/src/utils/SkDeferredCanvas.cpp#...
src/utils/SkDeferredCanvas.cpp:389: void DeferredDevice::skipPendingCommands() {
Do we still need this function?

[junov1, 2012-09-20 16:13:52]

https://codereview.appspot.com/6542047/diff/1/src/utils/SkDeferredCanvas.cpp#...
> src/utils/SkDeferredCanvas.cpp:389: void DeferredDevice::skipPendingCommands()
{
> Do we still need this function?

I plan to rewrite it differently in a follow-up CL.

[junov1, 2012-09-20 16:14:53]

patch

[Leon, 2012-09-20 17:26:53]

On 2012/09/20 16:14:53, junov1 wrote:
> patch

lgtm

[reed1, 2012-09-20 17:48:19]

I know I had asked for a bench, but I had not imagined that we would run *all*
benches also in this silent mode. Do you think that is necessary/helpful, or can
we just have a bench written just to test that aspect. Not sure I have a clear
answer in my mind.

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

https://codereview.appspot.com/6542047/diff/7001/include/pipe/SkGPipe.h#newco...
include/pipe/SkGPipe.h:39: // length must be a multiple of 4
Do we really need/want 3 default arguments?
Can we add some documentation for these (only the first 2 are mentioned above)

[junov1, 2012-09-20 17:58:53]

On 2012/09/20 17:48:19, reed1 wrote:
> I know I had asked for a bench, but I had not imagined that we would run *all*
> benches also in this silent mode. Do you think that is necessary/helpful, or
can
> we just have a bench written just to test that aspect. Not sure I have a clear
> answer in my mind.

Since the silent mode may have different performance implications depending on
the types of commands that are processed, I think it is relevant to run all
benches in this mode.  However, I wonder whether this mode should run on the
bots (test value vs. added cycle time).

[reed1, 2012-09-20 18:02:00]

Good point on both counts. Perhaps we default it to off for now, and/or monitor
total bench-bot time to see.

[junov1, 2012-09-20 18:07:54]

On 2012/09/20 18:02:00, reed1 wrote:
> Good point on both counts. Perhaps we default it to off for now, and/or
monitor
> total bench-bot time to see.

Btw, in my next patch, which will make use of this api for the skip on clear
optimization, I will add a bench that triggers the optimization. That will give
us a bench test that tracks the performance of silent playback on all configs
when running bench with --mode deferred

[junov1, 2012-09-20 18:38:11]

On 2012/09/20 17:48:19, reed1 wrote:
https://codereview.appspot.com/6542047/diff/7001/include/pipe/SkGPipe.h
> File include/pipe/SkGPipe.h (right):
> 
>
https://codereview.appspot.com/6542047/diff/7001/include/pipe/SkGPipe.h#newco...
> include/pipe/SkGPipe.h:39: // length must be a multiple of 4
> Do we really need/want 3 default arguments?
> Can we add some documentation for these (only the first 2 are mentioned above)

Done. Latest patch changes two bools into a bitmask.

[junov1, 2012-09-20 18:38:42]

Wait a minute, forgot to add doc...

[junov1, 2012-09-20 19:06:49]

On 2012/09/20 18:38:42, junov1 wrote:
> Wait a minute, forgot to add doc...

Done.

Figured out that read atom reads a group of ops that correspond to a single
canvas command.

[Leon, 2012-09-20 19:12:12]

On 2012/09/20 19:06:49, junov1 wrote:
> On 2012/09/20 18:38:42, junov1 wrote:
> > Wait a minute, forgot to add doc...
> 
> Done.
> 
> Figured out that read atom reads a group of ops that correspond to a single
> canvas command.

lgtm