Revise DrawAtlasBatch to get rid of one copy when generating the batch.

src/gpu/batches/GrDrawAtlasBatch.cpp

Index: src/gpu/batches/GrDrawAtlasBatch.cpp
diff --git a/src/gpu/batches/GrDrawAtlasBatch.cpp b/src/gpu/batches/GrDrawAtlasBatch.cpp
index c1f244362c16691c35d698f25a437aab2aef63e1..faea9ecbdd5192ccb306a09e9f7bd90b3d7fdf54 100644
--- a/src/gpu/batches/GrDrawAtlasBatch.cpp
+++ b/src/gpu/batches/GrDrawAtlasBatch.cpp
@@ -8,6 +8,7 @@
 #include "GrDrawAtlasBatch.h"
 #include "GrBatchTest.h"
 #include "SkRandom.h"
+#include "SkRSXform.h"
 
 void GrDrawAtlasBatch::initBatchTracker(const GrPipelineOptimizations& opt) {
     // Handle any color overrides
@@ -24,42 +25,27 @@ void GrDrawAtlasBatch::initBatchTracker(const GrPipelineOptimizations& opt) {
     fCoverageIgnored = !opt.readsCoverage();
 }
 
-static const GrGeometryProcessor* set_vertex_attributes(bool hasLocalCoords,
-                                                        bool hasColors,
-                                                        int* colorOffset,
-                                                        int* texOffset,
+static const GrGeometryProcessor* set_vertex_attributes(bool hasColors,
                                                         GrColor color,
                                                         const SkMatrix& viewMatrix,
                                                         bool coverageIgnored) {
     using namespace GrDefaultGeoProcFactory;
-    *texOffset = -1;
-    *colorOffset = -1;
     Color gpColor(color);
     if (hasColors) {
         gpColor.fType = Color::kAttribute_Type;
     }
     
     Coverage coverage(coverageIgnored ? Coverage::kNone_Type : Coverage::kSolid_Type);
-    LocalCoords localCoords(hasLocalCoords ? LocalCoords::kHasExplicit_Type :
-                            LocalCoords::kUsePosition_Type);
-    if (hasLocalCoords && hasColors) {
-        *colorOffset = sizeof(SkPoint);
-        *texOffset = sizeof(SkPoint) + sizeof(GrColor);
-    } else if (hasLocalCoords) {
-        *texOffset = sizeof(SkPoint);
-    } else if (hasColors) {
-        *colorOffset = sizeof(SkPoint);
-    }
+    LocalCoords localCoords(LocalCoords::kHasExplicit_Type);
     return GrDefaultGeoProcFactory::Create(gpColor, coverage, localCoords, viewMatrix);
 }
 
 void GrDrawAtlasBatch::generateGeometry(GrBatchTarget* batchTarget) {
-    int colorOffset = -1, texOffset = -1;
     // Setup geometry processor
-    SkAutoTUnref<const GrGeometryProcessor> gp(
-                                set_vertex_attributes(true, this->hasColors(), &colorOffset,
-                                                      &texOffset, this->color(), this->viewMatrix(),
-                                                      this->coverageIgnored()));
+    SkAutoTUnref<const GrGeometryProcessor> gp(set_vertex_attributes(this->hasColors(),
+                                                                     this->color(),
+                                                                     this->viewMatrix(),
+                                                                     this->coverageIgnored()));
     
     batchTarget->initDraw(gp, this->pipeline());
     
@@ -69,57 +55,106 @@ void GrDrawAtlasBatch::generateGeometry(GrBatchTarget* batchTarget) {
              + (this->hasColors() ? sizeof(GrColor) : 0));
     
     QuadHelper helper;
-    int numQuads = this->vertexCount()/4;
+    int numQuads = this->quadCount();
     void* verts = helper.init(batchTarget, vertexStride, numQuads);
     if (!verts) {
         SkDebugf("Could not allocate vertices\n");
         return;
     }
     
-    int vertexOffset = 0;
+    uint8_t* vertPtr = reinterpret_cast<uint8_t*>(verts);
     for (int i = 0; i < instanceCount; i++) {
         const Geometry& args = fGeoData[i];
         
-        for (int j = 0; j < args.fPositions.count(); ++j) {
-            *((SkPoint*)verts) = args.fPositions[j];
-            if (this->hasColors()) {
-                *(GrColor*)((intptr_t)verts + colorOffset) = args.fColors[j];
-            }
-            *(SkPoint*)((intptr_t)verts + texOffset) = args.fLocalCoords[j];
-            verts = (void*)((intptr_t)verts + vertexStride);
-            vertexOffset++;
-        }
+        size_t allocSize = args.fVerts.count();
+        memcpy(vertPtr, args.fVerts.begin(), allocSize);
+        vertPtr += allocSize;
     }
     helper.issueDraw(batchTarget);
 }
 
 GrDrawAtlasBatch::GrDrawAtlasBatch(const Geometry& geometry, const SkMatrix& viewMatrix,
-                                   const SkPoint* positions, int vertexCount,
-                                   const GrColor* colors, const SkPoint* localCoords,
-                                   const SkRect& bounds) {
+                                   int spriteCount, const SkRSXform* xforms, const SkRect* rects,
+                                   const SkColor* colors) {
     this->initClassID<GrDrawAtlasBatch>();
-    SkASSERT(positions);
-    SkASSERT(localCoords);
+    SkASSERT(xforms);
+    SkASSERT(rects);
     
     fViewMatrix = viewMatrix;
     Geometry& installedGeo = fGeoData.push_back(geometry);
     
-    installedGeo.fPositions.append(vertexCount, positions);
-    
+    // figure out stride and offsets

[robertphillips, 2015/08/11 15:32:05]

// Order within the vertex is: position [color] texture ?

[jvanverth1, 2015/08/11 20:22:55]

Done.

+    SkColor dummyColor;
+    const SkColor* colorPtr = &dummyColor;
+    size_t colorStep = 0;
+    size_t colorOffset = sizeof(SkPoint);
+    size_t texOffset = sizeof(SkPoint);

[robertphillips, 2015/08/11 15:32:05]

stride -> vertexStride ?

[jvanverth1, 2015/08/11 20:22:55]

Done.

+    size_t stride = 2*sizeof(SkPoint);
+    fHasColors = false;
     if (colors) {
-        installedGeo.fColors.append(vertexCount, colors);
+        colorOffset = sizeof(SkPoint);
+        texOffset += sizeof(GrColor);
+        stride += sizeof(GrColor);
+        colorPtr = colors;
+        colorStep = sizeof(GrColor);
         fHasColors = true;
-    } else {
-        fHasColors = false;
     }
     
-    installedGeo.fLocalCoords.append(vertexCount, localCoords);
-    fVertexCount = vertexCount;
+    // compute buffer size and alloc buffer
+    fQuadCount = spriteCount;
+    size_t allocSize = 4*stride*spriteCount;
+    installedGeo.fVerts.reset(allocSize);
+    uint8_t* currVertex = installedGeo.fVerts.begin();
+    
+    SkRect bounds;

[robertphillips, 2015/08/11 15:32:05]

Hmmm ... isn't this going to force the bounds to always include the origin?

[jvanverth1, 2015/08/11 20:22:55]

Done.

+    sk_bzero(&bounds, sizeof(SkRect));
+    for (int spriteIndex = 0; spriteIndex < spriteCount; ++spriteIndex) {
+        // transform rect
+        SkPoint quad[4];
+        xforms[spriteIndex].toQuad(rects[spriteIndex].width(), rects[spriteIndex].height(), quad);
+ 
+        // copy to verts
+        *(reinterpret_cast<SkPoint*>(currVertex)) = quad[0];

[robertphillips, 2015/08/11 15:32:05]

Hmmm ... could we have a second separate loop if we have colors? It is kind of
odd to keep copying over the same color just to overwrite it.

[jvanverth1, 2015/08/11 20:22:55]

Talked to mtlklein and revised to make it cleaner. No perf difference that I can
see.

+        *(reinterpret_cast<SkColor*>(currVertex+colorOffset)) = *colorPtr;
+        *(reinterpret_cast<SkPoint*>(currVertex+texOffset)) =
+                            SkPoint::Make(rects[spriteIndex].fLeft, rects[spriteIndex].fTop);
+        bounds.growToInclude(quad[0].fX, quad[0].fY);
+        currVertex += stride;
+
+        *(reinterpret_cast<SkPoint*>(currVertex)) = quad[1];
+        *(reinterpret_cast<SkColor*>(currVertex+colorOffset)) = *colorPtr;
+        *(reinterpret_cast<SkPoint*>(currVertex+texOffset)) =
+                            SkPoint::Make(rects[spriteIndex].fRight, rects[spriteIndex].fTop);
+        bounds.growToInclude(quad[1].fX, quad[1].fY);
+        currVertex += stride;
+        
+        *(reinterpret_cast<SkPoint*>(currVertex)) = quad[2];
+        *(reinterpret_cast<SkColor*>(currVertex+colorOffset)) = *colorPtr;
+        *(reinterpret_cast<SkPoint*>(currVertex+texOffset)) =
+                            SkPoint::Make(rects[spriteIndex].fRight, rects[spriteIndex].fBottom);
+        bounds.growToInclude(quad[2].fX, quad[2].fY);
+        currVertex += stride;
+        
+        *(reinterpret_cast<SkPoint*>(currVertex)) = quad[3];
+        *(reinterpret_cast<SkColor*>(currVertex+colorOffset)) = *colorPtr;
+        *(reinterpret_cast<SkPoint*>(currVertex+texOffset)) =
+                            SkPoint::Make(rects[spriteIndex].fLeft, rects[spriteIndex].fBottom);
+        bounds.growToInclude(quad[3].fX, quad[3].fY);
+        currVertex += stride;
+        
+        // step to next sprite
+        colorPtr += colorStep;
+    }
     
+    viewMatrix.mapRect(&bounds);
+    // outset for a half pixel in each direction to account for snapping in non-AA case
+    bounds.outset(0.5f, 0.5f);
     this->setBounds(bounds);
 }
  
 bool GrDrawAtlasBatch::onCombineIfPossible(GrBatch* t) {
+    return false;

[jvanverth1, 2015/08/11 14:49:50]

Not sure if this should be here or not.

[robertphillips, 2015/08/11 15:01:06]

I think we should just let it batch or add a big comment saying "We don't think
this will ever batch in the real world so we just ignore the possibility here".

[jvanverth1, 2015/08/11 15:27:43]

Re-enabled it.

+    
     if (!this->pipeline()->isEqual(*t->pipeline())) {
         return false;
     }
@@ -143,7 +178,7 @@ bool GrDrawAtlasBatch::onCombineIfPossible(GrBatch* t) {
         fColor = GrColor_ILLEGAL;
     }
     fGeoData.push_back_n(that->geoData()->count(), that->geoData()->begin());
-    fVertexCount += that->vertexCount();
+    fQuadCount += that->quadCount();
     
     this->joinBounds(that->bounds());
     return true;
@@ -151,62 +186,66 @@ bool GrDrawAtlasBatch::onCombineIfPossible(GrBatch* t) {
 
 #ifdef GR_TEST_UTILS
 

[robertphillips, 2015/08/11 15:32:05]

Do we need to pass in the width & height of the texture so we can randomize the
anchor points too?

[jvanverth1, 2015/08/11 20:22:55]

Added random anchor points. It doesn't affect the texture coordinates, so
texture size shouldn't matter.

-static SkPoint random_point(SkRandom* random, SkScalar min, SkScalar max) {
-    SkPoint p;
-    p.fX = random->nextRangeScalar(min, max);
-    p.fY = random->nextRangeScalar(min, max);
-    return p;
+static SkRSXform random_xform(SkRandom* random) {
+    static const SkScalar kMinExtent = -100.f;
+    static const SkScalar kMaxExtent = 100.f;
+    static const SkScalar kMinScale = 0.1f;
+    static const SkScalar kMaxScale = 100.f;
+    static const SkScalar kMinRotate = -SK_ScalarPI;
+    static const SkScalar kMaxRotate = SK_ScalarPI;
+
+    SkRSXform xform = SkRSXform::MakeFromRadians(random->nextRangeScalar(kMinScale, kMaxScale),
+                                                 random->nextRangeScalar(kMinRotate, kMaxRotate),
+                                                 random->nextRangeScalar(kMinExtent, kMaxExtent),
+                                                 random->nextRangeScalar(kMinExtent, kMaxExtent),
+                                                 0.0, 0.0f);
+    return xform;
+}
+
+static SkRect random_texRect(SkRandom* random) {
+    static const SkScalar kMinCoord = 0.0f;
+    static const SkScalar kMaxCoord = 1024.f;
+    SkRect texRect = SkRect::MakeLTRB(random->nextRangeScalar(kMinCoord, kMaxCoord),
+                                      random->nextRangeScalar(kMinCoord, kMaxCoord),
+                                      random->nextRangeScalar(kMinCoord, kMaxCoord),
+                                      random->nextRangeScalar(kMinCoord, kMaxCoord));

[robertphillips, 2015/08/11 15:32:05]

Do we need a sort call here ?

[jvanverth1, 2015/08/11 20:22:54]

Done.

+    return texRect;
 }
 

[robertphillips, 2015/08/11 15:32:05]

Seems like 'count' should be an int.

[jvanverth1, 2015/08/11 20:22:55]

Done.

-static void randomize_params(size_t count, SkScalar min, SkScalar max,
-                             SkRandom* random,
-                             SkTArray<SkPoint>* positions,
-                             SkTArray<SkPoint>* texCoords,
+static void randomize_params(size_t count, SkRandom* random,
+                             SkTArray<SkRSXform>* xforms,
+                             SkTArray<SkRect>* texRects,
                              SkTArray<GrColor>* colors, bool hasColors) {
     for (uint32_t v = 0; v < count; v++) {
-        positions->push_back(random_point(random, min, max));
-        texCoords->push_back(random_point(random, min, max));
+        xforms->push_back(random_xform(random));
+        texRects->push_back(random_texRect(random));
         if (hasColors) {
             colors->push_back(GrRandomColor(random));
         }
     }
 }
 
-
 BATCH_TEST_DEFINE(GrDrawAtlasBatch) {
     uint32_t spriteCount = random->nextRangeU(1, 100);
     

[robertphillips, 2015/08/11 15:32:05]

(spriteCount) on xforms, texRects & colors ?

[jvanverth1, 2015/08/11 20:22:55]

Done.

-    // TODO make 'sensible' indexbuffers
-    SkTArray<SkPoint> positions;
-    SkTArray<SkPoint> texCoords;
+    SkTArray<SkRSXform> xforms;
+    SkTArray<SkRect> texRects;
     SkTArray<GrColor> colors;
     
     bool hasColors = random->nextBool();
     
-    uint32_t vertexCount = 4*spriteCount;
-    
-    static const SkScalar kMinVertExtent = -100.f;
-    static const SkScalar kMaxVertExtent = 100.f;
-    randomize_params(vertexCount, kMinVertExtent, kMaxVertExtent,
+    randomize_params(spriteCount,
                      random,
-                     &positions,
-                     &texCoords,
+                     &xforms,
+                     &texRects,
                      &colors, hasColors);
     
     SkMatrix viewMatrix = GrTest::TestMatrix(random);
-    SkRect bounds;
-    SkDEBUGCODE(bool result = ) bounds.setBoundsCheck(positions.begin(), vertexCount);
-    SkASSERT(result);
-    
-    viewMatrix.mapRect(&bounds);
     
     GrDrawAtlasBatch::Geometry geometry;
     geometry.fColor = GrRandomColor(random);
-    return GrDrawAtlasBatch::Create(geometry, viewMatrix,
-                                    positions.begin(), vertexCount,
-                                    colors.begin(),
-                                    texCoords.begin(),
-                                    bounds);
+    return GrDrawAtlasBatch::Create(geometry, viewMatrix, spriteCount, xforms.begin(),
+                                    texRects.begin(), colors.begin());
 }
 
 #endif