Refactor SkPictureStateTree::Iterator to avoid use of kClip_SaveFlag.

src/core/SkPictureStateTree.cpp

 
 /*
  * Copyright 2012 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkPictureStateTree.h"
 #include "SkCanvas.h"
@@ skipping 81 matching lines @@
     : fDraws(&draws)
     , fCanvas(canvas)
     , fCurrentNode(root)
     , fPlaybackMatrix(canvas->getTotalMatrix())
     , fCurrentMatrix(NULL)
     , fPlaybackIndex(0)
     , fSave(false)
     , fValid(true) {
 }
 
-uint32_t SkPictureStateTree::Iterator::draw() {
+void SkPictureStateTree::Iterator::setCurrentMatrix(const SkMatrix* matrix) {
+    SkASSERT(NULL != matrix);
+
+    if (matrix == fCurrentMatrix) {
+        return;
+    }
+
+    // The matrix is in recording space, but we also inherit
+    // a playback matrix from out target canvas.
+    SkMatrix m = *matrix;
+    m.postConcat(fPlaybackMatrix);
+    fCanvas->setMatrix(m);
+    fCurrentMatrix = matrix;
+}
+
+uint32_t SkPictureStateTree::Iterator::nextDraw() {
     SkASSERT(this->isValid());
     if (fPlaybackIndex >= fDraws->count()) {
-        // restore back to where we started
-        fCanvas->setMatrix(fPlaybackMatrix);
         if (fCurrentNode->fFlags & Node::kSaveLayer_Flag) {
             fCanvas->restore();
         }
-        fCurrentNode = fCurrentNode->fParent;
-        while (NULL != fCurrentNode) {
+
+        for (fCurrentNode = fCurrentNode->fParent; fCurrentNode;
+             fCurrentNode = fCurrentNode->fParent) {
             if (fCurrentNode->fFlags & Node::kSave_Flag) {
                 fCanvas->restore();
             }
             if (fCurrentNode->fFlags & Node::kSaveLayer_Flag) {

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

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.

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

Done.

                 fCanvas->restore();
             }
-            fCurrentNode = fCurrentNode->fParent;
         }
+
+        fCanvas->setMatrix(fPlaybackMatrix);
+        fCurrentMatrix = NULL;
         return kDrawComplete;
     }
 
     Draw* draw = static_cast<Draw*>((*fDraws)[fPlaybackIndex]);
     Node* targetNode = draw->fNode;
 
     if (fSave) {
-        // FIXME: the save below depends on soon-to-be-deprecated
-        // SaveFlags behavior: it relies on matrix changes persisting
-        // after restore.
-        fCanvas->save(SkCanvas::kClip_SaveFlag);
+        fCanvas->save();
         fSave = false;
     }
 
     if (fCurrentNode != targetNode) {
         // If we're not at the target and we don't have a list of nodes to get there, we need to
         // figure out the path from our current node, to the target
         if (fNodes.count() == 0) {
             // Trace back up to a common ancestor, restoring to get our current state to match that
             // of the ancestor, and saving a list of nodes whose state we need to apply to get to
             // the target (we can restore up to the ancestor immediately, but we'll need to return
             // an offset for each node on the way down to the target, to apply the desired clips and
             // saveLayers, so it may take several draw() calls before the next draw actually occurs)
             Node* tmp = fCurrentNode;
             Node* ancestor = targetNode;
             while (tmp != ancestor) {
                 uint16_t currentLevel = tmp->fLevel;
                 uint16_t targetLevel = ancestor->fLevel;
                 if (currentLevel >= targetLevel) {
                     if (tmp != fCurrentNode && tmp->fFlags & Node::kSave_Flag) {
                         fCanvas->restore();
+                        // restore() may change the matrix, so we need to reapply.
+                        fCurrentMatrix = NULL;
                     }
                     if (tmp->fFlags & Node::kSaveLayer_Flag) {
                         fCanvas->restore();
+                        // restore() may change the matrix, so we need to reapply.
+                        fCurrentMatrix = NULL;
                     }
                     tmp = tmp->fParent;
                 }
                 if (currentLevel <= targetLevel) {
                     fNodes.push(ancestor);
                     ancestor = ancestor->fParent;
                 }
             }
 
             if (ancestor->fFlags & Node::kSave_Flag) {
                 if (fCurrentNode != ancestor) {
                     fCanvas->restore();
+                    // restore() may change the matrix, so we need to reapply.
+                    fCurrentMatrix = NULL;
                 }
                 if (targetNode != ancestor) {
-                    // FIXME: the save below depends on soon-to-be-deprecated
-                    // SaveFlags behavior: it relies on matrix changes persisting
-                    // after restore.
-                    fCanvas->save(SkCanvas::kClip_SaveFlag);
+                    fCanvas->save();
                 }
             }
             fCurrentNode = ancestor;
         }
 
         // If we're not at the target node yet, we'll need to return an offset to make the caller
         // apply the next clip or saveLayer.
         if (fCurrentNode != targetNode) {
-            if (fCurrentMatrix != fNodes.top()->fMatrix) {
-                fCurrentMatrix = fNodes.top()->fMatrix;
-                SkMatrix tmp = *fNodes.top()->fMatrix;
-                tmp.postConcat(fPlaybackMatrix);
-                fCanvas->setMatrix(tmp);
-            }
             uint32_t offset = fNodes.top()->fOffset;
             fCurrentNode = fNodes.top();
             fSave = fCurrentNode != targetNode && fCurrentNode->fFlags & Node::kSave_Flag;
             fNodes.pop();
+            this->setCurrentMatrix(fCurrentNode->fMatrix);
             return offset;
         }
     }
 
     // If we got this far, the clip/saveLayer state is all set, so we can proceed to set the matrix
     // for the draw, and return its offset.
-
-    if (fCurrentMatrix != draw->fMatrix) {
-        SkMatrix tmp = *draw->fMatrix;
-        tmp.postConcat(fPlaybackMatrix);
-        fCanvas->setMatrix(tmp);
-        fCurrentMatrix = draw->fMatrix;
-    }
+    this->setCurrentMatrix(draw->fMatrix);
 
     ++fPlaybackIndex;
     return draw->fOffset;
 }