On Fri, Sep 28, 2012 at 08:14:29AM -0600, Brian Paul wrote:
> 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 <bri...@vmware.com>
> >>
> >> One question below...
> >>
> >> On 09/27/2012 02:56 AM, Oliver McFadden wrote:
> >>> Signed-off-by: Oliver McFadden<oliver.mcfad...@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).
This sounds like a good long-term solution. I'll investigate further on
Monday.
>
> -Brian
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
--
Oliver McFadden.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
