Cleanup of shader building system

Cleanup of shader building system

this is a huge refactor and cleanup of the gl shader building system in
Skia.  The entire shader building pipeline is now part of
GrGLProgramCreator, which takes a gp, and some fps, and creates a
program.  I added some subclasses of GrGLProgram to handle the
eccentricities of Nvpr/Nvpres.  Outside of the builders folder
and GrGLPrograms, this change is basically just a rename


solo gp

BUG=skia:

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

Committed: https://skia.googlesource.com/skia/+/47bb38283072dc87dc93220cd2f370ca109972ff

[joshua.litt, 2014-09-28 22:49:03]

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

[joshua.litt, 2014-09-28 22:49:05]

Most of the refactoring for this patch is in GrGL*ProgramBuilder and
GrGLProgram.  I actually just moved code around for the most part, though the
Nvpr ES stuff I separated from GrGLSkiaProgramBuilder, formerly they both were
part of GrGLFullProgramBuilder.

[bsalomon, 2014-09-29 15:43:19]

Some early comments.

Is there any way this can be broken up into smaller CLs? I'm having trouble
following all the changes.

https://codereview.chromium.org/611653002/diff/310001/include/gpu/GrProcessor...
File include/gpu/GrProcessorStage.h (right):

https://codereview.chromium.org/611653002/diff/310001/include/gpu/GrProcessor...
include/gpu/GrProcessorStage.h:34: virtual ~GrProcessorStage() {}
must this be virtual? Could the model used for
GrGpuResourceRef/GrTProgramResourceRef?

https://codereview.chromium.org/611653002/diff/310001/src/gpu/GrAAConvexPathR...
File src/gpu/GrAAConvexPathRenderer.cpp (right):

https://codereview.chromium.org/611653002/diff/310001/src/gpu/GrAAConvexPathR...
src/gpu/GrAAConvexPathRenderer.cpp:21: #include
"gl/builders/GrGLSkiaProgramBuilder.h"
Do we need the word "Skia"?  Everything is "Skia"

