Add matrix dictionary to SkMatrixClipStateMgr

This CL doesn't radically change the behavior of the matrix\clip stack reducer. It just cleans up the matrix handling a bit.

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

[bsalomon, 2014-02-12 14:52:46]

lgtm but maybe Mike should take a look?

https://codereview.chromium.org/151033004/diff/1/src/core/SkMatrixClipStateMg...
File src/core/SkMatrixClipStateMgr.cpp (right):

https://codereview.chromium.org/151033004/diff/1/src/core/SkMatrixClipStateMg...
src/core/SkMatrixClipStateMgr.cpp:128: *fMatrixDict.append() = SkMatrix::I();
fMatrixDict.append()->reset() (writes but doesn't read)

https://codereview.chromium.org/151033004/diff/1/src/core/SkMatrixClipStateMgr.h
File src/core/SkMatrixClipStateMgr.h (right):

https://codereview.chromium.org/151033004/diff/1/src/core/SkMatrixClipStateMg...
src/core/SkMatrixClipStateMgr.h:385: SkTDArray<SkMatrix> fMatrixDict;
Should we use a hash here or does matrix deduping not really matter?

[mtklein, 2014-02-12 14:54:47]

https://codereview.chromium.org/151033004/diff/1/src/core/SkMatrixClipStateMgr.h
File src/core/SkMatrixClipStateMgr.h (right):

https://codereview.chromium.org/151033004/diff/1/src/core/SkMatrixClipStateMg...
src/core/SkMatrixClipStateMgr.h:385: SkTDArray<SkMatrix> fMatrixDict;
On 2014/02/12 14:52:47, bsalomon wrote:
> Should we use a hash here or does matrix deduping not really matter?

We stopped deduping matrices seen during recording because it didn't really
matter.  Unless you know you're explicitly adding the same matrix over and over
again here, I wouldn't bother.

[robertphillips, 2014-02-12 15:05:39]

https://codereview.chromium.org/151033004/diff/1/src/core/SkMatrixClipStateMg...
File src/core/SkMatrixClipStateMgr.cpp (right):

https://codereview.chromium.org/151033004/diff/1/src/core/SkMatrixClipStateMg...
src/core/SkMatrixClipStateMgr.cpp:128: *fMatrixDict.append() = SkMatrix::I();
On 2014/02/12 14:52:47, bsalomon wrote:
> fMatrixDict.append()->reset() (writes but doesn't read)

Done.

https://codereview.chromium.org/151033004/diff/1/src/core/SkMatrixClipStateMgr.h
File src/core/SkMatrixClipStateMgr.h (right):

https://codereview.chromium.org/151033004/diff/1/src/core/SkMatrixClipStateMg...
src/core/SkMatrixClipStateMgr.h:385: SkTDArray<SkMatrix> fMatrixDict;
Once this is more functional I will check the stats but I believe the
stack-based deduping will pay off but more extensive deduping (i.e., with a
hash) would not.

[robertphillips, 2014-02-12 15:19:53]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-02-12 15:20:02]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/robertphillips@google.com/151033004/9...

[commit-bot: I haz the power, 2014-02-12 15:20:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-12 15:20:05]

Failed to apply patch for src/core/SkMatrixClipStateMgr.cpp:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file src/core/SkMatrixClipStateMgr.cpp
  Hunk #1 FAILED at 12.
  Hunk #3 FAILED at 28.
  Hunk #4 FAILED at 65.
  Hunk #5 succeeded at 117 with fuzz 1.
  Hunk #6 FAILED at 219.
  4 out of 7 hunks FAILED -- saving rejects to file
src/core/SkMatrixClipStateMgr.cpp.rej

Patch:       src/core/SkMatrixClipStateMgr.cpp
Index: src/core/SkMatrixClipStateMgr.cpp
===================================================================
--- src/core/SkMatrixClipStateMgr.cpp	(revision 13413)
+++ src/core/SkMatrixClipStateMgr.cpp	(working copy)
@@ -12,7 +12,7 @@
                                                                const SkPath&
path, 
                                                                SkRegion::Op op,

                                                                bool doAA, 
-                                                               const SkMatrix&
matrix) {
+                                                               int matrixID) {
     int pathID = picRecord->addPathToHeap(path);
 
     ClipOp& newClip = fClips.push_back();
@@ -20,7 +20,7 @@
     newClip.fGeom.fPathID = pathID;
     newClip.fOp = op;
     newClip.fDoAA = doAA;
-    newClip.fMatrix = matrix;
+    newClip.fMatrixID = matrixID;
     newClip.fOffset = kInvalidJumpOffset;
     return false;
 }
@@ -28,33 +28,34 @@
 bool
SkMatrixClipStateMgr::MatrixClipState::ClipInfo::clipRegion(SkPictureRecord*
picRecord, 
                                                                  const
SkRegion& region, 
                                                                  SkRegion::Op
op, 
-                                                                 const
SkMatrix& matrix) {
+                                                                 int matrixID)
{
     // TODO: add a region dictionary so we don't have to copy the region in
here                                                                    
     ClipOp& newClip = fClips.push_back();
     newClip.fClipType = kRegion_ClipType;
     newClip.fGeom.fRegion = SkNEW(SkRegion(region));
     newClip.fOp = op;
     newClip.fDoAA = true;      // not necessary but sanity preserving
-    newClip.fMatrix = matrix;
+    newClip.fMatrixID = matrixID;
     newClip.fOffset = kInvalidJumpOffset;
     return false;
 }
 
