Make all blending up to GrOptDrawState be handled by the xp/xp factory.

Make all blending up to GrOptDrawState be handled by the xp/xp factory.

In this cl the blending information is extracted for the xp and stored in the ODS
which is then used as it currently is. In the follow up cl, an XP backend will be added
and at that point all blending work will take place inside XP's.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/7c66342a399b529634bed0fabfaa562db2c0dbd4

Committed: https://skia.googlesource.com/skia/+/9513143efa734bef0c1a0c7f945022572dbc8518

[egdaniel, 2014-11-26 20:19:49]

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

[egdaniel, 2014-11-26 20:19:50]

This is part 2 of XP stuff.

[egdaniel, 2014-12-04 19:16:06]

ptal. Updated from previously landed cls

[bsalomon, 2014-12-04 21:54:24]

https://codereview.chromium.org/759713002/diff/60001/gm/texdata.cpp
File gm/texdata.cpp (right):

https://codereview.chromium.org/759713002/diff/60001/gm/texdata.cpp#newcode102
gm/texdata.cpp:102: GrXPFactory* xpFactory =
GrPorterDuffXPFactory::Create(SkXfermode::kSrcOver_Mode);
I think we should have a shortcut on GrPaint/GrDS,
setPorterDuffMode(SkXfermode::Mode);

https://codereview.chromium.org/759713002/diff/60001/include/gpu/GrPaint.h
File include/gpu/GrPaint.h (left):

https://codereview.chromium.org/759713002/diff/60001/include/gpu/GrPaint.h#ol...
include/gpu/GrPaint.h:54: * Sets the blending coefficients to use to blend the
final primitive color with the
W00000000000000000000000000000000000T

https://codereview.chromium.org/759713002/diff/60001/include/gpu/GrXferProces...
File include/gpu/GrXferProcessor.h (right):

https://codereview.chromium.org/759713002/diff/60001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:36: enum BlendOptFlags {
Howabout just OptFlags? It's already scoped by GrXFERProcessor.

