[Mesa-dev] [PATCH] mesa: expose EXT_texture_compression_s3tc on GLES

[Erik Faye-Lund]

On Tue, 2018-10-30 at 08:20 -0400, Ilia Mirkin wrote:
> On Tue, Oct 30, 2018 at 7:59 AM Erik Faye-Lund
> <erik.faye-lund at collabora.com> wrote:
> > 
> > On Tue, 2018-10-30 at 07:44 -0400, Ilia Mirkin wrote:
> > > On Thu, Oct 25, 2018 at 6:59 AM Erik Faye-Lund
> > > <erik.faye-lund at collabora.com> wrote:
> > > > 
> > > > From: Marek Olšák <marek.olsak at amd.com>
> > > > 
> > > > The spec was modified to support GLES.
> > > > 
> > > > Tested-by: Erik Faye-Lund <erik.faye-lund at collabora.com>
> > > > ---
> > > > This replaces this patch:
> > > > https://patchwork.freedesktop.org/patch/257423/
> > > > 
> > > >  docs/relnotes/18.3.0.html        |  1 +
> > > >  src/mesa/main/extensions_table.h |  2 +-
> > > >  src/mesa/main/glformats.c        | 11 +++++++++++
> > > >  3 files changed, 13 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/docs/relnotes/18.3.0.html
> > > > b/docs/relnotes/18.3.0.html
> > > > index 5874d3fa330..e0061872de4 100644
> > > > --- a/docs/relnotes/18.3.0.html
> > > > +++ b/docs/relnotes/18.3.0.html
> > > > @@ -57,6 +57,7 @@ Note: some of the new features are only
> > > > available
> > > > with certain drivers.
> > > >  <li>GL_AMD_multi_draw_indirect on all GL 4.x drivers.</li>
> > > >  <li>GL_AMD_query_buffer_object on i965, nvc0, r600,
> > > > radeonsi.</li>
> > > >  <li>GL_EXT_disjoint_timer_query on radeonsi and most other
> > > > Gallium
> > > > drivers (ES extension)</li>
> > > > +<li>GL_EXT_texture_compression_s3tc on all drivers (ES
> > > > extension)<li>
> > > >  <li>GL_EXT_vertex_attrib_64bit on i965, nvc0, radeonsi.</li>
> > > >  <li>GL_EXT_window_rectangles on radeonsi.</li>
> > > >  <li>GL_KHR_texture_compression_astc_sliced_3d on
> > > > radeonsi.</li>
> > > > diff --git a/src/mesa/main/extensions_table.h
> > > > b/src/mesa/main/extensions_table.h
> > > > index 09bf923bd0e..47db1583135 100644
> > > > --- a/src/mesa/main/extensions_table.h
> > > > +++ b/src/mesa/main/extensions_table.h
> > > > @@ -278,7 +278,7 @@
> > > > EXT(EXT_texture_buffer                      ,
> > > > OES_texture_buffer
> > > >  EXT(EXT_texture_compression_dxt1            ,
> > > > ANGLE_texture_compression_dxt          , GLL, GLC, ES1, ES2,
> > > > 2004)
> > > >  EXT(EXT_texture_compression_latc            ,
> > > > EXT_texture_compression_latc           , GLL,  x ,  x ,  x ,
> > > > 2006)
> > > >  EXT(EXT_texture_compression_rgtc            ,
> > > > ARB_texture_compression_rgtc           , GLL, GLC,  x ,  x ,
> > > > 2004)
> > > > -EXT(EXT_texture_compression_s3tc            ,
> > > > EXT_texture_compression_s3tc           , GLL, GLC,  x ,  x ,
> > > > 2000)
> > > > +EXT(EXT_texture_compression_s3tc            ,
> > > > EXT_texture_compression_s3tc           , GLL, GLC,  x , ES2,
> > > > 2000)
> > > >  EXT(EXT_texture_cube_map                    ,
> > > > ARB_texture_cube_map                   , GLL,  x ,  x ,  x ,
> > > > 2001)
> > > >  EXT(EXT_texture_cube_map_array              ,
> > > > OES_texture_cube_map_array             ,  x ,  x ,  x ,  31,
> > > > 2014)
> > > >  EXT(EXT_texture_edge_clamp                  ,
> > > > dummy_true                             , GLL,  x ,  x ,  x ,
> > > > 1997)
> > > > diff --git a/src/mesa/main/glformats.c
> > > > b/src/mesa/main/glformats.c
> > > > index 6cb3435dea2..f8fc36e9311 100644
> > > > --- a/src/mesa/main/glformats.c
> > > > +++ b/src/mesa/main/glformats.c
> > > > @@ -2803,6 +2803,17 @@
> > > > _mesa_es3_error_check_format_and_type(const
> > > > struct gl_context *ctx,
> > > >        internalFormat = effectiveInternalFormat;
> > > >     }
> > > > 
> > > > +   /* The GLES variant of EXT_texture_compression_s3tc is very
> > > > vague and
> > > > +    * doesn't list valid types. Just do exactly what the spec
> > > > says.
> > > > +    */
> > > > +   if (ctx->Extensions.EXT_texture_compression_s3tc &&
> > > > +       (internalFormat == GL_COMPRESSED_RGB_S3TC_DXT1_EXT ||
> > > > +        internalFormat == GL_COMPRESSED_RGBA_S3TC_DXT1_EXT ||
> > > > +        internalFormat == GL_COMPRESSED_RGBA_S3TC_DXT3_EXT ||
> > > > +        internalFormat == GL_COMPRESSED_RGBA_S3TC_DXT5_EXT))
> > > > +      return format == GL_RGB || format == GL_RGBA ?
> > > > GL_NO_ERROR :
> > > > +                                                     GL_INVALI
> > > > D_OP
> > > > ERATION;
> > > 
> > > Presumably GL_RGB only for GL_COMPRESSED_RGB_S3TC_DXT1_EXT and
> > > GL_RGBA
> > > for the others?
> > > 
> > 
> > Actually, this is AFAICT what the comment refers to. The extension
> > spec
> > says:
> > 
> > "
> >     Components are then selected from the resulting R, G, B, or A
> >     values to obtain a texture with the base internal format
> > specified
> >     by <internalformat>, which must match <format> except when
> > <target>
> >     is TEXTURE_2D and <internalformat> is one of the following
> >     compressed formats: COMPRESSED_RGB_S3TC_DXT1_EXT,
> >     COMPRESSED_RGBA_S3TC_DXT1_EXT, COMPRESSED_RGBA_S3TC_DXT3_EXT,
> > or
> >     COMPRESSED_RGBA_S3TC_DXT5_EXT. In this case, conversion from
> > only
> >     RGB and RGBA formats are supported during texture image
> > processing.
> >     <format> values other than RBA or RGBA will result in the
> >     INVALID_OPERATION error. In all other cases where
> > <internalformat>
> >     does not match <format>, the error INVALID_OPERATION is
> > generated.
> > "
> > 
> > This implies, as far as I can read, that RGB <-> RGBA is allowed.
> > It's
> > a bit strange, I must admit, but it seems to be what the spec says.
> 
> Hm, I think you're right. Or at least not obviously wrong. And that's
> good enough for me (high bar, I know). Thanks for bearing with me!
> 
> Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>

Thanks for all the input! Pushed :)