Solo gp

Patch to create a distinct geometry processor.  The vast majority of this patch
is just a rename.  The meat is in GrGeometryProcessor, GrProcessor,
GrGL*Processor, GrProcessorStage, Gr*BackendProcessorFactory,
GrProcessUnitTestFactory, and the builders

BUG=skia:

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

[joshua.litt, 2014-09-20 01:09:40]

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

[joshua.litt, 2014-09-20 01:09:43]

Patch to create a distinct geometry processor.  The vast majority of this patch
is just a rename.  The meat is in GrGeometryProcessor, GrProcessor,
GrGL*Processor, GrProcessorStage, Gr*BackendProcessorFactory,
GrProcessUnitTestFactory, and the builders

[tfarina, 2014-09-20 03:05:45]

Could you include:

"Patch to create a distinct geometry processor.  The vast majority of this patch
is just a rename.  The meat is in GrGeometryProcessor, GrProcessor,
GrGL*Processor, GrProcessorStage, Gr*BackendProcessorFactory,
GrProcessUnitTestFactory, and the builders
"
in the CL description rather than just leaving it in rietveld's message?

This way we can read it in git log, not just 'solo gp'.

Thanks!

[bsalomon, 2014-09-22 14:56:39]

Comments up through SkGr.h, will keep reading.

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrGeometryP...
File include/gpu/GrGeometryProcessor.h (right):

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrGeometryP...
include/gpu/GrGeometryProcessor.h:12: class GrBackendGeometryProcessorFactory;
Can this file live in src/gpu?

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrGeometryP...
include/gpu/GrGeometryProcessor.h:15: * If an effect needs specialized vertex
shader code, then it must inherit from this class.
How about something like:

A GrGeomteryProcessor is used to perform computation in the vertex shader and
add support for custom vertex attributes. A GrGemeotryProcessor is typically
tied to the code that does a specific type of high-level primitive rendering
(e.g. anti-aliased circle rendering). The GrGeometryProcessor used for a draw is
specified using GrDrawState. There can only be one geometry processor active for
a draw. The custom vertex attributes required by the geometry processor must be
added to the vertex attribute array specified on the GrDrawState.
GrGeometryProcessor subclasses should be immutable after construction.

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrGeometryP...
include/gpu/GrGeometryProcessor.h:26: typedef SkSTArray<kMaxVertexAttribs,
GrShaderVar, true> VertexAttribArray;
Is it really a max of two? Why use a dynamic array then?

