Proposed plumbing to propagate save & restore

Given my current understanding of the Skia architecture here is a rough outline of how I would go about propagating SkCanvas' save & restore down to the ClipMaskManager. Is this even anywhere close to right?

[bsalomon, 2012-05-11 14:19:56]

On 2012/05/11 13:33:02, robertphillips wrote:

At the GrContext level and below we are treating this as a push/pop of the clip
but nothing else. Can the names reflect that (e.g. saveClip/restoreClip or
pushClip/popClip [We already use push/pop nomenclature for geometry, draw state,
etc]).

[robertphillips, 2012-05-11 15:27:59]

Done.

[bsalomon, 2012-05-11 15:31:35]

Sorry I meant to send this comment with my message from patchset 1 but I hit
reply at the bottom instead of publsh+mail at the left. :(

Mike may agree/disagree with this suggestion...

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

http://codereview.appspot.com/6203067/diff/1/src/core/SkCanvas.cpp#newcode692
src/core/SkCanvas.cpp:692: for (DeviceCM* curLayer = fMCRec->fTopLayer;
Why does every device, rather than just the top device get notified?

It might be a bit clearer if we called these functions didSave() and
willRestore() so it's obvious to the SkDevice subclass implementer when the
notification arrives.

[reed1, 2012-05-11 21:28:20]

http://codereview.appspot.com/6203067/diff/4001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

http://codereview.appspot.com/6203067/diff/4001/src/core/SkCanvas.cpp#newcode691
src/core/SkCanvas.cpp:691: 
I will look further, but are we guaranteed that each fDevice pointer in this
list is (a) never-null, and (b) different from all the other devices?

Perhaps related : do we need to communicate with more than the top-layer's
device when we do a save/restore?

[robertphillips, 2012-05-14 14:01:45]

I had overlooked the NULL device check in SkDrawIter's next method - fixed now.

http://codereview.appspot.com/6203067/diff/4001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

http://codereview.appspot.com/6203067/diff/4001/src/core/SkCanvas.cpp#newcode691
src/core/SkCanvas.cpp:691: 
Unless I've misinterpreted the code, it looks like in SkCanvas::drawPath,
drawRect and others that we loop over all the devices (potentially skipping
those w/ empty clips) drawing into them all. From this I assumed we would want
similar behavior of the save/restore calls.

[robertphillips, 2012-05-16 18:55:44]

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

http://codereview.appspot.com/6203067/diff/1/src/core/SkCanvas.cpp#newcode692
src/core/SkCanvas.cpp:692: for (DeviceCM* curLayer = fMCRec->fTopLayer;
Done. Renamed postSave & preRestore.

http://codereview.appspot.com/6203067/diff/8002/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

http://codereview.appspot.com/6203067/diff/8002/include/gpu/GrContext.h#newco...
include/gpu/GrContext.h:18: #include "../../src/gpu/GrGpu.h"
Moved implementation of pushClip & popClip to avoid adding this include

[reed1, 2012-05-16 19:10:13]

http://codereview.appspot.com/6203067/diff/7002/include/core/SkDevice.h
File include/core/SkDevice.h (left):

http://codereview.appspot.com/6203067/diff/7002/include/core/SkDevice.h#oldco...
include/core/SkDevice.h:345: 
Lets have some dox: when is it called, what is legal/expect for the subclass to
do, etc.

[robertphillips, 2012-05-16 19:26:14]

http://codereview.appspot.com/6203067/diff/7002/include/core/SkDevice.h
File include/core/SkDevice.h (left):

http://codereview.appspot.com/6203067/diff/7002/include/core/SkDevice.h#oldco...
include/core/SkDevice.h:345: 
On 2012/05/16 19:10:14, reed1 wrote:
> Lets have some dox: when is it called, what is legal/expect for the subclass
to
> do, etc.

Done.

[reed1, 2012-05-16 19:28:26]

non-gpu part: lgtm

[reed1, 2012-05-16 19:30:38]

http://codereview.appspot.com/6203067/diff/11/include/core/SkDevice.h
File include/core/SkDevice.h (right):

http://codereview.appspot.com/6203067/diff/11/include/core/SkDevice.h#newcode345
include/core/SkDevice.h:345: 
Can these be protected?

[robertphillips, 2012-05-16 19:40:14]

http://codereview.appspot.com/6203067/diff/11/include/core/SkDevice.h
File include/core/SkDevice.h (right):

http://codereview.appspot.com/6203067/diff/11/include/core/SkDevice.h#newcode345
include/core/SkDevice.h:345: 
Done - matched handling of onCreateCompatibleDevice.

[bsalomon, 2012-05-16 23:56:27]

Mostly LGTM. One Q: Does GrInOrderDrawBuffer not need a push/pop implementation?

http://codereview.appspot.com/6203067/diff/18002/include/core/SkDevice.h
File include/core/SkDevice.h (right):

http://codereview.appspot.com/6203067/diff/18002/include/core/SkDevice.h#newc...
include/core/SkDevice.h:357: * classes to initialize their state-dependant
caches.
sp, "dependent" unless "state-dependant" refers to recipients of state transfer
payments :)

http://codereview.appspot.com/6203067/diff/18002/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

http://codereview.appspot.com/6203067/diff/18002/include/gpu/GrContext.h#newc...
include/gpu/GrContext.h:683: virtual void pushClip();
brief comment of state of clip after push? (reset or preserved)

[robertphillips, 2012-05-22 15:02:57]

I don't think the GrInOrderDrawBuffer fits into this system. It has its own
logic to reuse clip stacks and (despite the pushClip function name) it flattens
the state & clip hierarchy into a list. So unless we refactored the
GrInOrderDrawBuffer to actually preserve the state & clip hierarchy it probably
wouldn't make sense to implement GrDrawTarget's pushClip and popClip methods for
it.

http://codereview.appspot.com/6203067/diff/18002/include/core/SkDevice.h
File include/core/SkDevice.h (right):

http://codereview.appspot.com/6203067/diff/18002/include/core/SkDevice.h#newc...
include/core/SkDevice.h:357: * classes to initialize their state-dependant
caches.
On 2012/05/16 23:56:28, bsalomon wrote:
> sp, "dependent" unless "state-dependant" refers to recipients of state
transfer
> payments :)

Done.

http://codereview.appspot.com/6203067/diff/18002/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

http://codereview.appspot.com/6203067/diff/18002/include/gpu/GrContext.h#newc...
include/gpu/GrContext.h:683: virtual void pushClip();
On 2012/05/16 23:56:28, bsalomon wrote:
> brief comment of state of clip after push? (reset or preserved)

Done.

[bsalomon, 2012-05-22 15:37:08]

If I'm using a GrIODB and I say:

target->setClip(clipA);
target->pushClip();
target->setClip(clipB);
target->popClip();
target->drawRect(rect);

won't my rect incorrectly get clipped against clipB instead of clipA?

http://codereview.appspot.com/6203067/diff/21001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

http://codereview.appspot.com/6203067/diff/21001/include/gpu/GrContext.h#newc...
include/gpu/GrContext.h:685: * After a push the clip cache (for the new level)
will empty. The
I think this statement could be a bit confusing to a user of GrContext (who
isn't necessarily aware of the clip cache or how clips are maintained). It could
be misconstrued as the clip itself being empty. I think we're documenting how
this is used not how it is implemented. Can we say something like:

Saves a copy of the clip on a stack that can be restored with popClip. pushClip
itself doesn't modify the current clip. It is more efficient to restore a
previously used clip using popClip() than explicitly setting the clip via
setClip().

[robertphillips, 2012-05-22 15:56:59]

What about renaming the pushClip/popClip methods to clipWasPushed/clipWasPopped?
The actual pushing & popping is being done by the canvas and these calls are
only notifying the lower level classes of a state change.

[robertphillips, 2012-05-22 20:25:09]

Renamed functions to clarify their purpose. Generalized comments in GrContext to
better describe their intended function.

[bsalomon, 2012-05-22 20:49:07]

On 2012/05/22 20:25:09, robertphillips wrote:
> Renamed functions to clarify their purpose. Generalized comments in GrContext
to
> better describe their intended function.

LGTM.

[robertphillips, 2012-05-23 11:44:44]

committed as r4034