Implement support for KHR_blend_equation_advanced

Implement support for KHR_blend_equation_advanced

Uses KHR(or NV)_blend_equation_advanced to implement custom Xfer modes
in hardware.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/8917d62ef4d9bde9ec4f879dc42b309b03a0ad98

[Chris Dalton, 2015-03-31 06:36:59]

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

[Chris Dalton, 2015-03-31 06:36:59]

I noticed ignored-tests.txt doesn't exist anymore. Is there something similar?
This will cause pixel diffs in a few gm's.

Depends on https://codereview.chromium.org/1049143002/
https://codereview.chromium.org/1039693004/

[Chris Dalton, 2015-03-31 10:51:49]

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/effects/GrAdvan...
File src/gpu/effects/GrAdvancedEquationXPFactory.cpp (right):

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/effects/GrAdvan...
src/gpu/effects/GrAdvancedEquationXPFactory.cpp:176: The only time it is still
true is when Sc == 0.
This is most likely wrong. I'll update the comment.

Question: With RGB coverage is what is the alpha channel's coverage? Is it 1? Or
is it something like the pixel-wide coverage?

[Mark Kilgard, 2015-04-03 18:30:04]

lgtm overall

please fix the extension detection to detect either the NVbea or KHRbea
extension so 10 months of drivers don't fall into the gap of just supporting
NVbea

I'm very pleased with the compositing algebra analysis in comments.  It's great
to see all the Skia blending math mapped efficiently mapped to OpenGL.

CC += Rik so he's aware of this work

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/effects/GrAdvan...
File src/gpu/effects/GrAdvancedEquationXPFactory.cpp (right):

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/effects/GrAdvan...
src/gpu/effects/GrAdvancedEquationXPFactory.cpp:76: It can be shown that when
X=Y=Z=1, canTweakAlphaForCoverage() is true.
great to see this analysis

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/effects/GrAdvan...
src/gpu/effects/GrAdvancedEquationXPFactory.cpp:128: = blend(f*Sca, Dca, f*Sa,
Da)  [definition of blend()]
great analysis

I think you should motivate the "Corner cases" analysis better by putting the
"General case" analysis first.

Then note the "General case" analysis has expressions dividing by Sa, Da, and f.
 This could be a problem if any of Sa, Da, or f were zero.

Since these values may be zero, that motivations the "Corners cases" analysis.

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/effects/GrAdvan...
src/gpu/effects/GrAdvancedEquationXPFactory.cpp:172: But when coverage is unique
per channel, this property no longer holds:
"when coverage is unique per channel" ??

huh?  What does that even mean?  Is something odd happening such as "ClearType"
style rendering?

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/effects/GrAdvan...
src/gpu/effects/GrAdvancedEquationXPFactory.cpp:178: (It would also work out if
we could know the dst color was trans black.)
I'm assume by "trans black" you mean transparent black or (0,0,0,0) for [Dca,Da]

FYI, Adobe uses the helpful phrasing "cleared to glass" for this condition.

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/effects/GrAdvan...
src/gpu/effects/GrAdvancedEquationXPFactory.cpp:182: 0 == (knownColor & ~(0xffu
<< GrColor_SHIFT_A)); // RGB=0
Is there not some constant to use instead of 0xffu?

Might be GrColor_ONE or such?

I'm assuming this expression is something about coverage not being unique by
channel but I don't really understand the motivation for such a situation in
Skia.

How often is this expression false?  Rarely I assume?

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/effects/GrAdvan...
src/gpu/effects/GrAdvancedEquationXPFactory.cpp:192: const GrProcOptInfo&
coveragePOI,
wish I knew what "XP" stands for

