Fixing SkPicture command pattern optimizations to make them work correctly with bounding box hierar…

src/core/SkPictureRecord.cpp

Index: src/core/SkPictureRecord.cpp
===================================================================
--- src/core/SkPictureRecord.cpp	(revision 8135)
+++ src/core/SkPictureRecord.cpp	(working copy)
@@ -502,20 +502,45 @@
 
 typedef bool (*PictureRecordOptProc)(SkWriter32* writer, int32_t offset,
                                      SkPaintDictionary* paintDict);
+enum PictureRecordOptType {
+    kRewind_OptType,  // Optimization rewinds the command stream
+    kCollapseSaveLayer_OptType,  // Optimization eliminates a save/restore pair
+};
 
+struct PictureRecordOpt {
+    PictureRecordOptProc fProc;
+    PictureRecordOptType fType;
+};
 /*
  * A list of the optimizations that are tried upon seeing a restore
  * TODO: add a real API for such optimizations
  *       Add the ability to fire optimizations on any op (not just RESTORE)
  */
-static const PictureRecordOptProc gPictureRecordOpts[] = {
-    collapse_save_clip_restore,
-#ifndef SK_IGNORE_PICTURE_RECORD_SAVE_LAYER_OPT
-    remove_save_layer1,
-    remove_save_layer2,
-#endif
+static const PictureRecordOpt gPictureRecordOpts[] = {
+    { collapse_save_clip_restore, kRewind_OptType },
+    { remove_save_layer1,         kCollapseSaveLayer_OptType },
+    { remove_save_layer2,         kCollapseSaveLayer_OptType }
 };
 

[robertphillips, 2013/03/14 18:55:44]

comment? // This method is called after the passed in optimization has been
applied ...
Would it be better to just pass in what is supposed to be done rather than the
index of the optimization?

+void SkPictureRecord::handleOptimization(int opt) {
+    switch (gPictureRecordOpts[opt].fType) {
+    case kCollapseSaveLayer_OptType:
+        if (NULL != fStateTree) {
+            fStateTree->saveCollapsed();
+        }
+        break;
+    case kRewind_OptType:

[robertphillips, 2013/03/14 18:55:44]

This is a rather ominous comment.

[Justin Novosad, 2013/03/14 20:04:14]

Done.

+        // No need do any rewinding on the state tree, we can just leave the
+        // unreferenced branches dangling.
+        if (NULL != fBoundingHierarchy) {
+            fBoundingHierarchy->rewindInserts();
+        }
+        break;
+    default:
+        SkASSERT(0);
+    }
+}
+
 void SkPictureRecord::restore() {
     // FIXME: SkDeferredCanvas needs to be refactored to respect
     // save/restore balancing so that the following test can be
@@ -536,10 +561,11 @@
     uint32_t initialOffset, size;
     size_t opt;
     for (opt = 0; opt < SK_ARRAY_COUNT(gPictureRecordOpts); ++opt) {
-        if ((*gPictureRecordOpts[opt])(&fWriter, fRestoreOffsetStack.top(), &fPaints)) {
+        if ((*gPictureRecordOpts[opt].fProc)(&fWriter, fRestoreOffsetStack.top(), &fPaints)) {
             // Some optimization fired so don't add the RESTORE
             size = 0;
             initialOffset = fWriter.size();

[robertphillips, 2013/03/14 18:55:44]

this->

[Justin Novosad, 2013/03/14 20:04:14]

Done.

+            handleOptimization(opt);
             break;
         }
     }