Add helper for maintaining clip/matrix state in non-contiguous picture playback.

Add helper for maintaining clip/matrix state in non-contiguous picture playback.
Committed: https://code.google.com/p/skia/source/detail?r=5483

[rileya, 2012-09-07 12:09:55]

This is the logic for keeping the matrix/clip state correct in R-Tree playback.
Currently I have it separate from the R-Tree (call appendDraw on this for each
draw, and it returns a pointer that I put in the R-Tree), since I'd imagined it
might be useful for some other purpose; that said, it could make the usage in
playback cleaner to make this contain and manage the R-Tree... any thoughts?

Also, the name's not so great, any suggestions for a better one?

[bsalomon, 2012-09-10 17:26:24]

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree.cpp
File src/core/SkPictureStateTree.cpp (right):

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.cpp:98: if (fCurrentNode->fFlags & Node::kSave_Flag)
{ fCanvas.restore(); }
Can one node ever have both of these flags?

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.cpp:107: Node* targetNode = draw->fNode;
I find this easier to understand compared to patchset 2 where target was a
member.

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.cpp:147: // apply the next clip or saveLayer.
Thx for the comments.

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree.h
File src/core/SkPictureStateTree.h (right):

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.h:28: struct Draw;
Why not just declare here?

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.h:37: Draw* appendDraw(uint32_t offset);
Does picture already use uint32_t for offsets? (Just wondering if it should be
size_t)

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.h:70: const static uint32_t kDrawComplete;
style nit "static const". Also, why not just assign here?

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.h:87: SkCanvas& fCanvas;
style nit: in general we don't use non-const refs, rather pointers.

[rileya, 2012-09-10 18:57:40]

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree.cpp
File src/core/SkPictureStateTree.cpp (right):

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.cpp:98: if (fCurrentNode->fFlags & Node::kSave_Flag)
{ fCanvas.restore(); }
On 2012/09/10 17:26:24, bsalomon wrote:
> Can one node ever have both of these flags?

Yes, it's possible for a node to represent a save layer, and then immediately be
followed by a series of save/restores (we want to be able to restore out of a
save without jumping out of the layer prematurely).

If we had:
saveLayer
  save
    transform
    draw
    ...
  restore
  save
    transform
    draw
    ...
  restore
restore

We wouldn't want to drop out of the layer after that first restore.

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree.h
File src/core/SkPictureStateTree.h (right):

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.h:28: struct Draw;
On 2012/09/10 17:26:24, bsalomon wrote:
> Why not just declare here?

Good call, declared Draw here; Iterator is a big declaration (and I'm looking
into inlining its draw() function, so it may get even larger...) though, so does
it make sense to define it below so as not to clutter up the beginning of the
file, or does it really matter?

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.h:37: Draw* appendDraw(uint32_t offset);
On 2012/09/10 17:26:24, bsalomon wrote:
> Does picture already use uint32_t for offsets? (Just wondering if it should be
> size_t)

Yeah, it uses SkReader32/SkWriter32, which do everything in uint32_t.

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.h:70: const static uint32_t kDrawComplete;
On 2012/09/10 17:26:24, bsalomon wrote:
> style nit "static const". Also, why not just assign here?

Fixed and assigned here (force of habit: I'd worked on a research project at one
point where assigning static constants in the header was forbidden).

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.h:87: SkCanvas& fCanvas;
On 2012/09/10 17:26:24, bsalomon wrote:
> style nit: in general we don't use non-const refs, rather pointers.
Fixed.

[bsalomon, 2012-09-11 13:43:43]

LGTM. Not crazy about the name, couldn't come up with anything better.

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree.h
File src/core/SkPictureStateTree.h (right):

https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.h:28: struct Draw;
On 2012/09/10 18:57:40, rileya wrote:
> On 2012/09/10 17:26:24, bsalomon wrote:
> > Why not just declare here?
> 
> Good call, declared Draw here; Iterator is a big declaration (and I'm looking
> into inlining its draw() function, so it may get even larger...) though, so
does
> it make sense to define it below so as not to clutter up the beginning of the
> file, or does it really matter?

I think its ok as is. The draw seemed small (and not totally obvious what it was
just from just the name in the forward decl).

Also, if you're going to inline a large function you can still declare it with
"inline" and then put it at the bottom of the .h file.

[rileya, 2012-09-11 14:58:17]

On 2012/09/11 13:43:43, bsalomon wrote:
> LGTM. Not crazy about the name, couldn't come up with anything better.
> 
> https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree.h
> File src/core/SkPictureStateTree.h (right):
> 
>
https://codereview.appspot.com/6509043/diff/6003/src/core/SkPictureStateTree....
> src/core/SkPictureStateTree.h:28: struct Draw;
> On 2012/09/10 18:57:40, rileya wrote:
> > On 2012/09/10 17:26:24, bsalomon wrote:
> > > Why not just declare here?
> > 
> > Good call, declared Draw here; Iterator is a big declaration (and I'm
looking
> > into inlining its draw() function, so it may get even larger...) though, so
> does
> > it make sense to define it below so as not to clutter up the beginning of
the
> > file, or does it really matter?
> 
> I think its ok as is. The draw seemed small (and not totally obvious what it
was
> just from just the name in the forward decl).
> 
> Also, if you're going to inline a large function you can still declare it with
> "inline" and then put it at the bottom of the .h file.

Landed as r5483.

After talking with Mike, I think it makes sense to just make the transforms into
nodes as well, rather than storing matrices explicitly (since we have to set the
matrices for each clip anyways, I don't think it really gains us much, and it
would also allow this class to be simplified *a lot*). It's functional as-is,
and I want to get the more pressing stuff in first, but I'll clean this up
afterwards.