fill rect batch refactor into separate batches

src/gpu/batches/GrBWFillRectBatch.cpp

Index: src/gpu/batches/GrBWFillRectBatch.cpp
diff --git a/src/gpu/batches/GrBWFillRectBatch.cpp b/src/gpu/batches/GrBWFillRectBatch.cpp
index c0c93c7614ddd2fae6f23c68b290153961585278..26f5a4f3e05a1c8766af96660293c41e71bdd55a 100644
--- a/src/gpu/batches/GrBWFillRectBatch.cpp
+++ b/src/gpu/batches/GrBWFillRectBatch.cpp
@@ -17,20 +17,34 @@ class GrBatchTarget;
 class SkMatrix;
 struct SkRect;
 
+/*
+ * BWFillRectBatch is templated to optionally allow the insertion of an additional
+ * attribute for explicit local coordinates and also to handle an optional localMatrix
+ * To use this template, an implementation must define the following static functions:
+ *     A Geometry struct
+ *
+ *     bool CanCombineLocalCoords(const SkMatrix& mine, const SkMatrix& theirs,
+ *                                bool usesLocalCoords)
+ *
+ *     GrDefaultGeoProcFactory::LocalCoords::Type LocalCoordsType()
+ *
+ *     bool StrideCheck(size_t vertexStride, bool canTweakAlphaForCoverage,
+ *                      bool usesLocalCoords)
+ *
+ *     void FillInAttributes(intptr_t startVertex, size_t vertexStride,
+ *                           SkPoint* fan0Position, const Geometry&)
+ *
+ *     const GrGeometryProcessor* CreateGP(const Geometry& geo,
+ *                                         const GrDefaultGeoProcFactory::Color& color,
+ *                                         const GrDefaultGeoProcFactory::Coverage& coverage)
+ */
+template <typename Base>

[bsalomon, 2015/08/14 14:37:48]

