Enable stencil clipping in mixed sampled render targets

src/gpu/gl/GrGLGpu.cpp

Index: src/gpu/gl/GrGLGpu.cpp
diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp
index c07f704d21342df6ebb7723c0358dca14351c903..ce7ce4be122e469476b882bddb51559bddedbba5 100644
--- a/src/gpu/gl/GrGLGpu.cpp
+++ b/src/gpu/gl/GrGLGpu.cpp
@@ -157,6 +157,12 @@ bool GrGLGpu::BlendCoeffReferencesConstant(GrBlendCoeff coeff) {
     GR_STATIC_ASSERT(kGrBlendCoeffCnt == SK_ARRAY_COUNT(gXfermodeCoeff2Blend));
 }
 
+// Sample locations that are colocated at pixel center

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

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?

+static const GrGLfloat gCenteredSampleLocations[32] = { 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5,
+                                                        0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5,
+                                                        0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5,
+                                                        0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5 };
+
 ///////////////////////////////////////////////////////////////////////////////
 
 // Used in the map of pixel configs to stencil format indices. This value is used to
@@ -1900,6 +1906,40 @@ bool GrGLGpu::onReadPixels(GrSurface* surface,
     return true;
 }
 
+void GrGLGpu::setProgrammableSampleLocations(GrRenderTarget* rt, bool useSampleLocations) {
+    if (0 == rt->numRasterSamples())
+        return;

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

Skia style uses braces for one line if's.

+
+    GrGLRenderTarget* target = static_cast<GrGLRenderTarget*>(rt->asRenderTarget());
+    SkASSERT(0 != target->renderFBOID());
+
+    if (useSampleLocations == target->usesProgrammableSampleLocations())
+        return;
+
+    if (GrGLRenderTarget::kProgrammable_SampleLocationsConfig != target->sampleLocationsConfig()) {
+        uint32_t rtID = target->getUniqueID();

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

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?

+        if (fHWBoundRenderTargetUniqueID != rtID) {
+            fStats.incRenderTargetBinds();
+            GL_CALL(BindFramebuffer(GR_GL_FRAMEBUFFER, target->renderFBOID()));
+            fHWBoundRenderTargetUniqueID = SK_InvalidUniqueID;
+        }
+
+        GL_CALL(FramebufferParameteri(GR_GL_FRAMEBUFFER, GR_GL_FRAMEBUFFER_PROGRAMMABLE_SAMPLE_LOCATIONS, true));
+        target->setSampleLocationsConfig(GrGLRenderTarget::kProgrammable_SampleLocationsConfig);
+
+        float defaultSampleLocations[32];

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

worth adding

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

+        for (int i = 0; i < rt->numRasterSamples(); i++) {
+            GL_CALL(GetMultisamplefv(GR_GL_SAMPLE_LOCATION_NV, i, &defaultSampleLocations[i*2]));

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

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.

+        }
+        target->setDefaultSampleLocations(rt->numRasterSamples(), defaultSampleLocations);

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

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?

+    }
+
+    GL_CALL(NamedFramebufferSampleLocationsfv(target->renderFBOID(), 0, rt->numRasterSamples(),
+                                              useSampleLocations ? gCenteredSampleLocations
+                                                                 : target->defaultSampleLocations()));
+    target->flagAsUsingProgrammableSampleLocations(useSampleLocations);
+}
+
 void GrGLGpu::flushRenderTarget(GrGLRenderTarget* target, const SkIRect* bound) {
 
     SkASSERT(target);
@@ -2150,6 +2190,15 @@ void GrGLGpu::flushStencil(const GrStencilSettings& stencilSettings) {
 void GrGLGpu::flushHWAAState(GrRenderTarget* rt, bool useHWAA) {
     SkASSERT(!useHWAA || rt->isStencilBufferMultisampled());
 
+    if (rt->hasMixedSamples()) {
+        if (useHWAA) {
+            setProgrammableSampleLocations(rt, false);
+        } else {
+            setProgrammableSampleLocations(rt, true);
+        }
+        useHWAA = true;

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

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?

+    }
+
     if (this->glCaps().multisampleDisableSupport()) {
         if (useHWAA) {
             if (kYes_TriState != fMSAAEnabled) {