[Mesa-dev] [PATCH] Allow setting GL_TEXTURE_COMPARE_MODE on a sampler object without ARB_shadow support.

[Brian Paul]

On 06/25/2015 05:11 PM, Matt Turner wrote:
> On Mon, Mar 23, 2015 at 12:25 PM, Stefan Dösinger
> <stefandoesinger at gmx.at> wrote:
>> This fixes a GL error warning on r200 in Wine.
>>
>> The GL_ARB_sampler_objects extension does not specify a dependency on
>> GL_ARB_shadow or GL_ARB_depth_texture for this value. Just set the value
>> and don't do anything else. It won't matter without a depth texture
>> being assigned anyway.
>> ---
>
> Brian, Ian -- you guys seem like you might have opinions about this.

Digging up the patch from March for reference:


 > This fixes a GL error warning on r200 in Wine.
 >
 > The GL_ARB_sampler_objects extension does not specify a dependency on
 > GL_ARB_shadow or GL_ARB_depth_texture for this value. Just set the value
 > and don't do anything else. It won't matter without a depth texture
 > being assigned anyway.

So I take it that Wine calls glSamplerParameteri(s, 
GL_TEXTURE_COMPARE_MODE, mode) even when GL_ARB_shadow is not supported 
and you get a bunch of GL errors?

Has this been reported upstream to Wine so they can fix it?

I see this sort of thing all the time in Windows OpenGL apps and it's 
tempting to just silence some obscure GL errors, but it's a slippery 
slope.  When I'm debugging a new GL app, seeing GL errors for all the 
corner cases can actually be very helpful and I'd hate to lose that.

In this particular case, given that GL_ARB_shadow is pretty widely 
supported except on old devices like r200, I guess I could live with a 
code change though.  Comments below.



 > ---
 >   src/mesa/main/samplerobj.c | 35 +++++++++++++----------------------
 >   1 file changed, 13 insertions(+), 22 deletions(-)
 >
 > diff --git a/src/mesa/main/samplerobj.c b/src/mesa/main/samplerobj.c
 > index d66b0b5..6095efd 100644
 > --- a/src/mesa/main/samplerobj.c
 > +++ b/src/mesa/main/samplerobj.c
 > @@ -621,12 +621,18 @@ static GLuint
 >   set_sampler_compare_mode(struct gl_context *ctx,
 >                            struct gl_sampler_object *samp, GLint param)
 >   {
 > -   if (!ctx->Extensions.ARB_shadow)
 > -      return INVALID_PNAME;
 > -

The simplest solution, if we do want to change the code, is to replace 
the above condition with:

    if (!ctx->Extensions.ARB_shadow)
       return GL_FALSE;  /* no state change, no error */


 >      if (samp->CompareMode == param)
 >         return GL_FALSE;
 >
 > +   if (!ctx->Extensions.ARB_shadow) {
 > +      if (param == GL_NONE ||
 > +          param == GL_COMPARE_R_TO_TEXTURE_ARB) {
 > +         samp->CompareMode = param;
 > +         return GL_FALSE;
 > +      }
 > +      return INVALID_PARAM;
 > +   }
 > +

This chunk wouldn't be needed.  Same below.


 >      if (param == GL_NONE ||
 >          param == GL_COMPARE_R_TO_TEXTURE_ARB) {
 >         flush(ctx);
 > @@ -642,9 +648,6 @@ static GLuint
 >   set_sampler_compare_func(struct gl_context *ctx,
 >                            struct gl_sampler_object *samp, GLint param)
 >   {
 > -   if (!ctx->Extensions.ARB_shadow)
 > -      return INVALID_PNAME;
 > -
 >      if (samp->CompareFunc == param)
 >         return GL_FALSE;
 >
 > @@ -657,6 +660,10 @@ set_sampler_compare_func(struct gl_context *ctx,
 >      case GL_GREATER:
 >      case GL_ALWAYS:
 >      case GL_NEVER:
 > +      if(!ctx->Extensions.ARB_shadow) {
 > +         samp->CompareFunc = param;
 > +         return GL_FALSE;
 > +      }
 >         flush(ctx);
 >         samp->CompareFunc = param;
 >         return GL_TRUE;
 > @@ -1329,13 +1336,9 @@ _mesa_GetSamplerParameteriv(GLuint sampler, 
GLenum pname, GLint *params)
 >         *params = IROUND(sampObj->LodBias);
 >         break;
 >      case GL_TEXTURE_COMPARE_MODE:
 > -      if (!ctx->Extensions.ARB_shadow)
 > -         goto invalid_pname;

Again, if we do decide to change the code, just replace the extension 
check with a comment saying we're not checking for ARB_shadow to avoid 
Wine errors.


 >         *params = sampObj->CompareMode;
 >         break;
 >      case GL_TEXTURE_COMPARE_FUNC:
 > -      if (!ctx->Extensions.ARB_shadow)
 > -         goto invalid_pname;
 >         *params = sampObj->CompareFunc;
 >         break;
 >      case GL_TEXTURE_MAX_ANISOTROPY_EXT:
 > @@ -1418,13 +1421,9 @@ _mesa_GetSamplerParameterfv(GLuint sampler, 
GLenum pname, GLfloat *params)
 >         *params = sampObj->LodBias;
 >         break;
 >      case GL_TEXTURE_COMPARE_MODE:
 > -      if (!ctx->Extensions.ARB_shadow)
 > -         goto invalid_pname;
 >         *params = (GLfloat) sampObj->CompareMode;
 >         break;
 >      case GL_TEXTURE_COMPARE_FUNC:
 > -      if (!ctx->Extensions.ARB_shadow)
 > -         goto invalid_pname;
 >         *params = (GLfloat) sampObj->CompareFunc;
 >         break;
 >      case GL_TEXTURE_MAX_ANISOTROPY_EXT:
 > @@ -1497,13 +1496,9 @@ _mesa_GetSamplerParameterIiv(GLuint sampler, 
GLenum pname, GLint *params)
 >         *params = (GLint) sampObj->LodBias;
 >         break;
 >      case GL_TEXTURE_COMPARE_MODE:
 > -      if (!ctx->Extensions.ARB_shadow)
 > -         goto invalid_pname;
 >         *params = sampObj->CompareMode;
 >         break;
 >      case GL_TEXTURE_COMPARE_FUNC:
 > -      if (!ctx->Extensions.ARB_shadow)
 > -         goto invalid_pname;
 >         *params = sampObj->CompareFunc;
 >         break;
 >      case GL_TEXTURE_MAX_ANISOTROPY_EXT:
 > @@ -1576,13 +1571,9 @@ _mesa_GetSamplerParameterIuiv(GLuint sampler, 
GLenum pname, GLuint *params)
 >         *params = (GLuint) sampObj->LodBias;
 >         break;
 >      case GL_TEXTURE_COMPARE_MODE:
 > -      if (!ctx->Extensions.ARB_shadow)
 > -         goto invalid_pname;
 >         *params = sampObj->CompareMode;
 >         break;
 >      case GL_TEXTURE_COMPARE_FUNC:
 > -      if (!ctx->Extensions.ARB_shadow)
 > -         goto invalid_pname;
 >         *params = sampObj->CompareFunc;
 >         break;
 >      case GL_TEXTURE_MAX_ANISOTROPY_EXT:
 >