fill rect batch refactor into separate batches

src/gpu/batches/GrBWFillRectBatch.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "GrBWFillRectBatch.h"
 
 #include "GrBatchTarget.h"
 #include "GrColor.h"
 #include "GrDefaultGeoProcFactory.h"
 #include "GrPrimitiveProcessor.h"
 #include "GrVertexBatch.h"
 
 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"; }
 
     void getInvariantOutputColor(GrInitInvariantOutput* out) const override {
         // When this is called on a batch, there is only one geometry bundle
         out->setKnownFourComponents(fGeoData[0].fColor);
     }
 
     void getInvariantOutputCoverage(GrInitInvariantOutput* out) const override {
         out->setKnownSingleComponent(0xff);
     }
 
     void initBatchTracker(const GrPipelineOptimizations& init) override {
         // Handle any color overrides
         if (!init.readsColor()) {
             fGeoData[0].fColor = GrColor_ILLEGAL;
         }
         init.getOverrideColorIfSet(&fGeoData[0].fColor);
 
         // setup batch properties
         fBatch.fColorIgnored = !init.readsColor();
         fBatch.fColor = fGeoData[0].fColor;
         fBatch.fUsesLocalCoords = init.readsLocalCoords();
         fBatch.fCoverageIgnored = !init.readsCoverage();
     }
 
     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;
         }
 
         batchTarget->initDraw(gp, this->pipeline());
 
         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);
 
         if (!vertices) {
             return;
         }
 
         for (int i = 0; i < instanceCount; i++) {
             const Geometry& geom = fGeoData[i];
 
             intptr_t offset = reinterpret_cast<intptr_t>(vertices) +
                               kVerticesPerQuad * i * vertexStride;
             SkPoint* positions = reinterpret_cast<SkPoint*>(offset);
 
             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);
             for (int j = 0; j < 4; ++j) {
                 *vertColor = geom.fColor;
                 vertColor = (GrColor*) ((intptr_t) vertColor + vertexStride);
             }
         }
 
         helper.issueDraw(batchTarget);
     }
 
     SkSTArray<1, Geometry, true>* geoData() { return &fGeoData; }
 
+    // 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);
+    }
+
 private:
-    BWFillRectBatch(const Geometry& geometry) {
-        this->initClassID<BWFillRectBatch>();
-        fGeoData.push_back(geometry);
-
-        fBounds = geometry.fRect;
-        geometry.fViewMatrix.mapRect(&fBounds);
+    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 {
         BWFillRectBatch* that = t->cast<BWFillRectBatch>();
         if (!GrPipeline::CanCombine(*this->pipeline(), this->bounds(), *that->pipeline(),
                                     that->bounds(), caps)) {
             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;
         }
         fGeoData.push_back_n(that->geoData()->count(), that->geoData()->begin());
         this->joinBounds(that->bounds());
         return true;
     }
 
 
     /** We always use per-vertex colors so that rects can be batched across color changes. Sometimes
         we  have explicit local coords and sometimes not. We *could* always provide explicit local
         coords and just duplicate the positions when the caller hasn't provided a local coord rect,
         but we haven't seen a use case which frequently switches between local rect and no local
         rect draws.
 
         The color param is used to determine whether the opaque hint can be set on the draw state.
         The caller must populate the vertex colors itself.
 
         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 {
         GrColor fColor;
         bool fUsesLocalCoords;
         bool fColorIgnored;
         bool fCoverageIgnored;
     };
 
     BatchTracker fBatch;
     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);
 }
 };
 
 ///////////////////////////////////////////////////////////////////////////////////////////////////
 
 #ifdef GR_TEST_UTILS
 
 #include "GrBatchTest.h"
 
 DRAW_BATCH_TEST_DEFINE(RectBatch) {
-    BWFillRectBatch::Geometry geometry;
-    geometry.fColor = GrRandomColor(random);
+    GrColor color = GrRandomColor(random);
+    SkRect rect = GrTest::TestRect(random);
+    SkRect localRect = GrTest::TestRect(random);
+    SkMatrix viewMatrix = GrTest::TestMatrixInvertible(random);
+    SkMatrix localMatrix = GrTest::TestMatrix(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);
+    bool hasLocalRect = random->nextBool();
+    bool hasLocalMatrix = random->nextBool();
+    return GrBWFillRectBatch::Create(color, viewMatrix, rect, hasLocalRect ? &localRect : nullptr,
+                                     hasLocalMatrix ? &localMatrix : nullptr);
 }
 
 #endif