SkRasterPipeline: simplify impl and remove need to rewire stages

This builds the stages correctly wired from the get-go.  With a little clever
setup, we can also design around the previous error cases like having no stages
or pipelines that call st->next() off the end.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2149443002

Review-Url: https://codereview.chromium.org/2149443002

diff --git a/src/core/SkRasterPipeline.cpp b/src/core/SkRasterPipeline.cpp
index 6a8f109..8991428 100644 (file)
--- a/src/core/SkRasterPipeline.cpp
+++ b/src/core/SkRasterPipeline.cpp
@@ -9,57 +9,35 @@
 
 SkRasterPipeline::SkRasterPipeline() {}
 
-void SkRasterPipeline::append(SkRasterPipeline::Fn body, const void* body_ctx,
-                              SkRasterPipeline::Fn tail, const void* tail_ctx) {
-    // We can't add more stages after being rewired to run().
-    SkASSERT(!fReadyToRun);
-
-    // For now, just stash the stage's function in its own fNext slot.
-    // We'll rewire our stages before running the pipeline so fNext makes sense.
-    fBody.push_back({ body, const_cast<void*>(body_ctx) });
-    fTail.push_back({ tail, const_cast<void*>(tail_ctx) });
+void SkRasterPipeline::append(SkRasterPipeline::Fn body_fn, const void* body_ctx,
+                              SkRasterPipeline::Fn tail_fn, const void* tail_ctx) {
+    // Each stage holds its own context and the next function to call.
+    // So the pipeline itself has to hold onto the first function that starts the pipeline.
+    (fBody.empty() ? fBodyStart : fBody.back().fNext) = body_fn;
+    (fTail.empty() ? fTailStart : fTail.back().fNext) = tail_fn;
+
+    // Each last stage starts with its next function set to JustReturn as a safety net.
+    // It'll be overwritten by the next call to append().
+    fBody.push_back({ &JustReturn, const_cast<void*>(body_ctx) });
+    fTail.push_back({ &JustReturn, const_cast<void*>(tail_ctx) });
 }
 
 void SkRasterPipeline::run(size_t n) {
-    if (fBody.empty() || fTail.empty()) {
-        return;
-    }
-
-    if (!fReadyToRun) {
-        auto rewire = [](Stages* stages) {
-            SkASSERT(!stages->empty());
-
-            // Rotate the fNext pointers so they point to the next function to
-            // call, not function we're currently calling as set by append().
-            auto start = stages->front().fNext;
-            for (int i = 0; i < stages->count() - 1; i++) {
-                (*stages)[i].fNext = (*stages)[i+1].fNext;
-            }
-            stages->back().fNext = start;  // This is a pretty handy place to stash this.
-        };
-        rewire(&fBody);
-        rewire(&fTail);
-        fReadyToRun = true;
-    }
-
     // It's fastest to start uninitialized if the compilers all let us.  If not, next fastest is 0.
     Sk4f v;
 
-    auto start_body = fBody.back().fNext,  // See rewire().
-         start_tail = fTail.back().fNext;
-
-    auto body = fBody.begin(),
-         tail = fTail.begin();
-
     size_t x = 0;
     while (n >= 4) {
-        start_body(body, x, v,v,v,v, v,v,v,v);
+        fBodyStart(fBody.begin(), x, v,v,v,v, v,v,v,v);
         x += 4;
         n -= 4;
     }
     while (n > 0) {
-        start_tail(tail, x, v,v,v,v, v,v,v,v);
+        fTailStart(fTail.begin(), x, v,v,v,v, v,v,v,v);
         x += 1;
         n -= 1;
     }
 }
+
+void SK_VECTORCALL SkRasterPipeline::JustReturn(Stage*, size_t, Sk4f,Sk4f,Sk4f,Sk4f,
+                                                                Sk4f,Sk4f,Sk4f,Sk4f) {}

diff --git a/src/core/SkRasterPipeline.h b/src/core/SkRasterPipeline.h
index 8ae7bb1..186ee65 100644 (file)
--- a/src/core/SkRasterPipeline.h
+++ b/src/core/SkRasterPipeline.h
@@ -96,9 +96,16 @@ public:
 private:
     using Stages = SkSTArray<10, Stage, /*MEM_COPY=*/true>;
 
+    // This no-op default makes fBodyStart and fTailStart unconditionally safe to call,
+    // and is always the last stage's fNext as a sort of safety net to make sure even a
+    // buggy pipeline can't walk off its own end.
+    static void SK_VECTORCALL JustReturn(Stage*, size_t, Sk4f,Sk4f,Sk4f,Sk4f,
+                                                         Sk4f,Sk4f,Sk4f,Sk4f);
+
     Stages fBody,
            fTail;
-    bool   fReadyToRun = false;
+    Fn fBodyStart = &JustReturn,
+       fTailStart = &JustReturn;
 };
 
 #endif//SkRasterPipeline_DEFINED

diff --git a/tests/SkRasterPipelineTest.cpp b/tests/SkRasterPipelineTest.cpp
index 1db0206..beb517b 100644 (file)
--- a/tests/SkRasterPipelineTest.cpp
+++ b/tests/SkRasterPipelineTest.cpp
@@ -85,3 +85,17 @@ DEF_TEST(SkRasterPipeline, r) {
     REPORTER_ASSERT(r, dst_vals[3] == 16);
     REPORTER_ASSERT(r, dst_vals[4] == 25);
 }
+
+DEF_TEST(SkRasterPipeline_empty, r) {
+    // No asserts... just a test that this is safe to run.
+    SkRasterPipeline p;
+    p.run(20);
+}
+
+DEF_TEST(SkRasterPipeline_nonsense, r) {
+    // No asserts... just a test that this is safe to run and terminates.
+    // square() always calls st->next(); this makes sure we've always got something there to call.
+    SkRasterPipeline p;
+    p.append(square);
+    p.run(20);
+}