Start using type traits in src/record instead of macros.

src/record/SkRecordOpts.cpp

Index: src/record/SkRecordOpts.cpp
diff --git a/src/record/SkRecordOpts.cpp b/src/record/SkRecordOpts.cpp
index 4b35b33a3c20683ff1601879bf0504718c328a06..a3b6dfae43315bd4cac8adc31d4fb61d79f9523c 100644
--- a/src/record/SkRecordOpts.cpp
+++ b/src/record/SkRecordOpts.cpp
@@ -7,6 +7,7 @@
 
 #include "SkRecordOpts.h"
 
+#include "SkRecordTraits.h"
 #include "SkRecords.h"
 #include "SkTDArray.h"
 
@@ -40,9 +41,28 @@ public:
     explicit SaveRestoreNooper(SkRecord* record)
         : Common(record), fSave(kInactive), fChanged(false) {}
 
-    // Most drawing commands reset to inactive state without nooping anything.
+    // Drawing commands reset state to inactive without nooping.
     template <typename T>
-    void operator()(T*) { fSave = kInactive; }
+    void operator()(T*, SK_ENABLE_IF(SkRecords::IsDraw<T>)) { fSave = kInactive; }
+
+    // Most non-drawing commands can be ignored.
+    template <typename T>
+    void operator()(T*, SK_ENABLE_IF(!SkRecords::IsDraw<T>)) {}
+
+    void operator()(SkRecords::Save* r) {
+        fSave = SkCanvas::kMatrixClip_SaveFlag == r->flags ? this->index() : kInactive;
+    }
+
+    void operator()(SkRecords::Restore* r) {
+        if (fSave != kInactive) {
+            // Remove everything between the save and restore, inclusive on both sides.
+            fChanged = true;
+            for (unsigned i = fSave; i <= this->index(); i++) {
+                fRecord->replace<SkRecords::NoOp>(i);
+            }
+            fSave = kInactive;

[f(malita), 2014/04/28 14:20:44]

This seems to only noop the top-most save/restore frame - can we extend it to
apply to parent frames also? (I'm guessing we'll need a stack for tracking
fSave, so not sure whether it's going to be a net gain in the end).

[mtklein, 2014/04/28 15:47:05]

Yep, as written this only noops away innermost save-restore blocks.

I'm hoping we can get by with just running the optimization repeatedly until it
reaches its fixpoint, as we're doing in SkRecordNoopSaveRestores().  It sort of
peels back noop save-restore blocks one layer at a time.

As we've got things right now, this is the only optimization that's really able
to enable other optimizations, and even then, it can really only enable itself. 
That'll change when we have 4 or 5 of these pattern matchers interacting at
once, and we'll probably want to fixpoint them all as a batch rather than one
after the other.  The future is fancy.

+        }
+    }
 
     bool changed() const { return fChanged; }
 
@@ -52,39 +72,6 @@ private:
     bool fChanged;
 };
 
