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

src/core/SkPictureRecord.cpp

 
 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 #include "SkPictureRecord.h"
 #include "SkTSearch.h"
 #include "SkPixelRef.h"
@@ skipping 484 matching lines @@
     SkDebugf("Collapse [%d out of %d] %g%spn", gCollapseCount, gCollapseCalls,
              (double)gCollapseCount / gCollapseCalls, "%");
 #endif
 
     writer->rewindToOffset(saveOffset);
     return true;
 }
 
 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
     // turned on permanently.
 #if 0
     SkASSERT(fRestoreOffsetStack.count() > 1);
 #endif
 
     // check for underflow
     if (fRestoreOffsetStack.count() == 0) {
         return;
     }
 
     if (fRestoreOffsetStack.count() == fFirstSavedLayerIndex) {
         fFirstSavedLayerIndex = kNoSavedLayerIndex;
     }
 
     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;
         }
     }
 
     if (SK_ARRAY_COUNT(gPictureRecordOpts) == opt) {
         // No optimization fired so add the RESTORE
         fillRestoreOffsetPlaceholdersForCurrentStackLevel((uint32_t)fWriter.size());
         size = 1 * kUInt32Size; // RESTORE consists solely of 1 op code
         initialOffset = this->addDraw(RESTORE, &size);
     }
@@ skipping 862 matching lines @@
 void SkPictureRecord::validateRegions() const {
     int count = fRegions.count();
     SkASSERT((unsigned) count < 0x1000);
     for (int index = 0; index < count; index++) {
         const SkFlatData* region = fRegions[index];
         SkASSERT(region);
 //        region->validate();
     }
 }
 #endif