Culling API

src/core/SkPictureRecord.cpp

Index: src/core/SkPictureRecord.cpp
diff --git a/src/core/SkPictureRecord.cpp b/src/core/SkPictureRecord.cpp
index be86951ae803f768035906d3fd76ca0bf93a6d9d..60cd1f594807dbcb5923c57e843bc537f6eb8772 100644
--- a/src/core/SkPictureRecord.cpp
+++ b/src/core/SkPictureRecord.cpp
@@ -76,6 +76,8 @@ static inline uint32_t getPaintOffset(DrawType op, uint32_t opSize) {
         0,  // CLIP_REGION - no paint
         0,  // CLIP_RECT - no paint
         0,  // CLIP_RRECT - no paint

[robertphillips, 2014/02/25 15:58:41]

move to match other moves?

[f(malita), 2014/02/25 16:50:28]

Will do.

+        0,  // PUSH_CULL - no paint
+        0,  // POP_CULL - no paint
         0,  // CONCAT - no paint
         1,  // DRAW_BITMAP - right after op code
         1,  // DRAW_BITMAP_MATRIX - right after op code
@@ -1548,6 +1550,60 @@ void SkPictureRecord::endCommentGroup() {
     this->validate(initialOffset, size);
 }
 
+// [op/size] [rect] [skip offset]
+static const uint32_t kPushCullOpSize = 2 * kUInt32Size + sizeof(SkRect);
+void SkPictureRecord::onPushCull(const SkRect& cullRect) {

[robertphillips, 2014/02/25 15:58:41]

Don't we also want to assert that the current cull rect is contained inside the
cull rect on the top of the stack (if there is one)?

[f(malita), 2014/02/25 16:50:28]

At a high level it surely makes sense (and my initial thought was to do exactly
that), but it comes at a cost: either the client (blink) or the API will need to
keep a cull rect stack and intersect against its top.

Here's a common Blink pattern (think ads or pictures strip): one small-size
clipping div with a whole bunch of children, only a few of them visible at a
time. The parent div pushes its cull bounds, but then its children are
blissfully unaware of their parent clipping and would naturally push culls
outside their parent's.

Options:

1) Blink tracks cull rects and only pushes the intersection
2) Skia tracks cull rects and always intersects against the last one
3) do nothing and allow overflowing culls - the end result (as far as playback
skipping is concerned) is still correct, but maybe there are other implications
down the line where this would cause issues

What do you guys think?

I guess we can attain #2 with minimal overhead in SkPictureRecord::onPushCull
(we are tracking cull rects for optimizations, etc), but that wouldn't help with
an assert in the non-virtual entry point.

+    // Skip identical cull rects.
+    if (!fCullOffsetStack.isEmpty()) {
+        const SkRect& prevCull = fWriter.readTAt<SkRect>(fCullOffsetStack.top() - sizeof(SkRect));
+        if (prevCull == cullRect) {
+            // Skipped culls are tracked on the stack, but they point to the previous offset.
+            fCullOffsetStack.push(fCullOffsetStack.top());
+            return;
+        }
+    }
+
+    uint32_t size = kPushCullOpSize;
+    size_t initialOffset = this->addDraw(PUSH_CULL, &size);
+    // PUSH_CULL's size should stay constant (used to rewind).
+    SkASSERT(size == kPushCullOpSize);
+

[robertphillips, 2014/02/25 15:58:41]

this->

+    addRect(cullRect);
+    fCullOffsetStack.push(fWriter.bytesWritten());

[robertphillips, 2014/02/25 15:58:41]

this->

+    addInt(0);
+    this->validate(initialOffset, size);
+}
+
+void SkPictureRecord::onPopCull() {
+    SkASSERT(!fCullOffsetStack.isEmpty());
+
+    uint32_t cullSkipOffset = fCullOffsetStack.top();
+    fCullOffsetStack.pop();
+
+    // Skipped push, do the same for pop.
+    if (!fCullOffsetStack.isEmpty() && cullSkipOffset == fCullOffsetStack.top()) {
+        return;
+    }
+
+    // Collapse empty push/pop pairs.
+    if ((size_t)(cullSkipOffset + kUInt32Size) == fWriter.bytesWritten()) {
+        SkASSERT(fWriter.bytesWritten() >= kPushCullOpSize);

[robertphillips, 2014/02/25 15:58:41]

can we have a peek_op static function for this?

+        SkASSERT(fWriter.readTAt<uint32_t>(
+                     fWriter.bytesWritten() - kPushCullOpSize) >> 24 == PUSH_CULL);
+        fWriter.rewindToOffset(fWriter.bytesWritten() - kPushCullOpSize);
+        return;
+    }
+
+    // op only
+    uint32_t size = kUInt32Size;
+    size_t initialOffset = this->addDraw(POP_CULL, &size);
+
+    // update the cull skip offset to point past this op.
+    fWriter.overwriteTAt<uint32_t>(cullSkipOffset, fWriter.bytesWritten());
+
+    this->validate(initialOffset, size);
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 
 SkSurface* SkPictureRecord::onNewSurface(const SkImageInfo& info) {