-// If the command doesn't draw anything, that doesn't reset the state back to inactive.
-// TODO(mtklein): do this with some sort of template-based trait mechanism instead of macros
-#define DOESNT_DRAW(T) template <> void SaveRestoreNooper::operator()(SkRecords::T*) {}
-DOESNT_DRAW(NoOp)
-DOESNT_DRAW(Concat)
-DOESNT_DRAW(SetMatrix)
-DOESNT_DRAW(ClipRect)
-DOESNT_DRAW(ClipRRect)
-DOESNT_DRAW(ClipPath)
-DOESNT_DRAW(ClipRegion)
-DOESNT_DRAW(PairedPushCull)
-DOESNT_DRAW(PushCull)
-DOESNT_DRAW(PopCull)
-#undef DOESNT_DRAW
-
-template <>
-void SaveRestoreNooper::operator()(SkRecords::Save* r) {
-    fSave = SkCanvas::kMatrixClip_SaveFlag == r->flags ? this->index() : kInactive;
-}
-
-template <>
-void SaveRestoreNooper::operator()(SkRecords::Restore* r) {
-    if (fSave != kInactive) {
-        // Remove everything between the save and restore, inclusive on both sides.
-        fChanged = true;
-        for (unsigned i = fSave; i <= this->index(); i++) {
-            fRecord->replace<SkRecords::NoOp>(i);
-        }
-        fSave = kInactive;
-    }
-}
-
-
 // Tries to replace PushCull with PairedPushCull, which lets us skip to the paired PopCull
 // when the canvas can quickReject the cull rect.
 class CullAnnotator : public Common {
@@ -92,8 +79,24 @@ public:
     explicit CullAnnotator(SkRecord* record) : Common(record) {}
 
     // Do nothing to most ops.
-    template <typename T>
-    void operator()(T*) {}
+    template <typename T> void operator()(T*) {}
+
+    void operator()(SkRecords::PushCull* push) {
+        Pair pair = { this->index(), push };
+        fPushStack.push(pair);
+    }
+
+    void operator()(SkRecords::PopCull* pop) {
+        Pair push = fPushStack.top();
+        fPushStack.pop();
+
+        SkASSERT(this->index() > push.index);
+        unsigned skip = this->index() - push.index;
+
+        SkRecords::Adopted<SkRecords::PushCull> adopted(push.command);
+        SkNEW_PLACEMENT_ARGS(fRecord->replace<SkRecords::PairedPushCull>(push.index, adopted),
+                             SkRecords::PairedPushCull, (&adopted, skip));
+    }
 
 private:
     struct Pair {
@@ -104,68 +107,46 @@ private:
     SkTDArray<Pair> fPushStack;
 };
 
-template <>
-void CullAnnotator::operator()(SkRecords::PushCull* push) {
-    Pair pair = { this->index(), push };
-    fPushStack.push(pair);
-}
-
-template <>
-void CullAnnotator::operator()(SkRecords::PopCull* pop) {
-    Pair push = fPushStack.top();
-    fPushStack.pop();
-
-    SkASSERT(this->index() > push.index);
-    unsigned skip = this->index() - push.index;
-
-    SkRecords::Adopted<SkRecords::PushCull> adopted(push.command);
-    SkNEW_PLACEMENT_ARGS(fRecord->replace<SkRecords::PairedPushCull>(push.index, adopted),
-                         SkRecords::PairedPushCull, (&adopted, skip));
-}
-
 // Replaces DrawPosText with DrawPosTextH when all Y coordinates are equal.
 class StrengthReducer : public Common {
 public:
     explicit StrengthReducer(SkRecord* record) : Common(record) {}
 
     // Do nothing to most ops.
-    template <typename T>
-    void operator()(T*) {}
-};
+    template <typename T> void operator()(T*) {}
 
-template <>
-void StrengthReducer::operator()(SkRecords::DrawPosText* r) {
-    const unsigned points = r->paint.countText(r->text, r->byteLength);
-    if (points == 0) {
-        // No point (ha!).
-        return;
-    }
-
-    const SkScalar firstY = r->pos[0].fY;
-    for (unsigned i = 1; i < points; i++) {
-        if (r->pos[i].fY != firstY) {
-            // Needs the full strength of DrawPosText.
+    void operator()(SkRecords::DrawPosText* r) {
+        const unsigned points = r->paint.countText(r->text, r->byteLength);
+        if (points == 0) {
+            // No point (ha!).
             return;
         }
-    }
-    // All ys are the same.  We can replace DrawPosText with DrawPosTextH.
-
-    // r->pos is points SkPoints, [(x,y),(x,y),(x,y),(x,y), ... ].
-    // We're going to squint and look at that as 2*points SkScalars, [x,y,x,y,x,y,x,y, ...].
-    // Then we'll rearrange things so all the xs are in order up front, clobbering the ys.
-    SK_COMPILE_ASSERT(sizeof(SkPoint) == 2 * sizeof(SkScalar), SquintingIsNotSafe);
-    SkScalar* scalars = &r->pos[0].fX;
-    for (unsigned i = 0; i < 2*points; i += 2) {
-        scalars[i/2] = scalars[i];
-    }
 
-    // Extend lifetime of r to the end of the method so we can copy its parts.
-    SkRecords::Adopted<SkRecords::DrawPosText> adopted(r);
-    SkNEW_PLACEMENT_ARGS(fRecord->replace<SkRecords::DrawPosTextH>(this->index(), adopted),
-                         SkRecords::DrawPosTextH,
-                         (r->text, r->byteLength, scalars, firstY, r->paint));
-}
+        const SkScalar firstY = r->pos[0].fY;
+        for (unsigned i = 1; i < points; i++) {
+            if (r->pos[i].fY != firstY) {
+                // Needs the full strength of DrawPosText.
+                return;
+            }
+        }
+        // All ys are the same.  We can replace DrawPosText with DrawPosTextH.
+
+        // r->pos is points SkPoints, [(x,y),(x,y),(x,y),(x,y), ... ].
+        // We're going to squint and look at that as 2*points SkScalars, [x,y,x,y,x,y,x,y, ...].
+        // Then we'll rearrange things so all the xs are in order up front, clobbering the ys.
+        SK_COMPILE_ASSERT(sizeof(SkPoint) == 2 * sizeof(SkScalar), SquintingIsNotSafe);
+        SkScalar* scalars = &r->pos[0].fX;
+        for (unsigned i = 0; i < 2*points; i += 2) {
+            scalars[i/2] = scalars[i];
+        }
 
+        // Extend lifetime of r to the end of the method so we can copy its parts.
+        SkRecords::Adopted<SkRecords::DrawPosText> adopted(r);
+        SkNEW_PLACEMENT_ARGS(fRecord->replace<SkRecords::DrawPosTextH>(this->index(), adopted),
+                             SkRecords::DrawPosTextH,
+                             (r->text, r->byteLength, scalars, firstY, r->paint));
+    }
+};
 
 // Tries to replace DrawPosTextH with BoundedDrawPosTextH, which knows conservative upper and lower
 // bounds to use with SkCanvas::quickRejectY.
@@ -174,37 +155,37 @@ public:
     explicit TextBounder(SkRecord* record) : Common(record) {}
 
     // Do nothing to most ops.
-    template <typename T>
-    void operator()(T*) {}
-};
+    template <typename T> void operator()(T*) {}
 
