Prevent picture recording from over optimizing the culling of clips.

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"
 #include "SkRRect.h"
 #include "SkBBoxHierarchy.h"
 #include "SkDevice.h"
 #include "SkPictureStateTree.h"
 
 #define MIN_WRITER_SIZE 16384
 #define HEAP_BLOCK_SIZE 4096
 
 enum {
     // just need a value that save or getSaveCount would never return
     kNoInitialSave = -1,
 };
 
 // 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);
 
 SkPictureRecord::SkPictureRecord(uint32_t flags, SkDevice* device) :
         INHERITED(device),
         fBoundingHierarchy(NULL),
         fStateTree(NULL),
         fFlattenableHeap(HEAP_BLOCK_SIZE),
         fMatrices(&fFlattenableHeap),
         fPaints(&fFlattenableHeap),
@@ skipping 105 matching lines @@
     return this->INHERITED::setDevice(device);
 }
 
 int SkPictureRecord::save(SaveFlags flags) {
     // record the offset to us, making it non-positive to distinguish a save
     // from a clip entry.
     fRestoreOffsetStack.push(-(int32_t)fWriter.size());
 
     // 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);
 
     validate(initialOffset, size);
     return this->INHERITED::save(flags);
 }
 
 int SkPictureRecord::saveLayer(const SkRect* bounds, const SkPaint* paint,
                                SaveFlags flags) {
     // record the offset to us, making it non-positive to distinguish a save
@@ skipping 310 matching lines @@
 
     // now offset points to a save
     offset = -offset;
     uint32_t opSize;
     DrawType op = peek_op_and_size(writer, offset, &opSize);
     if (SAVE_LAYER == op) {
         // not ready to cull these out yet (mrr)
         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).
     int32_t saveOffset = offset;
 
     offset += opSize;
     while (offset < restoreOffset) {
         op = peek_op_and_size(writer, offset, &opSize);
         if ((op > CONCAT && op < ROTATE) || (SAVE_LAYER == op)) {
             // drawing verb, abort
@@ skipping 980 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