https://codereview.chromium.org/759713002/diff/60001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:84: virtual void getBlendInfo(BlendInfo*
blendInfo) const {
= 0?

https://codereview.chromium.org/759713002/diff/60001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:144: // TODO: remove need for isCoverageDrawing
once coverageDrawing is its own XP.
Do we need colorWriteDisabled?

Can we just not emit an XP in that case or something? Seems like we need some
thought on what it means to have an XP and color writes disabled.

[egdaniel, 2014-12-08 14:52:06]

updated

https://codereview.chromium.org/759713002/diff/60001/gm/texdata.cpp
File gm/texdata.cpp (right):

https://codereview.chromium.org/759713002/diff/60001/gm/texdata.cpp#newcode102
gm/texdata.cpp:102: GrXPFactory* xpFactory =
GrPorterDuffXPFactory::Create(SkXfermode::kSrcOver_Mode);
On 2014/12/04 21:54:24, bsalomon wrote:
> I think we should have a shortcut on GrPaint/GrDS,
> setPorterDuffMode(SkXfermode::Mode);
> 

Done.

https://codereview.chromium.org/759713002/diff/60001/include/gpu/GrXferProces...
File include/gpu/GrXferProcessor.h (right):

https://codereview.chromium.org/759713002/diff/60001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:36: enum BlendOptFlags {
On 2014/12/04 21:54:24, bsalomon wrote:
> Howabout just OptFlags? It's already scoped by GrXFERProcessor.

Done.

https://codereview.chromium.org/759713002/diff/60001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:84: virtual void getBlendInfo(BlendInfo*
blendInfo) const {
On 2014/12/04 21:54:24, bsalomon wrote:
> = 0?

Done.

https://codereview.chromium.org/759713002/diff/60001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:144: // TODO: remove need for isCoverageDrawing
once coverageDrawing is its own XP.
On 2014/12/04 21:54:24, bsalomon wrote:
> Do we need colorWriteDisabled?
> 
> Can we just not emit an XP in that case or something? Seems like we need some
> thought on what it means to have an XP and color writes disabled.

Nothing changed as of now. But from our previous discussion later on we will ad
a colorWriteDisabled XP that will handle a lot of the logic here.

Added some additional todo comments on where to remove colorWriteDisabled logic.

[bsalomon, 2014-12-08 15:38:52]

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

https://codereview.chromium.org/759713002/diff/80001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:338:
drawState->setPorterDuffXPFactory(kOne_GrBlendCoeff, kZero_GrBlendCoeff);
Just FYI, these will all be coverage drawing xp in the future.

https://codereview.chromium.org/759713002/diff/80001/src/gpu/GrDistanceFieldT...
File src/gpu/GrDistanceFieldTextContext.cpp (right):

https://codereview.chromium.org/759713002/diff/80001/src/gpu/GrDistanceFieldT...
src/gpu/GrDistanceFieldTextContext.cpp:640: //GrColor colorNoPreMul =
skcolor_to_grcolor_nopremultiply(filteredColor);
remove this?

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

https://codereview.chromium.org/759713002/diff/80001/src/gpu/GrDrawState.h#ne...
src/gpu/GrDrawState.h:235: 
extra \n

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

https://codereview.chromium.org/759713002/diff/80001/src/gpu/GrOptDrawState.c...
src/gpu/GrOptDrawState.cpp:35: if (xferProcessor.get()) {
xferProcessor casts to bool, don't need .get()

https://codereview.chromium.org/759713002/diff/80001/src/gpu/GrOptDrawState.c...
src/gpu/GrOptDrawState.cpp:50: if (NULL == xferProcessor.get() ||
((GrXferProcessor::kSkipDraw_OptFlag & optFlags) &&
if (!xferProcessor || ...

https://codereview.chromium.org/759713002/diff/80001/src/gpu/effects/GrPorter...
File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right):

https://codereview.chromium.org/759713002/diff/80001/src/gpu/effects/GrPorter...
src/gpu/effects/GrPorterDuffXferProcessor.cpp:67: : fSrcBlend(srcBlend),
fDstBlend(dstBlend), fBlendConstant(constant) {
Why does this take a constant? Shouldn't it deduce the constant from the
invariant output of the color stages?

[egdaniel, 2014-12-08 16:23:13]

Patchset #6 (id:100001) has been deleted

[egdaniel, 2014-12-08 16:24:02]

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

https://codereview.chromium.org/759713002/diff/80001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:338:
drawState->setPorterDuffXPFactory(kOne_GrBlendCoeff, kZero_GrBlendCoeff);
On 2014/12/08 15:38:51, bsalomon wrote:
> Just FYI, these will all be coverage drawing xp in the future.

Okay. Added todo to annotate this fact.

https://codereview.chromium.org/759713002/diff/80001/src/gpu/GrDistanceFieldT...
File src/gpu/GrDistanceFieldTextContext.cpp (right):

https://codereview.chromium.org/759713002/diff/80001/src/gpu/GrDistanceFieldT...
src/gpu/GrDistanceFieldTextContext.cpp:640: //GrColor colorNoPreMul =
skcolor_to_grcolor_nopremultiply(filteredColor);
On 2014/12/08 15:38:51, bsalomon wrote:
> remove this?

Done.

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

https://codereview.chromium.org/759713002/diff/80001/src/gpu/GrDrawState.h#ne...
src/gpu/GrDrawState.h:235: 
On 2014/12/08 15:38:51, bsalomon wrote:
> extra \n

Done.

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

https://codereview.chromium.org/759713002/diff/80001/src/gpu/GrOptDrawState.c...
src/gpu/GrOptDrawState.cpp:35: if (xferProcessor.get()) {
On 2014/12/08 15:38:51, bsalomon wrote:
> xferProcessor casts to bool, don't need .get()

Done.

https://codereview.chromium.org/759713002/diff/80001/src/gpu/GrOptDrawState.c...
src/gpu/GrOptDrawState.cpp:50: if (NULL == xferProcessor.get() ||
((GrXferProcessor::kSkipDraw_OptFlag & optFlags) &&
On 2014/12/08 15:38:51, bsalomon wrote:
> if (!xferProcessor || ...

Done.

https://codereview.chromium.org/759713002/diff/80001/src/gpu/effects/GrPorter...
File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right):

https://codereview.chromium.org/759713002/diff/80001/src/gpu/effects/GrPorter...
src/gpu/effects/GrPorterDuffXferProcessor.cpp:67: : fSrcBlend(srcBlend),
fDstBlend(dstBlend), fBlendConstant(constant) {
On 2014/12/08 15:38:51, bsalomon wrote:
> Why does this take a constant? Shouldn't it deduce the constant from the
> invariant output of the color stages?

The currently the constant gets deduced during the createXferProcessor(...) when
we go from XPF->XP. I choose here since this is the same spot where we are
looking for RGB coverage and changing coeffs. The other option would be to put
the logic in the getOptimizations call on XP, but setting the blend constant
doesn't really feel like an optimization as much as a requirement.

Another option would be for the constructor to take in the color and coverage
invariants and shift the logic from the createXferProcessor to the XP::Create()
or XP:ctor() functions.

[bsalomon, 2014-12-08 18:35:22]

https://codereview.chromium.org/759713002/diff/80001/src/gpu/effects/GrPorter...
File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right):

https://codereview.chromium.org/759713002/diff/80001/src/gpu/effects/GrPorter...
src/gpu/effects/GrPorterDuffXferProcessor.cpp:67: : fSrcBlend(srcBlend),
fDstBlend(dstBlend), fBlendConstant(constant) {
On 2014/12/08 16:24:01, egdaniel wrote:
> On 2014/12/08 15:38:51, bsalomon wrote:
> > Why does this take a constant? Shouldn't it deduce the constant from the
> > invariant output of the color stages?
> 
> The currently the constant gets deduced during the createXferProcessor(...)
when
> we go from XPF->XP. I choose here since this is the same spot where we are
> looking for RGB coverage and changing coeffs. The other option would be to put
> the logic in the getOptimizations call on XP, but setting the blend constant
> doesn't really feel like an optimization as much as a requirement.
> 
> Another option would be for the constructor to take in the color and coverage
> invariants and shift the logic from the createXferProcessor to the
XP::Create()
> or XP:ctor() functions.

You can ignore my comment. I was thinking this was the PDXPF cons not PDXP.
lgtm

[egdaniel, 2014-12-08 18:53:52]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-12-08 18:54:11]

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

[commit-bot: I haz the power, 2014-12-08 18:56:14]

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

[commit-bot: I haz the power, 2014-12-08 18:56:15]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)

[egdaniel, 2014-12-08 19:10:59]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-12-08 19:11:14]

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

[commit-bot: I haz the power, 2014-12-08 19:20:45]

Committed patchset #7 (id:140001) as
https://skia.googlesource.com/skia/+/7c66342a399b529634bed0fabfaa562db2c0dbd4

[egdaniel, 2014-12-08 21:25:25]

A revert of this CL (patchset #7 id:140001) has been created in
https://codereview.chromium.org/766653008/ by egdaniel@google.com.

The reason for reverting is: break many gm's.

[egdaniel, 2014-12-09 17:41:10]

Minor update to work with new LCD check

[bsalomon, 2014-12-09 18:02:13]

lgtm

[egdaniel, 2014-12-09 19:11:57]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-12-09 19:12:52]

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

[commit-bot: I haz the power, 2014-12-09 19:15:56]

Committed patchset #9 (id:180001) as
https://skia.googlesource.com/skia/+/9513143efa734bef0c1a0c7f945022572dbc8518