Opt state takes a GP instead of a GeometryStage

Opt state takes a GP instead of a GeometryStage

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/71856d520461ae025a0332aa0ce9735a096d9baf

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

[joshua.litt, 2014-10-09 16:54:56]

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

[egdaniel, 2014-10-09 17:17:13]

Is it possible to split the GStage -> GProcessor change and the removing of
color/coverage stage arrays in the GL pipeline into separate CL's?

I feel like both things are not directly intertwined and neither is a trivial
change.

[joshua.litt, 2014-10-09 17:22:02]

On 2014/10/09 17:17:13, egdaniel wrote:
> Is it possible to split the GStage -> GProcessor change and the removing of
> color/coverage stage arrays in the GL pipeline into separate CL's?
> 
> I feel like both things are not directly intertwined and neither is a trivial
> change.

I think this change is primarily adding a GP itself to the optstate.  I
shouldn't have added the removal of the color / coverage stage arrays in the
same CL, but at this point they are too intertwined to fix.

[bsalomon, 2014-10-09 18:35:45]

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

https://codereview.chromium.org/637003003/diff/160001/include/gpu/GrProcessor...
include/gpu/GrProcessor.h:217: * pointer). The GrCoordTransform is typically a
member field of the GrProcessor subclass. When
GrFragmentProcessor subclass?

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

https://codereview.chromium.org/637003003/diff/160001/include/gpu/GrProcessor...
include/gpu/GrProcessorStage.h:34: virtual ~GrFragmentStage() {}
delete virtual?

https://codereview.chromium.org/637003003/diff/160001/include/gpu/GrProcessor...
include/gpu/GrProcessorStage.h:152: virtual const GrFragmentProcessor*
getProcessor() const { return fProc.get(); }
delete virtual?

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

https://codereview.chromium.org/637003003/diff/160001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:182: SkASSERT(optState.hasGeometryProcessor());
Something feels off here... all these asserts make it seem like a round
peg/square hole problem. Is the problem that we have GrGLInstalledProcessors
which always assumes it is an array?

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

