Initial CL to move color / coverage off of drawstate

This cl moves color and coverage off of drawstate.  In an effort to keep this CL manageable, I have left the compute invariant input / output in a bit of a strange state(fixing this will be complicated).

In addition, NVPR makes this very complicated, and I haven't quite figured out a good way to handle it, so for now color and coverage DO live on optstate, but I will figure out some way to refactor that in future CLs.

BUG=skia:

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

[joshualitt, 2014-12-05 20:50:55]

Patchset #2 (id:20001) has been deleted

[joshualitt, 2014-12-08 18:15:07]

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

[joshualitt, 2014-12-08 18:17:11]

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

[joshualitt, 2014-12-08 18:17:12]

Brian, PTAL.  This cl just plumbs color around until we have a GP.  GP does not
setup uniforms yet, and instead color and coverage live on optstate.  This was
to keep the change "small".

NVPR is giving me headaches, and I can't think of a really clean way to handle
the optimization stuff with it.

[joshualitt, 2014-12-08 18:17:30]

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

[joshualitt, 2014-12-08 18:18:42]

adding Greg as well for feedback.  I know the computing invariant I/O stuff is
not exactly like we discussed, but I didn't want to clean it up in the same CL
where I'm touching 50 files already.  I will figure out a way to fix it up.

[bsalomon, 2014-12-08 19:42:13]

https://codereview.chromium.org/783763002/diff/60001/include/gpu/GrColor.h
File include/gpu/GrColor.h (right):

