Use stencil ops that don't issue writes whenever possible

Use stencil ops that don't issue writes whenever possible

Minimize the number of stencil writes by using GL_KEEP whenever possible.
This can positively affect the HW performance of the stencil test.

BUG=skia:3952
Committed: https://skia.googlesource.com/skia/+/c5d58f07aa6f4c47337d769e0e2e69c79669f91a

[vbuzinov, 2015-08-18 10:25:09]

vbuzinov@nvidia.com changed reviewers:
+ bsalomon@google.com, cdalton@nvidia.com, kkinnunen@nvidia.com

[vbuzinov, 2015-08-18 10:25:09]

Hey Chris and Brian,

As discussed in the issue, this change does away with stencil writes in cases
where they are unnecessary.

On top of that, second parameter (dpfail) to glStencilOp/glStencilOpSeparate is
replaced with the initial value of GL_KEEP, which shouldn't have any effect in
skia since depth test is always disabled, but this may hint the GL driver to do
additional optimization.

[bsalomon, 2015-08-18 13:15:25]

lgtm

[Mark Kilgard, 2015-08-18 14:51:47]

markkilgard@gmail.com changed reviewers:
+ markkilgard@gmail.com

[Mark Kilgard, 2015-08-18 14:51:47]

LGTM

Some additional comments...

This change leads to multiple "good" outcomes, though all pretty marginal in
their performance improvements.

KEEP is a "do nothing" operation while ZERO must actually write zero to stencil
samples failing the stencil test.  So KEEP is good for power and performance.

KEEP also allows us in NV_framebuffer_mixed_samples mode to use a more efficient
covering geometry (i.e. less conservative, so covering fewer needless pixels).

[Chris Dalton, 2015-08-18 19:33:11]

Nice to recover this missing nvpr perf.

There still are a few non-nvpr places throughout GrStencil.cpp,
GrClipMaskManager.cpp, etc. that could probably use a GL_KEEP here and there. I
don't know if we want to try and roll that into this change or just leave it for
a new one.

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

https://codereview.chromium.org/1288923005/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:2121: GR_GL_CALL(gl, StencilOp(glFailOp, GR_GL_KEEP,
glPassOp));
I'm not sure it seems correct to modify this part.

Shouldn't we just use the ops supplied in the stencil settings? And if we want
something to be "keep", then the code above this level is responsible to set it
accordingly?

[vbuzinov, 2015-08-21 12:16:58]

Answered to Chris inline. I am going to submit this as is to keep things simple,
optionally adding more stencil op changes in a later patch.

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

https://codereview.chromium.org/1288923005/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:2121: GR_GL_CALL(gl, StencilOp(glFailOp, GR_GL_KEEP,
glPassOp));
On 2015/08/18 19:33:11, Chris Dalton wrote:
> I'm not sure it seems correct to modify this part.
> 
> Shouldn't we just use the ops supplied in the stencil settings? And if we want
> something to be "keep", then the code above this level is responsible to set
it
> accordingly?

Chris, stencil settings don't contain a separate value for depth test fail case,
since skia never uses the depth test.  Effectively we are not changing the
existing behavior here in any way, other than hinting the driver.

[vbuzinov, 2015-08-21 12:17:14]

The CQ bit was checked by vbuzinov@nvidia.com

[commit-bot: I haz the power, 2015-08-21 12:17:26]

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

[commit-bot: I haz the power, 2015-08-21 12:24:28]

Committed patchset #1 (id:1) as
https://skia.googlesource.com/skia/+/c5d58f07aa6f4c47337d769e0e2e69c79669f91a