added support for PLS path rendering

added support for PLS path rendering

BUG=skia:3555
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1541903002

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

Committed: https://skia.googlesource.com/skia/+/2279325d539700ee3da29d6e874b3b3ce1dcf49c

[ethannicholas, 2015-12-21 19:19:20]

Description was changed from

==========
added support for PLS path rendering

BUG=skia:3555
==========

to

==========
added support for PLS path rendering

BUG=skia:3555
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[ethannicholas, 2015-12-21 19:20:49]

The CQ bit was checked by ethannicholas@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-21 19:20:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1541903002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1541903002/1

[commit-bot: I haz the power, 2015-12-21 19:21:38]

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

[commit-bot: I haz the power, 2015-12-21 19:21:38]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)

[ethannicholas, 2015-12-21 19:27:16]

The CQ bit was checked by ethannicholas@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-21 19:27:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1541903002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1541903002/20001

[commit-bot: I haz the power, 2015-12-21 19:29:28]

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

[commit-bot: I haz the power, 2015-12-21 19:29:29]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[ethannicholas, 2015-12-21 19:43:28]

The CQ bit was checked by ethannicholas@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-21 19:43:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1541903002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1541903002/40001

[commit-bot: I haz the power, 2015-12-21 19:58:47]

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

[commit-bot: I haz the power, 2015-12-21 19:58:48]

Dry run: This issue passed the CQ dry run.

[ethannicholas, 2015-12-21 19:59:59]

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

[bsalomon, 2016-01-04 15:33:47]

Initial pass without having delved into the PLS GrPR subclass itself.

https://codereview.chromium.org/1541903002/diff/60001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/1541903002/diff/60001/src/gpu/GrDrawTarget.cp...
src/gpu/GrDrawTarget.cpp:518: #ifdef ENABLE_PLS
There is an SkTPin that does the same thing.

https://codereview.chromium.org/1541903002/diff/60001/src/gpu/GrDrawTarget.cp...
src/gpu/GrDrawTarget.cpp:535: // FIXME shouldn't have to cast to GL
implementation here
The viewport rect isn't really right because it refects the "origin" of the
render target while the scissor rect does not. I htink you can just use
SkIRect::MakeWH(rt->width(), rt->height()) here.

https://codereview.chromium.org/1541903002/diff/60001/src/gpu/GrPLSGeometryPr...
File src/gpu/GrPLSGeometryProcessor.h (right):

https://codereview.chromium.org/1541903002/diff/60001/src/gpu/GrPLSGeometryPr...
src/gpu/GrPLSGeometryProcessor.h:13: #define PLS_DATA_DECL "__pixel_localEXT
PLSData {\n"\
Seems like this should be in a GL/GLSL specific file. Also it seems particular
to using PLS for path rendering and not a generic PLS thing.

https://codereview.chromium.org/1541903002/diff/60001/src/gpu/GrPLSGeometryPr...
src/gpu/GrPLSGeometryProcessor.h:28: }
untab }

https://codereview.chromium.org/1541903002/diff/60001/src/gpu/batches/GrTesse...
File src/gpu/batches/GrTessellatingPathRenderer.h (right):

https://codereview.chromium.org/1541903002/diff/60001/src/gpu/batches/GrTesse...
src/gpu/batches/GrTessellatingPathRenderer.h:14: #define
TESSELLATION_LOGGING_ENABLED 0
Maybe we can pull tessellation into it's own utility header/cpp apart from the
current GrTessellatingPathRenderer subclass that uses it? Perhaps as a precursor
CL to this one?

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

