Add Coverage Drawing XP

Add Coverage Drawing XP

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/8750924a1470e8215b2a344155259b93062d3fa2

[egdaniel, 2014-12-16 17:11:18]

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

[bsalomon, 2014-12-16 17:35:56]

https://codereview.chromium.org/808813002/diff/20001/src/gpu/GrClipMaskManage...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/808813002/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:431: // We want to invert the coverage here
we do??

https://codereview.chromium.org/808813002/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:600: if (!this->drawElement(&drawState,
GrColor_WHITE, dst, element, pr)) {
Can drawElement just not take a color then?

https://codereview.chromium.org/808813002/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:605: // We have to make a new drawstate because
the drawElement call may call into
I'm not sure this comment adds much value. A new drawstate for a new draw, seems
like we're justfying something that is self-explanatory/. It was the old
behavior that was weird/stateful.

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
File src/gpu/effects/GrCoverageSetOpXP.cpp (right):

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
src/gpu/effects/GrCoverageSetOpXP.cpp:37: 
don't need virtuals on these overrides. Also can declare the ~ override instead
of virtual.

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
src/gpu/effects/GrCoverageSetOpXP.cpp:105: default:
If you've exhausted all the possible values for k*_Op then just omit this. We
get a warning that all the cases aren't covered which is more useful than a
runtime failure.

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
src/gpu/effects/GrCoverageSetOpXP.cpp:210: bool
GrCoverageSetOpXPFactory::getOpaqueAndKnownColor(const GrProcOptInfo& colorPOI,
This is kind of confusing now that we have a coverage only XP. Also, seems like
the opaque part of the name refers to how this is used upstack and not about
what it is computing. Is opaqueness different than !willBlendWithDst()?

getKnownOutputValues()?

Also, how does this interact with secondary output? Should we have two
GrColor*/uint32_t*s? Should it just return false if there is a secondary output?

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
File src/gpu/effects/GrCoverageSetOpXP.h (right):

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
src/gpu/effects/GrCoverageSetOpXP.h:19: * This Xfer Processor is used to blend
the coverage together with the Dst given a specific set
"This xfer processor directly blends the src coverage with the dst using a set
operator. It is useful for rendering coverage masks using CSG. It can optionally
invert the src coverage before applying the set operator." ?

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
src/gpu/effects/GrCoverageSetOpXP.h:75: return false;
seems like it does, right? It uses the color variants of the blend coeffs, seems
like it'd just work.

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
src/gpu/effects/GrCoverageSetOpXP.h:86: bool canTweakAlphaForCoverage() const
SK_OVERRIDE { return false; }
can't wait to kill this guy

[egdaniel, 2014-12-17 21:12:38]

ptal

https://codereview.chromium.org/808813002/diff/20001/src/gpu/GrClipMaskManage...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/808813002/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:431: // We want to invert the coverage here
On 2014/12/16 17:35:56, bsalomon wrote:
> we do??

changed to false and changed to use coverageProcessor below. I manually checked
that we draw with the same set of bugs as we currently do for these temp paths.

https://codereview.chromium.org/808813002/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:600: if (!this->drawElement(&drawState,
GrColor_WHITE, dst, element, pr)) {
On 2014/12/16 17:35:56, bsalomon wrote:
> Can drawElement just not take a color then?

Done.

https://codereview.chromium.org/808813002/diff/20001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:605: // We have to make a new drawstate because
the drawElement call may call into
On 2014/12/16 17:35:56, bsalomon wrote:
> I'm not sure this comment adds much value. A new drawstate for a new draw,
seems
> like we're justfying something that is self-explanatory/. It was the old
> behavior that was weird/stateful.

removed

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
File src/gpu/effects/GrCoverageSetOpXP.cpp (right):

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
src/gpu/effects/GrCoverageSetOpXP.cpp:37: 
On 2014/12/16 17:35:56, bsalomon wrote:
> don't need virtuals on these overrides. Also can declare the ~ override
instead
> of virtual.

Done.

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
src/gpu/effects/GrCoverageSetOpXP.cpp:105: default:
On 2014/12/16 17:35:56, bsalomon wrote:
> If you've exhausted all the possible values for k*_Op then just omit this. We
> get a warning that all the cases aren't covered which is more useful than a
> runtime failure.

Done.

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
src/gpu/effects/GrCoverageSetOpXP.cpp:210: bool
GrCoverageSetOpXPFactory::getOpaqueAndKnownColor(const GrProcOptInfo& colorPOI,
On 2014/12/16 17:35:56, bsalomon wrote:
> This is kind of confusing now that we have a coverage only XP. Also, seems
like
> the opaque part of the name refers to how this is used upstack and not about
> what it is computing. Is opaqueness different than !willBlendWithDst()?
> 
> getKnownOutputValues()?
> 
> Also, how does this interact with secondary output? Should we have two
> GrColor*/uint32_t*s? Should it just return false if there is a secondary
output?
> 

As per our conversation, I will clean up these functions in a follow up cl.

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
File src/gpu/effects/GrCoverageSetOpXP.h (right):

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
src/gpu/effects/GrCoverageSetOpXP.h:19: * This Xfer Processor is used to blend
the coverage together with the Dst given a specific set
On 2014/12/16 17:35:56, bsalomon wrote:
> "This xfer processor directly blends the src coverage with the dst using a set
> operator. It is useful for rendering coverage masks using CSG. It can
optionally
> invert the src coverage before applying the set operator." ?

Done.

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
src/gpu/effects/GrCoverageSetOpXP.h:75: return false;
On 2014/12/16 17:35:56, bsalomon wrote:
> seems like it does, right? It uses the color variants of the blend coeffs,
seems
> like it'd just work.

Done.

https://codereview.chromium.org/808813002/diff/20001/src/gpu/effects/GrCovera...
src/gpu/effects/GrCoverageSetOpXP.h:86: bool canTweakAlphaForCoverage() const
SK_OVERRIDE { return false; }
On 2014/12/16 17:35:56, bsalomon wrote:
> can't wait to kill this guy

you are not alone...

[bsalomon, 2014-12-17 21:15:47]

lgtm

[egdaniel, 2014-12-17 21:28:35]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-12-17 21:29:05]

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

[commit-bot: I haz the power, 2014-12-17 21:37:17]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/8750924a1470e8215b2a344155259b93062d3fa2