Start on simplifying generateGeometry() overrides

Start on simplifying generateGeometry() overrides

Committed: https://skia.googlesource.com/skia/+/f28381c6866cad92af8ebe5b9d2db074613b1963

Committed: https://skia.googlesource.com/skia/+/b5238a7571c243ba4a154a62575570c3078b3741

[bsalomon, 2015-05-02 21:19:41]

bsalomon@google.com changed reviewers:
+ joshualitt@google.com

[bsalomon, 2015-05-02 21:19:42]

This is an (incomplete!) attempt to remove some redundant code from
GrBatch::generateGeomety() overrides. It attempts to reduce the amount of boiler
plate code for drawing indexed instances, especially for quads.

The real heart of the changes is in GrDrawTarget.h and GrBatch.h/cpp.

Ignore the changes to DFPR and BatchTextContext. I just commented out enough to
get it compiling. These are yet to be tackled.

Is this a good idea?

[bsalomon, 2015-05-03 13:47:36]

On 2015/05/02 21:19:42, bsalomon wrote:
> This is an (incomplete!) attempt to remove some redundant code from
> GrBatch::generateGeomety() overrides. It attempts to reduce the amount of
boiler
> plate code for drawing indexed instances, especially for quads.
> 
> The real heart of the changes is in GrDrawTarget.h and GrBatch.h/cpp.
> 
> Ignore the changes to DFPR and BatchTextContext. I just commented out enough
to
> get it compiling. These are yet to be tackled.
> 
> Is this a good idea?

I completed and extended it... I think something like this is a good idea given
it removes ~300LOC.

[joshualitt, 2015-05-04 13:48:28]

I like this change, but I'm not really in love with the .init methods which take
ten arguments.  However, they are probably unavoidable to make sure we init
these draws with all of the necessary arguments.  Some questions:

https://codereview.chromium.org/1122673002/diff/130001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