https://codereview.chromium.org/1541903002/diff/60001/src/gpu/effects/GrPorte...
src/gpu/effects/GrPorterDuffXferProcessor.cpp:750: if
(optimizations.fOverrides.fUsePLSDstRead) {
Maybe time to rename GrPipelineOptimizations... seems like this isn't an
optimization but rather a requirement for correct rendering.

https://codereview.chromium.org/1541903002/diff/60001/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/1541903002/diff/60001/src/gpu/gl/GrGLGpu.h#ne...
src/gpu/gl/GrGLGpu.h:552: bool                        fHWPLSEnabled;
Any possibility this is in an unknown state, say after non Skia code uses the
OGL context?

https://codereview.chromium.org/1541903002/diff/60001/src/gpu/glsl/GrGLSLFrag...
File src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp (right):

https://codereview.chromium.org/1541903002/diff/60001/src/gpu/glsl/GrGLSLFrag...
src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp:174: return "pls.dstColor";
Should we make it possible for the GP to provide this name rather than assuming
it is "pls.dstColor"?

[ethannicholas, 2016-01-13 17:47:30]

Patchset #6 (id:100001) has been deleted

[bsalomon, 2016-01-13 19:15:34]

https://codereview.chromium.org/1541903002/diff/140001/include/gpu/GrCaps.h
File include/gpu/GrCaps.h (right):

https://codereview.chromium.org/1541903002/diff/140001/include/gpu/GrCaps.h#n...
include/gpu/GrCaps.h:84: int pixelLocalStorageSize() const { return
fPixelLocalStorageSize; }
Maybe a comment that if > 0 then PLS is supported and the returned value
indicates the number of bytes of storage available?

https://codereview.chromium.org/1541903002/diff/140001/include/gpu/GrTypesPriv.h
File include/gpu/GrTypesPriv.h (right):

https://codereview.chromium.org/1541903002/diff/140001/include/gpu/GrTypesPri...
include/gpu/GrTypesPriv.h:101: return (type >= 1 && type <= 6) || type == 10;
Maybe make the new enum contiguous with the existing numeric types?

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/GrDrawTarget.c...
src/gpu/GrDrawTarget.cpp:529: #ifdef ENABLE_PLS
untab

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/GrGpu.h#newcode38
src/gpu/GrGpu.h:38: //#define ENABLE_PLS
Since this macro spans multiple files maybe prefix with GR_ and define in
GrConfig.h

#if !defined(GR_ENABLE_PLS)
    #define GR_ENABLE_PLS 0
#endif

In Gr-land we tend to define boolean macros to 0 or 1 rather than have them
defined or not defined.

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/GrPLSGeometryP...
File src/gpu/GrPLSGeometryProcessor.h (right):

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/GrPLSGeometryP...
src/gpu/GrPLSGeometryProcessor.h:19: SkRect fBounds;
public?

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/GrPathRenderer...
File src/gpu/GrPathRendererChain.cpp (right):

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/GrPathRenderer...
src/gpu/GrPathRendererChain.cpp:38: #ifdef ENABLE_PLS
untab

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/GrPrimitivePro...
File src/gpu/GrPrimitiveProcessor.h (right):

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/GrPrimitivePro...
src/gpu/GrPrimitiveProcessor.h:53: kDraw_State,
The style is that tag after the "_" is the name of the enum type, e.g.
kDraw_GrPixelLocalStorageState.

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/batches/GrTess...
File src/gpu/batches/GrTessellatingPathRenderer.h (right):

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/batches/GrTess...
src/gpu/batches/GrTessellatingPathRenderer.h:21: * Vertices are used in three
ways: first, the path contours are converted into a
I thought you already moved all this stuff to a utility file.

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

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/gl/GrGLCaps.cp...
src/gpu/gl/GrGLCaps.cpp:275: #define
GL_MAX_SHADER_PIXEL_LOCAL_STORAGE_FAST_SIZE_EXT 0x8F63
Define in GrGLDefines.h, nix the _EXT, prefix GR_.

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:43: #define GL_SHADER_PIXEL_LOCAL_STORAGE_EXT 0x8F64
GrGLDefines.h

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:231: #ifdef ENABLE_PLS
untab

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:232: if
(this->glCaps().shaderCaps()->pixelLocalStorageSize() > 0) {
Check for framebuffer_fetch too?

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:290: #ifdef ENABLE_PLS
Any code that won't be run without the GrPLSPR subclass being installed need not
be protected by the macro. Probably better that it isn't so that it fail to
compile due to changes in later CLs.

You could make the macro just toggle whether GrGLCaps::pixelLocalStorageSize()
always returns zero, and leave everything else in a default build.

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:320: fshaderTxt.append("#extension
GL_ARM_shader_framebuffer_fetch : require\n");
This extension has aliases, see GrGLSLCaps()::fbFetchExtensionString()

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/gl/GrGLGpu.h#n...
src/gpu/gl/GrGLGpu.h:555: #ifdef ENABLE_PLS
untab

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/glsl/GrGLSLSha...
File src/gpu/glsl/GrGLSLShaderVar.h (right):

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/glsl/GrGLSLSha...
src/gpu/glsl/GrGLSLShaderVar.h:167: if (fType == kInt_GrSLType &&
(this->getTypeModifier() == kVaryingIn_TypeModifier ||
Maybe create a GrSLTypeIsInt() so that we'd don't have to ammend this when we
add unsigned, vec2i, ...

[egdaniel, 2016-01-13 21:47:19]

so quick thoughts

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/effects/GrCove...
File src/gpu/effects/GrCoverageSetOpXP.cpp (right):

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/effects/GrCove...
src/gpu/effects/GrCoverageSetOpXP.cpp:316: return
optimizations.fOverrides.fUsePLSDstRead;
I am slightly worried about the need to check PLS in each XPFs willReadDst since
it is not an obvious thing to need to check for if we ever make a new XP.
However it can't really go in the base class since each XP does handle it
slightly different.

Maybe at least on the base class virtual lets add or update the comment to make
it clear the subclass needs to respect the optimizations, specifically pls.

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/effects/GrCove...
src/gpu/effects/GrCoverageSetOpXP.cpp:323: const DstTexture* dst) const {
extra spaces

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

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/effects/GrCust...
src/gpu/effects/GrCustomXfermode.cpp:59: 
extra \n

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/gl/builders/Gr...
File src/gpu/gl/builders/GrGLProgramBuilder.h (right):

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/gl/builders/Gr...
src/gpu/gl/builders/GrGLProgramBuilder.h:85: int index,
just heads up, these emit functions have been moved to GrGLSLProgramBuilder

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/gl/builders/Gr...
src/gpu/gl/builders/GrGLProgramBuilder.h:91: virtual void
emitAndInstallXferProc(const GrXferProcessor&,
why was this made virtual?

[ethannicholas, 2016-01-19 22:00:18]

Patchset #8 (id:160001) has been deleted

[ethannicholas, 2016-01-20 17:51:05]

https://codereview.chromium.org/1541903002/diff/140001/include/gpu/GrTypesPriv.h
File include/gpu/GrTypesPriv.h (right):

https://codereview.chromium.org/1541903002/diff/140001/include/gpu/GrTypesPri...
include/gpu/GrTypesPriv.h:101: return (type >= 1 && type <= 6) || type == 10;
On 2016/01/13 19:15:33, bsalomon wrote:
> Maybe make the new enum contiguous with the existing numeric types?

I'm not strongly opposed to that, but there are so many spots checking the
numeric values of the enums right now I felt that not shuffling them around was
a better change. You think it's worth moving?

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/batches/GrTess...
File src/gpu/batches/GrTessellatingPathRenderer.h (right):

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/batches/GrTess...
src/gpu/batches/GrTessellatingPathRenderer.h:21: * Vertices are used in three
ways: first, the path contours are converted into a
On 2016/01/13 19:15:34, bsalomon wrote:
> I thought you already moved all this stuff to a utility file.

I did; just failed to quash this during the merge. Oops!

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/gl/builders/Gr...
File src/gpu/gl/builders/GrGLProgramBuilder.h (right):

https://codereview.chromium.org/1541903002/diff/140001/src/gpu/gl/builders/Gr...
src/gpu/gl/builders/GrGLProgramBuilder.h:91: virtual void
emitAndInstallXferProc(const GrXferProcessor&,
On 2016/01/13 21:47:19, egdaniel wrote:
> why was this made virtual?

Had needed it at one point, failed to remove the virtual when that changed.
Fixed!

[bsalomon, 2016-01-20 18:27:31]

Some more minor stuff

https://codereview.chromium.org/1541903002/diff/140001/include/gpu/GrTypesPriv.h
File include/gpu/GrTypesPriv.h (right):

https://codereview.chromium.org/1541903002/diff/140001/include/gpu/GrTypesPri...
include/gpu/GrTypesPriv.h:101: return (type >= 1 && type <= 6) || type == 10;
On 2016/01/20 17:51:05, ethannicholas wrote:
> On 2016/01/13 19:15:33, bsalomon wrote:
> > Maybe make the new enum contiguous with the existing numeric types?
> 
> I'm not strongly opposed to that, but there are so many spots checking the
> numeric values of the enums right now I felt that not shuffling them around
was
> a better change. You think it's worth moving?

It seemed more intuitive to me for the long term, but I haven't really looked at
how many places assume the current values. No strong preference.

https://codereview.chromium.org/1541903002/diff/200001/src/gpu/batches/GrPLSP...
File src/gpu/batches/GrPLSPathRenderer.cpp (right):

https://codereview.chromium.org/1541903002/diff/200001/src/gpu/batches/GrPLSP...
src/gpu/batches/GrPLSPathRenderer.cpp:115: if (!outset(&result[0], pts[1],
pts[2])) {
Would it be better to compute the three normalized vectors here to avoid
calculating each 3 times in the outset function?

https://codereview.chromium.org/1541903002/diff/200001/src/gpu/batches/GrPLSP...
src/gpu/batches/GrPLSPathRenderer.cpp:292: varyingHandler->addVarying("Vertex1",
&v1, kHigh_GrSLPrecision);
You shouldn't need \ns in your strings

https://codereview.chromium.org/1541903002/diff/200001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1541903002/diff/200001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2254: if
(this->glCaps().shaderCaps()->plsPathRenderingSupport() && fMSAAEnabled ==
kNo_TriState) {
Why the fMSAAEnabled check here?

https://codereview.chromium.org/1541903002/diff/200001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2259: if (kYes_TriState == fHWScissorSettings.fEnabled) {
Probably better to just call this->disableScissor() which will do the check and
update fHWScissorSettings if changed. We don't really need to reset it to the
previous state below, that will happen on the next draw iff we need it.

[ethannicholas, 2016-01-20 22:15:39]

Patchset #10 (id:220001) has been deleted

[ethannicholas, 2016-01-20 22:25:04]

https://codereview.chromium.org/1541903002/diff/200001/src/gpu/batches/GrPLSP...
File src/gpu/batches/GrPLSPathRenderer.cpp (right):

https://codereview.chromium.org/1541903002/diff/200001/src/gpu/batches/GrPLSP...
src/gpu/batches/GrPLSPathRenderer.cpp:115: if (!outset(&result[0], pts[1],
pts[2])) {
On 2016/01/20 18:27:31, bsalomon wrote:
> Would it be better to compute the three normalized vectors here to avoid
> calculating each 3 times in the outset function?

It certainly would!

https://codereview.chromium.org/1541903002/diff/200001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1541903002/diff/200001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2254: if
(this->glCaps().shaderCaps()->plsPathRenderingSupport() && fMSAAEnabled ==
kNo_TriState) {
On 2016/01/20 18:27:31, bsalomon wrote:
> Why the fMSAAEnabled check here?

Enabling PLS in the presence of MSAA leads to GL_INVALID_OPERATION.

[bsalomon, 2016-01-21 16:58:12]

https://codereview.chromium.org/1541903002/diff/260001/src/gpu/batches/GrPLSP...
File src/gpu/batches/GrPLSPathRenderer.cpp (right):

https://codereview.chromium.org/1541903002/diff/260001/src/gpu/batches/GrPLSP...
src/gpu/batches/GrPLSPathRenderer.cpp:706: fsBuilder->codeAppendf("%s =
vec4(coverage + 0.25);", args.fOutputCoverage);
+0.25 intended?

https://codereview.chromium.org/1541903002/diff/260001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1541903002/diff/260001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2261: flushHWAAState(nullptr, false);
this->

https://codereview.chromium.org/1541903002/diff/260001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2270: }
fPLSHasBeenUsed = false?

[ethannicholas, 2016-01-21 17:13:41]

https://codereview.chromium.org/1541903002/diff/260001/src/gpu/batches/GrPLSP...
File src/gpu/batches/GrPLSPathRenderer.cpp (right):

https://codereview.chromium.org/1541903002/diff/260001/src/gpu/batches/GrPLSP...
src/gpu/batches/GrPLSPathRenderer.cpp:706: fsBuilder->codeAppendf("%s =
vec4(coverage + 0.25);", args.fOutputCoverage);
On 2016/01/21 16:58:12, bsalomon wrote:
> +0.25 intended?

Crap, added it for debugging yesterday and forgot about it. Thanks for the
catch.

https://codereview.chromium.org/1541903002/diff/260001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1541903002/diff/260001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2270: }
On 2016/01/21 16:58:12, bsalomon wrote:
> fPLSHasBeenUsed = false?

Unfortunately once PLS has been used, we have to KEEP using it... I'm not sure
what (if any) condition would allow us to stop using this workaround once we
have started :-( I'll add a comment explaining this.

[bsalomon, 2016-01-21 17:24:49]

On 2016/01/21 17:13:41, ethannicholas wrote:
>
https://codereview.chromium.org/1541903002/diff/260001/src/gpu/batches/GrPLSP...
> File src/gpu/batches/GrPLSPathRenderer.cpp (right):
> 
>
https://codereview.chromium.org/1541903002/diff/260001/src/gpu/batches/GrPLSP...
> src/gpu/batches/GrPLSPathRenderer.cpp:706: fsBuilder->codeAppendf("%s =
> vec4(coverage + 0.25);", args.fOutputCoverage);
> On 2016/01/21 16:58:12, bsalomon wrote:
> > +0.25 intended?
> 
> Crap, added it for debugging yesterday and forgot about it. Thanks for the
> catch.
> 
> https://codereview.chromium.org/1541903002/diff/260001/src/gpu/gl/GrGLGpu.cpp
> File src/gpu/gl/GrGLGpu.cpp (right):
> 
>
https://codereview.chromium.org/1541903002/diff/260001/src/gpu/gl/GrGLGpu.cpp...
> src/gpu/gl/GrGLGpu.cpp:2270: }
> On 2016/01/21 16:58:12, bsalomon wrote:
> > fPLSHasBeenUsed = false?
> 
> Unfortunately once PLS has been used, we have to KEEP using it... I'm not sure
> what (if any) condition would allow us to stop using this workaround once we
> have started :-( I'll add a comment explaining this.

Ok lgtm otherwise

[ethannicholas, 2016-01-21 20:35:06]

The CQ bit was checked by ethannicholas@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 20:35:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1541903002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1541903002/280001

[commit-bot: I haz the power, 2016-01-21 20:36:06]

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

[commit-bot: I haz the power, 2016-01-21 20:36:07]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Build-Mac10.9-Clang-Arm7-Debug-iOS-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.9-Clang-...)
  Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[ethannicholas, 2016-01-21 20:48:31]

The CQ bit was checked by ethannicholas@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 20:48:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1541903002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1541903002/300001

[commit-bot: I haz the power, 2016-01-21 20:49:44]

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

[commit-bot: I haz the power, 2016-01-21 20:49:46]

Dry run: 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...)

[ethannicholas, 2016-01-21 21:04:30]

The CQ bit was checked by ethannicholas@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 21:04:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1541903002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1541903002/320001

[commit-bot: I haz the power, 2016-01-21 21:07:04]

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

[commit-bot: I haz the power, 2016-01-21 21:07:05]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[ethannicholas, 2016-01-21 21:15:34]

The CQ bit was checked by ethannicholas@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 21:15:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1541903002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1541903002/340001

[ethannicholas, 2016-01-21 21:50:48]

Updated in response to Greg's offline comments and some dry-run issues.

[bsalomon, 2016-01-21 21:54:09]

A couple last minute nits

https://codereview.chromium.org/1541903002/diff/340001/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/1541903002/diff/340001/src/gpu/gl/GrGLGpu.h#n...
src/gpu/gl/GrGLGpu.h:570: bool fPLSHasBeenUsed = false;
Hm.. we don't use this type of init anywhere else. Not opposed but for
consistency, can we go with init in the constructor for now?

https://codereview.chromium.org/1541903002/diff/340001/src/gpu/glsl/GrGLSLPLS...
File src/gpu/glsl/GrGLSLPLSPathRendering.h (right):

https://codereview.chromium.org/1541903002/diff/340001/src/gpu/glsl/GrGLSLPLS...
src/gpu/glsl/GrGLSLPLSPathRendering.h:8: #define PLS_DSTCOLOR_NAME 
"pls.dstColor"
Let's prefix these with GR_GL_ since they aren't local to one file.

[egdaniel, 2016-01-21 21:55:13]

all xfer processor stuff lgtm, can't say about other parts

[ethannicholas, 2016-01-22 14:17:18]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2016-01-22 14:17:19]

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

[commit-bot: I haz the power, 2016-01-22 14:17:22]

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

[commit-bot: I haz the power, 2016-01-22 14:48:49]

Description was changed from

==========
added support for PLS path rendering

BUG=skia:3555
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
added support for PLS path rendering

BUG=skia:3555
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/7df3f5e127f8016d17b637cc48a6a4718f1a6822
==========

[commit-bot: I haz the power, 2016-01-22 14:48:50]

Committed patchset #16 (id:360001) as
https://skia.googlesource.com/skia/+/7df3f5e127f8016d17b637cc48a6a4718f1a6822

[ethannicholas, 2016-01-22 17:39:52]

A revert of this CL (patchset #16 id:360001) has been created in
https://codereview.chromium.org/1626553002/ by ethannicholas@google.com.

The reason for reverting is: ASAN failure at src/gpu/GrXferProcessor.cpp:224.

[ethannicholas, 2016-01-29 18:38:53]

Patchset #17 (id:380001) has been deleted

[ethannicholas, 2016-01-29 18:44:06]

Patchset #17 (id:400001) has been deleted

[ethannicholas, 2016-01-29 18:48:42]

Description was changed from

==========
added support for PLS path rendering

BUG=skia:3555
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/7df3f5e127f8016d17b637cc48a6a4718f1a6822
==========

to

==========
added support for PLS path rendering

BUG=skia:3555
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/7df3f5e127f8016d17b637cc48a6a4718f1a6822
==========

[ethannicholas, 2016-01-29 18:56:02]

Patchset #17 (id:420001) has been deleted

[ethannicholas, 2016-01-29 19:04:22]

Patchset #17 (id:440001) has been deleted

[ethannicholas, 2016-01-29 19:05:56]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2016-01-29 19:05:56]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com,
egdaniel@google.com
Link to the patchset: https://codereview.chromium.org/1541903002/#ps460001
(title: "fix for ASAN failure")

[commit-bot: I haz the power, 2016-01-29 19:06:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1541903002/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1541903002/460001

[commit-bot: I haz the power, 2016-01-30 17:59:14]

Description was changed from

==========
added support for PLS path rendering

BUG=skia:3555
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/7df3f5e127f8016d17b637cc48a6a4718f1a6822
==========

to

==========
added support for PLS path rendering

BUG=skia:3555
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

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

Committed:
https://skia.googlesource.com/skia/+/2279325d539700ee3da29d6e874b3b3ce1dcf49c
==========

[commit-bot: I haz the power, 2016-01-30 17:59:15]

Committed patchset #17 (id:460001) as
https://skia.googlesource.com/skia/+/2279325d539700ee3da29d6e874b3b3ce1dcf49c

[mtklein, 2016-01-31 16:31:28]

On 2016/01/30 17:59:15, commit-bot: I haz the power wrote:
> Committed patchset #17 (id:460001) as
> https://skia.googlesource.com/skia/+/2279325d539700ee3da29d6e874b3b3ce1dcf49c

FYI, it looks like this commit has caused the GPU ASAN and Valgrind bots to go
red, probably both from use of uninitialized data:
 
https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G...
 
https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-S...