Refactoring SkDeferredCanvas to use SkGPipe.

Refactoring SkDeferredCanvas to use SkGPipe.
Keeping the refactor hidden behind a config macro for now.

TEST=covered by existing skia gm tests
BUG=https://code.google.com/p/chromium/issues/detail?id=133432
Committed: https://code.google.com/p/skia/source/detail?r=4656

[junov1, 2012-07-17 18:43:21]

PTAL

[junov1, 2012-07-17 18:49:39]

Will merge with changes in r4639 and re-upload soon.

[junov1, 2012-07-17 18:56:30]

Patch set 3 has merge conflicts resolved, and still passes gm tests.

[reed1, 2012-07-17 19:14:29]

Questions around flush().

Is it sufficient for us to add

/**
 * Calling this will force a notify() to the controller if there was any
deferred writes to the stream,
 * and will force the writer to request a new block from the controller on the
 * next draw call to its recording canvas.
 *
 * After calling this, the writer will retain no references to the current
buffer, but the recording
 * canvas will be intact, with its save/restore stack intact.
 */
void SkGPipeWriter::flushRecording();

My (naive) reading of the CL is that canvas::flush() was just called to
effectively trigger this sort of behavior.

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

https://codereview.appspot.com/6405054/diff/3001/include/pipe/SkGPipe.h#newco...
include/pipe/SkGPipe.h:73: *  This is called to request that all recorded draw
commands be
is "written out to the block(s)" also a correct description? If so, I like it
better, since it doesn't really require any cooperation with the "readers"
(whatever they are)

https://codereview.appspot.com/6405054/diff/3001/include/pipe/SkGPipe.h#newco...
include/pipe/SkGPipe.h:75: *  reader will execute immediately. This method only
needs to be
What does the sentence "This method only needs ..." really mean, from the
perspective of someone implementing a controller?

https://codereview.appspot.com/6405054/diff/3001/include/pipe/SkGPipe.h#newco...
include/pipe/SkGPipe.h:81: */
Why the assert(0)? If you want to force all subclasses to implement this, isn't
= 0 a better way?

[Leon, 2012-07-17 19:25:56]

Sorry for the duplicate comments. They keep disappearing while I look at the
files.

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

https://codereview.appspot.com/6405054/diff/3001/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:984: fBlockSize = fBytesNotified;
= 0?

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

https://codereview.appspot.com/6405054/diff/3001/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:536:
SkGPipeWriter::kSharedAddressSpace_SkGPipeFlag);
This flag is intended to be supplemental to CrossProcess_Flag. Without it, this
flag is meaningless.

https://codereview.appspot.com/6405054/diff/3001/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:622: // FIXME: Make SkGPipe flatten
software-backed non-immutable bitmaps
I suppose you only want to flatten sometimes? If you pass the CrossProcess_Flag,
it will flatten them all the time, though it has other effects. Either way, the
code is already there; we may just need to add a flag.

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

https://codereview.appspot.com/6405054/diff/2002/include/pipe/SkGPipe.h#newco...
include/pipe/SkGPipe.h:74: *  immediately comitted to the reader. It does not
guarantee that the
committed

https://codereview.appspot.com/6405054/diff/2002/include/pipe/SkGPipe.h#newco...
include/pipe/SkGPipe.h:77: *  SkGPipeCanvas. The next draw command after a call
to
I do not think we should reference a class from a cpp file in a public header.

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

https://codereview.appspot.com/6405054/diff/2002/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:1086: fBlockSize = fBytesNotified;
= 0?

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

https://codereview.appspot.com/6405054/diff/2002/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:536:
SkGPipeWriter::kSharedAddressSpace_SkGPipeFlag);
This flag does not do anything by itself.

[junov1, 2012-07-17 19:44:53]

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

https://codereview.appspot.com/6405054/diff/3001/include/pipe/SkGPipe.h#newco...
include/pipe/SkGPipe.h:73: *  This is called to request that all recorded draw
commands be
On 2012/07/17 19:14:29, reed1 wrote:
> is "written out to the block(s)" also a correct description? If so, I like it
> better, since it doesn't really require any cooperation with the "readers"
> (whatever they are)
No, I really meant what I wrote, this is for cases where we need the controller
to push everything out to the reader immediately. For example, in the case of a
canvas to canvas copy, we need to flush the source canvas's draw operations all
the way down the pipe to get them rendered now.

https://codereview.appspot.com/6405054/diff/3001/include/pipe/SkGPipe.h#newco...
include/pipe/SkGPipe.h:75: *  reader will execute immediately. This method only
needs to be
Yes, that is what I meant. I will make it clearer.

https://codereview.appspot.com/6405054/diff/3001/include/pipe/SkGPipe.h#newco...
include/pipe/SkGPipe.h:81: */
On 2012/07/17 19:14:29, reed1 wrote:
> Why the assert(0)? If you want to force all subclasses to implement this,
isn't
> = 0 a better way?
Actually, I do not want to force all subclasses to implement this. Only the
subclasses that serve use cases that require  flushing need to implement it.  I
other words, implement it only if it is used.

[junov1, 2012-07-17 19:57:37]

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

https://codereview.appspot.com/6405054/diff/3001/src/pipe/SkGPipeWrite.cpp#ne...
src/pipe/SkGPipeWrite.cpp:984: fBlockSize = fBytesNotified;
On 2012/07/17 19:25:56, scroggo-work wrote:
> = 0?
Good idea, +readable

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

https://codereview.appspot.com/6405054/diff/3001/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:622: // FIXME: Make SkGPipe flatten
software-backed non-immutable bitmaps
A flag that I could be turned on/off at will during recording would do the
trick.  Alternately, we could duplicate the logic behind the
kFlattenMutableNonTexturePixelRefs_RecordingFlag of SkPicture.

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

