Enable stencil clipping in mixed sampled render targets

Implement stencil clipping in mixed sampled render targets

This change enables multisampled clipping for mixed sampled
render targets.  Previously clipping in mixed samples config
behaved the same as in the gpu config.

In order to retrofit non-MSAA draw methods, programmable sample
locations are used in order to colocate all samples at (0.5, 0.5).
Requires support for NV_sample_locations.

BUG=skia:4399
Committed: https://skia.googlesource.com/skia/+/3e77ba96d56d15db30ac6d8ccb900e30aafcbb16

[vbuzinov, 2015-09-21 12:08:48]

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

[vbuzinov, 2015-09-21 12:08:48]

Hey guys!  I believe I have a correctly working patch ready.  Could you take a
look and give me your comments?

In this updated version, programmable sample locations are enabled once per
render target, its default sample locations are queried and saved in an array. 
Whenever a skia shape is drawn, samples are colocated at pixel center, when an
NVPR shape is drawn, samples are restored to default.  Multisampling is always
on.

In clip mask manager, two of the previous numColorSamples/isUnifiedMultisampled
sites replaced with appropriate numStencilSamples/isStencilBufferMultisampled to
enable multisampled clipping.  In one spot, I believe isUnifiedMultisampled()
check is behaving correctly, but there is a comment next to it saying the
coverage based AA clips won't work correctly with MSAA.  If we abide this
comment, the perf drops significantly in many cases, if left as in the patch, we
get 10-15% perf gains in amazon and frantzen and get even results in cases that
do little or no clipping.  There are no rendering differences except some slight
off-by-1 differences similar to the difference between gpu and msaa4.

I still need to check I haven't broken the code for non-NVPR-compatible hardware
and that the Windows build is unaffected as well.

Maybe some modifications could be simplified or relocated to more appropriate
places.  Any suggestions?
Thanks!

[Mark Kilgard, 2015-09-21 17:03:50]

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

[Mark Kilgard, 2015-09-21 17:03:51]

> Enable stencil clipping in mixed sampled render targets

Excellent.

So this replaces having to CPU-rasterize a coverage mask and then download it as
a texture and have shaders sample this coverage mask texture?  So I'd expect
this improved approach should be much faster than that.

I had comments about GrGLGpu.cpp and doing glGetMultisamplefv.  I think this
should get before checkin.

Otherwise, this looks good.

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

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:160: // Sample locations that are colocated at pixel
center
for the sake of documentation, I recommend 2*16 instead of 32

will you need 0.5f to quite warnings on any platform Skia cares about?

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1920: uint32_t rtID = target->getUniqueID();
is it worth considering using glNamedFramebufferParameteri (OpenGL 4.5) here to
eliminate the need for the glBindFramebuffer?

This would create a dependency on OpenGL 4.5 (or EXT_direct_state_access)...

Perhaps we are already bound to the framebuffer we need coming through here?

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1930: float defaultSampleLocations[32];
worth adding

assert(rt->numRasterSamples()*2 == SK_ARRAY_COUNT(gCenteredSampleLocations));
assert(rt->numRasterSamples()*2 == SK_ARRAY_COUNT(defaultSampleLocations));

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1932: GL_CALL(GetMultisamplefv(GR_GL_SAMPLE_LOCATION_NV,
i, &defaultSampleLocations[i*2]));
calling a glGetMultisamplefv here many times (as many as 16 times) is rather
expensive

it would make more sense to stash the default sample locations once (per
GrContext?) to avoid querying them.  This is particularly true given the command
buffer usage in Chrome.

Also, glGetMultisamplefv depends on the currently bound drawable FBO.  So you'd
need the glBindFramebuffer command above for this query to work, and hence
couldn't use the DSA version of glFramebufferParameteri.  Another reasons to
stash the default locations once per context.

Alternatively, you could just hard code the sample positions so they don't have
to be queried at all.  Microsoft specified the "default" sample positions for
1x, 2x, 4x, 8x, and 16x antialiasing.  The Microsoft locations are "good" in the
sense that the samples are well distributed for horizontal and vertical edges.

[Chris Dalton, 2015-09-21 17:12:46]

On 2015/09/21 17:03:51, Mark Kilgard wrote:
> So this replaces having to CPU-rasterize a coverage mask and then download it
as
> a texture and have shaders sample this coverage mask texture?

Not quite. The coverage mask was rasterized on the GPU so it's better than you
thought. However, with the new approach we get to use nvpr and we don't have to
change render targets, so it should still be a big improvement.

