[Mesa-dev] [PATCH 1/2] mesa: Implement ARB_texture_filter_minmax

[Ian Romanick]

On 11/20/2017 04:12 AM, Emil Velikov wrote:
> Hi Scott,
> 
> I think there's a few missed checks if the extensions is enabled.
> 
> On 14 November 2017 at 22:54, Scott D Phillips
> <scott.d.phillips at intel.com> wrote:
> 
>> @@ -218,6 +218,7 @@ _legal_parameters(struct gl_context *ctx, GLenum target, GLenum internalformat,
>>     case GL_VIEW_COMPATIBILITY_CLASS:
>>     case GL_NUM_TILING_TYPES_EXT:
>>     case GL_TILING_TYPES_EXT:
>> +   case GL_TEXTURE_REDUCTION_MODE_ARB:
> Here.

I have always thought that we should add negative tests when we enable a
new extension.  Specifically, tests that try to use the new enums in
various valid places on drivers that do not support the extension.
There are a couple tests like this, but not a lot.  The only one I could
find for sure was
tests/spec/arb_texture_buffer_object/negative-unsupported.c.

>> @@ -1464,6 +1489,9 @@ _mesa_GetSamplerParameteriv(GLuint sampler, GLenum pname, GLint *params)
>> @@ -1536,6 +1564,9 @@ _mesa_GetSamplerParameterfv(GLuint sampler, GLenum pname, GLfloat *params)
>> @@ -1608,6 +1639,9 @@ _mesa_GetSamplerParameterIiv(GLuint sampler, GLenum pname, GLint *params)
>> @@ -1680,6 +1714,9 @@ _mesa_GetSamplerParameterIuiv(GLuint sampler, GLenum pname, GLuint *params)
>>           goto invalid_pname;
>>        *params = (GLenum) sampObj->sRGBDecode;
>>        break;
>> +   case GL_TEXTURE_REDUCTION_MODE_ARB:
> And these four?
> 
>> --- a/src/mesa/main/texparam.c
>> +++ b/src/mesa/main/texparam.c
>> @@ -630,6 +630,25 @@ set_tex_parameteri(struct gl_context *ctx,
>>        }
>>        goto invalid_pname;
>>
>> +   case GL_TEXTURE_REDUCTION_MODE_ARB:
>> +      if (ctx->Extensions.ARB_texture_filter_minmax) {
>> +
>> +         if (!_mesa_target_allows_setting_sampler_parameters(texObj->Target))
>> +            goto invalid_enum;
>> +
>> +         if (texObj->Sampler.ReductionMode == params[0])
>> +            return GL_FALSE;
>> +         if (params[0] == GL_MIN ||
>> +             params[0] == GL_MAX ||
>> +             params[0] == GL_WEIGHTED_AVERAGE_ARB) {
>> +            flush(ctx);
>> +            texObj->Sampler.ReductionMode = params[0];
>> +            return GL_TRUE;
>> +         }
>> +         goto invalid_param;
>> +      }
>> +      goto invalid_pname;
>> +
> Using the 'return early' approach will make it easier to read ...
> although the function is inconsistent so meh.
> Just an example, I'm talking about:
> 
>    case GL_TEXTURE_REDUCTION_MODE_ARB:
>       if (!ctx->Extensions.ARB_texture_filter_minmax)
>          goto invalid_pname;
> 
>       if (!_mesa_target_allows_setting_sampler_parameters(texObj->Target))
>          goto invalid_enum;
> 
>       if (texObj->Sampler.ReductionMode == params[0])
>          return GL_FALSE;
> 
>       if (params[0] != GL_MIN &&
>           params[0] != GL_MAX &&
>           params[0] != GL_WEIGHTED_AVERAGE_ARB)
>          goto invalid_param;
>       }
> 
>       flush(ctx);
>       texObj->Sampler.ReductionMode = params[0];
>       return GL_TRUE;
> 
> 
> HTH
> Emil
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>