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

Fri Sep 28 07:14:29 PDT 2012

On 09/27/2012 05:41 PM, Ian Romanick wrote:
> On 09/27/2012 06:28 AM, Brian Paul wrote:
>> Looks good to me, Oliver.
>>
>> Reviewed-by: Brian Paul <brianp at vmware.com>
>>
>> One question below...
>>
>> On 09/27/2012 02: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,
>>> diff --git a/src/mapi/glapi/gen/es_EXT.xml
>>> b/src/mapi/glapi/gen/es_EXT.xml
>>> index fc2ec62..2698110 100644
>>> --- a/src/mapi/glapi/gen/es_EXT.xml
>>> +++ b/src/mapi/glapi/gen/es_EXT.xml
>>> @@ -731,4 +731,10 @@
>>> <enum name="RG8_EXT"
>>> value="0x822B"/>
>>> </category>
>>>
>>> +<!-- 111. GL_ANGLE_texture_compression_dxt -->
>>> +<category name="ANGLE_texture_compression_dxt" number="111">
>>> +<enum name="COMPRESSED_RGBA_S3TC_DXT3_ANGLE" value="0x83F2"/>
>>> +<enum name="COMPRESSED_RGBA_S3TC_DXT5_ANGLE" value="0x83F3"/>
>>> +</category>
>>> +
>>> </OpenGLAPI>
>>> diff --git a/src/mesa/drivers/dri/intel/intel_extensions.c
>>> b/src/mesa/drivers/dri/intel/intel_extensions.c
>>> index 89f6c1e..8a46488 100755
>>> --- a/src/mesa/drivers/dri/intel/intel_extensions.c
>>> +++ b/src/mesa/drivers/dri/intel/intel_extensions.c
>>> @@ -182,6 +182,7 @@ intelInitExtensions(struct gl_context *ctx)
>>> }
>>>
>>> if (intel->ctx.Mesa_DXTn) {
>>> + ctx->Extensions.ANGLE_texture_compression_dxt = true;
>>> ctx->Extensions.EXT_texture_compression_s3tc = true;
>>> ctx->Extensions.S3_s3tc = true;
>>> }
>>> diff --git a/src/mesa/main/APIspec.xml b/src/mesa/main/APIspec.xml
>>> index a65c5c5..c396952 100644
>>> --- a/src/mesa/main/APIspec.xml
>>> +++ b/src/mesa/main/APIspec.xml
>>> @@ -2172,6 +2172,9 @@
>>> <category name="NV_draw_buffers"/>
>>> <category name="NV_read_buffer"/>
>>>
>>> + <!-- GL_ANGLE_texture_compression_dxt -->
>>> + <category name="ANGLE_texture_compression_dxt"/>
>>> +
>>> <function name="DrawBuffersNV" template="DrawBuffers"/>
>>> <function name="ReadBufferNV" template="ReadBuffer"/>
>>>
>>> diff --git a/src/mesa/main/extensions.c b/src/mesa/main/extensions.c
>>> index bd7c7ba..4971ebc 100644
>>> --- a/src/mesa/main/extensions.c
>>> +++ b/src/mesa/main/extensions.c
>>> @@ -195,6 +195,8 @@ static const struct extension extension_table[]
>>> = {
>>> { "GL_EXT_texture3D",
>>> o(EXT_texture3D), GLL, 1996 },
>>> { "GL_EXT_texture_array",
>>> o(EXT_texture_array), GL, 2006 },
>>> { "GL_EXT_texture_compression_dxt1",
>>> o(EXT_texture_compression_s3tc), GL | ES1 | ES2, 2004 },
>>> + { "GL_ANGLE_texture_compression_dxt3",
>>> o(ANGLE_texture_compression_dxt), ES2, 2011 },
>>> + { "GL_ANGLE_texture_compression_dxt5",
>>> o(ANGLE_texture_compression_dxt), ES2, 2011 },
>>> { "GL_EXT_texture_compression_latc",
>>> o(EXT_texture_compression_latc), GL, 2006 },
>>> { "GL_EXT_texture_compression_rgtc",
>>> o(ARB_texture_compression_rgtc), GL, 2004 },
>>> { "GL_EXT_texture_compression_s3tc",
>>> o(EXT_texture_compression_s3tc), GL, 2000 },
>>> @@ -480,6 +482,7 @@ _mesa_enable_sw_extensions(struct gl_context *ctx)
>>> ctx->Extensions.EXT_gpu_program_parameters = GL_TRUE;
>>> _mesa_enable_extension(ctx, "GL_3DFX_texture_compression_FXT1");
>>> if (ctx->Mesa_DXTn) {
>>> + ctx->Extensions.ANGLE_texture_compression_dxt = GL_TRUE;
>>> _mesa_enable_extension(ctx, "GL_EXT_texture_compression_s3tc");
>>> _mesa_enable_extension(ctx, "GL_S3_s3tc");
>>> }
>>> diff --git a/src/mesa/main/glformats.c b/src/mesa/main/glformats.c
>>> index 04029c0..ccdf56b 100644
>>> --- a/src/mesa/main/glformats.c
>>> +++ b/src/mesa/main/glformats.c
>>> @@ -791,8 +791,10 @@ _mesa_is_compressed_format(struct gl_context
>>> *ctx, GLenum format)
>>> return ctx->Extensions.EXT_texture_compression_s3tc;
>>> case GL_COMPRESSED_RGBA_S3TC_DXT3_EXT:
>>> case GL_COMPRESSED_RGBA_S3TC_DXT5_EXT:
>>> - return _mesa_is_desktop_gl(ctx)
>>> -&& ctx->Extensions.EXT_texture_compression_s3tc;
>>> + return (_mesa_is_desktop_gl(ctx)&&
>>> + ctx->Extensions.EXT_texture_compression_s3tc) ||
>>> + (ctx->API == API_OPENGLES2&&
>>> + ctx->Extensions.ANGLE_texture_compression_dxt);
>>
>> If an extension like this is marked as being "ES2" in extensions.c why
>> do we need to check ctx->API==API_OPENGLES2? It seems to me that we
>> should only have to test ctx->Extensions.ANGLE_texture_compression_dxt
>> and not the API (as you did in the _mesa_choose_tex_format() change).
>
> The controls in extensions.c only select whether or not the string is
> advertised to the application. It doesn't control whether or not the
> driver sets the flag in gl_context::Extensions. I can imagine a driver
> always setting the ANGLE flags but not the EXT flags.

We could easily write a function that loops over the extension list 
and disabled those which aren't applicable to the context's API.  It 
would be called after the driver's done enabling its extensions.

This would have several benefits:

1. Simplified feature checks.  Instead of writing "if 
(ctx->Extensions.FOO_bar && API==something)" it would just be "if 
(ctx->Extensions.FOO_bar)".  Quicker to write, easier to read, less 
error-prone, etc.  There's lots of places where we're checking the 
Extensions flags, but not the API versions.  Adding API checks (for ES 
in particular) would be a lot of churn.  I know you've done some of 
that already.

2. If an existing extension is made available on a new API, we could 
make the change with a single line of code (change the API flags in 
the extension_table[] array).

-Brian