[Mesa-dev] [PATCH 1/5] mesa: Add infrastructure for GL_ARB_texture_mirror_clamp_to_edge.

[Rico Schüller]

On 18.10.2013 17:31, Ian Romanick wrote:
> With the one comment below fixed (and Marek's comment on patch 2 fixed),
> the series is:
> 
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
> 
> You are, however, missing the most important patch!  You need the patch
> that updates docs/GL3.txt and docs/relnotes/10.0.html. :)
I wasn't sure about the wording in GL3.txt. I think there needs to be a
nice description, what means what... as there are contrary information
in that file:
- GL_ARB_explicit_attrib_location DONE (i915, i965, r300, r600, swrast)
- GL_ARB_texture_storage DONE (i965, r300, r600, swrast, gallium)
- Red and red/green texture formats DONE (i965, swrast, gallium)

Some information are redundant, e.g. if r600 supports it, shouldn't then
also gallium support it? Or does it mean, if gallium supports it, then
all gallium drivers have support for it? Maybe someone could clarify,
what comment has what meaning... then of course I'll add the
information. :-)

> 
> On 10/18/2013 02:20 AM, Rico Schüller wrote:
>> ---
>>  src/mesa/main/extensions.c |  1 +
>>  src/mesa/main/mtypes.h     |  1 +
>>  src/mesa/main/samplerobj.c |  2 +-
>>  src/mesa/main/texparam.c   | 12 +++++++++---
>>  4 Dateien geändert, 12 Zeilen hinzugefügt(+), 4 Zeilen entfernt(-)
>>
>> diff --git a/src/mesa/main/extensions.c b/src/mesa/main/extensions.c
>> index 2507fdf..bf8d7a3 100644
>> --- a/src/mesa/main/extensions.c
>> +++ b/src/mesa/main/extensions.c
>> @@ -144,6 +144,7 @@ static const struct extension extension_table[] = {
>>     { "GL_ARB_texture_float",                       o(ARB_texture_float),                       GL,             2004 },
>>     { "GL_ARB_texture_gather",                      o(ARB_texture_gather),                      GL,             2009 },
>>     { "GL_ARB_texture_mirrored_repeat",             o(dummy_true),                              GLL,            2001 },
>> +   { "GL_ARB_texture_mirror_clamp_to_edge",        o(ARB_texture_mirror_clamp_to_edge),        GL,             2013 },
>>     { "GL_ARB_texture_multisample",                 o(ARB_texture_multisample),                 GL,             2009 },
>>     { "GL_ARB_texture_non_power_of_two",            o(ARB_texture_non_power_of_two),            GL,             2003 },
>>     { "GL_ARB_texture_query_levels",                o(ARB_texture_query_levels),                GL,             2012 },
>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> index 15893ec..6374e8c 100644
>> --- a/src/mesa/main/mtypes.h
>> +++ b/src/mesa/main/mtypes.h
>> @@ -3218,6 +3218,7 @@ struct gl_extensions
>>     GLboolean ARB_texture_env_dot3;
>>     GLboolean ARB_texture_float;
>>     GLboolean ARB_texture_gather;
>> +   GLboolean ARB_texture_mirror_clamp_to_edge;
>>     GLboolean ARB_texture_multisample;
>>     GLboolean ARB_texture_non_power_of_two;
>>     GLboolean ARB_texture_query_levels;
>> diff --git a/src/mesa/main/samplerobj.c b/src/mesa/main/samplerobj.c
>> index c3b612c..7285ef5 100644
>> --- a/src/mesa/main/samplerobj.c
>> +++ b/src/mesa/main/samplerobj.c
>> @@ -305,7 +305,7 @@ validate_texture_wrap_mode(struct gl_context *ctx, GLenum wrap)
>>     case GL_MIRROR_CLAMP_EXT:
>>        return e->ATI_texture_mirror_once || e->EXT_texture_mirror_clamp;
>>     case GL_MIRROR_CLAMP_TO_EDGE_EXT:
>> -      return e->ATI_texture_mirror_once || e->EXT_texture_mirror_clamp;
>> +      return e->ATI_texture_mirror_once || e->EXT_texture_mirror_clamp || e->ARB_texture_mirror_clamp_to_edge;
>>     case GL_MIRROR_CLAMP_TO_BORDER_EXT:
>>        return e->EXT_texture_mirror_clamp;
>>     default:
>> diff --git a/src/mesa/main/texparam.c b/src/mesa/main/texparam.c
>> index 31723c3..50bc930 100644
>> --- a/src/mesa/main/texparam.c
>> +++ b/src/mesa/main/texparam.c
>> @@ -84,16 +84,22 @@ validate_texture_wrap_mode(struct gl_context * ctx, GLenum target, GLenum wrap)
>>        break;
>>  
>>     case GL_MIRROR_CLAMP_EXT:
>> -   case GL_MIRROR_CLAMP_TO_EDGE_EXT:
>>        supported = is_desktop_gl 
>>           && (e->ATI_texture_mirror_once || e->EXT_texture_mirror_clamp)
>> -	 && (target != GL_TEXTURE_RECTANGLE_NV)
>> +         && (target != GL_TEXTURE_RECTANGLE_NV)
>> +         && (target != GL_TEXTURE_EXTERNAL_OES);
>> +      break;
>> +
>> +   case GL_MIRROR_CLAMP_TO_EDGE_EXT:
>> +      supported = is_desktop_gl 
>> +         && (e->ATI_texture_mirror_once || e->EXT_texture_mirror_clamp || e->ARB_texture_mirror_clamp_to_edge)
>> +         && (target != GL_TEXTURE_RECTANGLE_NV)
>>           && (target != GL_TEXTURE_EXTERNAL_OES);
>>        break;
>>  
>>     case GL_MIRROR_CLAMP_TO_BORDER_EXT:
>>        supported = is_desktop_gl && e->EXT_texture_mirror_clamp
>> -	 && (target != GL_TEXTURE_RECTANGLE_NV)
>> +         && (target != GL_TEXTURE_RECTANGLE_NV)
>>           && (target != GL_TEXTURE_EXTERNAL_OES);
> 
> This change shouldn't be here.  It doesn't have anything to do with
> ARB_texture_mirror_clamp_to_edge, and I don't /think/ it's necessary
> anyway.  is_desktop_gl is already part of the condition, and
> GL_TEXTURE_EXTERNAL_OES is only available in ES.

I don't think there is something wrong here. I somehow don't get the
point. This (the last -/+) change is not really required, it changes
only the indentation. I think it is more consistent to use always spaces
instead of some lines tabs and other lines spaces and while I am at the
code, I thought I could fix the spacing. It does nothing else.

I only copied the "case" to allow supporting GL_MIRROR_CLAMP_TO_EDGE_EXT
and not GL_MIRROR_CLAMP_EXT for ARB_texture_mirror_clamp_to_edge. So
whatever is broken was broken before and may be fixed in another patch.
Maybe patch 842efb9447bd4c8c29599f1baeee8a380d5276c2 did introduce
something wrong? Could you please have a look and describe the problem
you may see a bit better?