From: mtklein <mtklein@chromium.org>
Date: Fri, 8 Aug 2014 17:05:19 +0000 (-0700)
Subject: SkRecord: Strip out cull-skipping and y-only drawPosTextH skipping.
X-Git-Tag: accepted/tizen/5.0/unified/20181102.025319~6406
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=f4078ad1ec42f549369ac4f639aab18d00afae95;p=platform%2Fupstream%2FlibSkiaSharp.git

SkRecord: Strip out cull-skipping and y-only drawPosTextH skipping.

These optimizations are outclassed by a general bounding-box hierarchy,
and are just going to make plugging that into SkRecordDraw more complicated.

BUG=skia:
R=robertphillips@google.com, mtklein@google.com

Author: mtklein@chromium.org

Review URL: https://codereview.chromium.org/452983002
---

diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp
index c3c767c..5af00b6 100644
--- a/src/core/SkRecordDraw.cpp
+++ b/src/core/SkRecordDraw.cpp
@@ -19,18 +19,6 @@ void SkRecordDraw(const SkRecord& record, SkCanvas* canvas, SkDrawPictureCallbac
 
 namespace SkRecords {
 
-bool Draw::skip(const PairedPushCull& r) {
-    if (fCanvas->quickReject(r.base->rect)) {
-        fIndex += r.skip;
-        return true;
-    }
-    return false;
-}
-
-bool Draw::skip(const BoundedDrawPosTextH& r) {
-    return fCanvas->quickRejectY(r.minY, r.maxY);
-}
-
 // FIXME: SkBitmaps are stateful, so we need to copy them to play back in multiple threads.
 static SkBitmap shallow_copy(const SkBitmap& bitmap) {
     return bitmap;
@@ -77,7 +65,4 @@ DRAW(DrawVertices, drawVertices(r.vmode, r.vertexCount, r.vertices, r.texs, r.co
                                 r.xmode.get(), r.indices, r.indexCount, r.paint));
 #undef DRAW
 
-template <> void Draw::draw(const PairedPushCull& r) { this->draw(*r.base); }
-template <> void Draw::draw(const BoundedDrawPosTextH& r) { this->draw(*r.base); }
-
 }  // namespace SkRecords
diff --git a/src/core/SkRecordDraw.h b/src/core/SkRecordDraw.h
index 92b94c4..a9557f4 100644
--- a/src/core/SkRecordDraw.h
+++ b/src/core/SkRecordDraw.h
@@ -27,25 +27,13 @@ public:
     void next() { ++fIndex; }
 
     template <typename T> void operator()(const T& r) {
-        if (!this->skip(r)) {
-            this->draw(r);
-        }
+        this->draw(r);
     }
 
 private:
     // No base case, so we'll be compile-time checked that we implement all possibilities.
     template <typename T> void draw(const T&);
 
-    // skip() should return true if we can skip this command, false if not.
-    // It may update fIndex directly to skip more than just this one command.
-
-    // Mostly we just blindly call fCanvas and let it handle quick rejects itself.
-    template <typename T> bool skip(const T&) { return false; }
-
-    // We add our own quick rejects for commands added by optimizations.
-    bool skip(const PairedPushCull&);
-    bool skip(const BoundedDrawPosTextH&);
-
     const SkMatrix fInitialCTM;
     SkCanvas* fCanvas;
     unsigned fIndex;
diff --git a/src/core/SkRecordOpts.cpp b/src/core/SkRecordOpts.cpp
index cb429cf..146e161 100644
--- a/src/core/SkRecordOpts.cpp
+++ b/src/core/SkRecordOpts.cpp
@@ -15,14 +15,11 @@ using namespace SkRecords;
 
 void SkRecordOptimize(SkRecord* record) {
     // TODO(mtklein): fuse independent optimizations to reduce number of passes?
-    SkRecordNoopCulls(record);
     SkRecordNoopSaveRestores(record);
     // TODO(mtklein): figure out why we draw differently and reenable
     //SkRecordNoopSaveLayerDrawRestores(record);
 
-    SkRecordAnnotateCullingPairs(record);
     SkRecordReduceDrawPosTextStrength(record);  // Helpful to run this before BoundDrawPosTextH.
-    SkRecordBoundDrawPosTextH(record);
 }
 
 // Most of the optimizations in this file are pattern-based.  These are all defined as structs with:
@@ -45,21 +42,6 @@ static bool apply(Pass* pass, SkRecord* record) {
     return changed;
 }
 
-struct CullNooper {
-    typedef Pattern3<Is<PushCull>, Star<Is<NoOp> >, Is<PopCull> > Pattern;
-
-    bool onMatch(SkRecord* record, Pattern* pattern, unsigned begin, unsigned end) {
-        record->replace<NoOp>(begin);  // PushCull
-        record->replace<NoOp>(end-1);  // PopCull
-        return true;
-    }
-};
-
-void SkRecordNoopCulls(SkRecord* record) {
-    CullNooper pass;
-    while (apply(&pass, record));
-}
-
 // Turns the logical NoOp Save and Restore in Save-Draw*-Restore patterns into actual NoOps.
 struct SaveOnlyDrawsRestoreNooper {
     typedef Pattern3<Is<Save>,
@@ -210,89 +192,3 @@ void SkRecordReduceDrawPosTextStrength(SkRecord* record) {
     apply(&pass, record);
 }
 
-// Tries to replace DrawPosTextH with BoundedDrawPosTextH, which knows conservative upper and lower
-// bounds to use with SkCanvas::quickRejectY.
-struct TextBounder {
-    typedef Pattern1<Is<DrawPosTextH> > Pattern;
-
-    bool onMatch(SkRecord* record, Pattern* pattern, unsigned begin, unsigned end) {
-        SkASSERT(end == begin + 1);
-        DrawPosTextH* draw = pattern->first<DrawPosTextH>();
-
-        // 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 (draw->paint.isVerticalText() || !draw->paint.canComputeFastBounds()) {
-            return false;
-        }
-
-        // 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 = draw->paint.getTextSize() * 1.5f;
-        SkDEBUGCODE(SkPaint::FontMetrics metrics;)
-        SkDEBUGCODE(draw->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, draw->y - buffer, SK_Scalar1, draw->y + buffer);
-        SkRect adjusted = draw->paint.computeFastBounds(bounds, &bounds);
-
-        Adopted<DrawPosTextH> adopted(draw);
-        SkNEW_PLACEMENT_ARGS(record->replace<BoundedDrawPosTextH>(begin, adopted),
-                             BoundedDrawPosTextH,
-                             (&adopted, adjusted.fTop, adjusted.fBottom));
-        return true;
-    }
-};
-void SkRecordBoundDrawPosTextH(SkRecord* record) {
-    TextBounder pass;
-    apply(&pass, record);
-}
-
-// Replaces PushCull with PairedPushCull, which lets us skip to the paired PopCull when the canvas
-// can quickReject the cull rect.
-// There's no efficient way (yet?) to express this one as a pattern, so we write a custom pass.
-class CullAnnotator {
-public:
-    // Do nothing to most ops.
-    template <typename T> void operator()(T*) {}
-
-    void operator()(PushCull* push) {
-        Pair pair = { fIndex, push };
-        fPushStack.push(pair);
-    }
-
-    void operator()(PopCull* pop) {
-        Pair push = fPushStack.top();
-        fPushStack.pop();
-
-        SkASSERT(fIndex > push.index);
-        unsigned skip = fIndex - push.index;
-
-        Adopted<PushCull> adopted(push.command);
-        SkNEW_PLACEMENT_ARGS(fRecord->replace<PairedPushCull>(push.index, adopted),
-                             PairedPushCull, (&adopted, skip));
-    }
-
-    void apply(SkRecord* record) {
-        for (fRecord = record, fIndex = 0; fIndex < record->count(); fIndex++) {
-            fRecord->mutate<void>(fIndex, *this);
-        }
-    }
-
-private:
-    struct Pair {
-        unsigned index;
-        PushCull* command;
-    };
-
-    SkTDArray<Pair> fPushStack;
-    SkRecord* fRecord;
-    unsigned fIndex;
-};
-void SkRecordAnnotateCullingPairs(SkRecord* record) {
-    CullAnnotator pass;
-    pass.apply(record);
-}
diff --git a/src/core/SkRecordOpts.h b/src/core/SkRecordOpts.h
index b535ec9..da72796 100644
--- a/src/core/SkRecordOpts.h
+++ b/src/core/SkRecordOpts.h
@@ -13,9 +13,6 @@
 // Run all optimizations in recommended order.
 void SkRecordOptimize(SkRecord*);
 
-// NoOp away pointless PushCull/PopCull pairs with nothing between them.
-void SkRecordNoopCulls(SkRecord*);
-
 // Turns logical no-op Save-[non-drawing command]*-Restore patterns into actual no-ops.
 void SkRecordNoopSaveRestores(SkRecord*);
 
@@ -23,13 +20,7 @@ void SkRecordNoopSaveRestores(SkRecord*);
 // draw, and no-op the SaveLayer and Restore.
 void SkRecordNoopSaveLayerDrawRestores(SkRecord*);
 
-// Annotates PushCull commands with the relative offset of their paired PopCull.
-void SkRecordAnnotateCullingPairs(SkRecord*);
-
 // Convert DrawPosText to DrawPosTextH when all the Y coordinates are equal.
 void SkRecordReduceDrawPosTextStrength(SkRecord*);
 
-// Calculate min and max Y bounds for DrawPosTextH commands, for use with SkCanvas::quickRejectY.
-void SkRecordBoundDrawPosTextH(SkRecord*);
-
 #endif//SkRecordOpts_DEFINED
diff --git a/src/core/SkRecords.h b/src/core/SkRecords.h
index 1de1675..347bc36 100644
--- a/src/core/SkRecords.h
+++ b/src/core/SkRecords.h
@@ -47,7 +47,6 @@ namespace SkRecords {
     M(SaveLayer)                                                    \
     M(PushCull)                                                     \
     M(PopCull)                                                      \
-    M(PairedPushCull)         /*From SkRecordAnnotateCullingPairs*/ \
     M(Concat)                                                       \
     M(SetMatrix)                                                    \
     M(ClipPath)                                                     \
@@ -73,8 +72,7 @@ namespace SkRecords {
     M(DrawSprite)                                                   \
     M(DrawText)                                                     \
     M(DrawTextOnPath)                                               \
-    M(DrawVertices)                                                 \
-    M(BoundedDrawPosTextH)    /*From SkRecordBoundDrawPosTextH*/
+    M(DrawVertices)
 
 // Defines SkRecords::Type, an enum of all record types.
 #define ENUM(T) T##_Type,
@@ -297,10 +295,6 @@ struct DrawVertices {
     int indexCount;
 };
 
-// Records added by optimizations.
-RECORD2(PairedPushCull, Adopted<PushCull>, base, unsigned, skip);
-RECORD3(BoundedDrawPosTextH, Adopted<DrawPosTextH>, base, SkScalar, minY, SkScalar, maxY);
-
 #undef RECORD0
 #undef RECORD1
 #undef RECORD2
diff --git a/tests/RecordDrawTest.cpp b/tests/RecordDrawTest.cpp
index 5c63b53..38e1222 100644
--- a/tests/RecordDrawTest.cpp
+++ b/tests/RecordDrawTest.cpp
@@ -18,15 +18,6 @@
 
 static const int W = 1920, H = 1080;
 
-static void draw_pos_text_h(SkCanvas* canvas, const char* text, SkScalar y) {
-    const size_t len = strlen(text);
-    SkAutoTMalloc<SkScalar> xpos(len);
-    for (size_t i = 0; i < len; i++) {
-        xpos[i] = (SkScalar)i;
-    }
-    canvas->drawPosTextH(text, len, xpos, y, SkPaint());
-}
-
 class JustOneDraw : public SkDrawPictureCallback {
 public:
     JustOneDraw() : fCalls(0) {}
@@ -71,63 +62,6 @@ DEF_TEST(RecordDraw_Unbalanced, r) {
     assert_type<SkRecords::Restore> (r, rerecord, 3);
 }
 
-// Rerecord into another SkRecord with a clip.
-static void record_clipped(const SkRecord& record, SkRect clip, SkRecord* clipped) {
-    SkRecorder recorder(clipped, W, H);
-    recorder.clipRect(clip);
-    SkRecordDraw(record, &recorder);
-}
-
-DEF_TEST(RecordDraw_PosTextHQuickReject, r) {
-    SkRecord record;
-    SkRecorder recorder(&record, W, H);
-
-    draw_pos_text_h(&recorder, "This will draw.", 20);
-    draw_pos_text_h(&recorder, "This won't.", 5000);
-
-    SkRecordBoundDrawPosTextH(&record);
-
-    SkRecord clipped;
-    record_clipped(record, SkRect::MakeLTRB(20, 20, 200, 200), &clipped);
-
-    REPORTER_ASSERT(r, 4 == clipped.count());
-    assert_type<SkRecords::ClipRect>    (r, clipped, 0);
-    assert_type<SkRecords::Save>        (r, clipped, 1);
-    assert_type<SkRecords::DrawPosTextH>(r, clipped, 2);
-    assert_type<SkRecords::Restore>     (r, clipped, 3);
-}
-
-DEF_TEST(RecordDraw_Culling, r) {
-    // Record these 7 drawing commands verbatim.
-    SkRecord record;
-    SkRecorder recorder(&record, W, H);
-
-    recorder.pushCull(SkRect::MakeWH(100, 100));
-        recorder.drawRect(SkRect::MakeWH(10, 10), SkPaint());
-        recorder.drawRect(SkRect::MakeWH(30, 30), SkPaint());
-        recorder.pushCull(SkRect::MakeWH(5, 5));
-            recorder.drawRect(SkRect::MakeWH(1, 1), SkPaint());
-        recorder.popCull();
-    recorder.popCull();
-
-    // Take a pass over to match up pushCulls and popCulls.
-    SkRecordAnnotateCullingPairs(&record);
-
-    // This clip intersects the outer cull, but allows us to quick reject the inner one.
-    SkRecord clipped;
-    record_clipped(record, SkRect::MakeLTRB(20, 20, 200, 200), &clipped);
-
-    // If culling weren't working, we'd see 3 more commands recorded here.
-    REPORTER_ASSERT(r, 7 == clipped.count());
-    assert_type<SkRecords::ClipRect>(r, clipped, 0);
-    assert_type<SkRecords::Save>    (r, clipped, 1);
-    assert_type<SkRecords::PushCull>(r, clipped, 2);
-    assert_type<SkRecords::DrawRect>(r, clipped, 3);
-    assert_type<SkRecords::DrawRect>(r, clipped, 4);
-    assert_type<SkRecords::PopCull> (r, clipped, 5);
-    assert_type<SkRecords::Restore> (r, clipped, 6);
-}
-
 DEF_TEST(RecordDraw_SetMatrixClobber, r) {
     // Set up an SkRecord that just scales by 2x,3x.
     SkRecord scaleRecord;
diff --git a/tests/RecordOptsTest.cpp b/tests/RecordOptsTest.cpp
index 6a9f08a..dca9482 100644
--- a/tests/RecordOptsTest.cpp
+++ b/tests/RecordOptsTest.cpp
@@ -16,55 +16,6 @@
 
 static const int W = 1920, H = 1080;
 
-DEF_TEST(RecordOpts_Culling, r) {
-    SkRecord record;
-    SkRecorder recorder(&record, W, H);
-
-    recorder.drawRect(SkRect::MakeWH(1000, 10000), SkPaint());
-
-    recorder.pushCull(SkRect::MakeWH(100, 100));
-        recorder.drawRect(SkRect::MakeWH(10, 10), SkPaint());
-        recorder.drawRect(SkRect::MakeWH(30, 30), SkPaint());
-        recorder.pushCull(SkRect::MakeWH(5, 5));
-            recorder.drawRect(SkRect::MakeWH(1, 1), SkPaint());
-        recorder.popCull();
-    recorder.popCull();
-
-    SkRecordAnnotateCullingPairs(&record);
-
-    REPORTER_ASSERT(r, 6 == assert_type<SkRecords::PairedPushCull>(r, record, 1)->skip);
-    REPORTER_ASSERT(r, 2 == assert_type<SkRecords::PairedPushCull>(r, record, 4)->skip);
-}
-
-DEF_TEST(RecordOpts_NoopCulls, r) {
-    SkRecord record;
-    SkRecorder recorder(&record, W, H);
-
-    // All should be nooped.
-    recorder.pushCull(SkRect::MakeWH(200, 200));
-        recorder.pushCull(SkRect::MakeWH(100, 100));
-        recorder.popCull();
-    recorder.popCull();
-
-    // Kept for now.  We could peel off a layer of culling.
-    recorder.pushCull(SkRect::MakeWH(5, 5));
-        recorder.pushCull(SkRect::MakeWH(5, 5));
-            recorder.drawRect(SkRect::MakeWH(1, 1), SkPaint());
-        recorder.popCull();
-    recorder.popCull();
-
-    SkRecordNoopCulls(&record);
-
-    for (unsigned i = 0; i < 4; i++) {
-        assert_type<SkRecords::NoOp>(r, record, i);
-    }
-    assert_type<SkRecords::PushCull>(r, record, 4);
-    assert_type<SkRecords::PushCull>(r, record, 5);
-    assert_type<SkRecords::DrawRect>(r, record, 6);
-    assert_type<SkRecords::PopCull>(r, record, 7);
-    assert_type<SkRecords::PopCull>(r, record, 8);
-}
-
 static void draw_pos_text(SkCanvas* canvas, const char* text, bool constantY) {
     const size_t len = strlen(text);
     SkAutoTMalloc<SkPoint> pos(len);
@@ -89,29 +40,6 @@ DEF_TEST(RecordOpts_StrengthReduction, r) {
     assert_type<SkRecords::DrawPosText>(r, record, 1);
 }
 
-DEF_TEST(RecordOpts_TextBounding, r) {
-    SkRecord record;
-    SkRecorder recorder(&record, W, H);
-
-    // First, get a drawPosTextH.  Here's a handy way.  Its text size will be the default (12).
-    draw_pos_text(&recorder, "This will be reduced to drawPosTextH.", true);
-    SkRecordReduceDrawPosTextStrength(&record);
-
-    const SkRecords::DrawPosTextH* original =
-        assert_type<SkRecords::DrawPosTextH>(r, record, 0);
-
-    // This should wrap the original DrawPosTextH with minY and maxY.
-    SkRecordBoundDrawPosTextH(&record);
-
-    const SkRecords::BoundedDrawPosTextH* bounded =
-        assert_type<SkRecords::BoundedDrawPosTextH>(r, record, 0);
-
-    const SkPaint defaults;
-    REPORTER_ASSERT(r, bounded->base == original);
-    REPORTER_ASSERT(r, bounded->minY <= SK_Scalar1 - defaults.getTextSize());
-    REPORTER_ASSERT(r, bounded->maxY >= SK_Scalar1 + defaults.getTextSize());
-}
-
 DEF_TEST(RecordOpts_NoopDrawSaveRestore, r) {
     SkRecord record;
     SkRecorder recorder(&record, W, H);