can I assume POI is "point of interest"?

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/gl/GrGLCaps.cpp...
src/gpu/gl/GrGLCaps.cpp:299: if
(ctxInfo.hasExtension("GL_KHR_blend_equation_advanced_coherent")) {
can you also look for NV_blend_equation_advanced_coherent here?

there was a lengthy time interval (10 months, maybe more?) before KHR
standardization where NVIDIA shipped NV_blend_equation_advanced (all Fermi and
all Kepler GPUs, except K1) & NV_blend_equation_advanced_coherent (K1 and all
Maxwell GPUs)

The KHR extension is a proper subset of the NV extension with identical enum
values for tokens in both the NV and KHR version.  So you should be able to look
for either string.

Stating this in a comment would be good historical context.

[Chris Dalton, 2015-04-17 08:37:19]

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/effects/GrAdvan...
File src/gpu/effects/GrAdvancedEquationXPFactory.cpp (right):

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/effects/GrAdvan...
src/gpu/effects/GrAdvancedEquationXPFactory.cpp:128: = blend(f*Sca, Dca, f*Sa,
Da)  [definition of blend()]
On 2015/04/03 18:30:04, Mark Kilgard wrote:
> great analysis
> 
> I think you should motivate the "Corner cases" analysis better by putting the
> "General case" analysis first.
> 
> Then note the "General case" analysis has expressions dividing by Sa, Da, and
f.
>  This could be a problem if any of Sa, Da, or f were zero.
> 
> Since these values may be zero, that motivations the "Corners cases" analysis.

Done.

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/effects/GrAdvan...
src/gpu/effects/GrAdvancedEquationXPFactory.cpp:172: But when coverage is unique
per channel, this property no longer holds:
On 2015/04/03 18:30:03, Mark Kilgard wrote:
> "when coverage is unique per channel" ??
> 
> huh?  What does that even mean?  Is something odd happening such as
"ClearType"
> style rendering?

Yeah, "RGB coverage" in the context of GrXferProcessor means "ClearType" style
rendering, where each color channel has a unique coverage.

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/effects/GrAdvan...
src/gpu/effects/GrAdvancedEquationXPFactory.cpp:182: 0 == (knownColor & ~(0xffu
<< GrColor_SHIFT_A)); // RGB=0
On 2015/04/03 18:30:03, Mark Kilgard wrote:
> Is there not some constant to use instead of 0xffu?
> 
> Might be GrColor_ONE or such?
> 
> I'm assuming this expression is something about coverage not being unique by
> channel but I don't really understand the motivation for such a situation in
> Skia.
> 
> How often is this expression false?  Rarely I assume?

That code is completely gone now.

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/effects/GrAdvan...
src/gpu/effects/GrAdvancedEquationXPFactory.cpp:192: const GrProcOptInfo&
coveragePOI,
On 2015/04/03 18:30:03, Mark Kilgard wrote:
> wish I knew what "XP" stands for
> 
> can I assume POI is "point of interest"?

XP = Xfer Processor = An object that controls the end of the fragment shader +
GL blend state to do blending.

POI = ProcOptInfo = Invariant data about the color/coverage outputs that can be
used to make optimizations in the shader.

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/1037123003/diff/80001/src/gpu/gl/GrGLCaps.cpp...
src/gpu/gl/GrGLCaps.cpp:299: if
(ctxInfo.hasExtension("GL_KHR_blend_equation_advanced_coherent")) {
On 2015/04/03 18:30:04, Mark Kilgard wrote:
> can you also look for NV_blend_equation_advanced_coherent here?
> 
> there was a lengthy time interval (10 months, maybe more?) before KHR
> standardization where NVIDIA shipped NV_blend_equation_advanced (all Fermi and
> all Kepler GPUs, except K1) & NV_blend_equation_advanced_coherent (K1 and all
> Maxwell GPUs)
> 
> The KHR extension is a proper subset of the NV extension with identical enum
> values for tokens in both the NV and KHR version.  So you should be able to
look
> for either string.
> 
> Stating this in a comment would be good historical context.

Done.

https://codereview.chromium.org/1037123003/diff/100001/src/gpu/effects/GrAdva...
File src/gpu/effects/GrAdvancedEquationXferProcessor.cpp (right):

https://codereview.chromium.org/1037123003/diff/100001/src/gpu/effects/GrAdva...
src/gpu/effects/GrAdvancedEquationXferProcessor.cpp:252: :
fXferProcessorWithCoverage.get());
TODO: This causes an assert:

  ../../src/gpu/GrMemoryPool.cpp:35: failed assertion "0 == fAllocationCnt"

Is there a workaround besides new-ing one every time?

[egdaniel, 2015-04-21 18:59:12]

https://codereview.chromium.org/1037123003/diff/120001/include/gpu/effects/Gr...
File include/gpu/effects/GrAdvancedEquationXferProcessor.h (right):

https://codereview.chromium.org/1037123003/diff/120001/include/gpu/effects/Gr...
include/gpu/effects/GrAdvancedEquationXferProcessor.h:29:
output->fBlendedColorFlags = 0;
I'm assuming there are cases where these are not always the true? Can we ever
tell what the output color is? Are there cases we don't blend with the dst
color? Any additional information can be beneficial since it may allow us to
apply other optimizations later on. Maybe look at the PorterDuffXPFactory and
some of the things we check there?

https://codereview.chromium.org/1037123003/diff/120001/src/gpu/effects/GrAdva...
File src/gpu/effects/GrAdvancedEquationXferProcessor.cpp (right):

https://codereview.chromium.org/1037123003/diff/120001/src/gpu/effects/GrAdva...
src/gpu/effects/GrAdvancedEquationXferProcessor.cpp:23:
GrXferProcessor::OptFlags getOptimizations(const GrProcOptInfo&, const
GrProcOptInfo&, bool,
Can we move this to the derived class and then return the ignore coverage flag
when we are in the case where we don't have any coverage (this allows the
Geometry processor to know it does not need to emit any coverage through the
stages).

https://codereview.chromium.org/1037123003/diff/120001/src/gpu/effects/GrAdva...
src/gpu/effects/GrAdvancedEquationXferProcessor.cpp:251: return
SkRef(coveragePOI.isSolidWhite() ? fXferProcessor.get()
As per your comment on previous patch, there really isn't a way of getting
around this without doing the SkNew's for each static that is being created.

If you wanted to avoid that, one approach would be to not templatize on
HasCoverage and have a single AdvEqXP. Then in its getOptimizations() you would
check the coverage and if it is solid set some internal data member to say your
ignoring coverage (and also return the ignore cov flag). This bool or flag then
would need to be part of your key as well. This is more how the other XPs are
organized.

Basically you have the options of leaving as is and doing the new for both
objects each time. Or doing the other way described above which means you carry
around an extra bool and slightly longer key. Either approach would be fine.

https://codereview.chromium.org/1037123003/diff/120001/src/gpu/effects/GrAdva...
src/gpu/effects/GrAdvancedEquationXferProcessor.cpp:258: if
(GrDrawTargetCaps::kAdvancedCoherent_BlendEquationSupport !=
caps.blendEquationSupport()) {
Just since I don't know these extensions very well. Is the Coherent version the
same as the normal version except it does not need barriers?

[Chris Dalton, 2015-04-21 20:39:31]

Thanks for the review! One question in the comments

https://codereview.chromium.org/1037123003/diff/120001/include/gpu/effects/Gr...
File include/gpu/effects/GrAdvancedEquationXferProcessor.h (right):

https://codereview.chromium.org/1037123003/diff/120001/include/gpu/effects/Gr...
include/gpu/effects/GrAdvancedEquationXferProcessor.h:29:
output->fBlendedColorFlags = 0;
On 2015/04/21 18:59:12, egdaniel wrote:
> I'm assuming there are cases where these are not always the true? Can we ever
> tell what the output color is? Are there cases we don't blend with the dst
> color? Any additional information can be beneficial since it may allow us to
> apply other optimizations later on. Maybe look at the PorterDuffXPFactory and
> some of the things we check there?

The KHR_blend_equation_advanced extension only adds support for the "advanced"
blend modes (the ones that can't be broken down to coefficients, plus screen).
These blend modes all use the dst color, even if the src alpha is 1, so we can't
make any conclusions about the output without knowing something about the dst.
(I copied this implementation from GrCustomXPFactory::getInvariantOutput.)

I'll try and summarize this into a comment.

https://codereview.chromium.org/1037123003/diff/120001/src/gpu/effects/GrAdva...
File src/gpu/effects/GrAdvancedEquationXferProcessor.cpp (right):

https://codereview.chromium.org/1037123003/diff/120001/src/gpu/effects/GrAdva...
src/gpu/effects/GrAdvancedEquationXferProcessor.cpp:23:
GrXferProcessor::OptFlags getOptimizations(const GrProcOptInfo&, const
GrProcOptInfo&, bool,
On 2015/04/21 18:59:12, egdaniel wrote:
> Can we move this to the derived class and then return the ignore coverage flag
> when we are in the case where we don't have any coverage (this allows the
> Geometry processor to know it does not need to emit any coverage through the
> stages).

Ok, that's a good idea.

https://codereview.chromium.org/1037123003/diff/120001/src/gpu/effects/GrAdva...
src/gpu/effects/GrAdvancedEquationXferProcessor.cpp:251: return
SkRef(coveragePOI.isSolidWhite() ? fXferProcessor.get()
On 2015/04/21 18:59:12, egdaniel wrote:
> As per your comment on previous patch, there really isn't a way of getting
> around this without doing the SkNew's for each static that is being created.
> 
> If you wanted to avoid that, one approach would be to not templatize on
> HasCoverage and have a single AdvEqXP. Then in its getOptimizations() you
would
> check the coverage and if it is solid set some internal data member to say
your
> ignoring coverage (and also return the ignore cov flag). This bool or flag
then
> would need to be part of your key as well. This is more how the other XPs are
> organized.
> 
> Basically you have the options of leaving as is and doing the new for both
> objects each time. Or doing the other way described above which means you
carry
> around an extra bool and slightly longer key. Either approach would be fine.

I'm not sure I completely follow option #1, how do I use it to avoid an SkNEW in
this function?

https://codereview.chromium.org/1037123003/diff/120001/src/gpu/effects/GrAdva...
src/gpu/effects/GrAdvancedEquationXferProcessor.cpp:258: if
(GrDrawTargetCaps::kAdvancedCoherent_BlendEquationSupport !=
caps.blendEquationSupport()) {
On 2015/04/21 18:59:12, egdaniel wrote:
> Just since I don't know these extensions very well. Is the Coherent version
the
> same as the normal version except it does not need barriers?

Yes, that's the difference.

For "coherent" mode, the GPU actually has special hardware in the ROP dedicated
to advanced blend modes that this extension allows us to use.

For "non-coherent" mode, there probably isn't any special hardware, but the
barrier lets the driver do some special mojo that isn't necessarily exposed
through the GL API.

[egdaniel, 2015-04-21 20:53:49]

https://codereview.chromium.org/1037123003/diff/120001/include/gpu/effects/Gr...
File include/gpu/effects/GrAdvancedEquationXferProcessor.h (right):

https://codereview.chromium.org/1037123003/diff/120001/include/gpu/effects/Gr...
include/gpu/effects/GrAdvancedEquationXferProcessor.h:29:
output->fBlendedColorFlags = 0;
On 2015/04/21 20:39:31, Chris Dalton wrote:
> On 2015/04/21 18:59:12, egdaniel wrote:
> > I'm assuming there are cases where these are not always the true? Can we
ever
> > tell what the output color is? Are there cases we don't blend with the dst
> > color? Any additional information can be beneficial since it may allow us to
> > apply other optimizations later on. Maybe look at the PorterDuffXPFactory
and
> > some of the things we check there?
> 
> The KHR_blend_equation_advanced extension only adds support for the "advanced"
> blend modes (the ones that can't be broken down to coefficients, plus screen).
> These blend modes all use the dst color, even if the src alpha is 1, so we
can't
> make any conclusions about the output without knowing something about the dst.
> (I copied this implementation from GrCustomXPFactory::getInvariantOutput.)
> 
> I'll try and summarize this into a comment.

Okay that makes sense. Yeah since this is implementing the same functions as
Custom was, of course they should have same known outputs.

https://codereview.chromium.org/1037123003/diff/120001/src/gpu/effects/GrAdva...
File src/gpu/effects/GrAdvancedEquationXferProcessor.cpp (right):

https://codereview.chromium.org/1037123003/diff/120001/src/gpu/effects/GrAdva...
src/gpu/effects/GrAdvancedEquationXferProcessor.cpp:251: return
SkRef(coveragePOI.isSolidWhite() ? fXferProcessor.get()
On 2015/04/21 20:39:31, Chris Dalton wrote:
> On 2015/04/21 18:59:12, egdaniel wrote:
> > As per your comment on previous patch, there really isn't a way of getting
> > around this without doing the SkNew's for each static that is being created.
> > 
> > If you wanted to avoid that, one approach would be to not templatize on
> > HasCoverage and have a single AdvEqXP. Then in its getOptimizations() you
> would
> > check the coverage and if it is solid set some internal data member to say
> your
> > ignoring coverage (and also return the ignore cov flag). This bool or flag
> then
> > would need to be part of your key as well. This is more how the other XPs
are
> > organized.
> > 
> > Basically you have the options of leaving as is and doing the new for both
> > objects each time. Or doing the other way described above which means you
> carry
> > around an extra bool and slightly longer key. Either approach would be fine.
> 
> I'm not sure I completely follow option #1, how do I use it to avoid an SkNEW
in
> this function?

Sorry wasn't the most clear typing. You can't get rid of the two SkNews if you
use the current approach. The only way to get rid of them would be to do option2
but that has the downside of carrying the extra bool around.

[Chris Dalton, 2015-04-22 19:17:01]

This patchset should address the comments.

https://codereview.chromium.org/1037123003/diff/140001/src/gpu/effects/GrAdva...
File src/gpu/effects/GrAdvancedEquationXferProcessor.cpp (right):

https://codereview.chromium.org/1037123003/diff/140001/src/gpu/effects/GrAdva...
src/gpu/effects/GrAdvancedEquationXferProcessor.cpp:221: return false;
One thing I haven't done yet is a fallback for RGB coverage. I think we could
make this work if we added 'bool hasRGBCoverage' to IsSupported(). Then
SkXfermode would also take a 'bool hasRGBCoverage' parameter, and end up only
returning this XP factory if hasRGBCoverage was false.

Do you think this could work? It would mean we would need to know about RGB
coverage by the time SkPaint2GrPaint was called. Is this something we ought to
do?

[Chris Dalton, 2015-04-23 16:45:49]

https://codereview.chromium.org/1037123003/diff/140001/include/gpu/GrXferProc...
File include/gpu/GrXferProcessor.h (right):

https://codereview.chromium.org/1037123003/diff/140001/include/gpu/GrXferProc...
include/gpu/GrXferProcessor.h:55: bool constexpr
GrBlendEquationIsAdvanced(GrBlendEquation equation) {
Are we allowing C++11-isms yet?

[bsalomon, 2015-04-23 16:53:33]

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

[bsalomon, 2015-04-23 16:53:33]

https://codereview.chromium.org/1037123003/diff/140001/include/gpu/GrXferProc...
File include/gpu/GrXferProcessor.h (right):

https://codereview.chromium.org/1037123003/diff/140001/include/gpu/GrXferProc...
include/gpu/GrXferProcessor.h:55: bool constexpr
GrBlendEquationIsAdvanced(GrBlendEquation equation) {
On 2015/04/23 16:45:49, Chris Dalton wrote:
> Are we allowing C++11-isms yet?

Some but not all. I'm not sure about the state of constexpr. Ben?

[bungeman-skia, 2015-04-23 17:05:08]

On 2015/04/23 16:53:33, bsalomon wrote:
>
https://codereview.chromium.org/1037123003/diff/140001/include/gpu/GrXferProc...
> File include/gpu/GrXferProcessor.h (right):
> 
>
https://codereview.chromium.org/1037123003/diff/140001/include/gpu/GrXferProc...
> include/gpu/GrXferProcessor.h:55: bool constexpr
> GrBlendEquationIsAdvanced(GrBlendEquation equation) {
> On 2015/04/23 16:45:49, Chris Dalton wrote:
> > Are we allowing C++11-isms yet?
> 
> Some but not all. I'm not sure about the state of constexpr. Ben?

https://chromium-cpp.appspot.com/#banned_list

msvc++ 2013 doesn't support constexpr, the next version should have basic
support.

[Chris Dalton, 2015-04-24 08:35:39]

This is ready for another round of reviews. Hopefully we're close now.

Also let me know regarding a fallback for RGB coverage. We technically could
work one in using GrCustomXfermode, although the porter duff modes could also do
something similar and don't, so I wonder if a fallback might not be necessary.

[bsalomon, 2015-04-24 16:22:22]

https://codereview.chromium.org/1037123003/diff/140001/src/gpu/effects/GrAdva...
File src/gpu/effects/GrAdvancedEquationXferProcessor.cpp (right):

https://codereview.chromium.org/1037123003/diff/140001/src/gpu/effects/GrAdva...
src/gpu/effects/GrAdvancedEquationXferProcessor.cpp:221: return false;
On 2015/04/22 19:17:01, Chris Dalton wrote:
> One thing I haven't done yet is a fallback for RGB coverage. I think we could
> make this work if we added 'bool hasRGBCoverage' to IsSupported(). Then
> SkXfermode would also take a 'bool hasRGBCoverage' parameter, and end up only
> returning this XP factory if hasRGBCoverage was false.
> 
> Do you think this could work? It would mean we would need to know about RGB
> coverage by the time SkPaint2GrPaint was called. Is this something we ought to
> do?

Unfortunately, we don't know until later whether the coverage is single channel
or not :(

[Chris Dalton, 2015-04-24 18:45:02]

https://codereview.chromium.org/1037123003/diff/140001/src/gpu/effects/GrAdva...
File src/gpu/effects/GrAdvancedEquationXferProcessor.cpp (right):

https://codereview.chromium.org/1037123003/diff/140001/src/gpu/effects/GrAdva...
src/gpu/effects/GrAdvancedEquationXferProcessor.cpp:221: return false;
On 2015/04/24 16:22:22, bsalomon wrote:
> On 2015/04/22 19:17:01, Chris Dalton wrote:
> > One thing I haven't done yet is a fallback for RGB coverage. I think we
could
> > make this work if we added 'bool hasRGBCoverage' to IsSupported(). Then
> > SkXfermode would also take a 'bool hasRGBCoverage' parameter, and end up
only
> > returning this XP factory if hasRGBCoverage was false.
> > 
> > Do you think this could work? It would mean we would need to know about RGB
> > coverage by the time SkPaint2GrPaint was called. Is this something we ought
to
> > do?
> 
> Unfortunately, we don't know until later whether the coverage is single
channel
> or not :(

Ok, then that isn't going to work :).

So considering the Porter Duff modes could also fallback to in-shader blending
for RGB coverage, but don't, I think it seems reasonable to just not support it
here either. In fact, Porter Duff only supports RGB coverage with src-over and a
known src color:

https://github.com/google/skia/blob/master/src/gpu/effects/GrPorterDuffXferPr...

[egdaniel, 2015-04-27 15:28:25]

https://codereview.chromium.org/1037123003/diff/160001/src/gpu/effects/GrAdva...
File src/gpu/effects/GrAdvancedEquationXferProcessor.cpp (right):

https://codereview.chromium.org/1037123003/diff/160001/src/gpu/effects/GrAdva...
src/gpu/effects/GrAdvancedEquationXferProcessor.cpp:241: AdvancedEquationXPBase*
AdvancedEquationXPBase::Create(bool hasCoverage, GrBlendEquation equation) {
Okay so it seems like I may have confused things with my set of comments from
before. In your original implementation for a given equation, you would only
ever make a single XPFactory that would always make exactly 2 XferProcessors. In
this new version you will make a new XferProcessor everytime an XP for a given
equation is used.

So maybe we could you go back to your original way of having the 2 SkAutoTUnrefs
on the XPFactory. But instead of making the 2 XferProcessors in the XPFactory
ctor, we just let the SkAutoTUnref default its object to NULL. Then in
onCreateXferProcessor we check if fXferProcessor.get() == NULL, and if so we
create it (using SkNew and reset on AutoTUnref) there before returning it? Same
with the coverage one. Does this approach seem reasonable?

[Chris Dalton, 2015-04-27 18:25:39]

https://codereview.chromium.org/1037123003/diff/160001/src/gpu/effects/GrAdva...
File src/gpu/effects/GrAdvancedEquationXferProcessor.cpp (right):

https://codereview.chromium.org/1037123003/diff/160001/src/gpu/effects/GrAdva...
src/gpu/effects/GrAdvancedEquationXferProcessor.cpp:241: AdvancedEquationXPBase*
AdvancedEquationXPBase::Create(bool hasCoverage, GrBlendEquation equation) {
On 2015/04/27 15:28:25, egdaniel wrote:
> Okay so it seems like I may have confused things with my set of comments from
> before. In your original implementation for a given equation, you would only
> ever make a single XPFactory that would always make exactly 2 XferProcessors.
In
> this new version you will make a new XferProcessor everytime an XP for a given
> equation is used.
> 
> So maybe we could you go back to your original way of having the 2
SkAutoTUnrefs
> on the XPFactory. But instead of making the 2 XferProcessors in the XPFactory
> ctor, we just let the SkAutoTUnref default its object to NULL. Then in
> onCreateXferProcessor we check if fXferProcessor.get() == NULL, and if so we
> create it (using SkNew and reset on AutoTUnref) there before returning it?
Same
> with the coverage one. Does this approach seem reasonable?

This is also how I would prefer to do it, and I like your suggestion to create
the individual XP's lazily.

I did manage to fix the assert by storing the factories as static SkAutoTUnrefs,
which causes them to get cleaned up on exit. However, I think this only works
because we got lucky and the factories get cleaned up before the GrMemoryPool,
which is what asserts that the global allocation count is zero. I don't know how
fragile this is and if the destruction order will change on different compilers
or OS's, but I'm not comfortable with it. Below is the assert for reference.
I'll start looking into a better way to handle this, and am open to suggestions.

 0x00000000008e1cc6 in GrMemoryPool::~GrMemoryPool (
     this=0xf47c00 <_ZZNK12_GLOBAL__N_118MemoryPoolAccessor4poolEvE5gPool>,
__in_chrg=<optimized out>)
     at ../../src/gpu/GrMemoryPool.cpp:35
 35	    SkASSERT(0 == fAllocationCnt);
 (gdb) bt
 #0  0x00000000008e1cc6 in GrMemoryPool::~GrMemoryPool (
     this=0xf47c00 <_ZZNK12_GLOBAL__N_118MemoryPoolAccessor4poolEvE5gPool>,
__in_chrg=<optimized out>)
     at ../../src/gpu/GrMemoryPool.cpp:35
 #1  0x00007ffff65ad259 in __run_exit_handlers (status=0, listp=0x7ffff692f6c8
<__exit_funcs>, 
     run_list_atexit=run_list_atexit@entry=true) at exit.c:82
 #2  0x00007ffff65ad2a5 in __GI_exit (status=<optimized out>) at exit.c:104
 #3  0x00007ffff6592ecc in __libc_start_main (main=0x5d4841 <main(int, char**)>,
argc=3, argv=0x7fffffffdaf8, 
     init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>,
stack_end=0x7fffffffdae8) at libc-start.c:321
 #4  0x0000000000406209 in _start ()

[egdaniel, 2015-04-27 18:57:13]

Okay so after some more thinking about this patch here, we should not be
creating a whole new XPFactory for handling this advanced blend extension. We
already have a very api neutral factory, GrCustomXPFactory, that handles all
these same blend modes that are being implemented here. The presence of this
extension is actually an implementation detail and should not be bubbled up to
the layer of XPFactory/XP. In general this advance blend should be a tool that
XPs can use under the hood and not its own XP.

Instead lets just adjust the CustomXP to internally know whether it will make a
dst copy, use a barrier, etc. Ideally we wouldn't add a new XPFactory or XP
class but it is also not the end of the world if an XPFactory creates a
specialized XP class in certain cases.

[Chris Dalton, 2015-05-01 20:24:10]

On 2015/04/27 18:57:13, egdaniel wrote:
> Okay so after some more thinking about this patch here, we should not be
> creating a whole new XPFactory for handling this advanced blend extension. We
> already have a very api neutral factory, GrCustomXPFactory, that handles all
> these same blend modes that are being implemented here. The presence of this
> extension is actually an implementation detail and should not be bubbled up to
> the layer of XPFactory/XP. In general this advance blend should be a tool that
> XPs can use under the hood and not its own XP.
> 
> Instead lets just adjust the CustomXP to internally know whether it will make
a
> dst copy, use a barrier, etc. Ideally we wouldn't add a new XPFactory or XP
> class but it is also not the end of the world if an XPFactory creates a
> specialized XP class in certain cases.

I set up a Windows system to test the Intel driver, and it turns out that their
implementation is not conformant at all. So for patchset 16 I blacklisted the
Intel driver.

This change is ready for another round of reviews.

[egdaniel, 2015-05-04 19:51:21]

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/effects/GrCust...
File src/gpu/effects/GrCustomXfermode.cpp (right):

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/effects/GrCust...
src/gpu/effects/GrCustomXfermode.cpp:564: if (!xp.hasHWBlendEquation() ||
caps.mustEnableAdvancedBlendEquations()) {
just to confirm, mustEnableAdvBE() means that we need to add the layout
qualifier into the shader right? And the NV version of the extension does not
need this right?

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/effects/GrCust...
src/gpu/effects/GrCustomXfermode.cpp:567: key |= xp.mode() << 4;
When adding the case to switch between all and individual ones we should make
sure that gets put into this key so that we don't build any extra programs when
using all.

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/effects/GrCust...
src/gpu/effects/GrCustomXfermode.cpp:594: emit_custom_xfermode_code(mode,
fsBuilder, args.fOutputPrimary, args.fInputColor, dstColor);
100 chars

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/effects/GrCust...
src/gpu/effects/GrCustomXfermode.cpp:705: bool
GrCustomXPFactory::canTweakAlphaForCoverage() const {
so we actually just removed this function since with our new batch world we only
looked at the flag returned from getOptimizations. So I guess move this proof
somewhere up there.

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/effects/GrCust...
src/gpu/effects/GrCustomXfermode.cpp:799: return true;
so I read through the proof on your first path. Off the top of my head I think
this proof works for the dst copy version as well. Is this true? Does it work
for the dst read if RGB coverage?

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/gl/builders/Gr...
File src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp (right):

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/gl/builders/Gr...
src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp:209:
this->addLayoutQualifier(layoutQualifierNames[advancedEquation -
kFirstAdvancedGrBlendEquation],
so this doesn't ever set to blend_support_all. I would have thought in most
cases blend_support_all might be faster since we can reuse programs (but it may
also add a bunch of slow conditionals to shader). I would say that we have a
caps for use all or use specific, and then check that here and do the
appropriate thing. My guess would be to default to "all" just to avoid compiles
of programs, but I don't have the greatest insight into which is better.

[Chris Dalton, 2015-05-04 20:33:12]

Just a couple quick questions about the blend_support_all cap. In comments.

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/effects/GrCust...
File src/gpu/effects/GrCustomXfermode.cpp (right):

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/effects/GrCust...
src/gpu/effects/GrCustomXfermode.cpp:564: if (!xp.hasHWBlendEquation() ||
caps.mustEnableAdvancedBlendEquations()) {
On 2015/05/04 19:51:21, egdaniel wrote:
> just to confirm, mustEnableAdvBE() means that we need to add the layout
> qualifier into the shader right? And the NV version of the extension does not
> need this right?

Yes, that's correct. And we need this check because there are drivers that
support NVbea but not KHRbea, meaning a KHRbea layout qualifier would fail to
compile on those platforms. It also makes it so we don't compile new shaders for
every blend equation on NVIDIA.

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/effects/GrCust...
src/gpu/effects/GrCustomXfermode.cpp:594: emit_custom_xfermode_code(mode,
fsBuilder, args.fOutputPrimary, args.fInputColor, dstColor);
On 2015/05/04 19:51:21, egdaniel wrote:
> 100 chars

Oops :)

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/effects/GrCust...
src/gpu/effects/GrCustomXfermode.cpp:799: return true;
On 2015/05/04 19:51:21, egdaniel wrote:
> so I read through the proof on your first path. Off the top of my head I think
> this proof works for the dst copy version as well. Is this true? Does it work
> for the dst read if RGB coverage?

Yes, it works for the dst read version because it proves the two formulas are
equivalent. i.e:

  blend(f * S, D) == f * blend(S, D) + (1-f) * D

So you get the same result if you modulate coverage into the src color before
(KHRbea mode), mix it into the dst after (dst read mode), and even if you do
some before and some after.

canTweakAlphaForCoverage() must imply the coverage is scalar, since there is
only one alpha channel, but I suppose we can say this also works for the RGB
coverage version (whatever that means, when there is per-channel coverage and
per-pixel coverage being mixed together ;) ).

Any coverage modulated into alpha will work since it's scalar. And the RGB
coverage will also work even though it's not scalar because the dst read modes
mix it in after the blend.

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/gl/builders/Gr...
File src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp (right):

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/gl/builders/Gr...
src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp:209:
this->addLayoutQualifier(layoutQualifierNames[advancedEquation -
kFirstAdvancedGrBlendEquation],
On 2015/05/04 19:51:21, egdaniel wrote:
> so this doesn't ever set to blend_support_all. I would have thought in most
> cases blend_support_all might be faster since we can reuse programs (but it
may
> also add a bunch of slow conditionals to shader). I would say that we have a
> caps for use all or use specific, and then check that here and do the
> appropriate thing. My guess would be to default to "all" just to avoid
compiles
> of programs, but I don't have the greatest insight into which is better.

Yeah I don't have much insight either, outside the NVIDA driver.

My thoughts might be to default to using unique qualifiers per equation though,
for a few reasons: 
* The spec took the effort to create them
* Shader compilation is a one-time cost
* An app out in the wild seems unlikely to recompile many of the same shaders
for different blend equations
* There is a potential per-pixel cost associated with using blend_support_all

We already have the 'mustEnableAdvancedBlendEquations()' cap that will do the
right thing on NVIDIA and avoid redundant compiles. I can add another cap for
blend_support_all, but it won't get used yet since Intel is blacklisted and
they're the only other vendor to currently support this extension. Do we still
want to make one? Do you want me to default it to "yes, use blend_support_all",
or "no"?

[egdaniel, 2015-05-04 21:04:51]

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/gl/builders/Gr...
File src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp (right):

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/gl/builders/Gr...
src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp:209:
this->addLayoutQualifier(layoutQualifierNames[advancedEquation -
kFirstAdvancedGrBlendEquation],
On 2015/05/04 20:33:12, Chris Dalton wrote:
> On 2015/05/04 19:51:21, egdaniel wrote:
> > so this doesn't ever set to blend_support_all. I would have thought in most
> > cases blend_support_all might be faster since we can reuse programs (but it
> may
> > also add a bunch of slow conditionals to shader). I would say that we have a
> > caps for use all or use specific, and then check that here and do the
> > appropriate thing. My guess would be to default to "all" just to avoid
> compiles
> > of programs, but I don't have the greatest insight into which is better.
> 
> Yeah I don't have much insight either, outside the NVIDA driver.
> 
> My thoughts might be to default to using unique qualifiers per equation
though,
> for a few reasons: 
> * The spec took the effort to create them
> * Shader compilation is a one-time cost
> * An app out in the wild seems unlikely to recompile many of the same shaders
> for different blend equations
> * There is a potential per-pixel cost associated with using blend_support_all
> 
> We already have the 'mustEnableAdvancedBlendEquations()' cap that will do the
> right thing on NVIDIA and avoid redundant compiles. I can add another cap for
> blend_support_all, but it won't get used yet since Intel is blacklisted and
> they're the only other vendor to currently support this extension. Do we still
> want to make one? Do you want me to default it to "yes, use
blend_support_all",
> or "no"?

Okay let's keep it as is and we'll just have a TODO on our end to test out other
devices. I think some of the new Adreno 400s suppport the KHR version.

[Chris Dalton, 2015-05-05 18:57:58]

I think this addresses all the comments.

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/effects/GrCust...
File src/gpu/effects/GrCustomXfermode.cpp (right):

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/effects/GrCust...
src/gpu/effects/GrCustomXfermode.cpp:567: key |= xp.mode() << 4;
On 2015/05/04 19:51:21, egdaniel wrote:
> When adding the case to switch between all and individual ones we should make
> sure that gets put into this key so that we don't build any extra programs
when
> using all.

Done.

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/effects/GrCust...
src/gpu/effects/GrCustomXfermode.cpp:594: emit_custom_xfermode_code(mode,
fsBuilder, args.fOutputPrimary, args.fInputColor, dstColor);
On 2015/05/04 19:51:21, egdaniel wrote:
> 100 chars

Done.

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/effects/GrCust...
src/gpu/effects/GrCustomXfermode.cpp:705: bool
GrCustomXPFactory::canTweakAlphaForCoverage() const {
On 2015/05/04 19:51:21, egdaniel wrote:
> so we actually just removed this function since with our new batch world we
only
> looked at the flag returned from getOptimizations. So I guess move this proof
> somewhere up there.

Done.

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/gl/builders/Gr...
File src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp (right):

https://codereview.chromium.org/1037123003/diff/300001/src/gpu/gl/builders/Gr...
src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp:209:
this->addLayoutQualifier(layoutQualifierNames[advancedEquation -
kFirstAdvancedGrBlendEquation],
On 2015/05/04 21:04:51, egdaniel wrote:
> On 2015/05/04 20:33:12, Chris Dalton wrote:
> > On 2015/05/04 19:51:21, egdaniel wrote:
> > > so this doesn't ever set to blend_support_all. I would have thought in
most
> > > cases blend_support_all might be faster since we can reuse programs (but
it
> > may
> > > also add a bunch of slow conditionals to shader). I would say that we have
a
> > > caps for use all or use specific, and then check that here and do the
> > > appropriate thing. My guess would be to default to "all" just to avoid
> > compiles
> > > of programs, but I don't have the greatest insight into which is better.
> > 
> > Yeah I don't have much insight either, outside the NVIDA driver.
> > 
> > My thoughts might be to default to using unique qualifiers per equation
> though,
> > for a few reasons: 
> > * The spec took the effort to create them
> > * Shader compilation is a one-time cost
> > * An app out in the wild seems unlikely to recompile many of the same
shaders
> > for different blend equations
> > * There is a potential per-pixel cost associated with using
blend_support_all
> > 
> > We already have the 'mustEnableAdvancedBlendEquations()' cap that will do
the
> > right thing on NVIDIA and avoid redundant compiles. I can add another cap
for
> > blend_support_all, but it won't get used yet since Intel is blacklisted and
> > they're the only other vendor to currently support this extension. Do we
still
> > want to make one? Do you want me to default it to "yes, use
> blend_support_all",
> > or "no"?
> 
> Okay let's keep it as is and we'll just have a TODO on our end to test out
other
> devices. I think some of the new Adreno 400s suppport the KHR version.

I actually went ahead and handled blend_support_all_equations after all :)

[egdaniel, 2015-05-05 21:20:01]

https://codereview.chromium.org/1037123003/diff/320001/src/gpu/GrDrawTargetCa...
File src/gpu/GrDrawTargetCaps.h (right):

https://codereview.chromium.org/1037123003/diff/320001/src/gpu/GrDrawTargetCa...
src/gpu/GrDrawTargetCaps.h:147: enum BlendEquationSupport {
Can BlendEquationSupport and related functions be moved into GL?

https://codereview.chromium.org/1037123003/diff/320001/src/gpu/effects/GrCust...
File src/gpu/effects/GrCustomXfermode.cpp (right):

https://codereview.chromium.org/1037123003/diff/320001/src/gpu/effects/GrCust...
src/gpu/effects/GrCustomXfermode.cpp:585: key |= xp.hasHWBlendEquation() << 2;
I'm trying to think if we can simplify this logic so a key always has bits that
mean the same thing.

Does it work if we just do:

uint32_t key = xp.numTextures();

key |= xp.hasCoverage() << 1;

key |= caps.advancedBlendEqnInteraction() << 2; // assuming this is invalid iff
!xp.hasHWBlendEquation

key | = xp.mode() << 4; // hwBlendEq comes direction from mode so can we just
use mode for both cases?

[Chris Dalton, 2015-05-06 16:06:27]

https://codereview.chromium.org/1037123003/diff/320001/src/gpu/GrDrawTargetCa...
File src/gpu/GrDrawTargetCaps.h (right):

https://codereview.chromium.org/1037123003/diff/320001/src/gpu/GrDrawTargetCa...
src/gpu/GrDrawTargetCaps.h:147: enum BlendEquationSupport {
On 2015/05/05 21:20:01, egdaniel wrote:
> Can BlendEquationSupport and related functions be moved into GL?

Not easily, most the places where we add support for advanced blend equations
are not in GL (getBlendInfo, GrBlendEquation, willNeedXferBarrier, xferBarrier,
GrXferBarrierType, willNeedDstCopy, getOptimizations, etc.)

Advanced blend equations are a hardware feature, all the OpenGL extension does
is give you access to our dedicated blend unit in the ROP. They will definitely
be available in any non-proprietary API that skia ends up on, so I actually
think Gr (not GL) feels like the right spot for it.

(I did put "AdvancedBlendEqnInteraction" in GrGLSLCaps because that part of the
extension felt like an API-specific idiosyncrasy)

https://codereview.chromium.org/1037123003/diff/320001/src/gpu/effects/GrCust...
File src/gpu/effects/GrCustomXfermode.cpp (right):

https://codereview.chromium.org/1037123003/diff/320001/src/gpu/effects/GrCust...
src/gpu/effects/GrCustomXfermode.cpp:585: key |= xp.hasHWBlendEquation() << 2;
On 2015/05/05 21:20:01, egdaniel wrote:
> I'm trying to think if we can simplify this logic so a key always has bits
that
> mean the same thing.
> 
> Does it work if we just do:
> 
> uint32_t key = xp.numTextures();
> 
> key |= xp.hasCoverage() << 1;
> 
> key |= caps.advancedBlendEqnInteraction() << 2; // assuming this is invalid
iff
> !xp.hasHWBlendEquation
> 
> key | = xp.mode() << 4; // hwBlendEq comes direction from mode so can we just
> use mode for both cases?

Good idea. I like it a lot better now.

[egdaniel, 2015-05-06 17:57:46]

lgtm

https://codereview.chromium.org/1037123003/diff/320001/src/gpu/GrDrawTargetCa...
File src/gpu/GrDrawTargetCaps.h (right):

https://codereview.chromium.org/1037123003/diff/320001/src/gpu/GrDrawTargetCa...
src/gpu/GrDrawTargetCaps.h:147: enum BlendEquationSupport {
On 2015/05/06 16:06:27, Chris Dalton wrote:
> On 2015/05/05 21:20:01, egdaniel wrote:
> > Can BlendEquationSupport and related functions be moved into GL?
> 
> Not easily, most the places where we add support for advanced blend equations
> are not in GL (getBlendInfo, GrBlendEquation, willNeedXferBarrier,
xferBarrier,
> GrXferBarrierType, willNeedDstCopy, getOptimizations, etc.)
> 
> Advanced blend equations are a hardware feature, all the OpenGL extension does
> is give you access to our dedicated blend unit in the ROP. They will
definitely
> be available in any non-proprietary API that skia ends up on, so I actually
> think Gr (not GL) feels like the right spot for it.
> 
> (I did put "AdvancedBlendEqnInteraction" in GrGLSLCaps because that part of
the
> extension felt like an API-specific idiosyncrasy)

sgtm

[Chris Dalton, 2015-05-06 19:15:14]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2015-05-06 19:15:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
markkilgard@gmail.com
Link to the patchset: https://codereview.chromium.org/1037123003/#ps360001
(title: "AdvancedBlendEqnInteraction -> AdvBlendEqInteraction")

[commit-bot: I haz the power, 2015-05-06 19:15:41]

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

[commit-bot: I haz the power, 2015-05-06 19:15:52]

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

[commit-bot: I haz the power, 2015-05-06 19:15:53]

Presubmit check for 1037123003-360001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com', 'djsollen@chromium.org', 'djsollen@google.com')

[bsalomon, 2015-05-06 20:01:11]

On 2015/05/06 19:15:53, I haz the power (commit-bot) wrote:
> Presubmit check for 1037123003-360001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Since the CL is editing public API, you must have an LGTM from one of:
> (mailto:, mailto:, mailto:,
> mailto:, mailto:, mailto:)

lgtm

[Chris Dalton, 2015-05-06 20:14:22]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2015-05-06 20:14:46]

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

[commit-bot: I haz the power, 2015-05-06 20:16:01]

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

[commit-bot: I haz the power, 2015-05-06 20:16:03]

Try jobs failed on following builders:
  Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)

[Chris Dalton, 2015-05-06 20:29:59]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2015-05-06 20:30:00]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com,
egdaniel@google.com, markkilgard@gmail.com
Link to the patchset: https://codereview.chromium.org/1037123003/#ps380001
(title: "Compiler warning")

[commit-bot: I haz the power, 2015-05-06 20:30:15]

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

[commit-bot: I haz the power, 2015-05-06 20:40:26]

Committed patchset #20 (id:380001) as
https://skia.googlesource.com/skia/+/8917d62ef4d9bde9ec4f879dc42b309b03a0ad98