First step to moving vertex attributes to the geometryProcessor

First step to moving vertex attributes to the geometryProcessor

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/2dd1ae016d7f297b433c3ea3a771ef8e01657c1f

[joshualitt, 2014-11-26 16:18:20]

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

[joshualitt, 2014-11-26 16:18:21]

Brian, can you mini-review GrDefaultGeoProcFactory?  It is by far the most
complex GP so if you think it looks good, then the rest of the changes should be
very straightforward.  I will send it out to the team afterwards.

Introduction
For the most part this change is really just a huge simplification.  The biggest
changes are to ProgramBuilder / DrawState / OptState / VertexShaderBuilder. 
Otherwise, the rest of the change is just kind of cleanup from those changes.

[bsalomon, 2014-11-26 18:10:15]

Just looked at src/gpu/GrDefaultGeoProcFactory.cpp

https://codereview.chromium.org/761563002/diff/100001/src/gpu/GrDefaultGeoPro...
File src/gpu/GrDefaultGeoProcFactory.cpp (right):

https://codereview.chromium.org/761563002/diff/100001/src/gpu/GrDefaultGeoPro...
src/gpu/GrDefaultGeoProcFactory.cpp:27: if (hasColor && hasLocalCoord &&
hasCoverage) {
I wonder if this would read better as a switch/case?

https://codereview.chromium.org/761563002/diff/100001/src/gpu/GrDefaultGeoPro...
src/gpu/GrDefaultGeoProcFactory.cpp:88: const DefaultGeoProc& gp =
args.fGP.cast<DefaultGeoProc>();
Curious why pos/lc are different than cov/col (get from vsb rather than args).

[joshualitt, 2014-11-26 18:43:09]

On 2014/11/26 18:10:15, bsalomon wrote:
> Just looked at src/gpu/GrDefaultGeoProcFactory.cpp
> 
>
https://codereview.chromium.org/761563002/diff/100001/src/gpu/GrDefaultGeoPro...
> File src/gpu/GrDefaultGeoProcFactory.cpp (right):
> 
>
https://codereview.chromium.org/761563002/diff/100001/src/gpu/GrDefaultGeoPro...
> src/gpu/GrDefaultGeoProcFactory.cpp:27: if (hasColor && hasLocalCoord &&
> hasCoverage) {
> I wonder if this would read better as a switch/case?
not 100% convinced its actually more readable(both ways are pretty unreadable,
but I digress
> 
>
https://codereview.chromium.org/761563002/diff/100001/src/gpu/GrDefaultGeoPro...
> src/gpu/GrDefaultGeoProcFactory.cpp:88: const DefaultGeoProc& gp =
> args.fGP.cast<DefaultGeoProc>();
> Curious why pos/lc are different than cov/col (get from vsb rather than args).
I will make this change in a follow up CL.  I was planning on allowing emitArgs
to be nonconst so that the GP could write its own output values.  This would
allow the attributes to be directly without an intermediary value.  However,
maybe its better to just make them the same as color / coverage.

Feedback incorporated. Feel free to look at the rest of the CL.  I will add Jim,
Robert, and Greg

[joshualitt, 2014-11-26 18:44:34]

joshualitt@google.com changed reviewers:
+ egdaniel@google.com, jvanverth@google.com, robertphillips@google.com

[joshualitt, 2014-11-26 18:47:59]

Adding Greg, Jim, and Robert.  No need for everyone to take a look at the whole
CL, feel free to look at parts you are familiar with.

The gist of this CL is I moved all vertex attribute management to the
GeometryProcessor.  There are no longer any fixed function attributes.  This
means geometry processor's must now setup all attributes they actually use. 
Many GPs are unchanged, but still have to provide a position and local coord
output in case we have coord transforms.  Other GPs now need to setup an
optional color attribute, and pass it through to the fragment shader.

[jvanverth1, 2014-12-01 16:00:03]

Looks good examining the font and oval code. Just some comments:

https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrBitmapTextCon...
File src/gpu/GrBitmapTextContext.h (right):

https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.h:13: class GrDrawState;
Do you need to add this?

https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrOvalRenderer.cpp
File src/gpu/GrOvalRenderer.cpp (right):

https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrOvalRenderer....
src/gpu/GrOvalRenderer.cpp:107: vsBuilder->codeAppendf("%s = %s;",
vsBuilder->localCoords(), ce.inPosition()->fName);
Do we need localCoords here? It doesn't look like it.

https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrOvalRenderer....
src/gpu/GrOvalRenderer.cpp:235: vsBuilder->codeAppendf("%s = %s;",
vsBuilder->localCoords(), ee.inPosition()->fName);
Same here about localCoords.

https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrOvalRenderer....
src/gpu/GrOvalRenderer.cpp:387: vsBuilder->codeAppendf("%s = %s;",
vsBuilder->localCoords(), ee.inPosition()->fName);
Ditto.

[joshualitt, 2014-12-01 16:11:59]

On 2014/12/01 16:00:03, jvanverth1 wrote:
> Looks good examining the font and oval code. Just some comments:
> 
>
https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrBitmapTextCon...
> File src/gpu/GrBitmapTextContext.h (right):
> 
>
https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrBitmapTextCon...
> src/gpu/GrBitmapTextContext.h:13: class GrDrawState;
> Do you need to add this?
> 
>
https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrOvalRenderer.cpp
> File src/gpu/GrOvalRenderer.cpp (right):
> 
>
https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrOvalRenderer....
> src/gpu/GrOvalRenderer.cpp:107: vsBuilder->codeAppendf("%s = %s;",
> vsBuilder->localCoords(), ce.inPosition()->fName);
> Do we need localCoords here? It doesn't look like it.
> 
>
https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrOvalRenderer....
> src/gpu/GrOvalRenderer.cpp:235: vsBuilder->codeAppendf("%s = %s;",
> vsBuilder->localCoords(), ee.inPosition()->fName);
> Same here about localCoords.
> 
>
https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrOvalRenderer....
> src/gpu/GrOvalRenderer.cpp:387: vsBuilder->codeAppendf("%s = %s;",
> vsBuilder->localCoords(), ee.inPosition()->fName);
> Ditto.

Good catch on the drawstate, I'll clean that up.

RE: the local coords stuff, what that line actually does is assign regular
position to the local coords shader output.  Its a bit unclear, and I'll try and
clean this up in future patches, ie to make the local coords default to be
position unless you explicitly need local coords.

[bsalomon, 2014-12-02 18:06:05]

https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrGeometryProce...
File src/gpu/GrGeometryProcessor.h (right):

https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.h:81: return fAttribs.push_back(attribute);
Is this right? Will we wind up with unaligned attributes?

https://codereview.chromium.org/761563002/diff/120001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLProgramBuilder.cpp (right):

https://codereview.chromium.org/761563002/diff/120001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.cpp:253: emitAndInstallProc(gp,
&outputColor, &outputCoverage);
this->

https://codereview.chromium.org/761563002/diff/120001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLProgramBuilder.h (right):

https://codereview.chromium.org/761563002/diff/120001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.h:264: void nameBuiltin(GrGLSLExpr4*,
const char* baseName);
This could use a comment

[joshualitt, 2014-12-02 20:48:58]

On 2014/12/02 18:06:05, bsalomon wrote:
>
https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrGeometryProce...
> File src/gpu/GrGeometryProcessor.h (right):
> 
>
https://codereview.chromium.org/761563002/diff/120001/src/gpu/GrGeometryProce...
> src/gpu/GrGeometryProcessor.h:81: return fAttribs.push_back(attribute);
> Is this right? Will we wind up with unaligned attributes?
> 
>
https://codereview.chromium.org/761563002/diff/120001/src/gpu/gl/builders/GrG...
> File src/gpu/gl/builders/GrGLProgramBuilder.cpp (right):
> 
>
https://codereview.chromium.org/761563002/diff/120001/src/gpu/gl/builders/GrG...
> src/gpu/gl/builders/GrGLProgramBuilder.cpp:253: emitAndInstallProc(gp,
> &outputColor, &outputCoverage);
> this->
> 
>
https://codereview.chromium.org/761563002/diff/120001/src/gpu/gl/builders/GrG...
> File src/gpu/gl/builders/GrGLProgramBuilder.h (right):
> 
>
https://codereview.chromium.org/761563002/diff/120001/src/gpu/gl/builders/GrG...
> src/gpu/gl/builders/GrGLProgramBuilder.h:264: void nameBuiltin(GrGLSLExpr4*,
> const char* baseName);
> This could use a comment

all feedback incorporated.  Per our discussions, I added asserts everywhere we
use the 'map as struct' technique and aligned the vertex attribute size to 4
bytes. The reason this wasn't failing before is simply because we don't use any
unaligned vertex attributes yet.

[bsalomon, 2014-12-02 21:49:12]

lgtm w/ one request for more clarity.

https://codereview.chromium.org/761563002/diff/160001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLProgramBuilder.h (right):

https://codereview.chromium.org/761563002/diff/160001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.h:265: // If GrGLSLExpr4 has a valid name
then it will use that instead
"If GrGLSLExpr4 is already just a single variable name then its name will be
used."?

Can we call it nameExpression or assignExpressionToVariable? nameBuiltin doesn't
seem clear to me.

[joshualitt, 2014-12-03 14:12:45]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-12-03 14:13:52]

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

[commit-bot: I haz the power, 2014-12-03 14:24:15]

Committed patchset #14 (id:260001) as
https://skia.googlesource.com/skia/+/2dd1ae016d7f297b433c3ea3a771ef8e01657c1f