Add XP to handle the cases where we disable color write.

Add XP to handle the cases where we disable color write.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/080e673b10ac607305f140ddb245e140ccde40c6

[egdaniel, 2014-12-19 16:20:41]

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

[bsalomon, 2014-12-19 16:40:35]

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

https://codereview.chromium.org/787233003/diff/1/include/gpu/GrXferProcessor....
include/gpu/GrXferProcessor.h:180: virtual bool canApplyCoverage(const
GrProcOptInfo& colorPOI,
does it fit on one line? :)

https://codereview.chromium.org/787233003/diff/1/include/gpu/GrXferProcessor....
include/gpu/GrXferProcessor.h:210: virtual bool willReadDst(const GrProcOptInfo&
colorPOI,
At first I thought this was a reintroduction of the recently departed
willBlendWithDst. This could be confusing since this class is all tied up with
blending src and dst colors. Maybe it should be requiresDstInFragmentShader?

https://codereview.chromium.org/787233003/diff/1/src/gpu/effects/GrDisableCol...
File src/gpu/effects/GrDisableColorXP.cpp (right):

https://codereview.chromium.org/787233003/diff/1/src/gpu/effects/GrDisableCol...
src/gpu/effects/GrDisableColorXP.cpp:22: static void GenKey(const GrProcessor&,
const GrGLCaps&,
one line?

https://codereview.chromium.org/787233003/diff/1/src/gpu/effects/GrDisableCol...
src/gpu/effects/GrDisableColorXP.cpp:53: GrXferProcessor*
GrDisableColorXPFactory::createXferProcessor(const GrProcOptInfo& /*colorPOI*/,
that named unused param warning only happens in headers, and I think only public
headers.

https://codereview.chromium.org/787233003/diff/1/src/gpu/effects/GrDisableCol...
File src/gpu/effects/GrDisableColorXP.h (right):

https://codereview.chromium.org/787233003/diff/1/src/gpu/effects/GrDisableCol...
src/gpu/effects/GrDisableColorXP.h:17: * This xfer processor disables color
writing. Thus color and coverage and ignored and no blending
and->are? stencilling->stenciling

https://codereview.chromium.org/787233003/diff/1/src/gpu/effects/GrDisableCol...
src/gpu/effects/GrDisableColorXP.h:28: virtual const char* name() const
SK_OVERRIDE { return "Disable Color"; }
no need for virtual

https://codereview.chromium.org/787233003/diff/1/src/gpu/effects/GrDisableCol...
src/gpu/effects/GrDisableColorXP.h:76: bool canTweakAlphaForCoverage() const
SK_OVERRIDE { return false; }
why not? it gets ignored anyway.

[egdaniel, 2014-12-19 19:53:45]

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

https://codereview.chromium.org/787233003/diff/1/include/gpu/GrXferProcessor....
include/gpu/GrXferProcessor.h:180: virtual bool canApplyCoverage(const
GrProcOptInfo& colorPOI,
On 2014/12/19 16:40:34, bsalomon wrote:
> does it fit on one line? :)

Nope

https://codereview.chromium.org/787233003/diff/1/include/gpu/GrXferProcessor....
include/gpu/GrXferProcessor.h:210: virtual bool willReadDst(const GrProcOptInfo&
colorPOI,
On 2014/12/19 16:40:34, bsalomon wrote:
> At first I thought this was a reintroduction of the recently departed
> willBlendWithDst. This could be confusing since this class is all tied up with
> blending src and dst colors. Maybe it should be requiresDstInFragmentShader? 

So the fact that it is even checking Fragment shaders right now is a side effect
of read dst not being xp only. And it also is checking the XP itself, but right
now all the XP's we have would simply be adding "|| false" to the checks. In the
future this function should really just be an inline function that just returns
false or true with no logic at all.

https://codereview.chromium.org/787233003/diff/1/src/gpu/effects/GrDisableCol...
File src/gpu/effects/GrDisableColorXP.h (right):

https://codereview.chromium.org/787233003/diff/1/src/gpu/effects/GrDisableCol...
src/gpu/effects/GrDisableColorXP.h:17: * This xfer processor disables color
writing. Thus color and coverage and ignored and no blending
On 2014/12/19 16:40:34, bsalomon wrote:
> and->are? stencilling->stenciling

You don't the the CORRECT british way to spell it? Stuck in your silly American
world?

https://codereview.chromium.org/787233003/diff/1/src/gpu/effects/GrDisableCol...
src/gpu/effects/GrDisableColorXP.h:28: virtual const char* name() const
SK_OVERRIDE { return "Disable Color"; }
On 2014/12/19 16:40:34, bsalomon wrote:
> no need for virtual

Done.

https://codereview.chromium.org/787233003/diff/1/src/gpu/effects/GrDisableCol...
src/gpu/effects/GrDisableColorXP.h:76: bool canTweakAlphaForCoverage() const
SK_OVERRIDE { return false; }
On 2014/12/19 16:40:34, bsalomon wrote:
> why not? it gets ignored anyway.

Sure. I guess my thought was if they installed this XP why spend the work to
recreate a color with coverage in alpha. So just return false so they don't do
the extra work. Are there cases where the caller would simply decide not to draw
if they got a false returned here?

[bsalomon, 2014-12-22 14:06:57]

lgtm

https://codereview.chromium.org/787233003/diff/1/src/gpu/effects/GrDisableCol...
File src/gpu/effects/GrDisableColorXP.h (right):

https://codereview.chromium.org/787233003/diff/1/src/gpu/effects/GrDisableCol...
src/gpu/effects/GrDisableColorXP.h:76: bool canTweakAlphaForCoverage() const
SK_OVERRIDE { return false; }
On 2014/12/19 19:53:45, egdaniel wrote:
> On 2014/12/19 16:40:34, bsalomon wrote:
> > why not? it gets ignored anyway.
> 
> Sure. I guess my thought was if they installed this XP why spend the work to
> recreate a color with coverage in alpha. So just return false so they don't do
> the extra work. Are there cases where the caller would simply decide not to
draw
> if they got a false returned here?

Callers might decide they need to upload color and coverage separately (as two
different vertex attributes) rather than as one. So saying true is a bit more
generous. It gives the caller the option to keep them separate or not. Since we
aren't going to read either, might as well let the caller take whatever it
thinks is the optimal path.

[egdaniel, 2014-12-22 15:23:50]

The CQ bit was checked by egdaniel@google.com

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

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

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

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