Update XPF invariant info to not account for conflation

Update XPF invariant info to not account for conflation

Renames getInvariantOutput to getInvariantBlendedColor on GrXPFactory
and redefines it to not account for coverage conflation. This is the
information that all the callsites actually wanted to know.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/1fa4572d1a32e9fbf102d7388efe5e44bef460f1

[Chris Dalton, 2015-06-02 00:07:35]

cdalton@nvidia.com changed reviewers:
+ bsalomon@google.com, egdaniel@google.com

[Chris Dalton, 2015-06-02 00:07:37]

The motivation for this change is that previously it would disallow combining
drawPaths calls when using mixed samples because of the coverage.

There are a lot of files but the changes are mostly mechanical.

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

https://codereview.chromium.org/1161273005/diff/1/src/gpu/GrDrawContext.cpp#n...
src/gpu/GrDrawContext.cpp:415: if (paint.isConstantBlendedColor(&clearColor)) {
Is this the usual path where rects get replaced by clears? I think it might have
had a bug where it wouldn't a catch, for example, a fullscreen clear to
trans-black since the output color wasn't opaque.

[bsalomon, 2015-06-02 13:44:33]

Seems like a clarifying change to me but I defer to Greg.

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

https://codereview.chromium.org/1161273005/diff/1/src/gpu/GrDrawContext.cpp#n...
src/gpu/GrDrawContext.cpp:415: if (paint.isConstantBlendedColor(&clearColor)) {
On 2015/06/02 00:07:36, Chris Dalton wrote:
> Is this the usual path where rects get replaced by clears? I think it might
have
> had a bug where it wouldn't a catch, for example, a fullscreen clear to
> trans-black since the output color wasn't opaque.

It is and I think you're right about that.

https://codereview.chromium.org/1161273005/diff/1/src/gpu/effects/GrCoverageS...
File src/gpu/effects/GrCoverageSetOpXP.cpp (right):

https://codereview.chromium.org/1161273005/diff/1/src/gpu/effects/GrCoverageS...
src/gpu/effects/GrCoverageSetOpXP.cpp:233: void
GrCoverageSetOpXPFactory::getInvariantBlendedColor(const GrProcOptInfo&
/*colorPOI*/,
Shouldn't need to comment out the param name. We only do that in public headers
because of a warning generated in a client's build system (Android I believe). I
kind of prefer not to have the clutter if we don't need it.

https://codereview.chromium.org/1161273005/diff/1/src/gpu/effects/GrCustomXfe...
File src/gpu/effects/GrCustomXfermode.cpp (right):

https://codereview.chromium.org/1161273005/diff/1/src/gpu/effects/GrCustomXfe...
src/gpu/effects/GrCustomXfermode.cpp:812: void
GrCustomXPFactory::getInvariantBlendedColor(const GrProcOptInfo& /*colorPOI*/,
ditto

[egdaniel, 2015-06-02 14:19:41]

https://codereview.chromium.org/1161273005/diff/1/include/gpu/GrXferProcessor.h
File include/gpu/GrXferProcessor.h (right):

https://codereview.chromium.org/1161273005/diff/1/include/gpu/GrXferProcessor...
include/gpu/GrXferProcessor.h:382: const GrProcOptInfo& coveragePOI,
Do we need coveragePOI to be in here anymore?

https://codereview.chromium.org/1161273005/diff/1/src/gpu/effects/GrCoverageS...
File src/gpu/effects/GrCoverageSetOpXP.cpp (right):

https://codereview.chromium.org/1161273005/diff/1/src/gpu/effects/GrCoverageS...
src/gpu/effects/GrCoverageSetOpXP.cpp:233: void
GrCoverageSetOpXPFactory::getInvariantBlendedColor(const GrProcOptInfo&
/*colorPOI*/,
Does this guy really make sense in this new BlendedColor world? I feel like
coveragePOI should no longer be part of this check and it is the only thing this
guy is looking at.

[Chris Dalton, 2015-06-02 16:06:07]

https://codereview.chromium.org/1161273005/diff/1/src/gpu/effects/GrCoverageS...
File src/gpu/effects/GrCoverageSetOpXP.cpp (right):

https://codereview.chromium.org/1161273005/diff/1/src/gpu/effects/GrCoverageS...
src/gpu/effects/GrCoverageSetOpXP.cpp:233: void
GrCoverageSetOpXPFactory::getInvariantBlendedColor(const GrProcOptInfo&
/*colorPOI*/,
On 2015/06/02 14:19:41, egdaniel wrote:
> Does this guy really make sense in this new BlendedColor world? I feel like
> coveragePOI should no longer be part of this check and it is the only thing
this
> guy is looking at.

My sense is that it does make sense, because this XP is writing the coverage
value as if it were a legitimate color output (this differs from the other XPs
that only use coverage to antialias the real color). All the current
optimizations in skia are relevant for this XP based on what it's returning
here.

That being said, I definitely agree it's unfortunate to include the coveragePOI
as a parameter this function now. A couple alternatives I can think of:

1) Remove coveragePOI from the arg list like you suggest. We can still set
fWillBlendWithDst in here, and the only optimization that looks at the known
color anyway is "clear instead of drawRect", which seems irrelevant since
GrClipMaskManager calls clear directly.

2) Rework this XP to use the color input instead. GrClipMaskManager would be
responsible to make sure the info comes in through that channel instead of the
coverage input.

Thinking about it some more, #1 seems pretty good...

[egdaniel, 2015-06-02 16:46:54]

https://codereview.chromium.org/1161273005/diff/1/src/gpu/effects/GrCoverageS...
File src/gpu/effects/GrCoverageSetOpXP.cpp (right):

https://codereview.chromium.org/1161273005/diff/1/src/gpu/effects/GrCoverageS...
src/gpu/effects/GrCoverageSetOpXP.cpp:233: void
GrCoverageSetOpXPFactory::getInvariantBlendedColor(const GrProcOptInfo&
/*colorPOI*/,
On 2015/06/02 16:06:06, Chris Dalton wrote:
> On 2015/06/02 14:19:41, egdaniel wrote:
> > Does this guy really make sense in this new BlendedColor world? I feel like
> > coveragePOI should no longer be part of this check and it is the only thing
> this
> > guy is looking at.
> 
> My sense is that it does make sense, because this XP is writing the coverage
> value as if it were a legitimate color output (this differs from the other XPs
> that only use coverage to antialias the real color). All the current
> optimizations in skia are relevant for this XP based on what it's returning
> here.
> 
> That being said, I definitely agree it's unfortunate to include the
coveragePOI
> as a parameter this function now. A couple alternatives I can think of:
> 
> 1) Remove coveragePOI from the arg list like you suggest. We can still set
> fWillBlendWithDst in here, and the only optimization that looks at the known
> color anyway is "clear instead of drawRect", which seems irrelevant since
> GrClipMaskManager calls clear directly.
> 
> 2) Rework this XP to use the color input instead. GrClipMaskManager would be
> responsible to make sure the info comes in through that channel instead of the
> coverage input.
> 
> Thinking about it some more, #1 seems pretty good...

So my vote would be for option 1. As you said we can still set fWillBelndWithDst
to true or false the same as before. However my main reason for not caring if we
"lose" optimizations here is that running through all our gm's, we never call
getInvariantOutput with a setCoverageOptXPF. So if we aren't using it ever, then
we should have a worse signature to support it. At least this is my opinion.

[Chris Dalton, 2015-06-02 17:07:12]

I'm liking this a lot better now without coverage in the signature.

https://codereview.chromium.org/1161273005/diff/1/include/gpu/GrXferProcessor.h
File include/gpu/GrXferProcessor.h (right):

https://codereview.chromium.org/1161273005/diff/1/include/gpu/GrXferProcessor...
include/gpu/GrXferProcessor.h:382: const GrProcOptInfo& coveragePOI,
On 2015/06/02 14:19:41, egdaniel wrote:
> Do we need coveragePOI to be in here anymore?

Done.

https://codereview.chromium.org/1161273005/diff/1/src/gpu/effects/GrCoverageS...
File src/gpu/effects/GrCoverageSetOpXP.cpp (right):

https://codereview.chromium.org/1161273005/diff/1/src/gpu/effects/GrCoverageS...
src/gpu/effects/GrCoverageSetOpXP.cpp:233: void
GrCoverageSetOpXPFactory::getInvariantBlendedColor(const GrProcOptInfo&
/*colorPOI*/,
On 2015/06/02 16:46:54, egdaniel wrote:
> On 2015/06/02 16:06:06, Chris Dalton wrote:
> > On 2015/06/02 14:19:41, egdaniel wrote:
> > > Does this guy really make sense in this new BlendedColor world? I feel
like
> > > coveragePOI should no longer be part of this check and it is the only
thing
> > this
> > > guy is looking at.
> > 
> > My sense is that it does make sense, because this XP is writing the coverage
> > value as if it were a legitimate color output (this differs from the other
XPs
> > that only use coverage to antialias the real color). All the current
> > optimizations in skia are relevant for this XP based on what it's returning
> > here.
> > 
> > That being said, I definitely agree it's unfortunate to include the
> coveragePOI
> > as a parameter this function now. A couple alternatives I can think of:
> > 
> > 1) Remove coveragePOI from the arg list like you suggest. We can still set
> > fWillBlendWithDst in here, and the only optimization that looks at the known
> > color anyway is "clear instead of drawRect", which seems irrelevant since
> > GrClipMaskManager calls clear directly.
> > 
> > 2) Rework this XP to use the color input instead. GrClipMaskManager would be
> > responsible to make sure the info comes in through that channel instead of
the
> > coverage input.
> > 
> > Thinking about it some more, #1 seems pretty good...
> 
> So my vote would be for option 1. As you said we can still set
fWillBelndWithDst
> to true or false the same as before. However my main reason for not caring if
we
> "lose" optimizations here is that running through all our gm's, we never call
> getInvariantOutput with a setCoverageOptXPF. So if we aren't using it ever,
then
> we should have a worse signature to support it. At least this is my opinion.

Agreed. Done.

[egdaniel, 2015-06-02 17:16:39]

lgtm

[Chris Dalton, 2015-06-02 17:17:13]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2015-06-02 17:17:32]

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

[commit-bot: I haz the power, 2015-06-02 17:19:04]

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

[commit-bot: I haz the power, 2015-06-02 17:19:05]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
  skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[Chris Dalton, 2015-06-02 17:24:48]

The CQ bit was checked by cdalton@nvidia.com to run a CQ dry run

[Chris Dalton, 2015-06-02 17:24:48]

The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com
Link to the patchset: https://codereview.chromium.org/1161273005/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2015-06-02 17:25:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1161273005/40001

[commit-bot: I haz the power, 2015-06-02 17:30:21]

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

[commit-bot: I haz the power, 2015-06-02 17:30:23]

Dry run: This issue passed the CQ dry run.

[Chris Dalton, 2015-06-02 17:30:46]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2015-06-02 17:31:06]

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

[commit-bot: I haz the power, 2015-06-02 17:32:42]

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

[commit-bot: I haz the power, 2015-06-02 17:32:43]

Try jobs failed on following builders:
  skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[bsalomon, 2015-06-02 17:38:56]

On 2015/06/02 17:32:43, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED,
>
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

lgtm

[Chris Dalton, 2015-06-02 17:42:32]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2015-06-02 17:42:49]

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

[commit-bot: I haz the power, 2015-06-02 17:43:45]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/1fa4572d1a32e9fbf102d7388efe5e44bef460f1