Refactor SkPictureStateTree::Iterator to avoid use of kClip_SaveFlag.

Refactor SkPictureStateTree::Iterator to avoid use of kClip_SaveFlag.

The current implementation relies on soon-to-be-deprecated
kClip_SaveFlag behavior. Updated to use default save flags
(kMatrixClip_SaveFlag) and stop assuming that the matrix survives
restore() calls.

R=junov@chromium.org,robertphillips@chromium.org,reed@google.com

Committed: http://code.google.com/p/skia/source/detail?r=14319

Committed: http://code.google.com/p/skia/source/detail?r=14421

[Justin Novosad, 2014-04-22 15:01:58]

Test?

[f(malita), 2014-04-22 15:09:04]

On 2014/04/22 15:01:58, junov wrote:
> Test?

What kind of test do you have in mind? This doesn't change behavior so I assume
existing GMs/bench/tests provide plenty of coverage.

(The initial attempt to simply drop kClip_SaveFlag in
https://codereview.chromium.org/241453003 caused 56 GM failures).

[Justin Novosad, 2014-04-22 15:15:21]

On 2014/04/22 15:09:04, Florin Malita wrote:
> On 2014/04/22 15:01:58, junov wrote:
> > Test?
> 
> What kind of test do you have in mind? This doesn't change behavior so I
assume
> existing GMs/bench/tests provide plenty of coverage.
> 
> (The initial attempt to simply drop kClip_SaveFlag in
> https://codereview.chromium.org/241453003 caused 56 GM failures).

Ah, that sounds like decent coverage then. Thanks.

[Justin Novosad, 2014-04-22 15:31:33]

General question: In what situation does fCanvas->restore() not restore the CTM
to the right thing?

https://codereview.chromium.org/246893005/diff/1/src/core/SkPictureStateTree.cpp
File src/core/SkPictureStateTree.cpp (left):

https://codereview.chromium.org/246893005/diff/1/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.cpp:111: while (NULL != fCurrentNode) {
The changes to this loop look mostly like a cleaner re-write. Is there anything
more going on here that I did not catch?

https://codereview.chromium.org/246893005/diff/1/src/core/SkPictureStateTree.cpp
File src/core/SkPictureStateTree.cpp (right):

https://codereview.chromium.org/246893005/diff/1/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.cpp:110: m.postConcat(fPlaybackMatrix);
If I understand correctly, this is because 'matrix' tranforms from local to
recording canvas space, and what we want is local to playback canvas space.
Correct? A little comment WBN.

https://codereview.chromium.org/246893005/diff/1/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.cpp:115: uint32_t
SkPictureStateTree::Iterator::nextDraw() {
+1 for the name change.

[f(malita), 2014-04-22 15:48:38]

https://codereview.chromium.org/246893005/diff/1/src/core/SkPictureStateTree.cpp
File src/core/SkPictureStateTree.cpp (left):

https://codereview.chromium.org/246893005/diff/1/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.cpp:111: while (NULL != fCurrentNode) {
On 2014/04/22 15:31:34, junov wrote:
> The changes to this loop look mostly like a cleaner re-write. Is there
anything
> more going on here that I did not catch?

Yes, the loop change itself is just cleanup.

The only significant change in this block is the deferral of
setMatrix(fPlaybackMatrix) until after all restore()s.

https://codereview.chromium.org/246893005/diff/1/src/core/SkPictureStateTree.cpp
File src/core/SkPictureStateTree.cpp (right):

https://codereview.chromium.org/246893005/diff/1/src/core/SkPictureStateTree....
src/core/SkPictureStateTree.cpp:110: m.postConcat(fPlaybackMatrix);
On 2014/04/22 15:31:34, junov wrote:
> If I understand correctly, this is because 'matrix' tranforms from local to
> recording canvas space, and what we want is local to playback canvas space.
> Correct? A little comment WBN.

Exactly. Added comment.

[Justin Novosad, 2014-04-22 17:03:31]

I would still like to better understand: In what situation does
fCanvas->restore() not restore the CTM
to the right thing?

[f(malita), 2014-04-22 18:02:46]

On 2014/04/22 17:03:31, junov wrote:
> I would still like to better understand: In what situation does
> fCanvas->restore() not restore the CTM
> to the right thing?

In the initial implementation (before r14253), there is no right thing for the
CTM to restore to: save()s are always passed kClip_SaveFlag to explicitly avoid
restoring the CTM => save()/restore() are only being used for managing clip
state, and not matrix state.

The matrix state is manipulated directly using setMatrix() before returning the
next draw. But note that there is an optimization attempt to avoid duplicates:

if (fCurrentMatrix != draw->fMatrix) {
...
}

The problem is this assumes fCurrentMatrix and the canvas CTM stay in sync - and
this is the case when only saving the clip. But when switching to full
clip/matrix saves, the matrix state may change on restore => in certain cases
fCurrentMatrix ends up out of sync and the optimization incorrectly skips
setting up the matrix for the draw.

The fix in this CL is to always invalidate fCurrentMatrix on restore, thus
forcing the next setMatrix() to take effect.

[Justin Novosad, 2014-04-22 18:10:40]

On 2014/04/22 18:02:46, Florin Malita wrote:
> On 2014/04/22 17:03:31, junov wrote:
> > I would still like to better understand: In what situation does
> > fCanvas->restore() not restore the CTM
> > to the right thing?
> 
> In the initial implementation (before r14253), there is no right thing for the
> CTM to restore to: save()s are always passed kClip_SaveFlag to explicitly
avoid
> restoring the CTM => save()/restore() are only being used for managing clip
> state, and not matrix state.
> 
> The matrix state is manipulated directly using setMatrix() before returning
the
> next draw. But note that there is an optimization attempt to avoid duplicates:
> 
> if (fCurrentMatrix != draw->fMatrix) {
> ...
> }
> 
> The problem is this assumes fCurrentMatrix and the canvas CTM stay in sync -
and
> this is the case when only saving the clip. But when switching to full
> clip/matrix saves, the matrix state may change on restore => in certain cases
> fCurrentMatrix ends up out of sync and the optimization incorrectly skips
> setting up the matrix for the draw.
> 
> The fix in this CL is to always invalidate fCurrentMatrix on restore, thus
> forcing the next setMatrix() to take effect.

And we don't care that this may add redundant calls to setMatrix?
Is it too complicated to determine for sure whether the matrix needs to be
invalidated?

[f(malita), 2014-04-22 18:20:10]

On 2014/04/22 18:10:40, junov wrote:
> On 2014/04/22 18:02:46, Florin Malita wrote:
> > The fix in this CL is to always invalidate fCurrentMatrix on restore, thus
> > forcing the next setMatrix() to take effect.
> 
> And we don't care that this may add redundant calls to setMatrix?

I was slightly worried about that, but the perf bots ran on patch set 1 and seem
to be happy.


> Is it too complicated to determine for sure whether the matrix needs to be
> invalidated?

That's a great idea, but it requires a better understanding of
SkPictureStateTree than I currently have. I'll look into it.

[Justin Novosad, 2014-04-22 18:34:34]

On 2014/04/22 18:20:10, Florin Malita wrote:
> On 2014/04/22 18:10:40, junov wrote:
> > On 2014/04/22 18:02:46, Florin Malita wrote:
> > > The fix in this CL is to always invalidate fCurrentMatrix on restore, thus
> > > forcing the next setMatrix() to take effect.
> > 
> > And we don't care that this may add redundant calls to setMatrix?
> 
> I was slightly worried about that, but the perf bots ran on patch set 1 and
seem
> to be happy.
> 
> 
> > Is it too complicated to determine for sure whether the matrix needs to be
> > invalidated?
> 
> That's a great idea, but it requires a better understanding of
> SkPictureStateTree than I currently have. I'll look into it.

Fair enough. lgtm

Mike, Robert, anything to add?

[robertphillips, 2014-04-22 18:44:17]

I have nothing to add since I'm not familiar with this part of the code.

[f(malita), 2014-04-23 02:30:20]

Had a chat with Mike about this, and he's on board with the general approach of
always invalidating the tracked matrix on restore().

[f(malita), 2014-04-23 02:30:25]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-04-23 02:30:39]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/fmalita@chromium.org/246893005/20001

[commit-bot: I haz the power, 2014-04-23 02:46:22]

Change committed as 14319

[bsalomon, 2014-04-25 15:12:53]

A revert of this CL has been created in
https://codereview.chromium.org/250733007/ by bsalomon@google.com.

The reason for reverting is:
https://code.google.com/p/chromium/issues/detail?id=366889.

[f(malita), 2014-04-25 17:53:03]

On 2014/04/25 15:12:53, bsalomon wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/250733007/ by mailto:bsalomon@google.com.
> 
> The reason for reverting is:
> https://code.google.com/p/chromium/issues/detail?id=366889.

That's a good indication that we don't have proper coverage for
SkPictureStateTree :(

For the next iteration I'm planning to leave the old impl in place and run it in
parallel in debug mode for validation.

[f(malita), 2014-04-25 19:46:34]

Found the bug:


-        fCurrentNode = fCurrentNode->fParent;
-        while (NULL != fCurrentNode) {
-            if (fCurrentNode->fFlags & Node::kSave_Flag) {
-                fCanvas->restore();
-            }
-            if (fCurrentNode->fFlags & Node::kSaveLayer_Flag) {
+
+        for (fCurrentNode = fCurrentNode->fParent; fCurrentNode;
+             fCurrentNode = fCurrentNode->fParent) {
+            if (fCurrentNode->fFlags & (Node::kSave_Flag |
Node::kSaveLayer_Flag)) {
                 fCanvas->restore();
             }
-            fCurrentNode = fCurrentNode->fParent;
         }


Those conditionals are independent and can both fire on the same iteration =>
they can't be combined, doh :(

Now on to write a test...

[f(malita), 2014-04-25 21:52:36]

Re-uploaded with fix + test. PTAL.

[Justin Novosad, 2014-04-25 22:43:41]

Looks fine to me, except that I find some comments should be added to explain a
few things that are non-trivial about this change.

https://codereview.chromium.org/246893005/diff/40001/src/core/SkPictureStateT...
File src/core/SkPictureStateTree.cpp (right):

https://codereview.chromium.org/246893005/diff/40001/src/core/SkPictureStateT...
src/core/SkPictureStateTree.cpp:129: if (fCurrentNode->fFlags &
Node::kSaveLayer_Flag) {
At first glance this code looks like it was not written as well as it could have
been. One might be tempted to put it back how you had it before upon seeing
this.  To avoid such distractions, I think you should add a comment right here
that states that restore() needs to be called twice when both flags are set.

https://codereview.chromium.org/246893005/diff/40001/tests/PictureStateTreeTe...
File tests/PictureStateTreeTest.cpp (right):

https://codereview.chromium.org/246893005/diff/40001/tests/PictureStateTreeTe...
tests/PictureStateTreeTest.cpp:23:
p3.setXfermodeMode(SkXfermode::kColorBurn_Mode);
This is a rarely use xfer mode. Any reason the test uses this one in particular?
 Perhaps you should add a comment to explain. More generally, it would be nice
to have more info regarding what edge cases this test aims to cover.

https://codereview.chromium.org/246893005/diff/40001/tests/PictureStateTreeTe...
tests/PictureStateTreeTest.cpp:67: REPORTER_ASSERT(reporter, *bm1.getAddr32(x,
y) == *bm2.getAddr32(x, y));
why not just a memcmp on the whole buffer.  I am just thinking that if this test
ever hits a snag, it will probably generate a massive text dump.

[f(malita), 2014-04-28 13:10:25]

https://codereview.chromium.org/246893005/diff/40001/src/core/SkPictureStateT...
File src/core/SkPictureStateTree.cpp (right):

https://codereview.chromium.org/246893005/diff/40001/src/core/SkPictureStateT...
src/core/SkPictureStateTree.cpp:129: if (fCurrentNode->fFlags &
Node::kSaveLayer_Flag) {
On 2014/04/25 22:43:41, junov wrote:
> At first glance this code looks like it was not written as well as it could
have
> been. One might be tempted to put it back how you had it before upon seeing
> this.  To avoid such distractions, I think you should add a comment right here
> that states that restore() needs to be called twice when both flags are set.

Done.

https://codereview.chromium.org/246893005/diff/40001/tests/PictureStateTreeTe...
File tests/PictureStateTreeTest.cpp (right):

https://codereview.chromium.org/246893005/diff/40001/tests/PictureStateTreeTe...
tests/PictureStateTreeTest.cpp:23:
p3.setXfermodeMode(SkXfermode::kColorBurn_Mode);
On 2014/04/25 22:43:41, junov wrote:
> This is a rarely use xfer mode. Any reason the test uses this one in
particular?
>  Perhaps you should add a comment to explain.

Just wanted an xfer mode where the result is a function of both inputs. But it's
probably much simpler to just use a semi-transparent paint instead - changed.

> More generally, it would be nice
> to have more info regarding what edge cases this test aims to cover.

Added some comments.

https://codereview.chromium.org/246893005/diff/40001/tests/PictureStateTreeTe...
tests/PictureStateTreeTest.cpp:67: REPORTER_ASSERT(reporter, *bm1.getAddr32(x,
y) == *bm2.getAddr32(x, y));
On 2014/04/25 22:43:41, junov wrote:
> why not just a memcmp on the whole buffer.  I am just thinking that if this
test
> ever hits a snag, it will probably generate a massive text dump.

Good point, done.

[Justin Novosad, 2014-04-28 15:25:26]

lgtm

[f(malita), 2014-04-28 20:11:17]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-04-28 20:11:30]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/fmalita@chromium.org/246893005/60001

[commit-bot: I haz the power, 2014-04-28 20:17:52]

Change committed as 14421