[Chris Dalton, 2015-09-22 08:35:24]

Hey, sorry for the onslaught of comments. They should be mostly all pretty minor
and easy to work out. It's exciting to see this all working!

https://codereview.chromium.org/1232103002/diff/20001/include/gpu/gl/GrGLFunc...
File include/gpu/gl/GrGLFunctions.h (right):

https://codereview.chromium.org/1232103002/diff/20001/include/gpu/gl/GrGLFunc...
include/gpu/gl/GrGLFunctions.h:398: typedef GrGLvoid (GR_GL_FUNCTION_TYPE*
GrGLGetMultisamplefvProc)(GrGLenum pname, GrGLuint index, const GrGLfloat *val);
glGetultisamplefvProc is actually GL 3.2 and GLES 3.1

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLAssemble...
File src/gpu/gl/GrGLAssembleInterface.cpp (right):

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLAssemble...
src/gpu/gl/GrGLAssembleInterface.cpp:458: GET_PROC_SUFFIX(GetMultisamplefv, NV);
For glGetMultisamplefv we should check for GL version 3.2, and we should also
load it for GLES version 3.1.

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

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLCaps.cpp...
src/gpu/gl/GrGLCaps.cpp:352: ctxInfo.hasExtension("GL_NV_sample_locations");
With our current plans to just put basic mixed samples support in the chromium
command buffer, it won't work to rope this extension in with mixed samples as
well. I'm afraid we're going to have to make a separate cap for sample
locations, and then check that specific cap before trying to use an MSAA stencil
clip.

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLDefines.h
File src/gpu/gl/GrGLDefines.h (right):

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLDefines....
src/gpu/gl/GrGLDefines.h:376: #define GR_GL_SAMPLE_LOCATION_NV            
0x8E50
Here we can use the more standard GL_SAMPLE_POSITION. It has the same value.

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

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1911: return;
Skia style uses braces for one line if's.

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1934:
target->setDefaultSampleLocations(rt->numRasterSamples(),
defaultSampleLocations);
We ought to set this up so it only queries the default sample locations once.
From the look of things, it will make this query every time we enable centered
sample locations, which would probably impact perf.

Also, do we need to remember the default locations? Or is it good enough to just
disable GL_FRAMEBUFFER_PROGRAMMABLE_SAMPLE_LOCATIONS?

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:2199: useHWAA = true;
I still think it's desirable to disable multisample rather than co-locating
sample positions if we don't need hwaa enabled for the stencil test. Can we find
a way to only co-locate the sample locations when we know there is a stencil
test?

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

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLGpu.h#ne...
src/gpu/gl/GrGLGpu.h:261: void setProgrammableSampleLocations(GrRenderTarget*
rt, bool useSampleLocations);
I wonder if this should have a different name, since its actual purpose seems to
be enabling centered, co-located sample positions?