https://codereview.chromium.org/783763002/diff/60001/include/gpu/GrColor.h#ne...
include/gpu/GrColor.h:70: #define GrColor_BLACK 0x0
I think this should be TRANSPARENT or TRANSPARENT_BLACK. BLACK sounds like
opaque black.

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrAAHairLinePath...
File src/gpu/GrAAHairLinePathRenderer.cpp (right):

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrAAHairLinePath...
src/gpu/GrAAHairLinePathRenderer.cpp:842:
drawState->setGeometryProcessor(GrDefaultGeoProcFactory::Create(color,
Does the default geo proc need to see the constant if it is inserted as an attr?
Will it get applied twice?

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrDrawState.h#ne...
src/gpu/GrDrawState.h:96: bool couldApplyCoverage(GrColor color, GrColor
coverage, const GrDrawTargetCaps& caps) const;
Do we really need the coverage field here? If someone is asking this question
than they want to try applying non-solid coverage. Or is there some way this can
go away in the future?

I'm wondering if there is a way to recast this to be less complicated. Would
something like this work:

couldInstallCoverageGP(bool gpColorWillBeOpaque, const GrDrawTargetCaps&)

?

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrDrawState.h#ne...
src/gpu/GrDrawState.h:101: bool hasSolidCoverage(GrColor coverage) const;
I assume this goes away when the GP outputs an invariant coverage? or no?

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrDrawState.h#ne...
src/gpu/GrDrawState.h:163: bool willEffectReadDstColor(GrColor color, GrColor
coverage) const;
Add a TODO to remove this function once only XP can read dst color?

[joshualitt, 2014-12-08 20:19:21]

Questions answered and some feedback incorporated

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrAAHairLinePath...
File src/gpu/GrAAHairLinePathRenderer.cpp (right):

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrAAHairLinePath...
src/gpu/GrAAHairLinePathRenderer.cpp:842:
drawState->setGeometryProcessor(GrDefaultGeoProcFactory::Create(color,
On 2014/12/08 19:42:13, bsalomon wrote:
> Does the default geo proc need to see the constant if it is inserted as an
attr?
> Will it get applied twice?

Yes, so we can calculate the optimization stuff.  It won't get applied twice
because for the time being the vertex attr will override the GP's desire to use
a uniform

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrDrawState.h#ne...
src/gpu/GrDrawState.h:96: bool couldApplyCoverage(GrColor color, GrColor
coverage, const GrDrawTargetCaps& caps) const;
On 2014/12/08 19:42:13, bsalomon wrote:
> Do we really need the coverage field here? If someone is asking this question
> than they want to try applying non-solid coverage. Or is there some way this
can
> go away in the future?
> 
> I'm wondering if there is a way to recast this to be less complicated. Would
> something like this work:
> 
> couldInstallCoverageGP(bool gpColorWillBeOpaque, const GrDrawTargetCaps&)
> 
> ?

So, this function is called in context before we have a GP, and its also called
before path rendering, oval rendering, etc.

I'm really not sure what the best thing to do here is, but it calls
calcInvariantCoverage which needs an input coverage.  Whatever we choose to do,
I think its not going to be that simple and I'd rather do it in a follow up cl.

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrDrawState.h#ne...
src/gpu/GrDrawState.h:101: bool hasSolidCoverage(GrColor coverage) const;
On 2014/12/08 19:42:13, bsalomon wrote:
> I assume this goes away when the GP outputs an invariant coverage? or no?

I think so yes

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrDrawState.h#ne...
src/gpu/GrDrawState.h:163: bool willEffectReadDstColor(GrColor color, GrColor
coverage) const;
On 2014/12/08 19:42:13, bsalomon wrote:
> Add a TODO to remove this function once only XP can read dst color?

Acknowledged.

[bsalomon, 2014-12-08 20:28:15]

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrAAHairLinePath...
File src/gpu/GrAAHairLinePathRenderer.cpp (right):

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrAAHairLinePath...
src/gpu/GrAAHairLinePathRenderer.cpp:842:
drawState->setGeometryProcessor(GrDefaultGeoProcFactory::Create(color,
On 2014/12/08 20:19:20, joshualitt wrote:
> On 2014/12/08 19:42:13, bsalomon wrote:
> > Does the default geo proc need to see the constant if it is inserted as an
> attr?
> > Will it get applied twice?
> 
> Yes, so we can calculate the optimization stuff.  It won't get applied twice
> because for the time being the vertex attr will override the GP's desire to
use
> a uniform

I think we should think about splitting apart the default geo guy soon.

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.chromium.org/783763002/diff/60001/src/gpu/GrDrawState.h#ne...
src/gpu/GrDrawState.h:96: bool couldApplyCoverage(GrColor color, GrColor
coverage, const GrDrawTargetCaps& caps) const;
On 2014/12/08 20:19:20, joshualitt wrote:
> On 2014/12/08 19:42:13, bsalomon wrote:
> > Do we really need the coverage field here? If someone is asking this
question
> > than they want to try applying non-solid coverage. Or is there some way this
> can
> > go away in the future?
> > 
> > I'm wondering if there is a way to recast this to be less complicated. Would
> > something like this work:
> > 
> > couldInstallCoverageGP(bool gpColorWillBeOpaque, const GrDrawTargetCaps&)
> > 
> > ?
> 
> So, this function is called in context before we have a GP, and its also
called
> before path rendering, oval rendering, etc.
> 
> I'm really not sure what the best thing to do here is, but it calls
> calcInvariantCoverage which needs an input coverage.  Whatever we choose to
do,
> I think its not going to be that simple and I'd rather do it in a follow up
cl.

Right that's why I think we should rephrase the question it asks as "could I
install a GP that produces fractional coverage". It covers both of those points.
We'd seed the first coverage fp with unkown single channel and seed color with
the passed in color (and perhaps allow for kInvalid for color).


If we land this as is I'd like to clean it up real soon. This is already a
confusing function and adding the params makes it even harder to understand.

[joshualitt, 2014-12-08 21:30:13]

> Right that's why I think we should rephrase the question it asks as "could I
> install a GP that produces fractional coverage". It covers both of those
points.
> We'd seed the first coverage fp with unkown single channel and seed color with
> the passed in color (and perhaps allow for kInvalid for color).
> 
> 
> If we land this as is I'd like to clean it up real soon. This is already a
> confusing function and adding the params makes it even harder to understand.

I cleaned up the external facing interface.  I didn't bother yet to cleanup the
internal facing interface, if possible I'd prefer to wait until after the PP
lands and we clean up the invariant input / output stuff

[bsalomon, 2014-12-08 21:50:54]

lgtm w/ two comment requests.

https://codereview.chromium.org/783763002/diff/120001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.chromium.org/783763002/diff/120001/src/gpu/GrDrawState.h#n...
src/gpu/GrDrawState.h:95: * intends to specify fractional pixel coverage (via
setCoverage(), through a coverage vertex
can you update this text to say "... fractional pixel coverage via a primitive
processor." or something like that?

https://codereview.chromium.org/783763002/diff/120001/src/gpu/GrDrawState.h#n...
src/gpu/GrDrawState.h:164: * TODO remove when we have a GP
XP not GP, right?

[joshualitt, 2014-12-09 20:59:51]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-12-09 21:00:40]

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

[commit-bot: I haz the power, 2014-12-09 21:02:48]

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

[commit-bot: I haz the power, 2014-12-09 21:02:50]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-GC...)
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[joshualitt, 2014-12-09 21:20:43]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-12-09 21:21:47]

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

[commit-bot: I haz the power, 2014-12-09 21:31:20]

Committed patchset #12 (id:240001) as
https://skia.googlesource.com/skia/+/2e3b3e369d79e78f7635d4c20e83a47ab571bdf2