https://codereview.chromium.org/1122673002/diff/130001/src/gpu/GrDrawTarget.h...
src/gpu/GrDrawTarget.h:230: class DrawInfo {
After this CL maybe we move this guy to his own header?

https://codereview.chromium.org/1122673002/diff/130001/src/gpu/GrDrawTarget.h...
src/gpu/GrDrawTarget.h:268: fIndexBuffer.reset(SkSafeRef(indexBuffer));
Why isn't this SkRef?

https://codereview.chromium.org/1122673002/diff/130001/src/gpu/effects/GrDash...
File src/gpu/effects/GrDashingEffect.cpp (right):

https://codereview.chromium.org/1122673002/diff/130001/src/gpu/effects/GrDash...
src/gpu/effects/GrDashingEffect.cpp:598: rectIndex++;
is this move intentional?

[bsalomon, 2015-05-04 15:43:27]

https://codereview.chromium.org/1122673002/diff/130001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

https://codereview.chromium.org/1122673002/diff/130001/src/gpu/GrDrawTarget.h...
src/gpu/GrDrawTarget.h:230: class DrawInfo {
On 2015/05/04 13:48:28, joshualitt wrote:
> After this CL maybe we move this guy to his own header?


Definitely. This has nothing to do with GrDrawTarget.

https://codereview.chromium.org/1122673002/diff/130001/src/gpu/GrDrawTarget.h...
src/gpu/GrDrawTarget.h:268: fIndexBuffer.reset(SkSafeRef(indexBuffer));
On 2015/05/04 13:48:28, joshualitt wrote:
> Why isn't this SkRef?

Should be... hold over from an older version that didn't have separate inits for
indexed and non-indexed.

https://codereview.chromium.org/1122673002/diff/130001/src/gpu/effects/GrDash...
File src/gpu/effects/GrDashingEffect.cpp (right):

https://codereview.chromium.org/1122673002/diff/130001/src/gpu/effects/GrDash...
src/gpu/effects/GrDashingEffect.cpp:598: rectIndex++;
On 2015/05/04 13:48:28, joshualitt wrote:
> is this move intentional?

No, undone.

[joshualitt, 2015-05-04 15:49:40]

On 2015/05/04 15:43:27, bsalomon wrote:
> https://codereview.chromium.org/1122673002/diff/130001/src/gpu/GrDrawTarget.h
> File src/gpu/GrDrawTarget.h (right):
> 
>
https://codereview.chromium.org/1122673002/diff/130001/src/gpu/GrDrawTarget.h...
> src/gpu/GrDrawTarget.h:230: class DrawInfo {
> On 2015/05/04 13:48:28, joshualitt wrote:
> > After this CL maybe we move this guy to his own header?
> 
> 
> Definitely. This has nothing to do with GrDrawTarget.
> 
>
https://codereview.chromium.org/1122673002/diff/130001/src/gpu/GrDrawTarget.h...
> src/gpu/GrDrawTarget.h:268: fIndexBuffer.reset(SkSafeRef(indexBuffer));
> On 2015/05/04 13:48:28, joshualitt wrote:
> > Why isn't this SkRef?
> 
> Should be... hold over from an older version that didn't have separate inits
for
> indexed and non-indexed.
> 
>
https://codereview.chromium.org/1122673002/diff/130001/src/gpu/effects/GrDash...
> File src/gpu/effects/GrDashingEffect.cpp (right):
> 
>
https://codereview.chromium.org/1122673002/diff/130001/src/gpu/effects/GrDash...
> src/gpu/effects/GrDashingEffect.cpp:598: rectIndex++;
> On 2015/05/04 13:48:28, joshualitt wrote:
> > is this move intentional?
> 
> No, undone.

lgtm

[bsalomon, 2015-05-04 16:24:10]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2015-05-04 16:24:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1122673002/160001

[commit-bot: I haz the power, 2015-05-04 17:16:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-04 17:16:59]

Failed to apply patch for gm/beziereffects.cpp:
While running git apply --index -3 -p1;
  error: patch failed: gm/beziereffects.cpp:16
  Falling back to three-way merge...
  Applied patch to 'gm/beziereffects.cpp' with conflicts.
  U gm/beziereffects.cpp

Patch:       gm/beziereffects.cpp
Index: gm/beziereffects.cpp
diff --git a/gm/beziereffects.cpp b/gm/beziereffects.cpp
index
656842289867710a072b6ae1456d409d0cbd9a93..d543c149c715a0c53274c89f2a2aa53a9ef2406f
100644
--- a/gm/beziereffects.cpp
+++ b/gm/beziereffects.cpp
@@ -16,7 +16,6 @@
 #include "GrBufferAllocPool.h"
 #include "GrContext.h"
 #include "GrPathUtils.h"
-#include "GrResourceProvider.h"
 #include "GrTest.h"
 #include "GrTestBatch.h"
 #include "SkColorPriv.h"
@@ -67,25 +66,14 @@ private:
     }
 
     void onGenerateGeometry(GrBatchTarget* batchTarget, const GrPipeline*
pipeline) override {
-        SkAutoTUnref<const GrIndexBuffer> indexBuffer(
-            batchTarget->resourceProvider()->refQuadIndexBuffer());
-
+        QuadHelper helper;
         size_t vertexStride = this->geometryProcessor()->getVertexStride();
-        const GrVertexBuffer* vertexBuffer;
-        int firstVertex;
-        void* vertices = batchTarget->vertexPool()->makeSpace(vertexStride,
-                                                              kVertsPerCubic,
-                                                              &vertexBuffer,
-                                                              &firstVertex);
-
-        if (!vertices || !indexBuffer) {
-            SkDebugf("Could not allocate buffers\n");
+        SkASSERT(vertexStride == sizeof(Vertex));
+        Vertex* verts = reinterpret_cast<Vertex*>(helper.init(batchTarget,
vertexStride, 1));
+        if (!verts) {
             return;
         }
 
-        SkASSERT(vertexStride == sizeof(Vertex));
-        Vertex* verts = reinterpret_cast<Vertex*>(vertices);
-
         verts[0].fPosition.setRectFan(fGeometry.fBounds.fLeft,
fGeometry.fBounds.fTop,
                                       fGeometry.fBounds.fRight,
fGeometry.fBounds.fBottom,
                                       sizeof(Vertex));
@@ -94,16 +82,7 @@ private:
             verts[v].fKLM[1] = eval_line(verts[v].fPosition, fKlmEqs + 3,
fSign);
             verts[v].fKLM[2] = eval_line(verts[v].fPosition, fKlmEqs + 6, 1.f);
         }
-
-        GrDrawTarget::DrawInfo drawInfo;
-        drawInfo.setPrimitiveType(kTriangleFan_GrPrimitiveType);
-        drawInfo.setVertexBuffer(vertexBuffer);
-        drawInfo.setStartVertex(firstVertex);
-        drawInfo.setVertexCount(kVertsPerCubic);
-        drawInfo.setStartIndex(0);
-        drawInfo.setIndexCount(kIndicesPerCubic);
-        drawInfo.setIndexBuffer(indexBuffer);
-        batchTarget->draw(drawInfo);
+        helper.issueDraws(batchTarget);
     }
 
     Geometry fGeometry;
@@ -475,42 +454,19 @@ private:
     }
 
     void onGenerateGeometry(GrBatchTarget* batchTarget, const GrPipeline*
pipeline) override {
-        SkAutoTUnref<const GrIndexBuffer> indexBuffer(
-            batchTarget->resourceProvider()->refQuadIndexBuffer());
-
+        QuadHelper helper;
         size_t vertexStride = this->geometryProcessor()->getVertexStride();
-        const GrVertexBuffer* vertexBuffer;
-        int firstVertex;
-
-        void* vertices = batchTarget->vertexPool()->makeSpace(vertexStride,
-                                                              kVertsPerCubic,
-                                                              &vertexBuffer,
-                                                              &firstVertex);
-
-        if (!vertices || !indexBuffer) {
-            SkDebugf("Could not allocate buffers\n");
+        SkASSERT(vertexStride == sizeof(Vertex));
+        GrDrawTarget::DrawInfo drawInfo;
+        Vertex* verts = reinterpret_cast<Vertex*>(helper.init(batchTarget,
vertexStride, 1));
+        if (!verts) {
             return;
         }
-
-        SkASSERT(vertexStride == sizeof(Vertex));
-        Vertex* verts = reinterpret_cast<Vertex*>(vertices);
-
         verts[0].fPosition.setRectFan(fGeometry.fBounds.fLeft,
fGeometry.fBounds.fTop,
                                       fGeometry.fBounds.fRight,
fGeometry.fBounds.fBottom,
                                       sizeof(Vertex));
-
         fDevToUV.apply<4, sizeof(Vertex), sizeof(SkPoint)>(verts);
-
-
-        GrDrawTarget::DrawInfo drawInfo;
-        drawInfo.setPrimitiveType(kTriangles_GrPrimitiveType);
-        drawInfo.setVertexBuffer(vertexBuffer);
-        drawInfo.setStartVertex(firstVertex);
-        drawInfo.setVertexCount(kVertsPerCubic);
-        drawInfo.setStartIndex(0);
-        drawInfo.setIndexCount(kIndicesPerCubic);
-        drawInfo.setIndexBuffer(indexBuffer);
-        batchTarget->draw(drawInfo);
+        helper.issueDraws(batchTarget);
     }
 
     Geometry fGeometry;

[bsalomon, 2015-05-04 18:27:14]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2015-05-04 18:27:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1122673002/160001

[commit-bot: I haz the power, 2015-05-04 18:27:49]

Committed patchset #10 (id:160001) as
https://skia.googlesource.com/skia/+/f28381c6866cad92af8ebe5b9d2db074613b1963

[bsalomon, 2015-05-04 18:41:04]

A revert of this CL (patchset #10 id:160001) has been created in
https://codereview.chromium.org/1124633003/ by bsalomon@google.com.

The reason for reverting is: Breaking bots.

[bsalomon, 2015-05-04 21:25:50]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-05-04 21:25:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
joshualitt@google.com
Link to the patchset: https://codereview.chromium.org/1122673002/#ps200001
(title: "whitespace, remove debug return")

[commit-bot: I haz the power, 2015-05-04 21:26:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1122673002/200001

[commit-bot: I haz the power, 2015-05-05 14:49:54]

Committed patchset #12 (id:200001) as
https://skia.googlesource.com/skia/+/b5238a7571c243ba4a154a62575570c3078b3741