Either that, or this method accepts an array of sample positions and the caller
supplies centered locdations (or whatever it wants).

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLInterfac...
File src/gpu/gl/GrGLInterface.cpp (right):

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLInterfac...
src/gpu/gl/GrGLInterface.cpp:704: if (glVer >= GR_GL_VER(4,3)) {
I think we also need to check the GL standard and make sure it isn't GLES before
validating FramebufferParameteri.

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLInterfac...
src/gpu/gl/GrGLInterface.cpp:708: if (NULL ==
fFunctions.fNamedFramebufferSampleLocationsfv) {
We should validate for this method when GL_NV_sample_locations is present,
correct? (Not GL version 4.3?)

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLInterfac...
src/gpu/gl/GrGLInterface.cpp:711: if (NULL == fFunctions.fGetMultisamplefv) {
We can validate for this method on GL 3.2 and GLES 3.1.

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLRenderTa...
File src/gpu/gl/GrGLRenderTarget.h (right):

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLRenderTa...
src/gpu/gl/GrGLRenderTarget.h:80: };
Just a thought -- we may want to consider putting the sample locations stuff in
GrRenderTargetPriv instead. What do you think?

[Chris Dalton, 2015-09-22 08:35:25]

Hey, sorry for the onslaught of comments. They should be mostly all pretty minor
and easy to work out. It's exciting to see this all working!

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLRenderTa...
File src/gpu/gl/GrGLRenderTarget.h (right):

https://codereview.chromium.org/1232103002/diff/20001/src/gpu/gl/GrGLRenderTa...
src/gpu/gl/GrGLRenderTarget.h:80: };
Just a thought -- we may want to consider putting the sample locations stuff in
GrRenderTargetPriv instead. What do you think?

[vbuzinov, 2015-09-22 13:10:13]

New revision, hopefully I haven't missed anything.  

As per Mark's suggestion, I hardcoded the default sample patterns so that we
don't need to call GetMultisample every time.  Microsoft's patterns seem to
match NV hw completely, except for the order of the coordinates in the sequence.

Please take a look at the new version. Thanks!

[Chris Dalton, 2015-09-23 03:42:51]

This looks really good overall. Just watch out for lines going over 100 columns.
Also, I am still of the opinion that if we are going to hard-code the sample
locations we should be doing it from the beginning of the render target's
lifetime, but hopefully we end up being able to enable/disable programmable
sample locations instead!

https://codereview.chromium.org/1232103002/diff/40001/src/gpu/gl/GrGLDefines.h
File src/gpu/gl/GrGLDefines.h (right):

https://codereview.chromium.org/1232103002/diff/40001/src/gpu/gl/GrGLDefines....
src/gpu/gl/GrGLDefines.h:376: #define GR_GL_SAMPLE_POSITION               
0x8E50
We might not need this at all anymore now that we took out glGetMultisamplefv,
right?

[vbuzinov, 2015-09-29 11:25:34]

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

[vbuzinov, 2015-09-29 11:25:34]

Adding Brian to review.

Hey Brian,

Could you take a look at this patch that adds the missing support for
multisampled clipping in mixed samples config?

In order to draw non-MSAA shapes with multisampling on, we use
NV_sample_locations to colocate all samples at (0.5, 0.5).  That required adding
some API changes in GrGLGpu as well, which I don't feel completely confident
about, could you take a look and give your suggestions?

You can find more info in the referenced skia issue.

Thanks!

--

New in this revision: no hardcoded sample locations, rely on the fact that
programmable sample locations are colocated at (0.5, 0.5) by default.  Also adds
a check for whether the stencil test is on, and only enables multisampling in
that case.

[Kimmo Kinnunen, 2015-09-29 12:45:24]

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

https://codereview.chromium.org/1232103002/diff/60001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:284: if (resetBits & kMisc_GrGLBackendState) {
The sample locations are probably part of GL state, similar to any other GL
state.

So the clients might change GL state between Skia calls. For example, they might
change the DepthMask. Similarly, they might change sample locations.

When client has changed GL state, they issue context reset, so that Skia forces
its view of GL state to the GL.

Thus, you probably need to program  the sample locations to 0.5, 0.5 here, even
though that's the GL default.

[Kimmo Kinnunen, 2015-09-29 12:53:37]

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

https://codereview.chromium.org/1232103002/diff/60001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:284: if (resetBits & kMisc_GrGLBackendState) {
On 2015/09/29 12:45:24, Kimmo Kinnunen wrote:
> Thus, you probably need to program  the sample locations to 0.5, 0.5 here,
even
> though that's the GL default.

Ah, never mind. The sample locations state is per fbo, and that can not be
changed by the client.

[bsalomon, 2015-09-29 17:41:57]

mostly looks good with some nits and Q about whether this should be on
GrGLRenderTarget rather than GrRenderTarget.

https://codereview.chromium.org/1232103002/diff/60001/include/gpu/GrRenderTar...
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1232103002/diff/60001/include/gpu/GrRenderTar...
include/gpu/GrRenderTarget.h:179: // sample locations are currently in use.
I'm wondering whether this should be in the GL subclass rather than the base
class. One reason is that the setting/reading of this is only valid during
playback to the GrGpu object. The GL subclasses of resources aren't known to
most all the classes that are used before the draw target is flushed to the GPU

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

https://codereview.chromium.org/1232103002/diff/60001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1912: GR_GL_FRAMEBUFFER_PROGRAMMABLE_SAMPLE_LOCATIONS,
useColocatedSampleLocations));
nit: wrap

https://codereview.chromium.org/1232103002/diff/60001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:2170: setColocatedSampleLocations(rt, false);
this->

https://codereview.chromium.org/1232103002/diff/60001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:2172: setColocatedSampleLocations(rt, true);
this->

[vbuzinov, 2015-09-30 06:08:29]

The new revision corrects things pointed out by Brian.

https://codereview.chromium.org/1232103002/diff/60001/include/gpu/GrRenderTar...
File include/gpu/GrRenderTarget.h (right):