https://codereview.chromium.org/637003003/diff/160001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.h:185: void setData(const GrOptDrawState& optState,
GrGLInstalledProcessors* installedProcessors) {
Is it useful that this operates on an array? It's getting really convoluted with
StageMapper.. can we just make a simpler non-templated function that is called
once for each processor? Maybe GrODS should just have one array of fragment
processors?

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

https://codereview.chromium.org/637003003/diff/160001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.cpp:113: * A function which emits a meta key into the
key builder.  This is required because shader code may
This comment needs some rework with this change. Maybe move the meta-key
discussion down to where you moved the code? Or have a get_fp_meta_key()
function? Or get_processor_meta_key() which is common to the gp & fp and called
by get_fp_meta_key()? The meta key stuff is kind of confusing and there are
hardcoded shift values... it'd be nice not to have it repeated and not near its
docs.

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

https://codereview.chromium.org/637003003/diff/160001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.h:197: void createAndEmitProcessors(const
EffectKeyProvider::EffectType,
Another place where it seems like we're trying to have it two ways with one
function and patching over the difference with a template.

[egdaniel, 2014-10-09 18:52:04]

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

https://codereview.chromium.org/637003003/diff/160001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.h:185: void setData(const GrOptDrawState& optState,
GrGLInstalledProcessors* installedProcessors) {
On 2014/10/09 18:35:45, bsalomon wrote:
> Is it useful that this operates on an array? It's getting really convoluted
with
> StageMapper.. can we just make a simpler non-templated function that is called
> once for each processor? Maybe GrODS should just have one array of fragment
> processors?

I can definitely make GrODS have a single FP array if that is request. It would
still have getColorStage(s), getCoverageStage(s), and add getFragmentStage(s).
Would this be suitable interfase or would you want some other type of access?
Also would have a numFragStages() I suppose.

[joshua.litt, 2014-10-09 19:02:13]

On 2014/10/09 18:52:04, egdaniel wrote:
> https://codereview.chromium.org/637003003/diff/160001/src/gpu/gl/GrGLProgram.h
> File src/gpu/gl/GrGLProgram.h (right):
> 
>
https://codereview.chromium.org/637003003/diff/160001/src/gpu/gl/GrGLProgram....
> src/gpu/gl/GrGLProgram.h:185: void setData(const GrOptDrawState& optState,
> GrGLInstalledProcessors* installedProcessors) {
> On 2014/10/09 18:35:45, bsalomon wrote:
> > Is it useful that this operates on an array? It's getting really convoluted
> with
> > StageMapper.. can we just make a simpler non-templated function that is
called
> > once for each processor? Maybe GrODS should just have one array of fragment
> > processors?
> 
> I can definitely make GrODS have a single FP array if that is request. It
would
> still have getColorStage(s), getCoverageStage(s), and add getFragmentStage(s).
> Would this be suitable interfase or would you want some other type of access?
> Also would have a numFragStages() I suppose.

Yea that interface works great, but it will only clean up the GLPrograms use
case and the ProgramsDesc use case.  Shall we also combine the keyproviders? 
This will clean up the creation and emitting in program builder.

[joshua.litt, 2014-10-09 19:05:14]

On 2014/10/09 19:02:13, joshua.litt wrote:
> On 2014/10/09 18:52:04, egdaniel wrote:
> >
https://codereview.chromium.org/637003003/diff/160001/src/gpu/gl/GrGLProgram.h
> > File src/gpu/gl/GrGLProgram.h (right):
> > 
> >
>
https://codereview.chromium.org/637003003/diff/160001/src/gpu/gl/GrGLProgram....
> > src/gpu/gl/GrGLProgram.h:185: void setData(const GrOptDrawState& optState,
> > GrGLInstalledProcessors* installedProcessors) {
> > On 2014/10/09 18:35:45, bsalomon wrote:
> > > Is it useful that this operates on an array? It's getting really
convoluted
> > with
> > > StageMapper.. can we just make a simpler non-templated function that is
> called
> > > once for each processor? Maybe GrODS should just have one array of
fragment
> > > processors?
> > 
> > I can definitely make GrODS have a single FP array if that is request. It
> would
> > still have getColorStage(s), getCoverageStage(s), and add
getFragmentStage(s).
> > Would this be suitable interfase or would you want some other type of
access?
> > Also would have a numFragStages() I suppose.
> 
> Yea that interface works great, but it will only clean up the GLPrograms use
> case and the ProgramsDesc use case.  Shall we also combine the keyproviders? 
> This will clean up the creation and emitting in program builder.

An alternative is to use functors and an enum, something like:
    enum Stage {
        Color_Stage,
        Coverage_Stage
    };

    template <class Functor, Stage>
    struct StageIter;

    template <class Functor>
    struct StageIter<Functor, Color_Stage> {
        void Each(const GrOptDrawState& opt, Functor f) {
            int count = opt.numColorStages();
            for (int i = 0; i < count; i++) {
                f(opt.getColorStage(i));
            }
        }
    };

    template <class Functor>
    struct StageIter<Functor, Coverage_Stage> {
        void Each(const GrOptDrawState& opt, Functor f) {
            int count = opt.numCoverageStages();
            for (int i = 0; i < count; i++) {
                f(opt.getCoverageStage(i));
            }
        }
    };

[joshua.litt, 2014-10-10 13:44:59]

feedback incorporated

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

https://codereview.chromium.org/637003003/diff/700001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.cpp:253: // TODO effects cannot zeros,
the fix is to allow effects to take the GrGLSLExpr4 directly
I'll fix this comment before submit

[joshua.litt, 2014-10-10 13:47:03]

On 2014/10/10 13:44:59, joshua.litt wrote:
> feedback incorporated
> 
>
https://codereview.chromium.org/637003003/diff/700001/src/gpu/gl/builders/GrG...
> File src/gpu/gl/builders/GrGLProgramBuilder.cpp (right):
> 
>
https://codereview.chromium.org/637003003/diff/700001/src/gpu/gl/builders/GrG...
> src/gpu/gl/builders/GrGLProgramBuilder.cpp:253: // TODO effects cannot zeros,
> the fix is to allow effects to take the GrGLSLExpr4 directly
> I'll fix this comment before submit

Oh, try results are on patch 14

[bsalomon, 2014-10-10 15:25:50]

This is so much clearer than before! Minor inline comments/suggestions.

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

https://codereview.chromium.org/637003003/diff/700001/include/gpu/GrProcessor...
include/gpu/GrProcessor.h:215: * Fragment Processor Subclasses call this from
their constructor to register coordinate
lower case Subclasses?

https://codereview.chromium.org/637003003/diff/700001/include/gpu/GrProcessor...
include/gpu/GrProcessor.h:216: * transformations. The effect subclass manages
the lifetime of the transformations (this
-effect- processor subclass

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

https://codereview.chromium.org/637003003/diff/700001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:177: int numEffects =
fFragmentProcessors->fProcs.count();
numProcessors?

https://codereview.chromium.org/637003003/diff/700001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:180: const GrProcessor& effect =
*stage.getProcessor();
processor?

https://codereview.chromium.org/637003003/diff/700001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:192: const SkMatrix& matrix =
get_transform_matrix(processor, ip->fUseLocalCoords, t);
We always have local coords of some kind. Usually they are an alias for the
positions. Maybe call this fLocalCoordAttrib? fSeparateLocalCoords?

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

https://codereview.chromium.org/637003003/diff/700001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.cpp:136: uint32_t classID =
proc.getFactory().effectClassID();
at some point we need to make this processorClassID(), kind of orthog to this
change, though.

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

https://codereview.chromium.org/637003003/diff/700001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.h:162: kColor_EffectType,
kFragmentProcessor_EffectType

Or better yet:

kGeometry_ProcessorType,
kFragment_ProcessorType

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

https://codereview.chromium.org/637003003/diff/700001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.cpp:226: void
GrGLProgramBuilder::createAndEmitProcessors(const GrOptDrawState& optState,
Should we be using the word "install" rather than "create"? Just wondering since
the result is captured in installed processor objects.

[joshua.litt, 2014-10-10 15:47:42]

feedback inc, I'll cleanup effetClassID on another CL

[bsalomon, 2014-10-10 15:50:33]

lgtm

[joshua.litt, 2014-10-10 16:36:12]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-10-10 16:36:38]

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

[commit-bot: I haz the power, 2014-10-10 16:43:21]

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

[commit-bot: I haz the power, 2014-10-10 16:43:22]

Try jobs failed on following builders:
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...)

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

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-10-10 16:49:22]

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

[commit-bot: I haz the power, 2014-10-10 16:56:59]

Committed patchset #18 (id:1500001) as 71856d520461ae025a0332aa0ce9735a096d9baf

[danakj, 2014-10-10 20:27:56]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2014-10-10 20:27:56]

Could this cause this failure?
http://build.chromium.org/p/chromium.gpu/builders/Mac%20Release%20%28ATI%29/b...

[joshua.litt, 2014-10-10 21:10:30]

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

The reason for reverting is: Revert because this may be causing a break in a
chrome gpu test.

[joshua.litt, 2014-10-10 21:17:34]

On 2014/10/10 21:10:30, joshua.litt wrote:
> A revert of this CL (patchset #18 id:1500001) has been created in
> https://codereview.chromium.org/647183002/ by mailto:joshualitt@chromium.org.
> 
> The reason for reverting is: Revert because this may be causing a break in a
> chrome gpu test.

Dana, I did a revert, but it actually looks like the build after that build
passed:
http://build.chromium.org/p/chromium.gpu/builders/Mac%20Release%20%28ATI%29/b...

Thoughts?

[danakj, 2014-10-10 21:44:13]

On 2014/10/10 21:17:34, joshua.litt wrote:
> On 2014/10/10 21:10:30, joshua.litt wrote:
> > A revert of this CL (patchset #18 id:1500001) has been created in
> > https://codereview.chromium.org/647183002/ by
mailto:joshualitt@chromium.org.
> > 
> > The reason for reverting is: Revert because this may be causing a break in a
> > chrome gpu test.
> 
> Dana, I did a revert, but it actually looks like the build after that build
> passed:
>
http://build.chromium.org/p/chromium.gpu/builders/Mac%20Release%20%28ATI%29/b...
> 
> Thoughts?

It seems like a flaky failure, is there something in here that could cause
flake? It doesn't /seem/ to me like something that would flake from the bot
environment, and may be something earlier caused the issue that only failed on
your change. I tried looking at some earlier builds and didn't see anything that
could be related. In the worst case if you can't figure it out locally or on try
bots, you could try reland and see what happens. Let's see if it continues to
fail also until the next skia roll.

[joshua.litt, 2014-10-11 00:38:10]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-10-11 00:39:11]

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

[commit-bot: I haz the power, 2014-10-11 00:47:26]

Committed patchset #19 (id:1740001) as a5305a110ab5201d5dadd40cbe711582d5ac4996

[joshualitt, 2014-10-16 14:32:22]

On 2014/10/11 00:47:26, I haz the power (commit-bot) wrote:
> Committed patchset #19 (id:1740001) as
a5305a110ab5201d5dadd40cbe711582d5ac4996

Dana, this hasn't happened in 100+ builds.  My guess is there is some kind of
race condition in the test itself.