https://codereview.appspot.com/6405054/diff/2002/include/pipe/SkGPipe.h#newco...
include/pipe/SkGPipe.h:77: *  SkGPipeCanvas. The next draw command after a call
to
On 2012/07/17 19:25:56, scroggo-work wrote:
> I do not think we should reference a class from a cpp file in a public header.
Oh yeah, I guess I could call it "the canvas created by SkGPipeWriter"

[Leon, 2012-07-17 20:08:28]

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

https://codereview.appspot.com/6405054/diff/3001/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:622: // FIXME: Make SkGPipe flatten
software-backed non-immutable bitmaps
On 2012/07/17 19:57:37, junov1 wrote:
> A flag that I could be turned on/off at will during recording would do the
> trick.  Alternately, we could duplicate the logic behind the
> kFlattenMutableNonTexturePixelRefs_RecordingFlag of SkPicture.

Does it depend on other conditions besides isImmutable? Or can there be a flag
that says "if (bitmap.isImmutable()) flatten"? Is that what SkPicture's flag
does?

[junov1, 2012-07-17 20:18:06]

Patch set 4 addresses review comments

[junov1, 2012-07-17 20:24:28]

On 2012/07/17 20:08:28, scroggo-work wrote:
>
https://codereview.appspot.com/6405054/diff/3001/src/utils/SkDeferredCanvas.cpp
> File src/utils/SkDeferredCanvas.cpp (right):
> 
>
https://codereview.appspot.com/6405054/diff/3001/src/utils/SkDeferredCanvas.c...
> src/utils/SkDeferredCanvas.cpp:622: // FIXME: Make SkGPipe flatten
> software-backed non-immutable bitmaps
> On 2012/07/17 19:57:37, junov1 wrote:
> > A flag that I could be turned on/off at will during recording would do the
> > trick.  Alternately, we could duplicate the logic behind the
> > kFlattenMutableNonTexturePixelRefs_RecordingFlag of SkPicture.
> 
> Does it depend on other conditions besides isImmutable? Or can there be a flag
> that says "if (bitmap.isImmutable()) flatten"? Is that what SkPicture's flag
> does?

It also verifies that bitmap.getTexture() returns NULL. Flattening the pixels of
a bitmap stored in GPU memory would require a readback, which we prefer to avoid
in whenever possible.

[Leon, 2012-07-17 21:38:16]

https://codereview.appspot.com/6405054/diff/13001/include/utils/SkDeferredCan...
File include/utils/SkDeferredCanvas.h (right):

https://codereview.appspot.com/6405054/diff/13001/include/utils/SkDeferredCan...
include/utils/SkDeferredCanvas.h:183: size_t fBlockSize;
I think you can do without this variable, since it is only used in requestBlock.

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

https://codereview.appspot.com/6405054/diff/13001/src/utils/SkDeferredCanvas....
src/utils/SkDeferredCanvas.cpp:526: fPipeWriter.endRecording();
Should the PipeController be reset, as well?

[Leon, 2012-07-18 13:59:53]

https://codereview.appspot.com/6405054/diff/13001/include/utils/SkDeferredCan...
File include/utils/SkDeferredCanvas.h (right):

https://codereview.appspot.com/6405054/diff/13001/include/utils/SkDeferredCan...
include/utils/SkDeferredCanvas.h:188: SkGPipeReader::Status fStatus;
Unused.

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

https://codereview.appspot.com/6405054/diff/13001/src/utils/SkDeferredCanvas....
src/utils/SkDeferredCanvas.cpp:464: fBlockList.push(PipeBlock(fBlock,
fBytesWritten));
Minor nit: can you not push(previousBloc)? (or eliminate previousBloc, which is
otherwise unused)

https://codereview.appspot.com/6405054/diff/13001/src/utils/SkDeferredCanvas....
src/utils/SkDeferredCanvas.cpp:466: if (minRequest > kMinBlockSize) {
SkMax32 is clearer.

[junov1, 2012-07-18 15:36:46]

Done.  New patch uploaded

[Leon, 2012-07-18 15:52:34]

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

https://codereview.appspot.com/6405054/diff/24001/src/utils/SkDeferredCanvas....
src/utils/SkDeferredCanvas.cpp:611: fRecordingCanvas =
fPicture.beginRecording(fImmediateDevice->width(),
Why does this not call

this->beginRecording()?

Should we not call fPipeWriter.startRecording() when using GPipe here?

[junov1, 2012-07-18 17:15:33]

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

https://codereview.appspot.com/6405054/diff/24001/src/utils/SkDeferredCanvas....
src/utils/SkDeferredCanvas.cpp:611: fRecordingCanvas =
fPicture.beginRecording(fImmediateDevice->width(),
On 2012/07/18 15:52:34, scroggo-work wrote:
> Why does this not call
> 
> this->beginRecording()?

Sure, that works.
> 
> Should we not call fPipeWriter.startRecording() when using GPipe here?

No. The call to fPipeWriter.flushRecording(true) does not end the recording, so
the recording does not need to be restarted.  Being able to do it this way is
the motivation for switching to SkGPipe.  By not restarting the recording, the
recording canvas gets to retain all of its state, including its clip/matrix
stack, which is key to solving bugs we have when using SkPicture.

[junov1, 2012-07-18 17:27:18]

patch for beginRecording thing.

[Leon, 2012-07-18 17:43:05]

lgtm. Mike said he was happy with it and was deferring to me for the approval,
so you're good to check it in.