This CL improves saveLayer handling in the SkMatrixClipStateMgr by:

1) no longer storing the clip skip offsets in the stack (since saveLayers can force multiple clip states to be open at one time)

2) writing out only the clips that are relative to the saveLayer's clip state

3) updates the testing harness to accept a save/restore bracketing a saveLayer/restore (since clips have to be applied to the saveLayer's result upon restore)

R=bsalomon@google.com, epoger@google.com

Author: robertphillips@google.com

Review URL: https://codereview.chromium.org/164823003

git-svn-id: http://skia.googlecode.com/svn/trunk@13497 2bbb7eff-a529-9590-31e7-b0007b416f81

3 files changed, 120 insertions, 94 deletions

diff --git a/src/core/SkMatrixClipStateMgr.cpp b/src/core/SkMatrixClipStateMgr.cpp
index 4cf99bca97..3f6294cfb6 100644
--- a/src/core/SkMatrixClipStateMgr.cpp
+++ b/src/core/SkMatrixClipStateMgr.cpp
@@ -21,7 +21,6 @@ bool SkMatrixClipStateMgr::MatrixClipState::ClipInfo::clipPath(SkPictureRecord*
     newClip->fOp = op;
     newClip->fDoAA = doAA;
     newClip->fMatrixID = matrixID;
-    newClip->fOffset = kInvalidJumpOffset;
     return false;
 }
 
@@ -36,7 +35,6 @@ bool SkMatrixClipStateMgr::MatrixClipState::ClipInfo::clipRegion(SkPictureRecord
     newClip->fOp = op;
     newClip->fDoAA = true;      // not necessary but sanity preserving
     newClip->fMatrixID = matrixID;
-    newClip->fOffset = kInvalidJumpOffset;
     return false;
 }
 
@@ -55,17 +53,10 @@ void SkMatrixClipStateMgr::writeDeltaMat(int currentMatID, int desiredMatID) {
 // Note: this only writes out the clips for the current save state. To get the
 // entire clip stack requires iterating of the entire matrix/clip stack.
 void SkMatrixClipStateMgr::MatrixClipState::ClipInfo::writeClip(int* curMatID,
-                                                                SkMatrixClipStateMgr* mgr,
-                                                                bool* overrideFirstOp) {
+                                                                SkMatrixClipStateMgr* mgr) {
     for (int i = 0; i < fClips.count(); ++i) {
         ClipOp& curClip = fClips[i];
 
-        SkRegion::Op op = curClip.fOp;
-        if (*overrideFirstOp) {
-            op = SkRegion::kReplace_Op;
-            *overrideFirstOp = false;
-        }
-
         // TODO: use the matrix ID to skip writing the identity matrix
         // over and over, i.e.:
         //  if (*curMatID != curClip.fMatrixID) {
@@ -79,43 +70,31 @@ void SkMatrixClipStateMgr::MatrixClipState::ClipInfo::writeClip(int* curMatID,
         mgr->writeDeltaMat(*curMatID, curClip.fMatrixID);
         *curMatID = curClip.fMatrixID;
 
+        int offset = 0;
+
         switch (curClip.fClipType) {
         case kRect_ClipType:
-            curClip.fOffset = mgr->getPicRecord()->recordClipRect(curClip.fGeom.fRRect.rect(),
-                                                                  op, curClip.fDoAA);
+            offset = mgr->getPicRecord()->recordClipRect(curClip.fGeom.fRRect.rect(),
+                                                         curClip.fOp, curClip.fDoAA);
             break;
         case kRRect_ClipType:
-            curClip.fOffset = mgr->getPicRecord()->recordClipRRect(curClip.fGeom.fRRect, op,
-                                                                   curClip.fDoAA);
+            offset = mgr->getPicRecord()->recordClipRRect(curClip.fGeom.fRRect, curClip.fOp,
+                                                         curClip.fDoAA);
             break;
         case kPath_ClipType:
-            curClip.fOffset = mgr->getPicRecord()->recordClipPath(curClip.fGeom.fPathID, op,
-                                                                  curClip.fDoAA);
+            offset = mgr->getPicRecord()->recordClipPath(curClip.fGeom.fPathID, curClip.fOp,
+                                                         curClip.fDoAA);
             break;
         case kRegion_ClipType: {
             const SkRegion* region = mgr->lookupRegion(curClip.fGeom.fRegionID);
-            curClip.fOffset = mgr->getPicRecord()->recordClipRegion(*region, op);
+            offset = mgr->getPicRecord()->recordClipRegion(*region, curClip.fOp);
             break;
         }
         default:
             SkASSERT(0);
         }
-    }
-}
 
-// Fill in the skip offsets for all the clips written in the current block
-void SkMatrixClipStateMgr::MatrixClipState::ClipInfo::fillInSkips(SkWriter32* writer,
-                                                                  int32_t restoreOffset) {
-    for (int i = 0; i < fClips.count(); ++i) {
-        ClipOp& curClip = fClips[i];
-
-        if (-1 == curClip.fOffset) {
-            continue;
-        }
-//        SkDEBUGCODE(uint32_t peek = writer->read32At(curClip.fOffset);)
-//        SkASSERT(-1 == peek);
-        writer->overwriteTAt(curClip.fOffset, restoreOffset);
-        SkDEBUGCODE(curClip.fOffset = -1;)
+        mgr->addClipOffset(offset);
     }
 }
 
@@ -126,6 +105,8 @@ SkMatrixClipStateMgr::SkMatrixClipStateMgr()
                        sizeof(fMatrixClipStackStorage))
     , fCurOpenStateID(kIdentityWideOpenStateID) {
 
+    fSkipOffsets = SkNEW(SkTDArray<int>);
+
     // The first slot in the matrix dictionary is reserved for the identity matrix
     fMatrixDict.append()->reset();
 
@@ -137,12 +118,12 @@ SkMatrixClipStateMgr::~SkMatrixClipStateMgr() {
     for (int i = 0; i < fRegionDict.count(); ++i) {
         SkDELETE(fRegionDict[i]);
     }
+
+    SkDELETE(fSkipOffsets);
 }
 
 
-int SkMatrixClipStateMgr::save(SkCanvas::SaveFlags flags) {
-    SkDEBUGCODE(this->validate();)
-
+int SkMatrixClipStateMgr::MCStackPush(SkCanvas::SaveFlags flags) {
     MatrixClipState* newTop = (MatrixClipState*)fMatrixClipStack.push_back();
     new (newTop) MatrixClipState(fCurMCState, flags); // balanced in restore()
     fCurMCState = newTop;
@@ -152,14 +133,29 @@ int SkMatrixClipStateMgr::save(SkCanvas::SaveFlags flags) {
     return fMatrixClipStack.count();
 }
 
+int SkMatrixClipStateMgr::save(SkCanvas::SaveFlags flags) {
+    SkDEBUGCODE(this->validate();)
+
+    return this->MCStackPush(flags);
+}
+
 int SkMatrixClipStateMgr::saveLayer(const SkRect* bounds, const SkPaint* paint,
                                     SkCanvas::SaveFlags flags) {
-    int result = this->save(flags);
+    // Since the saveLayer call draws something we need to potentially dump
+    // out the MC state
+    this->call(kOther_CallType);
+
+    int result = this->MCStackPush(flags);
     ++fCurMCState->fLayerID;
     fCurMCState->fIsSaveLayer = true;
 
-    fCurMCState->fSaveLayerBracketed = this->call(kOther_CallType);
     fCurMCState->fSaveLayerBaseStateID = fCurOpenStateID;
+    fCurMCState->fSavedSkipOffsets = fSkipOffsets;
+
+    // TODO: recycle these rather then new & deleting them on every saveLayer/
+    // restore
+    fSkipOffsets = SkNEW(SkTDArray<int>);
+
     fPicRecord->recordSaveLayer(bounds, paint,
                                 (SkCanvas::SaveFlags)(flags| SkCanvas::kMatrixClip_SaveFlag));
     return result;
@@ -170,21 +166,20 @@ void SkMatrixClipStateMgr::restore() {
 
     if (fCurMCState->fIsSaveLayer) {
         if (fCurMCState->fSaveLayerBaseStateID != fCurOpenStateID) {
-            fPicRecord->recordRestore(); // Close the open block
+            fPicRecord->recordRestore(); // Close the open block inside the saveLayer
         }
         // The saveLayer's don't carry any matrix or clip state in the
         // new scheme so make sure the saveLayer's recordRestore doesn't
         // try to finalize them (i.e., fill in their skip offsets).
         fPicRecord->recordRestore(false); // close of saveLayer
 
-        // Close the Save that brackets the saveLayer. TODO: this doesn't handle
-        // the skip offsets correctly
-        if (fCurMCState->fSaveLayerBracketed) {
-            fPicRecord->recordRestore(false);
-        }
+        fCurOpenStateID = fCurMCState->fSaveLayerBaseStateID;
 
-        // MC states can be allowed to fuse across saveLayer/restore boundaries
-        fCurOpenStateID = kIdentityWideOpenStateID;
+        SkASSERT(0 == fSkipOffsets->count());
+        SkASSERT(NULL != fCurMCState->fSavedSkipOffsets);
+
+        SkDELETE(fSkipOffsets);
+        fSkipOffsets = fCurMCState->fSavedSkipOffsets;
     }
 
     fCurMCState->~MatrixClipState();       // balanced in save()
@@ -220,7 +215,11 @@ bool SkMatrixClipStateMgr::call(CallType callType) {
         return false;
     }
 
-    if (kIdentityWideOpenStateID != fCurOpenStateID) {
+    if (kIdentityWideOpenStateID != fCurOpenStateID && 
+                  (!fCurMCState->fIsSaveLayer || 
+                   fCurMCState->fSaveLayerBaseStateID != fCurOpenStateID)) {
+        // Don't write a restore if the open state is one in which a saveLayer
+        // is nested. The save after the saveLayer's restore will close it.
         fPicRecord->recordRestore();    // Close the open block
     }
 
@@ -230,17 +229,40 @@ bool SkMatrixClipStateMgr::call(CallType callType) {
     fPicRecord->recordSave(SkCanvas::kMatrixClip_SaveFlag);
 
     // write out clips
-    SkDeque::F2BIter iter(fMatrixClipStack);
-    bool firstClip = true;
-
-    int curMatID = kIdentityMatID;
-    for (const MatrixClipState* state = (const MatrixClipState*) iter.next();
+    SkDeque::Iter iter(fMatrixClipStack, SkDeque::Iter::kBack_IterStart);
+    const MatrixClipState* state;
+    // Loop back across the MC states until the last saveLayer. The MC
+    // state in front of the saveLayer has already been written out.
+    for (state = (const MatrixClipState*) iter.prev();
          state != NULL;
-         state = (const MatrixClipState*) iter.next()) {
-         state->fClipInfo->writeClip(&curMatID, this, &firstClip);
+         state = (const MatrixClipState*) iter.prev()) {
+        if (state->fIsSaveLayer) {
+            break;
+        }
+    }
+
+    if (NULL == state) {
+        // There was no saveLayer in the MC stack so we need to output them all
+        iter.reset(fMatrixClipStack, SkDeque::Iter::kFront_IterStart);
+        state = (const MatrixClipState*) iter.next();
+    } else {
+        // SkDeque's iterators actually return the previous location so we
+        // need to reverse and go forward one to get back on track.
+        iter.next(); 
+        SkDEBUGCODE(const MatrixClipState* test = (const MatrixClipState*)) iter.next(); 
+        SkASSERT(test == state);
+    }
+
+    int curMatID = NULL != state ? state->fMatrixInfo->getID(this) : kIdentityMatID;
+    for ( ; state != NULL; state = (const MatrixClipState*) iter.next()) {
+         state->fClipInfo->writeClip(&curMatID, this);
     }
 
     // write out matrix
+    // TODO: this test isn't quite right. It should be:
+    //   if (curMatID != fCurMCState->fMatrixInfo->getID(this)) {
+    // but right now the testing harness always expects a matrix if
+    // the matrices are non-I
     if (kIdentityMatID != fCurMCState->fMatrixInfo->getID(this)) {
         // TODO: writing out the delta matrix here is an artifact of the writing
         // out of the entire clip stack (with its matrices). Ultimately we will
@@ -253,6 +275,17 @@ bool SkMatrixClipStateMgr::call(CallType callType) {
     return true;
 }
 
+// Fill in the skip offsets for all the clips written in the current block
+void SkMatrixClipStateMgr::fillInSkips(SkWriter32* writer, int32_t restoreOffset) {
+    for (int i = 0; i < fSkipOffsets->count(); ++i) {
+        SkDEBUGCODE(int32_t peek = writer->readTAt<int32_t>((*fSkipOffsets)[i]);)
+        SkASSERT(-1 == peek);
+        writer->overwriteTAt<int32_t>((*fSkipOffsets)[i], restoreOffset);
+    }
+
+    fSkipOffsets->rewind();
+}
+
 void SkMatrixClipStateMgr::finish() {
     if (kIdentityWideOpenStateID != fCurOpenStateID) {
         fPicRecord->recordRestore();    // Close the open block
@@ -262,16 +295,23 @@ void SkMatrixClipStateMgr::finish() {
 
 #ifdef SK_DEBUG
 void SkMatrixClipStateMgr::validate() {
-    if (fCurOpenStateID == fCurMCState->fMCStateID) {
-        // The current state is the active one so all its skip offsets should
-        // still be -1
-        SkDeque::F2BIter iter(fMatrixClipStack);
-
-        for (const MatrixClipState* state = (const MatrixClipState*) iter.next();
+    if (fCurOpenStateID == fCurMCState->fMCStateID && 
+            (!fCurMCState->fIsSaveLayer || 
+             fCurOpenStateID != fCurMCState->fSaveLayerBaseStateID)) {
+        // The current state is the active one so it should have a skip
+        // offset for each clip
+        SkDeque::Iter iter(fMatrixClipStack, SkDeque::Iter::kBack_IterStart);
+        int clipCount = 0;
+        for (const MatrixClipState* state = (const MatrixClipState*) iter.prev();
              state != NULL;
-             state = (const MatrixClipState*) iter.next()) {
-            state->fClipInfo->checkOffsetNotEqual(-1);
+             state = (const MatrixClipState*) iter.prev()) {
+             clipCount += state->fClipInfo->numClips();
+             if (state->fIsSaveLayer) {
+                 break;
+             }
         }
+
+        SkASSERT(fSkipOffsets->count() == clipCount);
     }
 }
 #endif
diff --git a/src/core/SkMatrixClipStateMgr.h b/src/core/SkMatrixClipStateMgr.h
index 60f9fa0016..ef0a99dbfa 100644
--- a/src/core/SkMatrixClipStateMgr.h
+++ b/src/core/SkMatrixClipStateMgr.h
@@ -120,7 +120,6 @@ public:
                 newClip->fOp = op;
                 newClip->fDoAA = doAA;
                 newClip->fMatrixID = matrixID;
-                newClip->fOffset = kInvalidJumpOffset;
                 return false;
             }
 
@@ -134,7 +133,6 @@ public:
                 newClip->fOp = op;
                 newClip->fDoAA = doAA;
                 newClip->fMatrixID = matrixID;
-                newClip->fOffset = kInvalidJumpOffset;
                 return false;
             }
 
@@ -147,19 +145,10 @@ public:
                             int regionID,
                             SkRegion::Op op,
                             int matrixID);
-            void writeClip(int* curMatID,
-                           SkMatrixClipStateMgr* mgr,
-                           bool* overrideFirstOp);
-            void fillInSkips(SkWriter32* writer, int32_t restoreOffset);
+            void writeClip(int* curMatID, SkMatrixClipStateMgr* mgr);
+
+            SkDEBUGCODE(int numClips() const { return fClips.count(); })
 
-#ifdef SK_DEBUG
-            void checkOffsetNotEqual(int32_t offset) {
-                for (int i = 0; i < fClips.count(); ++i) {
-                    ClipOp& curClip = fClips[i];
-                    SkASSERT(offset != curClip.fOffset);
-                }
-            }
-#endif
         private:
             enum ClipType {
                 kRect_ClipType,
@@ -168,8 +157,6 @@ public:
                 kRegion_ClipType
             };
 
-            static const int kInvalidJumpOffset = -1;
-
             class ClipOp {
             public:
                 ClipType     fClipType;
@@ -185,10 +172,6 @@ public:
 
                 // The CTM in effect when this clip call was issued
                 int          fMatrixID;
-
-                // The offset of this clipOp's "jump-to-offset" location in the skp.
-                // -1 means the offset hasn't been written.
-                int32_t      fOffset;
             };
 
             SkTDArray<ClipOp> fClips;
@@ -249,7 +232,7 @@ public:
 
         // The next two fields are only valid when fIsSaveLayer is set.
         int32_t      fSaveLayerBaseStateID;
-        bool         fSaveLayerBracketed;
+        SkTDArray<int>* fSavedSkipOffsets;
 
 #ifdef SK_DEBUG
         MatrixClipState* fPrev; // debugging aid
@@ -347,17 +330,7 @@ public:
 
     bool call(CallType callType);
 
-    void fillInSkips(SkWriter32* writer, int32_t restoreOffset) {
-        // Since we write out the entire clip stack at each block start we
-        // need to update the skips for the entire stack each time too.
-        SkDeque::F2BIter iter(fMatrixClipStack);
-
-        for (const MatrixClipState* state = (const MatrixClipState*) iter.next();
-             state != NULL;
-             state = (const MatrixClipState*) iter.next()) {
-            state->fClipInfo->fillInSkips(writer, restoreOffset);
-        }
-    }
+    void fillInSkips(SkWriter32* writer, int32_t restoreOffset);
 
     void finish();
 
@@ -379,9 +352,23 @@ protected:
 
     // The MCStateID of the state currently in effect in the byte stream. 0 if none.
     int32_t          fCurOpenStateID;
+    // The skip offsets for the current open state. These are the locations in the 
+    // skp that must be filled in when the current open state is closed. These are
+    // here rather then distributed across the MatrixClipState's because saveLayers
+    // can cause MC states to be nested.
+    SkTDArray<int32_t>  *fSkipOffsets;
 
     SkDEBUGCODE(void validate();)
 
+    int MCStackPush(SkCanvas::SaveFlags flags);
+
+    void addClipOffset(int offset) {
+        SkASSERT(NULL != fSkipOffsets);
+        SkASSERT(kIdentityWideOpenStateID != fCurOpenStateID);
+
+        *fSkipOffsets->append() = offset;
+    }
+
     void writeDeltaMat(int currentMatID, int desiredMatID);
     static int32_t   NewMCStateID();
 
diff --git a/src/core/SkPictureRecord.cpp b/src/core/SkPictureRecord.cpp
index 5890ac4bf0..c14328b7a8 100644
--- a/src/core/SkPictureRecord.cpp
+++ b/src/core/SkPictureRecord.cpp
@@ -598,7 +598,6 @@ void SkPictureRecord::restore() {
         return;
     }
 
-    // TODO: don't write the restore to the op stream for normal saves
     fMCMgr.restore();
 #else
     // check for underflow