-template <>
-void TextBounder::operator()(SkRecords::DrawPosTextH* r) {
-    // If we're drawing vertical text, none of the checks we're about to do make any sense.
-    // We'll need to call SkPaint::computeFastBounds() later, so bail if that's not possible.
-    if (r->paint.isVerticalText() || !r->paint.canComputeFastBounds()) {
-        return;
+    void operator()(SkRecords::DrawPosTextH* r) {
+        // If we're drawing vertical text, none of the checks we're about to do make any sense.
+        // We'll need to call SkPaint::computeFastBounds() later, so bail if that's not possible.
+        if (r->paint.isVerticalText() || !r->paint.canComputeFastBounds()) {
+            return;
+        }
+
+        // Rather than checking the top and bottom font metrics, we guess.  Actually looking up the
+        // top and bottom metrics is slow, and this overapproximation should be good enough.
+        const SkScalar buffer = r->paint.getTextSize() * 1.5f;
+        SkDEBUGCODE(SkPaint::FontMetrics metrics;)
+        SkDEBUGCODE(r->paint.getFontMetrics(&metrics);)
+        SkASSERT(-buffer <= metrics.fTop);
+        SkASSERT(+buffer >= metrics.fBottom);
+
+        // Let the paint adjust the text bounds.  We don't care about left and right here, so we use
+        // 0 and 1 respectively just so the bounds rectangle isn't empty.
+        SkRect bounds;
+        bounds.set(0, r->y - buffer, SK_Scalar1, r->y + buffer);
+        SkRect adjusted = r->paint.computeFastBounds(bounds, &bounds);
+
+        SkRecords::Adopted<SkRecords::DrawPosTextH> adopted(r);
+        SkNEW_PLACEMENT_ARGS(
+                fRecord->replace<SkRecords::BoundedDrawPosTextH>(this->index(), adopted),
+                SkRecords::BoundedDrawPosTextH,
+                (&adopted, adjusted.fTop, adjusted.fBottom));
     }
+};
 
-    // Rather than checking the top and bottom font metrics, we guess.  Actually looking up the
-    // top and bottom metrics is slow, and this overapproximation should be good enough.
-    const SkScalar buffer = r->paint.getTextSize() * 1.5f;
-    SkDEBUGCODE(SkPaint::FontMetrics metrics;)
-    SkDEBUGCODE(r->paint.getFontMetrics(&metrics);)
-    SkASSERT(-buffer <= metrics.fTop);
-    SkASSERT(+buffer >= metrics.fBottom);
-
-    // Let the paint adjust the text bounds.  We don't care about left and right here, so we use
-    // 0 and 1 respectively just so the bounds rectangle isn't empty.
-    SkRect bounds;
-    bounds.set(0, r->y - buffer, SK_Scalar1, r->y + buffer);
-    SkRect adjusted = r->paint.computeFastBounds(bounds, &bounds);
-
-    SkRecords::Adopted<SkRecords::DrawPosTextH> adopted(r);
-    SkNEW_PLACEMENT_ARGS(fRecord->replace<SkRecords::BoundedDrawPosTextH>(this->index(), adopted),
-                         SkRecords::BoundedDrawPosTextH,
-                         (&adopted, adjusted.fTop, adjusted.fBottom));
-}
 
 template <typename Pass>
 static void run_pass(Pass& pass, SkRecord* record) {