https://codereview.chromium.org/611653002/diff/310001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/611653002/diff/310001/src/gpu/GrDrawTarget.cp...
src/gpu/GrDrawTarget.cpp:393: const GrGeometryProcessor* gp =
drawState.getGeometryProcessor()->getProcessor();
It seems weird that getGeometryProcessor doesn't return a GP. How about
getGeometryProcessorStage() or getGPStage()?

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:180: this->postSetData(drawType);
In other place we're using "will" and "did" instead of "pre" and "post", can we
adopt that here?

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:315: if (fMatrixState.fRenderTargetOrigin !=
rt->origin() ||
Aren't these things all accessible via GrODS?

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/GrGLProgram.h
File src/gpu/gl/GrGLProgram.h (right):

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.h:220: SkAutoTUnref<GrGLInstalledProcessors>    
fGeometryProcessor;
Let's either line up all the members or none of them

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.h:234: * Nvpr only has a fragment shader, NvprES ignores
the vertex shader, but both require specialized
Can we say NVPR 1.3+ rather than NvprES since the desktop drivers will support
the updated extension?

For class names we could have NVPR and LegacyNVPR (since the latter is to be
deleted).

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLESNvprProgramBuilder.h (right):

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLESNvprProgramBuilder.h:38: * A separable varying
(fragment shader input) is a varying that can be used also when vertex
line up the start of each line with "Add..." (or move the first line down one
line to line up with the rest)

[joshua.litt, 2014-09-29 21:05:22]

feedback incorporated

https://codereview.chromium.org/611653002/diff/310001/include/gpu/GrProcessor...
File include/gpu/GrProcessorStage.h (right):

https://codereview.chromium.org/611653002/diff/310001/include/gpu/GrProcessor...
include/gpu/GrProcessorStage.h:34: virtual ~GrProcessorStage() {}
On 2014/09/29 15:43:19, bsalomon wrote:
> must this be virtual? Could the model used for
> GrGpuResourceRef/GrTProgramResourceRef?

Yea I can totally use templates.  Mind if I do this as a follow up CL?

https://codereview.chromium.org/611653002/diff/310001/src/gpu/GrAAConvexPathR...
File src/gpu/GrAAConvexPathRenderer.cpp (right):

https://codereview.chromium.org/611653002/diff/310001/src/gpu/GrAAConvexPathR...
src/gpu/GrAAConvexPathRenderer.cpp:21: #include
"gl/builders/GrGLSkiaProgramBuilder.h"
In 2014/09/29 15:43:19, bsalomon wrote:
> Do we need the word "Skia"?  Everything is "Skia"

done

https://codereview.chromium.org/611653002/diff/310001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/611653002/diff/310001/src/gpu/GrDrawTarget.cp...
src/gpu/GrDrawTarget.cpp:393: const GrGeometryProcessor* gp =
drawState.getGeometryProcessor()->getProcessor();
On 2014/09/29 15:43:19, bsalomon wrote:
> It seems weird that getGeometryProcessor doesn't return a GP. How about
> getGeometryProcessorStage() or getGPStage()?

Mind if this is a follow up CL, I agree its poorly named.

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:180: this->postSetData(drawType);
On 2014/09/29 15:43:19, bsalomon wrote:
> In other place we're using "will" and "did" instead of "pre" and "post", can
we
> adopt that here?

Done.

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:315: if (fMatrixState.fRenderTargetOrigin !=
rt->origin() ||
On 2014/09/29 15:43:19, bsalomon wrote:
> Aren't these things all accessible via GrODS?

Done.

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/GrGLProgram.h
File src/gpu/gl/GrGLProgram.h (right):

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.h:220: SkAutoTUnref<GrGLInstalledProcessors>    
fGeometryProcessor;
On 2014/09/29 15:43:19, bsalomon wrote:
> Let's either line up all the members or none of them

done

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.h:234: * Nvpr only has a fragment shader, NvprES ignores
the vertex shader, but both require specialized
On 2014/09/29 15:43:19, bsalomon wrote:
> Can we say NVPR 1.3+ rather than NvprES since the desktop drivers will support
> the updated extension?
> 
> For class names we could have NVPR and LegacyNVPR (since the latter is to be
> deleted).

done

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLESNvprProgramBuilder.h (right):

https://codereview.chromium.org/611653002/diff/310001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLESNvprProgramBuilder.h:38: * A separable varying
(fragment shader input) is a varying that can be used also when vertex
On 2014/09/29 15:43:19, bsalomon wrote:
> line up the start of each line with "Add..." (or move the first line down one
> line to line up with the rest)

Done.

[joshua.litt, 2014-10-06 16:14:48]

Brian, after taking a closer look at the hierarchies, I think refactoring them
as we discussed would be unfeasible.

I'm trying just a basic rename to make it clearer who uses which classes and to
shorten names.  I know this patch is imperfect, and if its still really
confusing we can discuss alternative ways to break things out, but I'd really
like to land this because it cleans up the internals of shader building quite a
bit and makes the patch to use a GP instead of a GS in drawstate much simpler.

[bsalomon, 2014-10-06 18:49:40]

https://codereview.chromium.org/611653002/diff/350001/src/gpu/GrAARectRendere...
File src/gpu/GrAARectRenderer.cpp (right):

https://codereview.chromium.org/611653002/diff/350001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:61: GrGLGPFragmentShaderBuilder* fsBuilder =
builder->getFragmentShaderBuilder();
Can we kill the word Shader here?

i.e. GrGLGPFragmentBuilder

https://codereview.chromium.org/611653002/diff/350001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.chromium.org/611653002/diff/350001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:171: // of GLPrograms determine how to set coord
transforms
GLProgram? no s?

https://codereview.chromium.org/611653002/diff/350001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:200:
SkASSERT(!GrGpu::IsPathRenderingDrawType(drawType));
why don't we set data for nvpr? or is this just a generic assert that we're not
using this class for nvpr?

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

https://codereview.chromium.org/611653002/diff/350001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.cpp:405: GrGLProgram*
GrGLProgramBuilder::compileBindLinkCreate() {
finalize()?

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

https://codereview.chromium.org/611653002/diff/350001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.h:281: class GrGLInstalledProcessors :
public SkRefCnt {
Do think it still makes sense for this to be an array of processors rather than
just having one GrGLInstalledProcessor per processor?

[joshua.litt, 2014-10-06 19:16:57]

feedback incorporated

https://codereview.chromium.org/611653002/diff/350001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.chromium.org/611653002/diff/350001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:200:
SkASSERT(!GrGpu::IsPathRenderingDrawType(drawType));
On 2014/10/06 18:49:40, bsalomon wrote:
> why don't we set data for nvpr? or is this just a generic assert that we're
not
> using this class for nvpr?

Exactly, this is just a generic assert.  The only reason the didSetData call
exists is so  legacyNvpr can flush path tex gen.  When legacy NVPR disappears,
we can remove didSetData.  I'll add a comment to that effect

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

https://codereview.chromium.org/611653002/diff/350001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.h:281: class GrGLInstalledProcessors :
public SkRefCnt {
On 2014/10/06 18:49:40, bsalomon wrote:
> Do think it still makes sense for this to be an array of processors rather
than
> just having one GrGLInstalledProcessor per processor?

I agree this class is strange.  Let me leave a TODO to refactor this after I
remove the geometry stage.  Then I can split up this class into two, one for the
GP, one for fragment stages, and we can have arrays.

[bsalomon, 2014-10-06 19:28:01]

(close eyes) lgtm

[joshua.litt, 2014-10-07 18:47:54]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-10-07 18:48:33]

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

[commit-bot: I haz the power, 2014-10-07 19:16:44]

Committed patchset #25 (id:470001) as fe1233c3f12f81bb675718516bbb32f72af726ec

[joshua.litt, 2014-10-07 19:41:39]

A revert of this CL (patchset #25 id:470001) has been created in
https://codereview.chromium.org/635533005/ by joshualitt@chromium.org.

The reason for reverting is: Seems to have messed up windows 7 gms.

[joshua.litt, 2014-10-07 23:33:58]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-10-07 23:34:50]

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

[commit-bot: I haz the power, 2014-10-07 23:43:48]

Committed patchset #27 (id:490001) as 47bb38283072dc87dc93220cd2f370ca109972ff