Re: [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
>  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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

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
 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.

> @@ -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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ian Romanick]

On 11/16/2017 11:57 AM, Ilia Mirkin wrote:
> On Thu, Nov 16, 2017 at 2:49 PM, Ian Romanick  wrote:
>> On 11/14/2017 02:54 PM, Scott D Phillips wrote:
>>> This extension provides a new texture and sampler parameter
>>> (TEXTURE_REDUCTION_MODE_ARB) which allows applications to produce
>>> a filtered texel value by computing a component-wise minimum (MIN)
>>> or maximum (MAX) of the texels that would normally be averaged.
>>> ---
>>> CTS tests KHR-GL45.texture_filter_minmax_tests.* need a little TLC to
>>> pass with this series. Details in VK-GL-CTS issue: 849
>>>
>>>  src/mesa/main/attrib.c   |  4 
>>>  src/mesa/main/extensions_table.h |  1 +
>>>  src/mesa/main/formatquery.c  | 10 ++
>>>  src/mesa/main/mtypes.h   |  2 ++
>>>  src/mesa/main/samplerobj.c   | 37 +
>>>  src/mesa/main/texobj.c   |  2 ++
>>>  src/mesa/main/texobj.h   |  2 +-
>>>  src/mesa/main/texparam.c | 33 +
>>>  8 files changed, 90 insertions(+), 1 deletion(-)
>>>
>>
>> [lots of stuff trimmed]
>>
>>> diff --git a/src/mesa/main/extensions_table.h 
>>> b/src/mesa/main/extensions_table.h
>>> index 5b66e7d30d..c51ad80742 100644
>>> --- a/src/mesa/main/extensions_table.h
>>> +++ b/src/mesa/main/extensions_table.h
>>> @@ -146,6 +146,7 @@ EXT(ARB_texture_env_combine , 
>>> ARB_texture_env_combine
>>>  EXT(ARB_texture_env_crossbar, ARB_texture_env_crossbar 
>>>   , GLL,  x ,  x ,  x , 2001)
>>>  EXT(ARB_texture_env_dot3, ARB_texture_env_dot3 
>>>   , GLL,  x ,  x ,  x , 2001)
>>>  EXT(ARB_texture_filter_anisotropic  , 
>>> ARB_texture_filter_anisotropic , GLL, GLC,  x ,  x , 2017)
>>> +EXT(ARB_texture_filter_minmax   , ARB_texture_filter_minmax
>>>   , GLL, GLC,  x ,  x , 2017)
>>
>> Is this right?  The extension says OpenGL 3.3 is required, and we don't
>> (until Marek is done) do OpenGL 3.3 compatibility profile.
> 
> FWIW I took the view that spec writers are lazy on
> EXT_polygon_offset_clamp, which had similar text. Is this a bad thing
> to do? Should we stick to the text of the specs to the letter?
> 
> There are various GL 4.2/4.3 exts which require the previous GL
> version as well but we expose them anyways.
> 
> IMHO it's reasonable to be relaxed about this, unless there are
> obvious interactions that need explicit consideration. [Perhaps that's
> the case here, although I can't think of anything.]

Yeah, usually.  This spec depends on glGetInternalformat* too... so I
guess that's either 3.3 or GL_ARB_internalformat_query?  I thought
GL_ARB_internalformat_query was always enabled, but it's not.  It does
seem unlikely that a driver would have GL_ARB_texture_filter_minmax and
not have GL_ARB_internalformat_query.

> Cheers,
> 
>   -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ilia Mirkin]

On Thu, Nov 16, 2017 at 2:57 PM, Ilia Mirkin  wrote:
> On Thu, Nov 16, 2017 at 2:49 PM, Ian Romanick  wrote:
>> On 11/14/2017 02:54 PM, Scott D Phillips wrote:
>>> This extension provides a new texture and sampler parameter
>>> (TEXTURE_REDUCTION_MODE_ARB) which allows applications to produce
>>> a filtered texel value by computing a component-wise minimum (MIN)
>>> or maximum (MAX) of the texels that would normally be averaged.
>>> ---
>>> CTS tests KHR-GL45.texture_filter_minmax_tests.* need a little TLC to
>>> pass with this series. Details in VK-GL-CTS issue: 849
>>>
>>>  src/mesa/main/attrib.c   |  4 
>>>  src/mesa/main/extensions_table.h |  1 +
>>>  src/mesa/main/formatquery.c  | 10 ++
>>>  src/mesa/main/mtypes.h   |  2 ++
>>>  src/mesa/main/samplerobj.c   | 37 +
>>>  src/mesa/main/texobj.c   |  2 ++
>>>  src/mesa/main/texobj.h   |  2 +-
>>>  src/mesa/main/texparam.c | 33 +
>>>  8 files changed, 90 insertions(+), 1 deletion(-)
>>>
>>
>> [lots of stuff trimmed]
>>
>>> diff --git a/src/mesa/main/extensions_table.h 
>>> b/src/mesa/main/extensions_table.h
>>> index 5b66e7d30d..c51ad80742 100644
>>> --- a/src/mesa/main/extensions_table.h
>>> +++ b/src/mesa/main/extensions_table.h
>>> @@ -146,6 +146,7 @@ EXT(ARB_texture_env_combine , 
>>> ARB_texture_env_combine
>>>  EXT(ARB_texture_env_crossbar, ARB_texture_env_crossbar 
>>>   , GLL,  x ,  x ,  x , 2001)
>>>  EXT(ARB_texture_env_dot3, ARB_texture_env_dot3 
>>>   , GLL,  x ,  x ,  x , 2001)
>>>  EXT(ARB_texture_filter_anisotropic  , 
>>> ARB_texture_filter_anisotropic , GLL, GLC,  x ,  x , 2017)
>>> +EXT(ARB_texture_filter_minmax   , ARB_texture_filter_minmax
>>>   , GLL, GLC,  x ,  x , 2017)
>>
>> Is this right?  The extension says OpenGL 3.3 is required, and we don't
>> (until Marek is done) do OpenGL 3.3 compatibility profile.
>
> FWIW I took the view that spec writers are lazy on
> EXT_polygon_offset_clamp, which had similar text. Is this a bad thing
> to do? Should we stick to the text of the specs to the letter?
>
> There are various GL 4.2/4.3 exts which require the previous GL
> version as well but we expose them anyways.
>
> IMHO it's reasonable to be relaxed about this, unless there are
> obvious interactions that need explicit consideration. [Perhaps that's
> the case here, although I can't think of anything.]

Oh, it depends on ARB_sampler_objects, and that functionality was core
in GL 3.3 - probably that's why the spec listed it that way.

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ilia Mirkin]

On Thu, Nov 16, 2017 at 2:49 PM, Ian Romanick  wrote:
> On 11/14/2017 02:54 PM, Scott D Phillips wrote:
>> This extension provides a new texture and sampler parameter
>> (TEXTURE_REDUCTION_MODE_ARB) which allows applications to produce
>> a filtered texel value by computing a component-wise minimum (MIN)
>> or maximum (MAX) of the texels that would normally be averaged.
>> ---
>> CTS tests KHR-GL45.texture_filter_minmax_tests.* need a little TLC to
>> pass with this series. Details in VK-GL-CTS issue: 849
>>
>>  src/mesa/main/attrib.c   |  4 
>>  src/mesa/main/extensions_table.h |  1 +
>>  src/mesa/main/formatquery.c  | 10 ++
>>  src/mesa/main/mtypes.h   |  2 ++
>>  src/mesa/main/samplerobj.c   | 37 +
>>  src/mesa/main/texobj.c   |  2 ++
>>  src/mesa/main/texobj.h   |  2 +-
>>  src/mesa/main/texparam.c | 33 +
>>  8 files changed, 90 insertions(+), 1 deletion(-)
>>
>
> [lots of stuff trimmed]
>
>> diff --git a/src/mesa/main/extensions_table.h 
>> b/src/mesa/main/extensions_table.h
>> index 5b66e7d30d..c51ad80742 100644
>> --- a/src/mesa/main/extensions_table.h
>> +++ b/src/mesa/main/extensions_table.h
>> @@ -146,6 +146,7 @@ EXT(ARB_texture_env_combine , 
>> ARB_texture_env_combine
>>  EXT(ARB_texture_env_crossbar, ARB_texture_env_crossbar  
>>  , GLL,  x ,  x ,  x , 2001)
>>  EXT(ARB_texture_env_dot3, ARB_texture_env_dot3  
>>  , GLL,  x ,  x ,  x , 2001)
>>  EXT(ARB_texture_filter_anisotropic  , 
>> ARB_texture_filter_anisotropic , GLL, GLC,  x ,  x , 2017)
>> +EXT(ARB_texture_filter_minmax   , ARB_texture_filter_minmax 
>>  , GLL, GLC,  x ,  x , 2017)
>
> Is this right?  The extension says OpenGL 3.3 is required, and we don't
> (until Marek is done) do OpenGL 3.3 compatibility profile.

FWIW I took the view that spec writers are lazy on
EXT_polygon_offset_clamp, which had similar text. Is this a bad thing
to do? Should we stick to the text of the specs to the letter?

There are various GL 4.2/4.3 exts which require the previous GL
version as well but we expose them anyways.

IMHO it's reasonable to be relaxed about this, unless there are
obvious interactions that need explicit consideration. [Perhaps that's
the case here, although I can't think of anything.]

Cheers,

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ian Romanick]

On 11/14/2017 02:54 PM, Scott D Phillips wrote:
> This extension provides a new texture and sampler parameter
> (TEXTURE_REDUCTION_MODE_ARB) which allows applications to produce
> a filtered texel value by computing a component-wise minimum (MIN)
> or maximum (MAX) of the texels that would normally be averaged.
> ---
> CTS tests KHR-GL45.texture_filter_minmax_tests.* need a little TLC to
> pass with this series. Details in VK-GL-CTS issue: 849
> 
>  src/mesa/main/attrib.c   |  4 
>  src/mesa/main/extensions_table.h |  1 +
>  src/mesa/main/formatquery.c  | 10 ++
>  src/mesa/main/mtypes.h   |  2 ++
>  src/mesa/main/samplerobj.c   | 37 +
>  src/mesa/main/texobj.c   |  2 ++
>  src/mesa/main/texobj.h   |  2 +-
>  src/mesa/main/texparam.c | 33 +
>  8 files changed, 90 insertions(+), 1 deletion(-)
> 

[lots of stuff trimmed]

> diff --git a/src/mesa/main/extensions_table.h 
> b/src/mesa/main/extensions_table.h
> index 5b66e7d30d..c51ad80742 100644
> --- a/src/mesa/main/extensions_table.h
> +++ b/src/mesa/main/extensions_table.h
> @@ -146,6 +146,7 @@ EXT(ARB_texture_env_combine , 
> ARB_texture_env_combine
>  EXT(ARB_texture_env_crossbar, ARB_texture_env_crossbar   
> , GLL,  x ,  x ,  x , 2001)
>  EXT(ARB_texture_env_dot3, ARB_texture_env_dot3   
> , GLL,  x ,  x ,  x , 2001)
>  EXT(ARB_texture_filter_anisotropic  , ARB_texture_filter_anisotropic 
> , GLL, GLC,  x ,  x , 2017)
> +EXT(ARB_texture_filter_minmax   , ARB_texture_filter_minmax  
> , GLL, GLC,  x ,  x , 2017)

Is this right?  The extension says OpenGL 3.3 is required, and we don't
(until Marek is done) do OpenGL 3.3 compatibility profile.

>  EXT(ARB_texture_float   , ARB_texture_float  
> , GLL, GLC,  x ,  x , 2004)
>  EXT(ARB_texture_gather  , ARB_texture_gather 
> , GLL, GLC,  x ,  x , 2009)
>  EXT(ARB_texture_mirror_clamp_to_edge, 
> ARB_texture_mirror_clamp_to_edge   , GLL, GLC,  x ,  x , 2013)
> diff --git a/src/mesa/main/texparam.c b/src/mesa/main/texparam.c
> index 4a0f61eda8..94883b2210 100644
> --- 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) {
> +

Extra blank line here should be deleted.

> + 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;
> +
> default:
>goto invalid_pname;
> }
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev