Isolate VertexBuilder from GrGLShaderBuilder

Isolate VertexBuilder from GrGLShaderBuilder

Adds a nested class to GrGLShaderBuilder called VertexBuilder. Now
GrGLShaderBuilder can only modify the fragment shader directly. In
order to modify the vertex shader, the client code needs to call
getVertexShader, which will return null for vertex-less shaders.

Committed: http://code.google.com/p/skia/source/detail?r=11046

[Chris Dalton, 2013-08-30 16:13:39]

PTAL

[bsalomon, 2013-08-30 17:59:17]

Mostly looks good. Some comments (and requests for code comments) inline. The
first two style nits apply a bunch of places, I just noted them once.

https://codereview.chromium.org/23754003/diff/3001/src/effects/gradients/SkTw...
File src/effects/gradients/SkTwoPointConicalGradient.cpp (right):

https://codereview.chromium.org/23754003/diff/3001/src/effects/gradients/SkTw...
src/effects/gradients/SkTwoPointConicalGradient.cpp:509:
GrGLShaderBuilder::VertexBuilder* conical2BBuilder =
Could we call this vsBuilder, vertexBuilder, or vBuilder? Seems clearer.

https://codereview.chromium.org/23754003/diff/3001/src/effects/gradients/SkTw...
src/effects/gradients/SkTwoPointConicalGradient.cpp:511: if (conical2BBuilder) {
Not really my pref, but at least in Gr code we tend to say

if (NULL != conical2BBuilder)

rather than

if (conical2BBuilder)

to emphasize that it's a ptr not a bool.

https://codereview.chromium.org/23754003/diff/3001/src/gpu/gl/GrGLShaderBuild...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/23754003/diff/3001/src/gpu/gl/GrGLShaderBuild...
src/gpu/gl/GrGLShaderBuilder.cpp:110: fVertexBuilder.reset(new
VertexBuilder(this, desc));
SkNEW_ARGS(VertexBuilder, (this, desc));

https://codereview.chromium.org/23754003/diff/3001/src/gpu/gl/GrGLShaderBuild...
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.chromium.org/23754003/diff/3001/src/gpu/gl/GrGLShaderBuild...
src/gpu/gl/GrGLShaderBuilder.h:278: class VertexBuilder {
/** Helper class used to build the vertex shader...*/

https://codereview.chromium.org/23754003/diff/3001/src/gpu/gl/GrGLShaderBuild...
src/gpu/gl/GrGLShaderBuilder.h:368: VertexBuilder* getVertexBuilder() const {
return fVertexBuilder.get(); }
\n
/** Gets the vertex builder that is used to construct the vertex shader. It may
be NULL if.....*/

[Chris Dalton, 2013-08-30 19:10:30]

Hopefully this addresses all the comments.

Sorry it had to rebase  ...

https://codereview.chromium.org/23754003/diff/3001/src/effects/gradients/SkTw...
File src/effects/gradients/SkTwoPointConicalGradient.cpp (right):

https://codereview.chromium.org/23754003/diff/3001/src/effects/gradients/SkTw...
src/effects/gradients/SkTwoPointConicalGradient.cpp:509:
GrGLShaderBuilder::VertexBuilder* conical2BBuilder =
On 2013/08/30 17:59:17, bsalomon wrote:
> Could we call this vsBuilder, vertexBuilder, or vBuilder? Seems clearer.

Done.

https://codereview.chromium.org/23754003/diff/3001/src/effects/gradients/SkTw...
src/effects/gradients/SkTwoPointConicalGradient.cpp:511: if (conical2BBuilder) {
On 2013/08/30 17:59:17, bsalomon wrote:
> Not really my pref, but at least in Gr code we tend to say
> 
> if (NULL != conical2BBuilder)
> 
> rather than
> 
> if (conical2BBuilder)
> 
> to emphasize that it's a ptr not a bool.

Done.

https://codereview.chromium.org/23754003/diff/3001/src/gpu/gl/GrGLShaderBuild...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/23754003/diff/3001/src/gpu/gl/GrGLShaderBuild...
src/gpu/gl/GrGLShaderBuilder.cpp:110: fVertexBuilder.reset(new
VertexBuilder(this, desc));
On 2013/08/30 17:59:17, bsalomon wrote:
> SkNEW_ARGS(VertexBuilder, (this, desc));

Done.

https://codereview.chromium.org/23754003/diff/3001/src/gpu/gl/GrGLShaderBuild...
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.chromium.org/23754003/diff/3001/src/gpu/gl/GrGLShaderBuild...
src/gpu/gl/GrGLShaderBuilder.h:278: class VertexBuilder {
On 2013/08/30 17:59:17, bsalomon wrote:
> /** Helper class used to build the vertex shader...*/

Done.

[bsalomon, 2013-08-30 19:14:05]

On 2013/08/30 19:10:30, cdalton wrote:
> Hopefully this addresses all the comments.
> 
> Sorry it had to rebase  ...
> 
>
https://codereview.chromium.org/23754003/diff/3001/src/effects/gradients/SkTw...
> File src/effects/gradients/SkTwoPointConicalGradient.cpp (right):
> 
>
https://codereview.chromium.org/23754003/diff/3001/src/effects/gradients/SkTw...
> src/effects/gradients/SkTwoPointConicalGradient.cpp:509:
> GrGLShaderBuilder::VertexBuilder* conical2BBuilder =
> On 2013/08/30 17:59:17, bsalomon wrote:
> > Could we call this vsBuilder, vertexBuilder, or vBuilder? Seems clearer.
> 
> Done.
> 
>
https://codereview.chromium.org/23754003/diff/3001/src/effects/gradients/SkTw...
> src/effects/gradients/SkTwoPointConicalGradient.cpp:511: if (conical2BBuilder)
{
> On 2013/08/30 17:59:17, bsalomon wrote:
> > Not really my pref, but at least in Gr code we tend to say
> > 
> > if (NULL != conical2BBuilder)
> > 
> > rather than
> > 
> > if (conical2BBuilder)
> > 
> > to emphasize that it's a ptr not a bool.
> 
> Done.
> 
>
https://codereview.chromium.org/23754003/diff/3001/src/gpu/gl/GrGLShaderBuild...
> File src/gpu/gl/GrGLShaderBuilder.cpp (right):
> 
>
https://codereview.chromium.org/23754003/diff/3001/src/gpu/gl/GrGLShaderBuild...
> src/gpu/gl/GrGLShaderBuilder.cpp:110: fVertexBuilder.reset(new
> VertexBuilder(this, desc));
> On 2013/08/30 17:59:17, bsalomon wrote:
> > SkNEW_ARGS(VertexBuilder, (this, desc));
> 
> Done.
> 
>
https://codereview.chromium.org/23754003/diff/3001/src/gpu/gl/GrGLShaderBuild...
> File src/gpu/gl/GrGLShaderBuilder.h (right):
> 
>
https://codereview.chromium.org/23754003/diff/3001/src/gpu/gl/GrGLShaderBuild...
> src/gpu/gl/GrGLShaderBuilder.h:278: class VertexBuilder {
> On 2013/08/30 17:59:17, bsalomon wrote:
> > /** Helper class used to build the vertex shader...*/
> 
> Done.

Did you happen to get an error from gcl upload?

[Chris Dalton, 2013-08-30 19:35:26]

Try that

[bsalomon, 2013-08-30 19:38:59]

lgtm

[commit-bot: I haz the power, 2013-08-30 19:39:08]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/23754003/34001

[commit-bot: I haz the power, 2013-08-30 19:39:11]

Failed to apply patch for src/gpu/gl/GrGLProgram.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file src/gpu/gl/GrGLProgram.cpp
  Hunk #4 FAILED at 401.
  Hunk #6 FAILED at 477.
  2 out of 10 hunks FAILED -- saving rejects to file
src/gpu/gl/GrGLProgram.cpp.rej

Patch:       src/gpu/gl/GrGLProgram.cpp
Index: src/gpu/gl/GrGLProgram.cpp
diff --git a/src/gpu/gl/GrGLProgram.cpp b/src/gpu/gl/GrGLProgram.cpp
index
a1c4fd5d76fc591c9a7b07a88bcd5362c53f84ba..540ebcaeb32ae4ac1e6fe535327c941d85486723
100644
--- a/src/gpu/gl/GrGLProgram.cpp
+++ b/src/gpu/gl/GrGLProgram.cpp
@@ -222,10 +222,12 @@ void add_color_filter(GrGLShaderBuilder* builder,
 GrSLConstantVec GrGLProgram::genInputColor(GrGLShaderBuilder* builder,
SkString* inColor) {
     switch (fDesc.getHeader().fColorInput) {
         case GrGLProgramDesc::kAttribute_ColorInput: {
-            builder->addAttribute(kVec4f_GrSLType, COL_ATTR_NAME);
+            GrGLShaderBuilder::VertexBuilder* vertexBuilder =
builder->getVertexBuilder();
+            SkASSERT(NULL != vertexBuilder);
+            vertexBuilder->addAttribute(kVec4f_GrSLType, COL_ATTR_NAME);
             const char *vsName, *fsName;
-            builder->addVarying(kVec4f_GrSLType, "Color", &vsName, &fsName);
-            builder->vsCodeAppendf("\t%s = " COL_ATTR_NAME ";\n", vsName);
+            vertexBuilder->addVarying(kVec4f_GrSLType, "Color", &vsName,
&fsName);
+            vertexBuilder->vsCodeAppendf("\t%s = " COL_ATTR_NAME ";\n",
vsName);
             *inColor = fsName;
             return kNone_GrSLConstantVec;
         }
@@ -251,10 +253,12 @@ GrSLConstantVec
GrGLProgram::genInputColor(GrGLShaderBuilder* builder, SkString*
 GrSLConstantVec GrGLProgram::genInputCoverage(GrGLShaderBuilder* builder,
SkString* inCoverage) {
     switch (fDesc.getHeader().fCoverageInput) {
         case GrGLProgramDesc::kAttribute_ColorInput: {
-            builder->addAttribute(kVec4f_GrSLType, COV_ATTR_NAME);
+            GrGLShaderBuilder::VertexBuilder* vertexBuilder =
builder->getVertexBuilder();
+            SkASSERT(NULL != vertexBuilder);
+            vertexBuilder->addAttribute(kVec4f_GrSLType, COV_ATTR_NAME);
             const char *vsName, *fsName;
-            builder->addVarying(kVec4f_GrSLType, "Coverage", &vsName, &fsName);
-            builder->vsCodeAppendf("\t%s = " COV_ATTR_NAME ";\n", vsName);
+            vertexBuilder->addVarying(kVec4f_GrSLType, "Coverage", &vsName,
&fsName);
+            vertexBuilder->vsCodeAppendf("\t%s = " COV_ATTR_NAME ";\n",
vsName);
             *inCoverage = fsName;
             return kNone_GrSLConstantVec;
         }
@@ -278,28 +282,28 @@ GrSLConstantVec
GrGLProgram::genInputCoverage(GrGLShaderBuilder* builder, SkStri
     }
 }
 
-void GrGLProgram::genGeometryShader(GrGLShaderBuilder* builder) const {
+void GrGLProgram::genGeometryShader(GrGLShaderBuilder::VertexBuilder*
vertexBuilder) const {
 #if GR_GL_EXPERIMENTAL_GS
     // TODO: The builder should add all this glue code.
     if (fDesc.getHeader().fExperimentalGS) {
         SkASSERT(fContext.info().glslGeneration() >= k150_GrGLSLGeneration);
-        builder->fGSHeader.append("layout(triangles) in;\n"
-                                   "layout(triangle_strip, max_vertices = 6)
out;\n");
-        builder->gsCodeAppend("\tfor (int i = 0; i < 3; ++i) {\n"
-                              "\t\tgl_Position = gl_in[i].gl_Position;\n");
+        vertexBuilder->fGSHeader.append("layout(triangles) in;\n"
+                                        "layout(triangle_strip, max_vertices =
6) out;\n");
+        vertexBuilder->gsCodeAppend("\tfor (int i = 0; i < 3; ++i) {\n"
+                                    "\t\tgl_Position =
gl_in[i].gl_Position;\n");
         if (fDesc.getHeader().fEmitsPointSize) {
-            builder->gsCodeAppend("\t\tgl_PointSize = 1.0;\n");
+            vertexBuilder->gsCodeAppend("\t\tgl_PointSize = 1.0;\n");
         }
-        SkASSERT(builder->fGSInputs.count() == builder->fGSOutputs.count());
-        int count = builder->fGSInputs.count();
+        SkASSERT(vertexBuilder->fGSInputs.count() ==
vertexBuilder->fGSOutputs.count());
+        int count = vertexBuilder->fGSInputs.count();
         for (int i = 0; i < count; ++i) {
-            builder->gsCodeAppendf("\t\t%s = %s[i];\n",
-                                   builder->fGSOutputs[i].getName().c_str(),
-                                   builder->fGSInputs[i].getName().c_str());
+            vertexBuilder->gsCodeAppendf("\t\t%s = %s[i];\n",
+                                        
vertexBuilder->fGSOutputs[i].getName().c_str(),
+                                        
vertexBuilder->fGSInputs[i].getName().c_str());
         }
-        builder->gsCodeAppend("\t\tEmitVertex();\n"
-                              "\t}\n"
-                              "\tEndPrimitive();\n");
+        vertexBuilder->gsCodeAppend("\t\tEmitVertex();\n"
+                                    "\t}\n"
+                                    "\tEndPrimitive();\n");
     }
 #endif
 }
@@ -397,31 +401,34 @@ void expand_known_value4f(SkString* string,
GrSLConstantVec vec) {
 // compiles all the shaders from builder and stores the shader IDs
 bool GrGLProgram::compileShaders(const GrGLShaderBuilder& builder) {
 
-    SkString shader;
-
-    builder.vsGetShader(&shader);
-    if (c_PrintShaders) {
-        GrPrintf(shader.c_str());
-        GrPrintf("\n");
-    }
+    SkASSERT(!fVShaderID);
+    SkASSERT(!fGShaderID);
+    SkASSERT(!fFShaderID);
 
-    if (!(fVShaderID = compile_shader(fContext, GR_GL_VERTEX_SHADER, shader)))
{
-        return false;
-    }
-
-    fGShaderID = 0;
-#if GR_GL_EXPERIMENTAL_GS
-    if (fDesc.getHeader().fExperimentalGS) {
-        builder.gsGetShader(&shader);
+    SkString shader;
+    if (GrGLShaderBuilder::VertexBuilder* vertexBuilder =
builder.getVertexBuilder()) {
+        vertexBuilder->vsGetShader(&shader);
         if (c_PrintShaders) {
             GrPrintf(shader.c_str());
             GrPrintf("\n");
         }
-        if (!(fGShaderID = compile_shader(fContext, GR_GL_GEOMETRY_SHADER,
shader))) {
+        if (!(fVShaderID = compile_shader(fContext, GR_GL_VERTEX_SHADER,
shader))) {
             return false;
         }
-    }
+
+#if GR_GL_EXPERIMENTAL_GS
+        if (fDesc.getHeader().fExperimentalGS) {
+            vertexBuilder->gsGetShader(&shader);
+            if (c_PrintShaders) {
+                GrPrintf(shader.c_str());
+                GrPrintf("\n");
+            }
+            if (!(fGShaderID = compile_shader(fContext, GR_GL_GEOMETRY_SHADER,
shader))) {
+                return false;
+            }
+        }
 #endif
+    }
 
     builder.fsGetShader(&shader);
     if (c_PrintShaders) {
@@ -441,7 +448,28 @@ bool GrGLProgram::genProgram(const GrEffectStage*
colorStages[],
 
     const GrGLProgramDesc::KeyHeader& header = fDesc.getHeader();
 
-    GrGLShaderBuilder builder(fContext.info(), fUniformManager, fDesc);
+    bool needsVertexShader = true;
+
+    GrGLShaderBuilder builder(fContext.info(), fUniformManager, fDesc,
needsVertexShader);
+
+    if (GrGLShaderBuilder::VertexBuilder* vertexBuilder =
builder.getVertexBuilder()) {
+        const char* viewMName;
+        fUniformHandles.fViewMatrixUni =
builder.addUniform(GrGLShaderBuilder::kVertex_Visibility,
+                                                            kMat33f_GrSLType,
"ViewM", &viewMName);
+
+        vertexBuilder->vsCodeAppendf("\tvec3 pos3 = %s * vec3(%s, 1);\n"
+                                     "\tgl_Position = vec4(pos3.xy, 0,
pos3.z);\n",
+                                     viewMName,
vertexBuilder->positionAttribute().c_str());
+
+        // we output point size in the GS if present
+        if (header.fEmitsPointSize
+#if GR_GL_EXPERIMENTAL_GS
+            && !header.fExperimentalGS
+#endif
+            ) {
+            vertexBuilder->vsCodeAppend("\tgl_PointSize = 1.0;\n");
+        }
+    }
 
     // the dual source output has no canonical var name, have to
     // declare an output, which is incompatible with gl_FragColor/gl_FragData.
@@ -452,31 +480,13 @@ bool GrGLProgram::genProgram(const GrEffectStage*
colorStages[],
                                                   
declared_color_output_name(),
                                                    &colorOutput);
     if (isColorDeclared) {
-        builder.fFSOutputs.push_back(colorOutput);
+        builder.fsOutputAppend(colorOutput);
     }
 
-    const char* viewMName;
-    fUniformHandles.fViewMatrixUni =
builder.addUniform(GrGLShaderBuilder::kVertex_Visibility,
-                                                        kMat33f_GrSLType,
"ViewM", &viewMName);
-
-
-    builder.vsCodeAppendf("\tvec3 pos3 = %s * vec3(%s, 1);\n"
-                          "\tgl_Position = vec4(pos3.xy, 0, pos3.z);\n",
-                          viewMName,
builder.positionAttribute().getName().c_str());
-
     // incoming color to current stage being processed.
     SkString inColor;
     GrSLConstantVec knownColorValue = this->genInputColor(&builder, &inColor);
 
-    // we output point size in the GS if present
-    if (header.fEmitsPointSize
-#if GR_GL_EXPERIMENTAL_GS
-        && !header.fExperimentalGS
-#endif
-        ) {
-        builder.vsCodeAppend("\tgl_PointSize = 1.0;\n");
-    }
-
     // Get the coeffs for the Mode-based color filter, determine if color is
needed.
     SkXfermode::Coeff colorCoeff;
     SkXfermode::Coeff filterColorCoeff;
@@ -567,9 +577,9 @@ bool GrGLProgram::genProgram(const GrEffectStage*
colorStages[],
     GrGLProgramDesc::CoverageOutput coverageOutput =
         static_cast<GrGLProgramDesc::CoverageOutput>(header.fCoverageOutput);
     if (GrGLProgramDesc::CoverageOutputUsesSecondaryOutput(coverageOutput)) {
-        builder.fFSOutputs.push_back().set(kVec4f_GrSLType,
-                                           GrGLShaderVar::kOut_TypeModifier,
-                                           dual_source_output_name())…
(message too large)

[commit-bot: I haz the power, 2013-08-30 19:52:26]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/23754003/34001

[commit-bot: I haz the power, 2013-08-30 20:17:39]

Change committed as 11046