Fix setColocatedSampleLocations on ES and GL < 4.5

Fix setColocatedSampleLocations on ES and GL < 4.5

Updates setColocatedSampleLocations to use glFramebufferParameteri when
the DSA version glNamedFramebufferParameteri is not present.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/98cad6219b430eddf5528473311279f21dbd2e10

[Chris Dalton, 2015-11-03 07:21:30]

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

[Chris Dalton, 2015-11-03 07:21:31]

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

https://codereview.chromium.org/1415503008/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:2038: if (kGL_GrGLStandard == this->glStandard() &&
this->glVersion() >= GR_GL_VER(4,5)) {
Would it be ok/consistent/better to just check the function ptr itself for being
null? Then it's one check instead of two.

[bsalomon, 2015-11-03 15:21:12]

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

https://codereview.chromium.org/1415503008/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:2038: if (kGL_GrGLStandard == this->glStandard() &&
this->glVersion() >= GR_GL_VER(4,5)) {
On 2015/11/03 07:21:30, Chris Dalton wrote:
> Would it be ok/consistent/better to just check the function ptr itself for
being
> null? Then it's one check instead of two.

In general we don't do that. You could add a enum to GrGLCaps that indicates
which function to use.

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

https://codereview.chromium.org/1415503008/diff/1/src/gpu/gl/GrGLInterface.cp...
src/gpu/gl/GrGLInterface.cpp:718: fExtensions.has("GL_NV_sample_locations") ||
Here you check for the extensions but not in the interface assembler code?

[Chris Dalton, 2015-11-03 15:49:16]

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

https://codereview.chromium.org/1415503008/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:2038: if (kGL_GrGLStandard == this->glStandard() &&
this->glVersion() >= GR_GL_VER(4,5)) {
On 2015/11/03 15:21:12, bsalomon wrote:
> On 2015/11/03 07:21:30, Chris Dalton wrote:
> > Would it be ok/consistent/better to just check the function ptr itself for
> being
> > null? Then it's one check instead of two.
> 
> In general we don't do that. You could add a enum to GrGLCaps that indicates
> which function to use.

No problem. Would you actually prefer another cap? Or are we fine just checking
the GL version?

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

https://codereview.chromium.org/1415503008/diff/1/src/gpu/gl/GrGLInterface.cp...
src/gpu/gl/GrGLInterface.cpp:718: fExtensions.has("GL_NV_sample_locations") ||
On 2015/11/03 15:21:12, bsalomon wrote:
> Here you check for the extensions but not in the interface assembler code?

Yeah this check was a little OCD. glFramebufferParameteri is core, but the idea
was that these extensions depend on it (they're written against OpenGL versions
that have this function). I can just take the check out for the extensions.

[bsalomon, 2015-11-03 15:54:22]

lgtm

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

https://codereview.chromium.org/1415503008/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:2038: if (kGL_GrGLStandard == this->glStandard() &&
this->glVersion() >= GR_GL_VER(4,5)) {
On 2015/11/03 15:49:15, Chris Dalton wrote:
> On 2015/11/03 15:21:12, bsalomon wrote:
> > On 2015/11/03 07:21:30, Chris Dalton wrote:
> > > Would it be ok/consistent/better to just check the function ptr itself for
> > being
> > > null? Then it's one check instead of two.
> > 
> > In general we don't do that. You could add a enum to GrGLCaps that indicates
> > which function to use.
> 
> No problem. Would you actually prefer another cap? Or are we fine just
checking
> the GL version?

Whatever works for you

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

https://codereview.chromium.org/1415503008/diff/1/src/gpu/gl/GrGLInterface.cp...
src/gpu/gl/GrGLInterface.cpp:718: fExtensions.has("GL_NV_sample_locations") ||
On 2015/11/03 15:49:15, Chris Dalton wrote:
> On 2015/11/03 15:21:12, bsalomon wrote:
> > Here you check for the extensions but not in the interface assembler code?
> 
> Yeah this check was a little OCD. glFramebufferParameteri is core, but the
idea
> was that these extensions depend on it (they're written against OpenGL
versions
> that have this function). I can just take the check out for the extensions.

I suppose if a vendor shipped an ES 3.0 with the extension advertised then we'd
expect to be able to get the function. So maybe update the assembler code? I
don't really have a strong opinion though, so happy to go with whatever you
think is best.

[Mark Kilgard, 2015-11-03 15:55:22]

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

[Mark Kilgard, 2015-11-03 15:55:22]

LGTM

(as far as the OpenGL usage; I'm not sure about the Skia conventions involved)

[bsalomon, 2015-11-03 17:21:32]

lgtm

[Chris Dalton, 2015-11-03 17:21:55]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2015-11-03 17:21:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
markkilgard@gmail.com
Link to the patchset: https://codereview.chromium.org/1415503008/#ps20001
(title: "address comments")

[commit-bot: I haz the power, 2015-11-03 17:22:04]

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

[commit-bot: I haz the power, 2015-11-03 17:33:26]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/98cad6219b430eddf5528473311279f21dbd2e10

[Chris Dalton, 2015-11-10 17:59:45]

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

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