Also we typically don't use SkSTArray as a param or return type. Rather we might
store an SkSTArray as a member of a class but it gets returned as SkTArray (the
base class). The caller of this getVertexAttribs shouldn't care that this is a
STArray and not a TArray.

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrPaint.h
File include/gpu/GrPaint.h (right):

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrPaint.h#n...
include/gpu/GrPaint.h:112: * Helpers for adding color or coverage effects that
sample a texture. The matrix is applied
update comments (here and elsewhere) to say processor rather than effect? Or
save that for a separate CL?

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrProcessor.h
File include/gpu/GrProcessor.h (right):

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrProcessor...
include/gpu/GrProcessor.h:172: class GrFragmentProcessor : public GrProcessor {
Either for this land or in a future CL can you put this class and the macros
below in their own GrFramgmentProcessor header?

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

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrProcessor...
include/gpu/GrProcessorStage.h:165: return static_cast<const
GrFragmentProcessor*>(this->getProcessor());
Is the base class ever useful? I wonder if we should hide it's getProcesor
function in the protected section and only use the derived classes.

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrProgramEl...
File include/gpu/GrProgramElement.h (right):

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrProgramEl...
include/gpu/GrProgramElement.h:17: * Base class for GrProcessor (and future
GrGeometryProcessor). GrDrawState uses this to manage
delete the (and future ...) ?

Do you think it makes sense to keep this separate from GrProcessor? (I still do
think so).

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrTBackendP...
File include/gpu/GrTBackendProcessorFactory.h (right):

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrTBackendP...
include/gpu/GrTBackendProcessorFactory.h:23: * subclass. Additionally, It adds
the following requirements to the GrProcessor and GrGLProcessor
Probably my typo originally, but "It" shouldn't be capitalized.

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrTBackendP...
include/gpu/GrTBackendProcessorFactory.h:76: * every effect so far derives from
one of the following subclasses of GrTBackendProcessorFactory
Capitalize "Every", period at end of sentence?

Sorry for super-nits but comments in the public headers for the processor system
may be required reading when we get to the point of encouraging external code
bases to host their own specialized processors.

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrTBackendP...
include/gpu/GrTBackendProcessorFactory.h:81: class
GrTBackendGeometryProcessorFactory
Maybe the GP variant should be somewhere under src/gpu?

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrTypesPriv.h
File include/gpu/GrTypesPriv.h (right):

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrTypesPriv...
include/gpu/GrTypesPriv.h:179: enum GrProcessorEdgeType {
I think we need a better name.

GrPrimitiveEdgeType
GrShapeEdgeType
...

[bsalomon, 2014-09-22 15:35:24]

https://codereview.chromium.org/582963002/diff/120001/src/gpu/gl/GrGLProcessor.h
File src/gpu/gl/GrGLProcessor.h (right):

https://codereview.chromium.org/582963002/diff/120001/src/gpu/gl/GrGLProcesso...
src/gpu/gl/GrGLProcessor.h:113: @param samplers     One entry for each
GrTextureAccess of the GrProcessor that generated the
This comment was already busted, but how about:

Contains one entry for each GrTextureAccess added by the GrProcessor. These can
be passed...

https://codereview.chromium.org/582963002/diff/120001/src/gpu/gl/GrGLProgramD...
File src/gpu/gl/GrGLProgramDesc.cpp (right):

https://codereview.chromium.org/582963002/diff/120001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.cpp:118: * textures, attributes, etc for the space
allotted in the meta-key.
del "attributes"?

https://codereview.chromium.org/582963002/diff/120001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.cpp:244: effectKeySuccess |=
GetGeometryProcessorKey(gpStage, gpu->glCaps(),
I think this has always been wrong, but shouldn't we be and'ing and not or'ing
the success value?

Also, I think it was written this way to avoid lots of branches, but I'm not
sure that's actually better than just

if (!Get...Key(foo) || offset < max) {
    desc->fKey.reset();
    return false;
}

https://codereview.chromium.org/582963002/diff/120001/src/gpu/gl/GrGLProgramD...
File src/gpu/gl/GrGLProgramDesc.h (right):

https://codereview.chromium.org/582963002/diff/120001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.h:63: static bool Build(const GrOptDrawState&
optState,
IMO param names that just repeat the type name aren't useful in decls (optState,
drawType, gpu, ...) I usually delete them when I'm editing code. We're not
consistent one way or the other, though.

https://codereview.chromium.org/582963002/diff/120001/src/gpu/gl/GrGLProgramE...
File src/gpu/gl/GrGLProgramEffects.cpp (right):

https://codereview.chromium.org/582963002/diff/120001/src/gpu/gl/GrGLProgramE...
src/gpu/gl/GrGLProgramEffects.cpp:98: setDataInternal(gpu, drawType,
programDataManager, *effectStages, 0);
this->

https://codereview.chromium.org/582963002/diff/120001/src/gpu/gl/GrGLProgramE...
src/gpu/gl/GrGLProgramEffects.cpp:109: setDataInternal(gpu, drawType,
programDataManager, *effectStages[e], e);
this->

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

https://codereview.chromium.org/582963002/diff/120001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLFragmentShaderBuilder.h:14: class
GrGLBaseFragmentShaderBuilder : public GrGLShaderBuilder {
The name's are starting to get a bit confusing, I think.

What about something like:

GrGLFragmentShaderBuilder with subclasses GrGLFragmentProccesorBuilder and
GrGLGeometryProcessorFragmentBuilder? This might be off the mark because I'm not
sure I fully get it. We might need a nice comment somewhere describing the
ShaderBuilder hierarchy and what the different classes are used for.

[joshua.litt, 2014-09-22 18:14:45]

feedback incorporated

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrGeometryP...
File include/gpu/GrGeometryProcessor.h (right):

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrGeometryP...
include/gpu/GrGeometryProcessor.h:12: class GrBackendGeometryProcessorFactory;
On 2014/09/22 14:56:38, bsalomon wrote:
> Can this file live in src/gpu?

I will move it in a future CL, along with the factories and such.  I want to
move GrProcessorStage out of include

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrGeometryP...
include/gpu/GrGeometryProcessor.h:26: typedef SkSTArray<kMaxVertexAttribs,
GrShaderVar, true> VertexAttribArray;
This is just because of GLProgramsTest.  Eventually this will not be the case. 
I will put a comment to that effect here with a TODO

On 2014/09/22 14:56:38, bsalomon wrote:
> Is it really a max of two? Why use a dynamic array then?
> 
> Also we typically don't use SkSTArray as a param or return type. Rather we
might
> store an SkSTArray as a member of a class but it gets returned as SkTArray
(the
> base class). The caller of this getVertexAttribs shouldn't care that this is a
> STArray and not a TArray.

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrProcessor.h
File include/gpu/GrProcessor.h (right):

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrProcessor...
include/gpu/GrProcessor.h:172: class GrFragmentProcessor : public GrProcessor {
On 2014/09/22 14:56:38, bsalomon wrote:
> Either for this land or in a future CL can you put this class and the macros
> below in their own GrFramgmentProcessor header?

I will do it in a future CL because it means updating a ton of files.

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

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrProcessor...
include/gpu/GrProcessorStage.h:165: return static_cast<const
GrFragmentProcessor*>(this->getProcessor());
On 2014/09/22 14:56:38, bsalomon wrote:
> Is the base class ever useful? I wonder if we should hide it's getProcesor
> function in the protected section and only use the derived classes.

Well, its useful a bit right now.  Let me further refactor program builder.  It
may turn out we end up keeping these two classes entirely separated.

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrProgramEl...
File include/gpu/GrProgramElement.h (right):

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrProgramEl...
include/gpu/GrProgramElement.h:17: * Base class for GrProcessor (and future
GrGeometryProcessor). GrDrawState uses this to manage
On 2014/09/22 14:56:38, bsalomon wrote:
> delete the (and future ...) ?
> 
> Do you think it makes sense to keep this separate from GrProcessor? (I still
do
> think so).

Yes definitely, at the very least lets see how the XferProcessor affects things.

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrTBackendP...
File include/gpu/GrTBackendProcessorFactory.h (right):

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrTBackendP...
include/gpu/GrTBackendProcessorFactory.h:81: class
GrTBackendGeometryProcessorFactory
On 2014/09/22 14:56:38, bsalomon wrote:
> Maybe the GP variant should be somewhere under src/gpu?

Yes, I will follow up and move GrProcessorStage to src/gpu, and along with it do
these reorg cleanups

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

https://codereview.chromium.org/582963002/diff/120001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLFragmentShaderBuilder.h:14: class
GrGLBaseFragmentShaderBuilder : public GrGLShaderBuilder {
 I will use these names for now.  Immediately after this patch I will break out
the user interface from the backend building of shaders which should really
improve these files.

On 2014/09/22 15:35:24, bsalomon wrote:
> The name's are starting to get a bit confusing, I think.
> 
> What about something like:
> 
> GrGLFragmentShaderBuilder with subclasses GrGLFragmentProccesorBuilder and
> GrGLGeometryProcessorFragmentBuilder? This might be off the mark because I'm
not
> sure I fully get it. We might need a nice comment somewhere describing the
> ShaderBuilder hierarchy and what the different classes are used for.

[bsalomon, 2014-09-22 19:15:37]

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrPaint.h
File include/gpu/GrPaint.h (right):

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrPaint.h#n...
include/gpu/GrPaint.h:112: * Helpers for adding color or coverage effects that
sample a texture. The matrix is applied
On 2014/09/22 14:56:38, bsalomon wrote:
> update comments (here and elsewhere) to say processor rather than effect? Or
> save that for a separate CL?


I suspect there are still a ton of places that say effect in comments in other
files... Maybe a cleanup CL afterwards?

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrProgramEl...
File include/gpu/GrProgramElement.h (right):

https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrProgramEl...
include/gpu/GrProgramElement.h:17: * Base class for GrProcessor (and future
GrGeometryProcessor). GrDrawState uses this to manage
On 2014/09/22 18:14:44, joshua.litt wrote:
> On 2014/09/22 14:56:38, bsalomon wrote:
> > delete the (and future ...) ?
> > 
> > Do you think it makes sense to keep this separate from GrProcessor? (I still
> do
> > think so).
> 
> Yes definitely, at the very least lets see how the XferProcessor affects
things.

Shouldn't it just read "Base class for GrProcessor."?

I thought it went:

                              GrFragmentProcessor
GrProgramElement->GrProcessor<
                              GrGeometryProcessor

https://codereview.chromium.org/582963002/diff/140001/include/gpu/GrGeometryP...
File include/gpu/GrGeometryProcessor.h (right):

https://codereview.chromium.org/582963002/diff/140001/include/gpu/GrGeometryP...
include/gpu/GrGeometryProcessor.h:52: VertexAttribArray fVertexAttribs;
Might still make sense to use an SkSTArray<2 or 4, ...> here.

[joshua.litt, 2014-09-22 19:45:31]

On 2014/09/22 19:15:37, bsalomon wrote:
> https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrPaint.h
> File include/gpu/GrPaint.h (right):
> 
>
https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrPaint.h#n...
> include/gpu/GrPaint.h:112: * Helpers for adding color or coverage effects that
> sample a texture. The matrix is applied
> On 2014/09/22 14:56:38, bsalomon wrote:
> > update comments (here and elsewhere) to say processor rather than effect? Or
> > save that for a separate CL?
> 
> 
> I suspect there are still a ton of places that say effect in comments in other
> files... Maybe a cleanup CL afterwards?
Yes, sorry, I meant to add a note to that effect.  There will be many follow on
cleanup CLs after this one.
> 
>
https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrProgramEl...
> File include/gpu/GrProgramElement.h (right):
> 
>
https://codereview.chromium.org/582963002/diff/120001/include/gpu/GrProgramEl...
> include/gpu/GrProgramElement.h:17: * Base class for GrProcessor (and future
> GrGeometryProcessor). GrDrawState uses this to manage
> On 2014/09/22 18:14:44, joshua.litt wrote:
> > On 2014/09/22 14:56:38, bsalomon wrote:
> > > delete the (and future ...) ?
> > > 
> > > Do you think it makes sense to keep this separate from GrProcessor? (I
still
> > do
> > > think so).
> > 
> > Yes definitely, at the very least lets see how the XferProcessor affects
> things.
> 
> Shouldn't it just read "Base class for GrProcessor."?
> 
> I thought it went:
> 
>                               GrFragmentProcessor
> GrProgramElement->GrProcessor<
>                               GrGeometryProcessor
> 
>
https://codereview.chromium.org/582963002/diff/140001/include/gpu/GrGeometryP...
> File include/gpu/GrGeometryProcessor.h (right):
> 
>
https://codereview.chromium.org/582963002/diff/140001/include/gpu/GrGeometryP...
> include/gpu/GrGeometryProcessor.h:52: VertexAttribArray fVertexAttribs;
> Might still make sense to use an SkSTArray<2 or 4, ...> here.

[bsalomon, 2014-09-23 14:59:17]

lgtm

[joshua.litt, 2014-09-23 15:11:53]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-09-23 15:13:02]

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

[joshua.litt, 2014-09-23 15:30:22]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-09-23 15:31:02]

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

[joshua.litt, 2014-09-23 16:28:58]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-09-23 16:29:03]

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

[commit-bot: I haz the power, 2014-09-23 16:50:28]

Committed patchset #14 (id:260001) as b0a8a377f832c59cee939ad721e1f87d378b7142