[Mesa-dev] [PATCH v4 1/1] intel: add support for ANGLE_texture_compression_dxt.

[Kenneth Graunke]

On 09/28/2012 02:20 AM, Oliver McFadden wrote:
> On Fri, Sep 28, 2012 at 01:12:55AM -0700, Kenneth Graunke wrote:
>> On 09/27/2012 01:56 AM, Oliver McFadden wrote:
>>> Signed-off-by: Oliver McFadden <oliver.mcfadden at linux.intel.com>
>>> ---
>>> v4: Off-by-one on a couple of 'if (ctx->Mesa_DXTn)' lines, which could cause a
>>> crash.
>>>
>>>  src/glx/glxextensions.h                       |    3 ++-
>>>  src/mapi/glapi/gen/es_EXT.xml                 |    6 ++++++
>>>  src/mesa/drivers/dri/intel/intel_extensions.c |    1 +
>>>  src/mesa/main/APIspec.xml                     |    3 +++
>>>  src/mesa/main/extensions.c                    |    3 +++
>>>  src/mesa/main/glformats.c                     |    6 ++++--
>>>  src/mesa/main/glheader.h                      |   11 +++++++++++
>>>  src/mesa/main/mtypes.h                        |    1 +
>>>  src/mesa/main/texformat.c                     |    9 ++++++---
>>>  src/mesa/main/teximage.c                      |   10 ++++++++++
>>>  10 files changed, 47 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/glx/glxextensions.h b/src/glx/glxextensions.h
>>> index 90c27a7..9e072e4 100644
>>> --- a/src/glx/glxextensions.h
>>> +++ b/src/glx/glxextensions.h
>>> @@ -67,7 +67,8 @@ enum
>>>  
>>>  enum
>>>  {
>>> -   GL_ARB_depth_texture_bit = 0,
>>> +   GL_ANGLE_texture_compression_dxt_bit = 0,
>>> +   GL_ARB_depth_texture_bit,
>>>     GL_ARB_draw_buffers_bit,
>>>     GL_ARB_fragment_program_bit,
>>>     GL_ARB_fragment_program_shadow_bit,
>>
>> You shouldn't need to change GLX code to add a purely client-side
>> extension.  I believe you can safely drop this hunk (and please do).
> 
> I need ANGLE_texture_compression_dxt somewhere in the extension table,
> see src/mesa/main/extensions.c.  Unfortunately it's not possible to
> unconditionally enable this extension.  Brian complained about the
> ordering of names (alphabetical order) which is why I've added ANGLE at
> the top of the enum...

Yeah, but you already defined that bit (correctly) in struct
gl_extensions in mtypes.h.  This one is just superfluous, as far as I
can tell.

[snip]
>>> diff --git a/src/mesa/main/glheader.h b/src/mesa/main/glheader.h
>>> index e93ca30..33cda02 100644
>>> --- a/src/mesa/main/glheader.h
>>> +++ b/src/mesa/main/glheader.h
>>> @@ -59,6 +59,17 @@ extern "C" {
>>>  #endif
>>>  
>>>  
>>> +/* GL_ANGLE_texture_compression_dxt3 */
>>> +#ifndef GL_ANGLE_texture_compression_dxt3 
>>> +#define GL_COMPRESSED_RGBA_S3TC_DXT3_ANGLE                      0x83F2
>>> +#endif
>>> +
>>> +/* GL_ANGLE_texture_compression_dxt5 */
>>> +#ifndef GL_ANGLE_texture_compression_dxt5 
>>> +#define GL_COMPRESSED_RGBA_S3TC_DXT5_ANGLE                      0x83F3
>>> +#endif
>>
>> You can actually drop this hunk if you change the teximage.c code to use
>> the _EXT extension instead of the _ANGLE extension.  The enums are
>> identical (just as the formats are identical).  I'd prefer that.
> 
> Hmm, okay. I'd like to leave a GL_COMPRESSED_RGBA_S3TC_DXT[35]_ANGLE
> comment there for grep, but it's not a show-stopper though...

I guess I don't feel too strongly about it one way or another.

>> Otherwise, this looks good!  With those changes,
>> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
> 
> I will also wait for Brian's comments on your suggestions and my replies
> before rolling another patch.  I'll be happy to see this one done and
> pushed.  :-)

Sounds good.