Fix mixed samples stencil clip

Fix mixed samples stencil clip

Fixes rendering bugs and nondeterminism in gm.

Before, mixed samples stencil clip would try to infer whether the draw
wanted co-centered sample locations from within GrGLGpu, which caused
various errors. This change reworks it so the draw itself can request
the co-centered sample locations when it knows it will need them.

Also reduces framebuffer binds by moving the code that enables
GL_FRAMEBUFFER_PROGRAMMABLE_SAMPLE_LOCATIONS into flushRenderTarget.

Committed: https://skia.googlesource.com/skia/+/14184d5567b58085b6d8a6375796d405056f7f73

[Chris Dalton, 2015-11-06 06:49:22]

cdalton@nvidia.com changed reviewers:
+ bsalomon@google.com

[Chris Dalton, 2015-11-06 19:02:33]

Description was changed from

==========
Fix mixed samples stencil clip

Reworks mixed samples stencil clip to setup co-centered sample
locations as needed, directly from the GrClipMaskManager. Before it
would just assume that if we had a stencil test, it also meant we
wanted co-centered sample locations. This was incorrect even though it
accidentally worked on nvpr.

Also moves the code into flushRenderTarget that enables co-centered
samples, in an effort to reduce framebuffer binds.

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

to

==========
Fix mixed samples stencil clip

Reworks mixed samples stencil clip to setup co-centered sample
locations as needed, directly from the GrClipMaskManager. Before it
would try to infer whether we wanted co-centered sample locations
from GrGLGpu, and didn't always get it right. This fixes a few
rendering bugs that resulted from it.

Also moves the code into flushRenderTarget that enables co-centered
samples, in an effort to reduce framebuffer binds.

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

[Chris Dalton, 2015-11-09 18:26:36]

Description was changed from

==========
Fix mixed samples stencil clip

Reworks mixed samples stencil clip to setup co-centered sample
locations as needed, directly from the GrClipMaskManager. Before it
would try to infer whether we wanted co-centered sample locations
from GrGLGpu, and didn't always get it right. This fixes a few
rendering bugs that resulted from it.

Also moves the code into flushRenderTarget that enables co-centered
samples, in an effort to reduce framebuffer binds.

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

to

==========
Fix mixed samples stencil clip

Fixes rendering bugs and nondeterminism in gm.

Before, mixed samples stencil clip would try to infer whether the draw
wanted co-centered sample locations from within GrGLGpu, which caused
various errors. This change reworks it so the draw itself can request
the co-centered sample locations when it knows it will need them.

Also reduces framebuffer binds by moving the code that enables
GL_FRAMEBUFFER_PROGRAMMABLE_SAMPLE_LOCATIONS into flushRenderTarget.
==========

[Chris Dalton, 2015-11-09 18:41:58]

https://codereview.chromium.org/1431593006/diff/60001/src/gpu/GrClipMaskManag...
File src/gpu/GrClipMaskManager.h (right):

https://codereview.chromium.org/1431593006/diff/60001/src/gpu/GrClipMaskManag...
src/gpu/GrClipMaskManager.h:40: uint32_t blockedPipelineFlags() const { return
fBlockedPipelineFlags; }
This seemed like the right place to override the hwaa flags, but I'm not a huge
fan of the way it's done currently. Definitely open to suggestions.

[bsalomon, 2015-11-09 19:26:46]

https://codereview.chromium.org/1431593006/diff/60001/src/gpu/GrClipMaskManag...
File src/gpu/GrClipMaskManager.h (right):

https://codereview.chromium.org/1431593006/diff/60001/src/gpu/GrClipMaskManag...
src/gpu/GrClipMaskManager.h:40: uint32_t blockedPipelineFlags() const { return
fBlockedPipelineFlags; }
On 2015/11/09 18:41:58, Chris Dalton wrote:
> This seemed like the right place to override the hwaa flags, but I'm not a
huge
> fan of the way it's done currently. Definitely open to suggestions.

Are the blocked flags used anywhere?

https://codereview.chromium.org/1431593006/diff/60001/src/gpu/GrPipeline.h
File src/gpu/GrPipeline.h (right):

https://codereview.chromium.org/1431593006/diff/60001/src/gpu/GrPipeline.h#ne...
src/gpu/GrPipeline.h:42: struct CreateArgs {
Perhaps CreateArgs should take an GrAppliedClip rather than exposing these
flags?

[Chris Dalton, 2015-11-09 19:49:36]

https://codereview.chromium.org/1431593006/diff/60001/src/gpu/GrClipMaskManag...
File src/gpu/GrClipMaskManager.h (right):

https://codereview.chromium.org/1431593006/diff/60001/src/gpu/GrClipMaskManag...
src/gpu/GrClipMaskManager.h:40: uint32_t blockedPipelineFlags() const { return
fBlockedPipelineFlags; }
On 2015/11/09 19:26:46, bsalomon wrote:
> On 2015/11/09 18:41:58, Chris Dalton wrote:
> > This seemed like the right place to override the hwaa flags, but I'm not a
> huge
> > fan of the way it's done currently. Definitely open to suggestions.
> 
> Are the blocked flags used anywhere?

No, I only added it for philosophical reasons.. However, your other comment got
me thinking that the real problem here is that GrAppliedClip doesn't need to
expose pipeline flags, but rather can have a very specific option to do the one
thing GrClipMaskManager was using those flags for. I'll make the change.

https://codereview.chromium.org/1431593006/diff/60001/src/gpu/GrPipeline.h
File src/gpu/GrPipeline.h (right):

https://codereview.chromium.org/1431593006/diff/60001/src/gpu/GrPipeline.h#ne...
src/gpu/GrPipeline.h:42: struct CreateArgs {
On 2015/11/09 19:26:46, bsalomon wrote:
> Perhaps CreateArgs should take an GrAppliedClip rather than exposing these
> flags?

Ok, that's a good idea. And GrAppliedClip doesn't need to know these flags, but
rather can have a very specific bool to do exactly what it wants -- enable hwaa
but with co-centered sample locations.

[Chris Dalton, 2015-11-09 21:48:58]

Thanks for the suggestions, I like it a lot better now.

[bsalomon, 2015-11-09 22:36:22]

lgtm

[Chris Dalton, 2015-11-09 22:59:43]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2015-11-09 22:59:56]

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

[commit-bot: I haz the power, 2015-11-09 23:12:22]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/14184d5567b58085b6d8a6375796d405056f7f73

[Chris Dalton, 2015-11-10 17:58:41]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1407063011/ by cdalton@nvidia.com.

The reason for reverting is: Co-centered sample locations are not needed to do
stencil clip with mixed samples..