https://codereview.chromium.org/1232103002/diff/60001/include/gpu/GrRenderTar...
include/gpu/GrRenderTarget.h:179: // sample locations are currently in use.
On 2015/09/29 17:41:57, bsalomon wrote:
> I'm wondering whether this should be in the GL subclass rather than the base
> class. One reason is that the setting/reading of this is only valid during
> playback to the GrGpu object. The GL subclasses of resources aren't known to
> most all the classes that are used before the draw target is flushed to the
GPU

I agree, I think this property belongs to the GL subclass (that is where I had
it originally). Thanks!

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

https://codereview.chromium.org/1232103002/diff/60001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1912: GR_GL_FRAMEBUFFER_PROGRAMMABLE_SAMPLE_LOCATIONS,
useColocatedSampleLocations));
On 2015/09/29 17:41:57, bsalomon wrote:
> nit: wrap

Done.

https://codereview.chromium.org/1232103002/diff/60001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:2170: setColocatedSampleLocations(rt, false);
On 2015/09/29 17:41:57, bsalomon wrote:
> this->

Done.

https://codereview.chromium.org/1232103002/diff/60001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:2172: setColocatedSampleLocations(rt, true);
On 2015/09/29 17:41:57, bsalomon wrote:
> this->

Done.

[bsalomon, 2015-09-30 13:38:33]

lgtm

[Mark Kilgard, 2015-09-30 17:17:47]

lgtm

Minor comments.  Looking for ARB_sample_locations, in addition to
NV_sample_locations, is a little more future oriented.

https://codereview.chromium.org/1232103002/diff/80001/src/gpu/gl/GrGLAssemble...
File src/gpu/gl/GrGLAssembleInterface.cpp (right):

https://codereview.chromium.org/1232103002/diff/80001/src/gpu/gl/GrGLAssemble...
src/gpu/gl/GrGLAssembleInterface.cpp:454: GET_PROC(NamedFramebufferParameteri);
Technically, the ARB_framebuffer_no_attachments provides the
glNamedFramebufferParameteriEXT when EXT_direct_state_access is also provided. 
This would allow pre-OpenGL 4.5 drivers to have access to the
NamedFramebufferParameter interface.

Probably not worth adding here since we expect to use
glNamedFramebufferParameteri with ARB_sample_locations.  Best to keep it simple
I think.

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

https://codereview.chromium.org/1232103002/diff/80001/src/gpu/gl/GrGLCaps.cpp...
src/gpu/gl/GrGLCaps.cpp:358: glslCaps->fProgrammableSampleLocationsSupport =
ctxInfo.hasExtension("GL_NV_sample_locations");
probably also worth looking for ARB_sample_locations extension name here.

NVIDIA products will support both the NV (predated the ARB version) and, in
newer versions, the ARB version.

Since we currently plan on just toggling the
GL_FRAMEBUFFER_PROGRAMMABLE_SAMPLE_LOCATIONS_ARB FBO state and the ARB and NV
versions have the same token values, I recommend detecting either extension.

So

glslCaps->fProgrammableSampleLocationsSupport =
ctxInfo.hasExtension("GL_NV_sample_locations") ||
ctxInfo.hasExtension("GL_ARB_sample_locations");

https://codereview.chromium.org/1232103002/diff/80001/src/gpu/gl/GrGLDefines.h
File src/gpu/gl/GrGLDefines.h (right):

https://codereview.chromium.org/1232103002/diff/80001/src/gpu/gl/GrGLDefines....
src/gpu/gl/GrGLDefines.h:892: /* GL_NV_sample_locations */
commend could say GL_ARB_sample_locations too

[vbuzinov, 2015-10-01 05:55:19]

Adding a check for GL_ARB_sample_locations as per Mark's suggestion and
submitting the patch.

[vbuzinov, 2015-10-01 05:55:31]

The CQ bit was checked by vbuzinov@nvidia.com

[vbuzinov, 2015-10-01 05:55:32]

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

[commit-bot: I haz the power, 2015-10-01 05:55:39]

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

[Mark Kilgard, 2015-10-01 06:00:00]

good fixes

lgtm

Worth noting that this patch makes sure that clipping is properly antialiased on
a sub-pixel basis.  This is good for clipping quality and superior to the
fractional coverage I believe Ganesh is doing currently. 
NV_framebuffer_mixed_samples makes this all possible.

Nice work, Seva.

[commit-bot: I haz the power, 2015-10-01 06:02:12]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/3e77ba96d56d15db30ac6d8ccb900e30aafcbb16

[bsalomon, 2015-11-10 18:03:18]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1426993007/ by bsalomon@google.com.

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