-void SkMatrixClipStateMgr::WriteDeltaMat(SkPictureRecord* picRecord,
-                                         const SkMatrix& current, 
-                                         const SkMatrix& desired) {
+void SkMatrixClipStateMgr::writeDeltaMat(int currentMatID, int desiredMatID) {
+    const SkMatrix& current = this->lookupMat(currentMatID);
+    const SkMatrix& desired = this->lookupMat(desiredMatID);
+
     SkMatrix delta;
     bool result = current.invert(&delta);
     if (result) {
         delta.preConcat(desired);
     }
-    picRecord->recordConcat(delta);
+    fPicRecord->recordConcat(delta);
 }
 
 // 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(SkMatrix*
curMat, 
-                                                               
SkPictureRecord* picRecord, 
+void SkMatrixClipStateMgr::MatrixClipState::ClipInfo::writeClip(int* curMatID, 
+                                                               
SkMatrixClipStateMgr* mgr,
                                                                 bool*
overrideFirstOp) {
     for (int i = 0; i < fClips.count(); ++i) {
         ClipOp& curClip = fClips[i];
@@ -65,29 +66,34 @@
             *overrideFirstOp = false;
         }
 
-        // TODO: re-add an internal matrix dictionary to not write out
-        // redundant matrices.
+        // TODO: use the matrix ID to skip writing the identity matrix
+        // over and over, i.e.:
+        //  if (*curMatID != curClip.fMatrixID) {
+        //      mgr->writeDeltaMat...
+        //      *curMatID...
+        //  }
+        // Right now this optimization would throw off the testing harness.
         // TODO: right now we're writing out the delta matrix from the prior
         // matrix state. This is a side-effect of writing out the entire
         // clip stack and should be resolved when that is fixed.
-        SkMatrixClipStateMgr::WriteDeltaMat(picRecord, *curMat,
curClip.fMatrix);
-        *curMat = curClip.fMatrix;
+        mgr->writeDeltaMat(*curMatID, curClip.fMatrixID);
+        *curMatID = curClip.fMatrixID;
             
         switch (curClip.fClipType) {
         case kRect_ClipType:
-            curClip.fOffset =
picRecord->recordClipRect(curClip.fGeom.fRRect.rect(), 
-                                                        op, curClip.fDoAA);
+            curClip.fOffset =
mgr->getPicRecord()->recordClipRect(curClip.fGeom.fRRect.rect(), 
+                                                                  op,
curClip.fDoAA);
             break;
         case kRRect_ClipType:
-            curClip.fOffset = picRecord->recordClipRRect(curClip.fGeom.fRRect,
op, 
-                                                         curClip.fDoAA);
+            curClip.fOffset =
mgr->getPicRecord()->recordClipRRect(curClip.fGeom.fRRect, op, 
+                                                                  
curClip.fDoAA);
             break;
         case kPath_ClipType:
-            curClip.fOffset = picRecord->recordClipPath(curClip.fGeom.fPathID,
op, 
-                                                        curClip.fDoAA);
+            curClip.fOffset =
mgr->getPicRecord()->recordClipPath(curClip.fGeom.fPathID, op, 
+                                                                 
curClip.fDoAA);
             break;
         case kRegion_ClipType:
-            curClip.fOffset =
picRecord->recordClipRegion(*curClip.fGeom.fRegion, op);
+            curClip.fOffset =
mgr->getPicRecord()->recordClipRegion(*curClip.fGeom.fRegion, op);
             break;
         default:
             SkASSERT(0);
@@ -117,6 +123,10 @@
                        fMatrixClipStackStorage, 
                        sizeof(fMatrixClipStackStorage))
     , fCurOpenStateID(kIdentityWideOpenStateID) {
+
+    // The first slot in the matrix dictionary is reserved for the identity
matrix
+    fMatrixDict.append()->reset();
+
     fCurMCState = (MatrixClipState*)fMatrixClipStack.push_back();
     new (fCurMCState) MatrixClipState(NULL, 0);    // balanced in restore()
 }
@@ -215,19 +225,19 @@
     SkDeque::F2BIter iter(fMatrixClipStack);
     bool firstClip = true;
 
-    SkMatrix curMat = SkMatrix::I();
+    int curMatID = kIdentityMatID;
     for (const MatrixClipState* state = (const MatrixClipState*) iter.next();
          state != NULL;
          state = (const MatrixClipState*) iter.next()) {
-         state->fClipInfo->writeClip(&curMat, fPicRecord, &firstClip);
+         state->fClipInfo->writeClip(&curMatID, this, &firstClip);
     }
 
     // write out matrix
-    if (!fCurMCState->fMatrixInfo->fMatrix.isIdentity()) {
+    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

         // write out the CTM here when the clip state is collapsed to a single
path.
-        WriteDeltaMat(fPicRecord, curMat, fCurMCState->fMatrixInfo->fMatrix);
+        this->writeDeltaMat(curMatID, fCurMCState->fMatrixInfo->getID(this));
     }
 
     SkDEBUGCODE(this->validate();)
@@ -257,3 +267,12 @@
     }
 }
 #endif
+
+int SkMatrixClipStateMgr::addMatToDict(const SkMatrix& mat) {
+    if (mat.isIdentity()) {
+        return kIdentityMatID;
+    }
+
+    *fMatrixDict.append() = mat;
+    return fMatrixDict.count()-1;
+}

[robertphillips, 2014-02-12 15:34:39]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-02-12 15:34:42]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/robertphillips@google.com/151033004/1...

[commit-bot: I haz the power, 2014-02-12 15:48:07]

Change committed as 13420