Prevent picture recording from over optimizing the culling of clips.

src/core/SkPictureRecord.cpp

Index: src/core/SkPictureRecord.cpp
diff --git a/src/core/SkPictureRecord.cpp b/src/core/SkPictureRecord.cpp
index 915736373d0aba5df1ac3116a09607984a006811..d47c1e24ad26c8bf22a1a49c81788955774cd52e 100644
--- a/src/core/SkPictureRecord.cpp
+++ b/src/core/SkPictureRecord.cpp
@@ -24,6 +24,7 @@ enum {
 // A lot of basic types get stored as a uint32_t: bools, ints, paint indices, etc.
 static int const kUInt32Size = 4;
 
+static const uint32_t kSaveSize = 2 * kUInt32Size;
 static const uint32_t kSaveLayerNoBoundsSize = 4 * kUInt32Size;
 static const uint32_t kSaveLayerWithBoundsSize = 4 * kUInt32Size + sizeof(SkRect);
 
@@ -149,6 +150,8 @@ int SkPictureRecord::save(SaveFlags flags) {
 
     // op + flags
     uint32_t size = 2 * kUInt32Size;
+    SkASSERT(kSaveSize == size);

[mtklein, 2013/08/13 13:57:53]

This seems a little roundabout to me, given that they're both literally "2 *
kUInt32Size".  Why not
  uint32_t size = kSaveSize;
?

[robertphillips, 2013/08/13 14:33:03]

Mainly for solidarity with 
kSaveLayerNoBoundsSize and kSaveLayerWithBoundsSize

+
     uint32_t initialOffset = this->addDraw(SAVE, &size);
     addInt(flags);
 
@@ -479,6 +482,16 @@ static bool collapse_save_clip_restore(SkWriter32* writer, int32_t offset,
         return false;
     }
     SkASSERT(SAVE == op);
+    SkASSERT(kSaveSize == opSize);
+
+    // get the save flag (last 4-bytes of the space allocated for the opSize)
+    SkCanvas::SaveFlags saveFlag = (SkCanvas::SaveFlags) *writer->peek32(offset+4);

[mtklein, 2013/08/13 13:57:53]

This is a nit, but given that it's a bitwise enum, calling it flags instead of
flag can be a useful distinction.

[mtklein, 2013/08/13 13:57:53]

Again, a nit, but this guy can be const.

+    if (SkCanvas::kMatrixClip_SaveFlag != (SkCanvas::kMatrixClip_SaveFlag & saveFlag)) {

[mtklein, 2013/08/13 13:57:53]

Maybe I'm brainfarting, but can't we write this as

if (SkCanvas::kMatrixClip_SaveFlag != saveFlag) ?

[robertphillips, 2013/08/13 14:33:03]

Unfortunately there are several other save flags that need to be filtered out.

[mtklein, 2013/08/13 14:40:47]

Either way, aren't we filtering out anything which is not kMatrixClip?

+        // This function's optimization is only correct for kMatrixClip style saves.
+        // TODO: set checkMatrix & checkClip booleans here and then check for the
+        // offending operations in the following loop.
+        return false;
+    }
 
     // Walk forward until we get back to either a draw-verb (abort) or we hit
     // our restore (success).