Add XferProcessor factory in GrPaint and GrDrawState.

Add XferProcessor factory in GrPaint and GrDrawState.

In this CL the XP should have zero effect on the actual rendering pipeline.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/378092f3d10b1dd62967f419c35cfefec7c10ee7

[egdaniel, 2014-11-26 19:26:48]

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

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

Part 1 of getting XP's in

[joshualitt, 2014-11-26 20:10:33]

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

[joshualitt, 2014-11-26 20:10:35]

some nits, looks good to me, but Brian should definitely take a look too.

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

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:27: */
Comment refers to GP

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:42: */
Is this code supposed to be commented out?

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:66: 
Is there a way to name this more generally?  ie, what is special about the kind
of blend mode LCD text requires?  I might not know what I'm talking about here
though.

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/GrPaint.cpp#newc...
src/gpu/GrPaint.cpp:125: 
extra newlines

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/effects/GrPorter...
src/gpu/effects/GrPorterDuffXferProcessor.cpp:36: fsBuilder->codeAppendf("\t\t%s
= %s;\n", outputColor, inputColor);
no need for \t or \n

[bsalomon, 2014-11-26 21:02:45]

https://codereview.chromium.org/751283002/diff/80001/include/core/SkXfermode.h
File include/core/SkXfermode.h (right):

https://codereview.chromium.org/751283002/diff/80001/include/core/SkXfermode....
include/core/SkXfermode.h:205: to call this with xpf NULL to simply test the
return value. If effect is non-NULL
Use "factory" instead of "effect"? I don't really follow the comment.

https://codereview.chromium.org/751283002/diff/80001/include/core/SkXfermode....
include/core/SkXfermode.h:219: static bool
asFragmentProcessorOrXPFactoryOrCoeff(SkXfermode*, GrFragmentProcessor**,
Yikes! I guess its just temporary. Do we really need coeff if we have
porter-duff xpf?

static member functions are capitalized.

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrPaint.h
File include/gpu/GrPaint.h (right):

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrPaint.h#ne...
include/gpu/GrPaint.h:83: SkASSERT(xpFactory);
This assert is done in SkRef().

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrPaint.h#ne...
include/gpu/GrPaint.h:136: SkASSERT(paint.hasXPFactory());
Why this assert?

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

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:31: true when the two prceossor are of the same
subclass (i.e. they return the same object from
Does this comment apply? Will these have a backend effect factory?

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:37: bool isEqual(const GrXferProcessor& that)
const {
non-virtual override?

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:38: /*
?

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:63: virtual const GrXferProcessor*
createXferProcessor() const = 0;
no inputs?

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:65: virtual bool supportsLCDText(const
GrDrawState&) const { return false; }
Can we call it LCDCoverage or RGBCoverage rather than LCDText?

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:67: virtual bool canApplyCoverage(const
GrDrawState&) const { return false; }
This and the above need some clear documentation about what they mean and what
answering true implies for the xp

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:89: #define GR_CREATE_STATIC_XP_FACTORY(NAME,
XPF_CLASS, ARGS)                                     \
You must be coming from a really old revision. There is now
GR_CREATE_STATIC_PROCESSOR(NAME, PROC_CLASS, ARGS) which should with any
processor subclass. I think you can just delete this.

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/GrDrawState.cpp#...
src/gpu/GrDrawState.cpp:135: fXPFactory.reset(GrDefaultXPFactory::Create());
why not NULL? Or if not, should we have the hasXPFactory() method since it seems
like we will always have one.

https://codereview.chromium.org/751283002/diff/80001/src/gpu/GrDrawState.cpp#...
src/gpu/GrDrawState.cpp:674: return (fColorProcInfo.isSingleComponent());
fCoverageProcInfo?

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/GrDrawState.h#ne...
src/gpu/GrDrawState.h:221: bool coverageWillBeSingleComponent() const;
needs to be public?

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/GrOptDrawState.c...
src/gpu/GrOptDrawState.cpp:113: SkSafeUnref(xpProcessor);
SkUnref() or ->unref(), no need for safety, right? Or can this the factory fail?
If so, do we not draw?

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/GrPaint.cpp#newc...
src/gpu/GrPaint.cpp:55: fXPFactory.reset(GrDefaultXPFactory::Create());
again, do we need the hasXPFactory if we install one by default?

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/GrXferProcessor....
src/gpu/GrXferProcessor.cpp:1: 
copyright header  + #include the header?

At least it what ensure the header's includes are sufficient.

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/SkGr.cpp#newcode488
src/gpu/SkGr.cpp:488: xpFactory = GrPorterDuffXPFactory::Create(sm, dm);
why not make the asThisThatOrTheOther function do this?

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/effects/GrPorter...
src/gpu/effects/GrPorterDuffXferProcessor.cpp:33: const TransformedCoordsArray&
coords,
Do these need coords? I don't think XPs should be allowed to use coordxforms..
skxfermodes are supposed to be position-independent. I guess this is because it
derives from fp?

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/effects/GrPorter...
src/gpu/effects/GrPorterDuffXferProcessor.h:33: virtual bool isValidColorStage()
const SK_OVERRIDE { return false; }
no need for virtual keyword w/ override.

where is tnis coming from? In my local checkout I don't see this virtual.

https://codereview.chromium.org/751283002/diff/80001/src/gpu/gl/GrGLXferProce...
File src/gpu/gl/GrGLXferProcessor.h (right):

https://codereview.chromium.org/751283002/diff/80001/src/gpu/gl/GrGLXferProce...
src/gpu/gl/GrGLXferProcessor.h:16: /**
All the comments in this file seem to be related to GrGLGeometryProcessor

[egdaniel, 2014-12-01 18:18:25]

Here is an updated version. The first version for whatever reason had a lot of
changes that were fixed in the follow up cl's but didn't get propagated back to
this first one. This version should be a lot cleaner.

https://codereview.chromium.org/751283002/diff/80001/include/core/SkXfermode.h
File include/core/SkXfermode.h (right):

https://codereview.chromium.org/751283002/diff/80001/include/core/SkXfermode....
include/core/SkXfermode.h:205: to call this with xpf NULL to simply test the
return value. If effect is non-NULL
On 2014/11/26 21:02:44, bsalomon wrote:
> Use "factory" instead of "effect"? I don't really follow the comment.

Done.

https://codereview.chromium.org/751283002/diff/80001/include/core/SkXfermode....
include/core/SkXfermode.h:219: static bool
asFragmentProcessorOrXPFactoryOrCoeff(SkXfermode*, GrFragmentProcessor**,
On 2014/11/26 21:02:44, bsalomon wrote:
> Yikes! I guess its just temporary. Do we really need coeff if we have
> porter-duff xpf?
> 
> static member functions are capitalized.

Better for now?

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrPaint.h
File include/gpu/GrPaint.h (right):

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrPaint.h#ne...
include/gpu/GrPaint.h:83: SkASSERT(xpFactory);
On 2014/11/26 21:02:44, bsalomon wrote:
> This assert is done in SkRef().

removed

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrPaint.h#ne...
include/gpu/GrPaint.h:136: SkASSERT(paint.hasXPFactory());
On 2014/11/26 21:02:44, bsalomon wrote:
> Why this assert?

gone

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

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:27: */
On 2014/11/26 20:10:34, joshualitt wrote:
> Comment refers to GP

Done.

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:31: true when the two prceossor are of the same
subclass (i.e. they return the same object from
On 2014/11/26 21:02:44, bsalomon wrote:
> Does this comment apply? Will these have a backend effect factory?

In this current version the XP is derived from a fragment processor so it does
have a backend effect factory from them. In the final XP cl, xp's will have
their own BEF and this statement will be valid.

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:37: bool isEqual(const GrXferProcessor& that)
const {
On 2014/11/26 21:02:45, bsalomon wrote:
> non-virtual override?

Code deleted since coeff's have been moved off base class

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:38: /*
On 2014/11/26 21:02:45, bsalomon wrote:
> ?

Done.

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:42: */
On 2014/11/26 20:10:34, joshualitt wrote:
> Is this code supposed to be commented out?

nope

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:63: virtual const GrXferProcessor*
createXferProcessor() const = 0;
On 2014/11/26 21:02:44, bsalomon wrote:
> no inputs?

In this cl it has none, but in the next DrawState is added in as input since we
will start doing blend optimizations there

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:65: virtual bool supportsLCDText(const
GrDrawState&) const { return false; }
On 2014/11/26 21:02:44, bsalomon wrote:
> Can we call it LCDCoverage or RGBCoverage rather than LCDText?

Done.

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:67: virtual bool canApplyCoverage(const
GrDrawState&) const { return false; }
On 2014/11/26 21:02:44, bsalomon wrote:
> This and the above need some clear documentation about what they mean and what
> answering true implies for the xp

Removed for now since it is unused in this CL. Will add back with documentation
on follow up cl that calls it.

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:89: #define GR_CREATE_STATIC_XP_FACTORY(NAME,
XPF_CLASS, ARGS)                                     \
On 2014/11/26 21:02:44, bsalomon wrote:
> You must be coming from a really old revision. There is now
> GR_CREATE_STATIC_PROCESSOR(NAME, PROC_CLASS, ARGS) which should with any
> processor subclass. I think you can just delete this.

This isn't a GrProcessor subclass, it is for GrXPFactory. Comment updated.

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/GrDrawState.cpp#...
src/gpu/GrDrawState.cpp:135: fXPFactory.reset(GrDefaultXPFactory::Create());
On 2014/11/26 21:02:45, bsalomon wrote:
> why not NULL? Or if not, should we have the hasXPFactory() method since it
seems
> like we will always have one.

I always want there to be an XP so a default one is always installed. Has
XPFactory() has been removed (it was only there really for debugging to assert
that I always did install and XP)

https://codereview.chromium.org/751283002/diff/80001/src/gpu/GrDrawState.cpp#...
src/gpu/GrDrawState.cpp:674: return (fColorProcInfo.isSingleComponent());
On 2014/11/26 21:02:45, bsalomon wrote:
> fCoverageProcInfo?

yeah was fixed in follow up cl, missed propagating the change up to this one.
Fixed

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/GrDrawState.h#ne...
src/gpu/GrDrawState.h:221: bool coverageWillBeSingleComponent() const;
On 2014/11/26 21:02:45, bsalomon wrote:
> needs to be public?

It is a query used by PD-XPF for checking RGBCoverage. Currently we hide DS's
invariant output from all classes except GrOptDrawState. I don't want to open
the flood gate to friending all effects.

I guess the change we can make is to have this function (and in future cl's
createXferProcessor(...)) take in the invariant color and coverage instead of
the draw state. But this does mess with the query's in the text context for
applyRGBCoverage since they won't have access to the DS invariants to pass in.

I kind of feel in the end public queries on the DS end up being clearer and
cleaner.

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/GrOptDrawState.c...
src/gpu/GrOptDrawState.cpp:113: SkSafeUnref(xpProcessor);
On 2014/11/26 21:02:45, bsalomon wrote:
> SkUnref() or ->unref(), no need for safety, right? Or can this the factory
fail?
> If so, do we not draw?

correct

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/GrPaint.cpp#newc...
src/gpu/GrPaint.cpp:55: fXPFactory.reset(GrDefaultXPFactory::Create());
On 2014/11/26 21:02:45, bsalomon wrote:
> again, do we need the hasXPFactory if we install one by default?

gone

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/GrXferProcessor....
src/gpu/GrXferProcessor.cpp:1: 
On 2014/11/26 21:02:45, bsalomon wrote:
> copyright header  + #include the header?
> 
> At least it what ensure the header's includes are sufficient.

Or just delete this file since it is never used ever

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/SkGr.cpp#newcode488
src/gpu/SkGr.cpp:488: xpFactory = GrPorterDuffXPFactory::Create(sm, dm);
On 2014/11/26 21:02:45, bsalomon wrote:
> why not make the asThisThatOrTheOther function do this?

Moved to awseomeOrGreatOrPerfectorAmazingFunctionNameFunction

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/effects/GrPorter...
src/gpu/effects/GrPorterDuffXferProcessor.cpp:33: const TransformedCoordsArray&
coords,
On 2014/11/26 21:02:45, bsalomon wrote:
> Do these need coords? I don't think XPs should be allowed to use coordxforms..
> skxfermodes are supposed to be position-independent. I guess this is because
it
> derives from fp?

Yeah for now that it comes from fp it needs it. Fixed in follow up cl where it
has its own backend

https://codereview.chromium.org/751283002/diff/80001/src/gpu/effects/GrPorter...
src/gpu/effects/GrPorterDuffXferProcessor.cpp:36: fsBuilder->codeAppendf("\t\t%s
= %s;\n", outputColor, inputColor);
On 2014/11/26 20:10:34, joshualitt wrote:
> no need for \t or \n

fine

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/effects/GrPorter...
src/gpu/effects/GrPorterDuffXferProcessor.h:33: virtual bool isValidColorStage()
const SK_OVERRIDE { return false; }
On 2014/11/26 21:02:45, bsalomon wrote:
> no need for virtual keyword w/ override.
> 
> where is tnis coming from? In my local checkout I don't see this virtual.

Old code that somehow didn't get deleted in clean up phase.

https://codereview.chromium.org/751283002/diff/80001/src/gpu/gl/GrGLXferProce...
File src/gpu/gl/GrGLXferProcessor.h (right):

https://codereview.chromium.org/751283002/diff/80001/src/gpu/gl/GrGLXferProce...
src/gpu/gl/GrGLXferProcessor.h:16: /**
On 2014/11/26 21:02:45, bsalomon wrote:
> All the comments in this file seem to be related to GrGLGeometryProcessor

This file is actually not even needed till the final cl so I am just going to
delete it for now

[bsalomon, 2014-12-01 19:24:40]

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

https://codereview.chromium.org/751283002/diff/80001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:89: #define GR_CREATE_STATIC_XP_FACTORY(NAME,
XPF_CLASS, ARGS)                                     \
On 2014/12/01 18:18:24, egdaniel wrote:
> On 2014/11/26 21:02:44, bsalomon wrote:
> > You must be coming from a really old revision. There is now
> > GR_CREATE_STATIC_PROCESSOR(NAME, PROC_CLASS, ARGS) which should with any
> > processor subclass. I think you can just delete this.
> 
> This isn't a GrProcessor subclass, it is for GrXPFactory. Comment updated.

Acknowledged.

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/GrDrawState.cpp#...
src/gpu/GrDrawState.cpp:674: return (fColorProcInfo.isSingleComponent());
On 2014/12/01 18:18:24, egdaniel wrote:
> On 2014/11/26 21:02:45, bsalomon wrote:
> > fCoverageProcInfo?
> 
> yeah was fixed in follow up cl, missed propagating the change up to this one.
> Fixed

I'm still not seeing it.

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

https://codereview.chromium.org/751283002/diff/80001/src/gpu/GrDrawState.h#ne...
src/gpu/GrDrawState.h:221: bool coverageWillBeSingleComponent() const;
On 2014/12/01 18:18:24, egdaniel wrote:
> On 2014/11/26 21:02:45, bsalomon wrote:
> > needs to be public?
> 
> It is a query used by PD-XPF for checking RGBCoverage. Currently we hide DS's
> invariant output from all classes except GrOptDrawState. I don't want to open
> the flood gate to friending all effects.

I don't think PD-XPF should be allowed to see GrDrawState at all. It's private.

> 
> I guess the change we can make is to have this function (and in future cl's
> createXferProcessor(...)) take in the invariant color and coverage instead of
> the draw state. But this does mess with the query's in the text context for
> applyRGBCoverage since they won't have access to the DS invariants to pass in.

I think we need some encapsulation of just the info that XP(F) is allowed to
see.

> 
> I kind of feel in the end public queries on the DS end up being clearer and
> cleaner.

https://codereview.chromium.org/751283002/diff/120001/include/core/SkXfermode.h
File include/core/SkXfermode.h (right):

https://codereview.chromium.org/751283002/diff/120001/include/core/SkXfermode...
include/core/SkXfermode.h:203: // We will always use the XPProcessor if there is
not background texture and the fragment if
XPFactory?

https://codereview.chromium.org/751283002/diff/120001/include/core/SkXfermode...
include/core/SkXfermode.h:208: to call this with xpf NULL to simply test the
return value. If factory is non-NULL
My mistake before, I think it should be "If xpf is non-NULL ..."

https://codereview.chromium.org/751283002/diff/120001/include/core/SkXfermode...
include/core/SkXfermode.h:222: static bool
AsFragmentProcessorOrXPFactory(SkXfermode*, GrFragmentProcessor**,
Can't wait for this to die. AFAIK there is only one caller and it never passes
anything but NULL for background. Can we remove that functionality now?

https://codereview.chromium.org/751283002/diff/120001/include/gpu/GrXferProce...
File include/gpu/GrXferProcessor.h (right):

https://codereview.chromium.org/751283002/diff/120001/include/gpu/GrXferProce...
include/gpu/GrXferProcessor.h:19: * A GrXferProcessor is used to combine the
output of the color and coverage stages into one or two
I think this comment will be confusing to someone who is extending Skia and not
familiar with Ganesh's internals. Here is my attempt at rewriting a subset of
the comment:

GrXferProcessor is responsible for implementing the xfer mode that blends the
src color and dst color. It does this by emitting fragment shader code and
controlling the fixed-function blend state. The inputs to its shader code are
the final computed src color and fractional pixel coverage. The
GrXferProcessor's shader code writes the fragment shader output color that goes
into the fixed-function blend. When dual-source blending is available, it may
also write a secondary fragment shader output color. When allowed by the backend
API, the GrXferProcessor may read the destination color. The GrXferProcessor is
responsible for setting the blend coefficients and blend constant color.

https://codereview.chromium.org/751283002/diff/120001/include/gpu/GrXferProce...
include/gpu/GrXferProcessor.h:31: virtual bool onIsEqual(const
GrFragmentProcessor&) const = 0;
pure virtual override?

https://codereview.chromium.org/751283002/diff/120001/include/gpu/GrXferProce...
include/gpu/GrXferProcessor.h:40: virtual bool supportsRGBCoverage(const
GrDrawState&) const { return false; }
comment about what this means?

Also, GrDrawState is not public so can't appear in a public header. I realize it
is forward declared, but an implementer of this function outside of src/ is not
allowed to know its definition.

https://codereview.chromium.org/751283002/diff/120001/src/gpu/effects/GrDefau...
File src/gpu/effects/GrDefaultXferProcessor.h (right):

https://codereview.chromium.org/751283002/diff/120001/src/gpu/effects/GrDefau...
src/gpu/effects/GrDefaultXferProcessor.h:13: class GrDefaultXPFactory : public
GrPorterDuffXPFactory {
Does it make sense to have this at all?

https://codereview.chromium.org/751283002/diff/120001/src/gpu/effects/GrPorte...
File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right):

https://codereview.chromium.org/751283002/diff/120001/src/gpu/effects/GrPorte...
src/gpu/effects/GrPorterDuffXferProcessor.cpp:70: 0 ==
drawState.numColorStages() && !drawState.coverageWillBeSingleComponent()) {
so what if this took something like

class GrXPFactory::InputInfo {
   GrColor  fKnownColorValues;
   uint32_t fKnownColorFlags;
   enum CoverageType {
       kSolid_CoverageType,
       kSingleComponent_CoverageType,
       kRGBA_CoverageType,
   } fCoverageType;
}

But why is it told what the coverage is single channel? It seems like we need to
ask this question before installing a four channel coverage.

https://codereview.chromium.org/751283002/diff/120001/src/gpu/effects/GrPorte...
File src/gpu/effects/GrPorterDuffXferProcessor.h (right):

https://codereview.chromium.org/751283002/diff/120001/src/gpu/effects/GrPorte...
src/gpu/effects/GrPorterDuffXferProcessor.h:60: virtual const GrXferProcessor*
createXferProcessor() const SK_OVERRIDE;
minor, no need for virtual w/ SK_OVERRIDE here and below.

[egdaniel, 2014-12-02 14:57:40]

Patchset #8 (id:140001) has been deleted

[egdaniel, 2014-12-02 15:07:10]

updates

https://codereview.chromium.org/751283002/diff/120001/include/core/SkXfermode.h
File include/core/SkXfermode.h (right):

https://codereview.chromium.org/751283002/diff/120001/include/core/SkXfermode...
include/core/SkXfermode.h:203: // We will always use the XPProcessor if there is
not background texture and the fragment if
On 2014/12/01 19:24:39, bsalomon wrote:
> XPFactory?

Done.

https://codereview.chromium.org/751283002/diff/120001/include/core/SkXfermode...
include/core/SkXfermode.h:208: to call this with xpf NULL to simply test the
return value. If factory is non-NULL
On 2014/12/01 19:24:39, bsalomon wrote:
> My mistake before, I think it should be "If xpf is non-NULL ..."

Done.

https://codereview.chromium.org/751283002/diff/120001/include/core/SkXfermode...
include/core/SkXfermode.h:222: static bool
AsFragmentProcessorOrXPFactory(SkXfermode*, GrFragmentProcessor**,
On 2014/12/01 19:24:39, bsalomon wrote:
> Can't wait for this to die. AFAIK there is only one caller and it never passes
> anything but NULL for background. Can we remove that functionality now?

Done.

https://codereview.chromium.org/751283002/diff/120001/include/gpu/GrXferProce...
File include/gpu/GrXferProcessor.h (right):

https://codereview.chromium.org/751283002/diff/120001/include/gpu/GrXferProce...
include/gpu/GrXferProcessor.h:19: * A GrXferProcessor is used to combine the
output of the color and coverage stages into one or two
On 2014/12/01 19:24:39, bsalomon wrote:
> I think this comment will be confusing to someone who is extending Skia and
not
> familiar with Ganesh's internals. Here is my attempt at rewriting a subset of
> the comment:
> 
> GrXferProcessor is responsible for implementing the xfer mode that blends the
> src color and dst color. It does this by emitting fragment shader code and
> controlling the fixed-function blend state. The inputs to its shader code are
> the final computed src color and fractional pixel coverage. The
> GrXferProcessor's shader code writes the fragment shader output color that
goes
> into the fixed-function blend. When dual-source blending is available, it may
> also write a secondary fragment shader output color. When allowed by the
backend
> API, the GrXferProcessor may read the destination color. The GrXferProcessor
is
> responsible for setting the blend coefficients and blend constant color.
> 

Done.

https://codereview.chromium.org/751283002/diff/120001/include/gpu/GrXferProce...
include/gpu/GrXferProcessor.h:31: virtual bool onIsEqual(const
GrFragmentProcessor&) const = 0;
On 2014/12/01 19:24:40, bsalomon wrote:
> pure virtual override?

because we all love code that does nothing of course

https://codereview.chromium.org/751283002/diff/120001/include/gpu/GrXferProce...
include/gpu/GrXferProcessor.h:40: virtual bool supportsRGBCoverage(const
GrDrawState&) const { return false; }
On 2014/12/01 19:24:40, bsalomon wrote:
> comment about what this means?
> 
> Also, GrDrawState is not public so can't appear in a public header. I realize
it
> is forward declared, but an implementer of this function outside of src/ is
not
> allowed to know its definition. 

Done.

https://codereview.chromium.org/751283002/diff/120001/src/gpu/effects/GrPorte...
File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right):

https://codereview.chromium.org/751283002/diff/120001/src/gpu/effects/GrPorte...
src/gpu/effects/GrPorterDuffXferProcessor.cpp:70: 0 ==
drawState.numColorStages() && !drawState.coverageWillBeSingleComponent()) {
On 2014/12/01 19:24:40, bsalomon wrote:
> so what if this took something like
> 
> class GrXPFactory::InputInfo {
>    GrColor  fKnownColorValues;
>    uint32_t fKnownColorFlags;
>    enum CoverageType {
>        kSolid_CoverageType,
>        kSingleComponent_CoverageType,
>        kRGBA_CoverageType,
>    } fCoverageType;
> }
> 
> But why is it told what the coverage is single channel? It seems like we need
to
> ask this question before installing a four channel coverage.

Done partially. The change to actually check before adding the the four channel
coverage will involve a decent amount of changes in the text contexts. So for
now we will function as we previously have and once all the initial XP cl's land
I will go back in a follow up cl to fix up the text stuff.

https://codereview.chromium.org/751283002/diff/120001/src/gpu/effects/GrPorte...
File src/gpu/effects/GrPorterDuffXferProcessor.h (right):

https://codereview.chromium.org/751283002/diff/120001/src/gpu/effects/GrPorte...
src/gpu/effects/GrPorterDuffXferProcessor.h:60: virtual const GrXferProcessor*
createXferProcessor() const SK_OVERRIDE;
On 2014/12/01 19:24:40, bsalomon wrote:
> minor, no need for virtual w/ SK_OVERRIDE here and below.

Done.

[bsalomon, 2014-12-02 15:36:32]

https://codereview.chromium.org/751283002/diff/160001/include/gpu/GrXferProce...
File include/gpu/GrXferProcessor.h (right):

https://codereview.chromium.org/751283002/diff/160001/include/gpu/GrXferProce...
include/gpu/GrXferProcessor.h:20: * coverage. The GrXferProcessor's shader code
writes the fragment shader output color that coes
coes->goes

https://codereview.chromium.org/751283002/diff/160001/include/gpu/GrXferProce...
include/gpu/GrXferProcessor.h:26: class GrXferProcessor : public
GrFragmentProcessor {
Somewhere we need an explanation (from a subclass author's perspective) of why
an XP and XPF. That is what there roles are and how they relate to each other.
Maybe something along the lines of the XPF is installed early before all the
other contributions to the shader are known (e.g. unknown whether we'll have to
consider fractional pixel coverage, whether the draw is opaque, ...). It exists
to install an XP that implements the desired blend once everything is finalized
and to answer queries about the capabilities of the XPs it can create. (this is
vague, needs specificity).

https://codereview.chromium.org/751283002/diff/160001/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/751283002/diff/160001/src/core/SkXfermode.cpp...
src/core/SkXfermode.cpp:703: *src = SkXfermode::kZero_Coeff;
Should this be dst?

Is it possible to stop having src/dst coeff params to this function? It seems
like the coeffs are always either baked into the XPF or in the case of an FP
result can be assumed to be one/zero.

https://codereview.chromium.org/751283002/diff/160001/src/gpu/GrDrawState.cpp
File src/gpu/GrDrawState.cpp (right):

https://codereview.chromium.org/751283002/diff/160001/src/gpu/GrDrawState.cpp...
src/gpu/GrDrawState.cpp:670: bool GrDrawState::coverageWillBeSingleComponent()
const {
Is this called anywhere?

[egdaniel, 2014-12-02 18:09:51]

another update

https://codereview.chromium.org/751283002/diff/160001/include/gpu/GrXferProce...
File include/gpu/GrXferProcessor.h (right):

https://codereview.chromium.org/751283002/diff/160001/include/gpu/GrXferProce...
include/gpu/GrXferProcessor.h:20: * coverage. The GrXferProcessor's shader code
writes the fragment shader output color that coes
On 2014/12/02 15:36:32, bsalomon wrote:
> coes->goes

Done.

https://codereview.chromium.org/751283002/diff/160001/include/gpu/GrXferProce...
include/gpu/GrXferProcessor.h:26: class GrXferProcessor : public
GrFragmentProcessor {
On 2014/12/02 15:36:32, bsalomon wrote:
> Somewhere we need an explanation (from a subclass author's perspective) of why
> an XP and XPF. That is what there roles are and how they relate to each other.
> Maybe something along the lines of the XPF is installed early before all the
> other contributions to the shader are known (e.g. unknown whether we'll have
to
> consider fractional pixel coverage, whether the draw is opaque, ...). It
exists
> to install an XP that implements the desired blend once everything is
finalized
> and to answer queries about the capabilities of the XPs it can create. (this
is
> vague, needs specificity).

Added below before GrXPFactory.

https://codereview.chromium.org/751283002/diff/160001/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/751283002/diff/160001/src/core/SkXfermode.cpp...
src/core/SkXfermode.cpp:703: *src = SkXfermode::kZero_Coeff;
On 2014/12/02 15:36:32, bsalomon wrote:
> Should this be dst?
> 
> Is it possible to stop having src/dst coeff params to this function? It seems
> like the coeffs are always either baked into the XPF or in the case of an FP
> result can be assumed to be one/zero.

Yeah should be dst. All these src and dst things go away in the next CL where
are blending is encapsulated inside the XP. In this CL blending is still handled
outside the XP.

https://codereview.chromium.org/751283002/diff/160001/src/gpu/GrDrawState.cpp
File src/gpu/GrDrawState.cpp (right):

https://codereview.chromium.org/751283002/diff/160001/src/gpu/GrDrawState.cpp...
src/gpu/GrDrawState.cpp:670: bool GrDrawState::coverageWillBeSingleComponent()
const {
On 2014/12/02 15:36:32, bsalomon wrote:
> Is this called anywhere?

Nope. Was used in the next CL when DS was passed into createXP, but I've removed
DS as a param and just pass in the invariant things directly.

[bsalomon, 2014-12-02 21:34:45]

lgtm

https://codereview.chromium.org/751283002/diff/200001/include/gpu/GrXferProce...
File include/gpu/GrXferProcessor.h (right):

https://codereview.chromium.org/751283002/diff/200001/include/gpu/GrXferProce...
include/gpu/GrXferProcessor.h:43: * creates will have. For example, will the XP
support RGB coverage or will the XP blend with the
"For example, can it create an XP that supports RGB coverage..." ?

[egdaniel, 2014-12-03 18:20:39]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-12-03 18:20:52]

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

[commit-bot: I haz the power, 2014-12-03 18:22:26]

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

[commit-bot: I haz the power, 2014-12-03 18:22:27]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)
  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-03 18:35:17]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-12-03 18:36:04]

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

[commit-bot: I haz the power, 2014-12-03 18:40:20]

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