Draft change to start pulling uniform color into GP

This change will ultimately pull uniform color, and to a much lesser degree uniform coverage, into GPs.  There are still some loose ends because drawstate has the ability to override the GP, but fixing these cleanly will have to wait until we have deferred geometry in place and can make attribute / uniform decisions on the fly.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/9b98932adaceb7ad0a617ade16616923f6bffe84

[joshualitt, 2014-12-04 20:10:14]

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

[joshualitt, 2014-12-04 20:13:41]

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

[joshualitt, 2014-12-04 20:13:42]

Brian, could you take a look at DefaultGeoProcFactory.cpp before I get carried
away and change all of the GPs to manage uniform color?

As per our discussion, I know this still leaves the relationship between
optstate / GP a bit odd.  Some of this will be fixed by the deferred geometry
project, when it will be feasible to track constant color / coverage and
override attribute decisions prior to the draw.  More will be fixed when we can
pass color into the GP constructor, ie late paint snapping.

[bsalomon, 2014-12-05 14:43:03]

https://codereview.chromium.org/746423007/diff/1/src/gpu/GrDefaultGeoProcFact...
File src/gpu/GrDefaultGeoProcFactory.cpp (right):

https://codereview.chromium.org/746423007/diff/1/src/gpu/GrDefaultGeoProcFact...
src/gpu/GrDefaultGeoProcFactory.cpp:71: struct BatchTracker {
can this be private?

https://codereview.chromium.org/746423007/diff/1/src/gpu/GrDefaultGeoProcFact...
src/gpu/GrDefaultGeoProcFactory.cpp:101: bool sameColors = ((left.fOutputColor
== kAttribute_Output &&
I think this might be a little easier to read with more early outs.

if (left.fOutputColor != right.fOutputColor) {
    return false;
}

if (kUniform == left.fOutputColor && left.fColor != right.fColor) {
    return false;
}

https://codereview.chromium.org/746423007/diff/1/src/gpu/GrDefaultGeoProcFact...
src/gpu/GrDefaultGeoProcFactory.cpp:246: fNewStyle = true;
?

https://codereview.chromium.org/746423007/diff/1/src/gpu/GrOptDrawState.cpp
File src/gpu/GrOptDrawState.cpp (right):

https://codereview.chromium.org/746423007/diff/1/src/gpu/GrOptDrawState.cpp#n...
src/gpu/GrOptDrawState.cpp:268: } else if
(!this->getGeometryProcessor()->isEqual(*that.getGeometryProcessor())) {
Should we get rid of isEqual() in favor of canBatch?

Howabout

/* non-virtual */ bool GrGP::canBatch(that) {
   if (this->classID() == that.classID()) {
        return /*virtual*/ this->onCanBatch(that);
   }
   return false;
}

[joshualitt, 2014-12-10 16:23:46]

So the fNewStyle is a debug hack.  In programbuilder, if the GP has this flag I
let it control its own destiny.  It goes away when all pathProcs and GPs can
emit themselves.

I will change the isEqual to canBatch at that point.

Anyway, this is updated.  If defaultGeoProc looks okay, I will update all of the
GPs and the pathProc accordingly to this kind of style.

[bsalomon, 2014-12-10 18:36:20]

https://codereview.chromium.org/746423007/diff/20001/src/gpu/GrDefaultGeoProc...
File src/gpu/GrDefaultGeoProcFactory.cpp (right):

https://codereview.chromium.org/746423007/diff/20001/src/gpu/GrDefaultGeoProc...
src/gpu/GrDefaultGeoProcFactory.cpp:62: if (kAttribute_Output !=
left.fOutputColor && left.fColor != right.fColor) {
need these comparisons for coverage too? also seems like kUniform_Output ==
would be a bit clearer. Isn't it more correct too? (If they're both AllOnes do
we need to compare?)

https://codereview.chromium.org/746423007/diff/20001/src/gpu/GrDefaultGeoProc...
src/gpu/GrDefaultGeoProcFactory.cpp:143: if (kUniform_Output ==
local.fOutputColor) {
I wonder whether we should be skipping uni uploads when the color is unchanged.

https://codereview.chromium.org/746423007/diff/20001/src/gpu/GrDefaultGeoProc...
src/gpu/GrDefaultGeoProcFactory.cpp:215: Output fOutputColor;
The names of these vars and enum don't seem right to me. fOutputColor sounds
like a color. Also, are they really outputs? They seem more like inputs to the
GP methods that the GP uses to get its color/coverage. Just curious, do we use
kAllOnes for color ever?

https://codereview.chromium.org/746423007/diff/20001/src/gpu/GrGeometryProces...
File src/gpu/GrGeometryProcessor.h (right):

https://codereview.chromium.org/746423007/diff/20001/src/gpu/GrGeometryProces...
src/gpu/GrGeometryProcessor.h:35: mutable SkAlignedSStorage<kMaxSize> fData;
Maybe these just shouldn't be const if this field needs to me mutable.

https://codereview.chromium.org/746423007/diff/20001/src/gpu/GrGeometryProces...
src/gpu/GrGeometryProcessor.h:163: // TODO this call is temporary.  Once we have
deferred geometry, GPs can always make themselves
can/should we fold this into isEqual, that is replace isEqual with a canBatch
that checks for classID equality and then calls a virtual?

https://codereview.chromium.org/746423007/diff/20001/src/gpu/GrGeometryProces...
src/gpu/GrGeometryProcessor.h:179: bool fNewStyle;
I know it's a hack but can you formalize this a little bit beyond a public data
member (clearer name, protected member, public getter, comment that'd make sense
to someone else)

[joshualitt, 2014-12-11 20:52:48]

Okay, Brian take a look.  There is still a TON of things to cleanup, especially
builder / program side, but I'd like to do much of that work in separate
patches.

[bsalomon, 2014-12-12 14:29:35]

Sorry for slow review, couldn't publish yesterday.

https://codereview.chromium.org/746423007/diff/100001/src/gpu/GrGeometryProce...
File src/gpu/GrGeometryProcessor.cpp (right):

https://codereview.chromium.org/746423007/diff/100001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.cpp:25: this->onGetInvariantOutputColor(out);
this flow seems odd to me... should we always be giving the subclass a whack at
it? I don't follow the different cases and how the responsibilities for this and
emitting color-related code are split between the base class and subclasses.

I think somewhere in the base class need a comment that is as clear as possible
about how color and coverage are computed in GPs (including overrides) and how
that ties together the code generation, color/cov input types, the new helpers,
batch trackers, etc. It's *really* hard to keep straight.

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

https://codereview.chromium.org/746423007/diff/100001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.h:59: enum GPInput {
Can this be nested inside GrGP? Otherwise it needs a Gr prefix. Also needs some
dox.

https://codereview.chromium.org/746423007/diff/100001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.h:164: * a simple override color passed into
initBatchTracker
From discussion with Greg, I think this will become

bool fColorOutputIgnored; // the GP doesn't write a color output
GrColor fColorOverride; // respect if !fColorOutputIgnored and not
Gr_ColorINVALID.

and similar for coverage, though maybe no need for override there...

https://codereview.chromium.org/746423007/diff/100001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.h:198: static GPInput GetColorInputType(const
InitBT& init, bool hasVertexColor) {
Document?

https://codereview.chromium.org/746423007/diff/100001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.h:210: static bool CanCombineOutput(GPInput left,
GrColor lColor, GPInput right, GrColor rColor) {
Document?

https://codereview.chromium.org/746423007/diff/100001/src/gpu/gl/GrGLGeometry...
File src/gpu/gl/GrGLGeometryProcessor.h (right):

https://codereview.chromium.org/746423007/diff/100001/src/gpu/gl/GrGLGeometry...
src/gpu/gl/GrGLGeometryProcessor.h:65: /* a helper for uploading uniform color
*/
should these helpers be protected?

Could use some more comments about how/why they are used.

https://codereview.chromium.org/746423007/diff/100001/src/gpu/gl/GrGLGeometry...
src/gpu/gl/GrGLGeometryProcessor.h:66: void setUniformColorIfRequired(const
GrGLProgramDataManager& pdman, GPInput output,
..., GPInput output, ...

head explodes

[joshualitt, 2014-12-12 23:12:03]

Okay, I've cleaned up some of the stuff in optstate, let me know if this patch
is cleaner.  I've also added the uniform caching functionality.

I'm still working on the major class comment for the top of GP.h

[bsalomon, 2014-12-15 15:31:14]

This is looking much, much cleaner. The new comments really help.

https://codereview.chromium.org/746423007/diff/200001/src/gpu/GrGeometryProce...
File src/gpu/GrGeometryProcessor.cpp (right):

https://codereview.chromium.org/746423007/diff/200001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.cpp:50: void
GrGLGeometryProcessor::setupColor(GrGLGPBuilder* pb,
Maybe call it setupPassThroughColor? Not sure if it helps.

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

https://codereview.chromium.org/746423007/diff/200001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.h:17: * The GrPrimitiveProcessor represents some
kind of geometric primitive.  This includes the shape
This comment is great!

Two minor suggestions:

maybe convey early in the second paragraph that the feedback loops are on the
CPU side and pertain to invariant portions of the GPU computation.

deferred geometry: maybe a quick sentence that system is evolving to do any
tessellation and mesh draws after a batching/reorder pass is applied (that is,
define "deferred geometry").

We need a better word than bundle, GrGeoData? GrPrimitive? ... doesn't need to
be resolved now.

https://codereview.chromium.org/746423007/diff/200001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.h:59: mutable SkAlignedSStorage<kMaxSize> fData;
still unclear to me why this is mutable and the thing is passed around as const

https://codereview.chromium.org/746423007/diff/200001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.h:109: * responsibility of the GrGeometryProcessor.
nice

https://codereview.chromium.org/746423007/diff/200001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.h:193: virtual bool onCanMakeEqual(const
GrBatchTracker& mine, const GrBatchTracker& theirs) const = 0;
move to private?

https://codereview.chromium.org/746423007/diff/200001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.h:221: * A simple helper function to determine by
what means the GrGeometryProcessor should use to
Can you throw in the word "optional" ? Just want to make sure it is clear that a
GrGP can implement color/cov however it wants so long as the override contract
is maintained.

https://codereview.chromium.org/746423007/diff/200001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.h:222: * provide color.  If we are given an override
color(ie the given overridecolor is NOT illegal)
can you say GrColor_ILLEGAL? I think it might be confusing to just say illegal.

https://codereview.chromium.org/746423007/diff/200001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.h:223: * then we must always emit that color via a
uniform.  Otherwise, if our color is ignored
Does it have to be via a uniform?  A GP could rewrite its bundles w/ the
override color and batch together bundles that have different overrides via
vertex attr (in the def. geo. world). I guess that's kind of the point of the
TODO below.

https://codereview.chromium.org/746423007/diff/200001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.h:310: if (this->classID() != that.classID() ||
!this->hasSameTextureAccesses(that)) {
Seems a little weird that path procs can even have texture accesses. Maybe we
can think about hiding the ability to add them and expose it only on GrProc
subclasses that need it (FP, GP, XP)

https://codereview.chromium.org/746423007/diff/200001/src/gpu/GrGeometryProce...
src/gpu/GrGeometryProcessor.h:314: return other.color() == this->color();
So if there is an override these two colors would reflect that right? And they'd
both be illegal if the GP color wasn't needed, right? Just double checking my
own understanding of things.

https://codereview.chromium.org/746423007/diff/200001/src/gpu/gl/GrGLGeometry...
File src/gpu/gl/GrGLGeometryProcessor.h (right):

https://codereview.chromium.org/746423007/diff/200001/src/gpu/gl/GrGLGeometry...
src/gpu/gl/GrGLGeometryProcessor.h:66: /* a helper which can setup vertex,
constant, or uniform color depending on inputType */
Maybe a little more info about the params, e.g. what is inputName? what shader
(fs, vs, ...) is it valid in? Does colorUniform remain uninit if or set to the
invalid handle if uniform color isn't used, same for colorAttr.

[joshualitt, 2014-12-15 16:52:03]

Updated, as mentioned, GrGLPathProcessor is not used currently, but the
canMakeEqual, initBatchTracker, etc are.

[bsalomon, 2014-12-15 17:22:17]

lgtm

[joshualitt, 2014-12-15 20:27:59]

joshualitt@google.com changed reviewers:
+ egdaniel@google.com

[joshualitt, 2014-12-15 20:29:06]

I cleaned up a bunch, pulled in more path processor stuff, and modified the XP. 
Adding Greg as well.

Note: I will rename the XP flags once I rebase on top of Greg's CL but I figured
I'd get this out know so I can hopefully land it tonight.

[bsalomon, 2014-12-15 20:45:17]

minor suggestion, lgtm if it lgtg

https://codereview.chromium.org/746423007/diff/300001/src/gpu/effects/GrBitma...
File src/gpu/effects/GrBitmapTextGeoProc.cpp (right):

https://codereview.chromium.org/746423007/diff/300001/src/gpu/effects/GrBitma...
src/gpu/effects/GrBitmapTextGeoProc.cpp:75: // TODO fix this
TODO: This requirement goes away with deferred geometry?

[joshualitt, 2014-12-15 20:53:04]

Greg, this patch is rebased and up to date.  PTAL

[egdaniel, 2014-12-15 21:00:30]

On 2014/12/15 20:53:04, joshualitt wrote:
> Greg, this patch is rebased and up to date.  PTAL

LGTM once you rename equality operator on GrOptDrawState

[joshualitt, 2014-12-15 22:12:44]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-12-15 22:13:22]

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

[commit-bot: I haz the power, 2014-12-15 22:16:31]

Committed patchset #19 (id:360001) as
https://skia.googlesource.com/skia/+/9b98932adaceb7ad0a617ade16616923f6bffe84