I don't Base is a good a name for the template parameter... it's not a Base
class.

 class BWFillRectBatch : public GrVertexBatch {
 public:
-    struct Geometry {
-        SkMatrix fViewMatrix;
-        SkRect fRect;
-        SkRect fLocalRect;
-        SkMatrix fLocalMatrix;
-        GrColor fColor;
-        bool fHasLocalRect;
-        bool fHasLocalMatrix;
-    };
+    typedef typename Base::Geometry Geometry;
 
-    static GrDrawBatch* Create(const Geometry& geometry) {
-        return SkNEW_ARGS(BWFillRectBatch, (geometry));
+    static BWFillRectBatch* Create() {
+        return SkNEW(BWFillRectBatch);
     }
 
     const char* name() const override { return "RectBatch"; }
@@ -59,7 +73,7 @@ public:
     }
 
     void generateGeometry(GrBatchTarget* batchTarget) override {
-        SkAutoTUnref<const GrGeometryProcessor> gp(this->createRectGP());
+        SkAutoTUnref<const GrGeometryProcessor> gp(this->createGP(fGeoData[0]));
         if (!gp) {
             SkDebugf("Could not create GrGeometryProcessor\n");
             return;
@@ -69,9 +83,7 @@ public:
 
         int instanceCount = fGeoData.count();
         size_t vertexStride = gp->getVertexStride();
-        SkASSERT(this->hasLocalRect() ?
-                 vertexStride == sizeof(GrDefaultGeoProcFactory::PositionColorLocalCoordAttr) :
-                 vertexStride == sizeof(GrDefaultGeoProcFactory::PositionColorAttr));
+        SkASSERT(Base::StrideCheck(vertexStride));
         QuadHelper helper;
         void* vertices = helper.init(batchTarget, vertexStride, instanceCount);
 
@@ -88,19 +100,10 @@ public:
 
             positions->setRectFan(geom.fRect.fLeft, geom.fRect.fTop,
                                   geom.fRect.fRight, geom.fRect.fBottom, vertexStride);

[robertphillips, 2015/08/14 14:42:57]

space before '(' ?

+            // TODO we are mapping the rect twice(three times in the GrDrawContext::drawRect case)
             geom.fViewMatrix.mapPointsWithStride(positions, vertexStride, kVerticesPerQuad);
 
-            // TODO we should only do this if local coords are being read
-            if (geom.fHasLocalRect) {
-                static const int kLocalOffset = sizeof(SkPoint) + sizeof(GrColor);
-                SkPoint* coords = reinterpret_cast<SkPoint*>(offset + kLocalOffset);
-                coords->setRectFan(geom.fLocalRect.fLeft, geom.fLocalRect.fTop,
-                                   geom.fLocalRect.fRight, geom.fLocalRect.fBottom,
-                                   vertexStride);
-                if (geom.fHasLocalMatrix) {
-                    geom.fLocalMatrix.mapPointsWithStride(coords, vertexStride, kVerticesPerQuad);
-                }
-            }
+            Base::FillInAttributes(offset, vertexStride, geom);
 
             static const int kColorOffset = sizeof(SkPoint);
             GrColor* vertColor = reinterpret_cast<GrColor*>(offset + kColorOffset);
@@ -115,22 +118,25 @@ public:
 
     SkSTArray<1, Geometry, true>* geoData() { return &fGeoData; }
 
-private:
-    BWFillRectBatch(const Geometry& geometry) {
-        this->initClassID<BWFillRectBatch>();
-        fGeoData.push_back(geometry);
+    // to avoid even the initial copy of the struct, we have a getter for the first item which
+    // is used to seed the batch with its initial geometry.  After seeding, the client should call
+    // init() so the Batch can initialize itself
+    Geometry* geometry() { return &fGeoData[0]; }
+    void init() {
+        const Geometry& geo = fGeoData[0];
+        geo.fViewMatrix.mapRect(&fBounds, geo.fRect);
+    }
 
-        fBounds = geometry.fRect;
-        geometry.fViewMatrix.mapRect(&fBounds);
+private:
+    BWFillRectBatch() {
+        this->initClassID<BWFillRectBatch<Base>>();
+        fGeoData.push_back();
     }
 
     GrColor color() const { return fBatch.fColor; }
     bool usesLocalCoords() const { return fBatch.fUsesLocalCoords; }
     bool colorIgnored() const { return fBatch.fColorIgnored; }
     const SkMatrix& viewMatrix() const { return fGeoData[0].fViewMatrix; }
-    const SkMatrix& localMatrix() const { return fGeoData[0].fLocalMatrix; }
-    bool hasLocalRect() const { return fGeoData[0].fHasLocalRect; }
-    bool hasLocalMatrix() const { return fGeoData[0].fHasLocalMatrix; }
     bool coverageIgnored() const { return fBatch.fCoverageIgnored; }
 
     bool onCombineIfPossible(GrBatch* t, const GrCaps& caps) override {
@@ -140,25 +146,11 @@ private:
             return false;
         }
 
-        if (this->hasLocalRect() != that->hasLocalRect()) {
+        if (!Base::CanCombineLocalCoords(this->viewMatrix(), that->viewMatrix(),
+                                         this->usesLocalCoords())) {
             return false;
         }
 
-        SkASSERT(this->usesLocalCoords() == that->usesLocalCoords());
-        if (!this->hasLocalRect() && this->usesLocalCoords()) {
-            if (!this->viewMatrix().cheapEqualTo(that->viewMatrix())) {
-                return false;
-            }
-
-            if (this->hasLocalMatrix() != that->hasLocalMatrix()) {
-                return false;
-            }
-
-            if (this->hasLocalMatrix() && !this->localMatrix().cheapEqualTo(that->localMatrix())) {
-                return false;
-            }
-        }
-
         if (this->color() != that->color()) {
             fBatch.fColor = GrColor_ILLEGAL;
         }
@@ -179,22 +171,12 @@ private:
 
         The vertex attrib order is always pos, color, [local coords].
      */
-    const GrGeometryProcessor* createRectGP() const {
+    const GrGeometryProcessor* createGP(const Geometry& geo) const {
         using namespace GrDefaultGeoProcFactory;
         Color color(Color::kAttribute_Type);
         Coverage coverage(this->coverageIgnored() ? Coverage::kNone_Type : Coverage::kSolid_Type);
 
-        // if we have a local rect, then we apply the localMatrix directly to the localRect to
-        // generate vertex local coords
-        if (this->hasLocalRect()) {
-            LocalCoords localCoords(LocalCoords::kHasExplicit_Type);
-            return GrDefaultGeoProcFactory::Create(color, coverage, localCoords, SkMatrix::I());
-        } else {
-            LocalCoords localCoords(LocalCoords::kUsePosition_Type,
-                                    this->hasLocalMatrix() ? &this->localMatrix() : NULL);
-            return GrDefaultGeoProcFactory::CreateForDeviceSpace(color, coverage, localCoords,
-                                                                 this->viewMatrix());
-        }
+        return Base::CreateGP(geo, color, coverage);
     }
 
     struct BatchTracker {
@@ -208,32 +190,183 @@ private:
     SkSTArray<1, Geometry, true> fGeoData;
 };
 
+/*
+ * Right now we have 4 variants of BWFillRect.  BWFillRect with no local matrix or local rect,

[bsalomon, 2015/08/14 14:37:48]

Maybe write this as bullets? Kind of hard to follow.

Comment on why this is has a templated Base class... pretty confusing, should be
carefully documented.

+ * BWFillRect who has a uniform localMatrix, BWFillRect who has a localRect, and a BWFillRect who
+ * applies its localMatrix to its localRect

[robertphillips, 2015/08/14 14:42:57]

// These classes are created by having two levels of templating. The first level
creates templated versions of GrBWFillRectBatchNoLocalRectImp and
GrBWFillRectBatchLocalRectImp ...

+ */
+template <class Base>
+class GrBWFillRectBatchNoLocalRectImp : public Base {
+public:
+    typedef typename Base::Geometry Geometry;
+
+    inline static bool CanCombineLocalCoords(const SkMatrix& mine, const SkMatrix& theirs,
+                                             bool usesLocalCoords) {
+        // We apply the viewmatrix to the rect points on the cpu.  However, if the pipeline uses
+        // local coords then we won't be able to batch.  We could actually upload the viewmatrix
+        // using vertex attributes in these cases, but haven't investigated that
+        return !usesLocalCoords || mine.cheapEqualTo(theirs);
+    }
+
+    inline static bool StrideCheck(size_t vertexStride) {
+        return vertexStride == sizeof(GrDefaultGeoProcFactory::PositionColorAttr);
+    }
+
+    inline static void FillInAttributes(intptr_t vertices, size_t vertexStride,

[robertphillips, 2015/08/14 14:42:57]

We don't have to invoke "Base::OnFillInAttributes(coords, vertexStride, args);"
here?

+                                        const Geometry& args) {}
+};
+
+template <class Base>
+class GrBWFillRectBatchLocalRectImp {
+public:
+    typedef typename Base::Geometry Geometry;
+
+    inline static bool CanCombineLocalCoords(const SkMatrix& mine, const SkMatrix& theirs,
+                                             bool usesLocalCoords) {

[robertphillips, 2015/08/14 14:42:57]

// We can always combine b.c. ...
?

+        return true;
+    }
+
+    inline static const GrGeometryProcessor* CreateGP(
+                                                const Geometry& geo,
+                                                const GrDefaultGeoProcFactory::Color& color,
+                                                const GrDefaultGeoProcFactory::Coverage& coverage) {
+        using namespace GrDefaultGeoProcFactory;
+        LocalCoords localCoords(LocalCoords::kHasExplicit_Type);
+        return GrDefaultGeoProcFactory::Create(color, coverage, localCoords, SkMatrix::I());
+    }
+
+    inline static bool StrideCheck(size_t vertexStride) {
+        return vertexStride == sizeof(GrDefaultGeoProcFactory::PositionColorLocalCoordAttr);
+    }
+
+    inline static void FillInAttributes(intptr_t vertices, size_t vertexStride,
+                                        const Geometry& args) {
+        static const int kLocalOffset = sizeof(SkPoint) + sizeof(GrColor);
+        SkPoint* coords = reinterpret_cast<SkPoint*>(vertices + kLocalOffset);
+        coords->setRectFan(args.fLocalRect.fLeft, args.fLocalRect.fTop,
+                           args.fLocalRect.fRight, args.fLocalRect.fBottom,
+                           vertexStride);
+        Base::OnFillInAttributes(coords, vertexStride, args);
+    }
+};
+
+class BWFillRectBatchSimpleImp {
+public:
+    struct Geometry {
+        SkMatrix fViewMatrix;
+        SkRect fRect;
+        GrColor fColor;
+    };
+
+    inline static const GrGeometryProcessor* CreateGP(
+                                                const Geometry& geo,
+                                                const GrDefaultGeoProcFactory::Color& color,
+                                                const GrDefaultGeoProcFactory::Coverage& coverage) {
+        using namespace GrDefaultGeoProcFactory;
+        LocalCoords localCoords(LocalCoords::kUsePosition_Type);
+        return GrDefaultGeoProcFactory::CreateForDeviceSpace(color, coverage, localCoords,
+                                                             geo.fViewMatrix);
+    }
+};
+
+class BWFillRectBatchLocalMatrixImp {
+public:
+    struct Geometry {
+        SkMatrix fViewMatrix;
+        SkMatrix fLocalMatrix;
+        SkRect fRect;
+        GrColor fColor;
+    };
+
+    inline static const GrGeometryProcessor* CreateGP(
+                                                const Geometry& geo,
+                                                const GrDefaultGeoProcFactory::Color& color,
+                                                const GrDefaultGeoProcFactory::Coverage& coverage) {
+        using namespace GrDefaultGeoProcFactory;
+        LocalCoords localCoords(LocalCoords::kUsePosition_Type, &geo.fLocalMatrix);
+        return GrDefaultGeoProcFactory::CreateForDeviceSpace(color, coverage, localCoords,
+                                                             geo.fViewMatrix);
+    }
+};
+
+class BWFillRectBatchLocalRectImp {
+public:
+    struct Geometry {
+        SkMatrix fViewMatrix;
+        SkRect fRect;
+        SkRect fLocalRect;
+        GrColor fColor;
+    };
+    inline static void OnFillInAttributes(SkPoint* coords, size_t vertexStride,

[robertphillips, 2015/08/14 14:42:57]

call base class ?

+                                          const Geometry& args) {}
+};
+
+class BWFillRectBatchLocalMatrixLocalRectImp {
+public:
+    struct Geometry {
+        SkMatrix fViewMatrix;
+        SkMatrix fLocalMatrix;
+        SkRect fRect;
+        SkRect fLocalRect;
+        GrColor fColor;
+    };
+    inline static void OnFillInAttributes(SkPoint* coords, size_t vertexStride,
+                                          const Geometry& args) {
+        args.fLocalMatrix.mapPointsWithStride(coords, vertexStride, kVerticesPerQuad);
+    }
+    static const int kVerticesPerQuad = 4;
+};
+
+
+typedef BWFillRectBatch<GrBWFillRectBatchNoLocalRectImp<BWFillRectBatchSimpleImp>> BWFillRectBatchSimple;
+typedef BWFillRectBatch<GrBWFillRectBatchNoLocalRectImp<BWFillRectBatchLocalMatrixImp>> BWFillRectBatchLocalMatrix;
+typedef BWFillRectBatch<GrBWFillRectBatchLocalRectImp<BWFillRectBatchLocalRectImp>> BWFillRectBatchLocalRect;
+typedef BWFillRectBatch<GrBWFillRectBatchLocalRectImp<BWFillRectBatchLocalMatrixLocalRectImp>> BWFillRectBatchLocalMatrixLocalRect;
+
 namespace GrBWFillRectBatch {
 GrDrawBatch* Create(GrColor color,
                     const SkMatrix& viewMatrix,
                     const SkRect& rect,
                     const SkRect* localRect,
                     const SkMatrix* localMatrix) {
-    BWFillRectBatch::Geometry geometry;
-    geometry.fColor = color;
-    geometry.fViewMatrix = viewMatrix;
-    geometry.fRect = rect;
-
-    if (localRect) {
-        geometry.fHasLocalRect = true;
-        geometry.fLocalRect = *localRect;
+    // TODO bubble these up as separate calls
+    if (localRect && localMatrix) {
+        BWFillRectBatchLocalMatrixLocalRect* batch = BWFillRectBatchLocalMatrixLocalRect::Create();
+        BWFillRectBatchLocalMatrixLocalRect::Geometry& geo = *batch->geometry();
+        geo.fColor = color;
+        geo.fViewMatrix = viewMatrix;
+        geo.fLocalMatrix = *localMatrix;
+        geo.fRect = rect;
+        geo.fLocalRect = *localRect;
+        batch->init();
+        return batch;
+    } else if (localRect) {
+        BWFillRectBatchLocalRect* batch = BWFillRectBatchLocalRect::Create();
+        BWFillRectBatchLocalRect::Geometry& geo = *batch->geometry();
+        geo.fColor = color;
+        geo.fViewMatrix = viewMatrix;
+        geo.fRect = rect;
+        geo.fLocalRect = *localRect;
+        batch->init();
+        return batch;
+    } else if (localMatrix) {
+        BWFillRectBatchLocalMatrix* batch = BWFillRectBatchLocalMatrix::Create();
+        BWFillRectBatchLocalMatrix::Geometry& geo = *batch->geometry();
+        geo.fColor = color;
+        geo.fViewMatrix = viewMatrix;
+        geo.fLocalMatrix = *localMatrix;
+        geo.fRect = rect;
+        batch->init();
+        return batch;
     } else {
-        geometry.fHasLocalRect = false;
+        BWFillRectBatchSimple* batch = BWFillRectBatchSimple::Create();
+        BWFillRectBatchSimple::Geometry& geo = *batch->geometry();
+        geo.fColor = color;
+        geo.fViewMatrix = viewMatrix;
+        geo.fRect = rect;
+        batch->init();
+        return batch;
     }
-
-    if (localMatrix) {
-        geometry.fHasLocalMatrix = true;
-        geometry.fLocalMatrix = *localMatrix;
-    } else {
-        geometry.fHasLocalMatrix = false;
-    }
-
-    return BWFillRectBatch::Create(geometry);
 }
 };
 
@@ -244,25 +377,16 @@ GrDrawBatch* Create(GrColor color,
 #include "GrBatchTest.h"
 
 DRAW_BATCH_TEST_DEFINE(RectBatch) {
-    BWFillRectBatch::Geometry geometry;
-    geometry.fColor = GrRandomColor(random);
-
-    geometry.fRect = GrTest::TestRect(random);
-    geometry.fHasLocalRect = random->nextBool();
-
-    if (geometry.fHasLocalRect) {
-        geometry.fViewMatrix = GrTest::TestMatrixInvertible(random);
-        geometry.fLocalRect = GrTest::TestRect(random);
-    } else {
-        geometry.fViewMatrix = GrTest::TestMatrix(random);
-    }
-
-    geometry.fHasLocalMatrix = random->nextBool();
-    if (geometry.fHasLocalMatrix) {
-        geometry.fLocalMatrix = GrTest::TestMatrix(random);
-    }
-
-    return BWFillRectBatch::Create(geometry);
+    GrColor color = GrRandomColor(random);
+    SkRect rect = GrTest::TestRect(random);
+    SkRect localRect = GrTest::TestRect(random);
+    SkMatrix viewMatrix = GrTest::TestMatrixInvertible(random);
+    SkMatrix localMatrix = GrTest::TestMatrix(random);
+
+    bool hasLocalRect = random->nextBool();
+    bool hasLocalMatrix = random->nextBool();
+    return GrBWFillRectBatch::Create(color, viewMatrix, rect, hasLocalRect ? &localRect : nullptr,
+                                     hasLocalMatrix ? &localMatrix : nullptr);
 }
 
 #endif