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

[Oliver McFadden]

On Fri, Sep 28, 2012 at 02:29:12AM -0700, Kenneth Graunke wrote:
> 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.

Oh, yes.  You're right; I haven't had my coffee this morning (nobody
check the actual time in EEST! ;-))

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

OK.

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

-- 
Oliver McFadden.