[Mesa-dev] [PATCH 2/2] st/mesa: don't choose DXT formats if we can't do DXT compression

Brian Paul brianp at vmware.com
Fri Feb 1 07:48:27 PST 2013


On 02/01/2013 08:13 AM, Jose Fonseca wrote:
>
>
> ----- Original Message -----
>> If we call glTexImage2D() with a generic compression format (e.g.
>> intFormat=GL_COMPRESSED_RGBA) we can't choose a DXT format if we
>> don't have the external DXT compression library.
>>
>> We weren't actually enforcing this before since the
>> pipe_screen::is_format_supported(DXT) query has no dependency on
>> the DXT compression library.
>>
>> Now if we're given a generic compressed format and we can't do DXT
>> compression we'll fall back to a non-compressed format.
>>
>> Note: This is a candidate for the stable branches.
>> ---
>>   src/mesa/state_tracker/st_cb_drawpixels.c |    6 ++-
>>   src/mesa/state_tracker/st_cb_texture.c    |    2 +-
>>   src/mesa/state_tracker/st_format.c        |   47
>>   ++++++++++++++++++++++++-----
>>   src/mesa/state_tracker/st_format.h        |    2 +-
>>   src/mesa/state_tracker/st_texture.c       |    3 +-
>>   5 files changed, 47 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/mesa/state_tracker/st_cb_drawpixels.c
>> b/src/mesa/state_tracker/st_cb_drawpixels.c
>> index c944b81..c3c5326 100644
>> --- a/src/mesa/state_tracker/st_cb_drawpixels.c
>> +++ b/src/mesa/state_tracker/st_cb_drawpixels.c
>> @@ -1499,14 +1499,16 @@ st_CopyPixels(struct gl_context *ctx, GLint
>> srcx, GLint srcy,
>>         if (type == GL_DEPTH) {
>>            texFormat = st_choose_format(screen, GL_DEPTH_COMPONENT,
>>                                         GL_NONE, GL_NONE,
>>                                         st->internal_target,
>> -				      sample_count, PIPE_BIND_DEPTH_STENCIL);
>> +                                      sample_count,
>> PIPE_BIND_DEPTH_STENCIL,
>> +                                      FALSE);
>>            assert(texFormat != PIPE_FORMAT_NONE);
>>         }
>>         else {
>>            /* default color format */
>>            texFormat = st_choose_format(screen, GL_RGBA,
>>                                         GL_NONE, GL_NONE,
>>                                         st->internal_target,
>> -                                      sample_count,
>> PIPE_BIND_SAMPLER_VIEW);
>> +                                      sample_count,
>> PIPE_BIND_SAMPLER_VIEW,
>> +                                      FALSE);
>>            assert(texFormat != PIPE_FORMAT_NONE);
>>         }
>>      }
>> diff --git a/src/mesa/state_tracker/st_cb_texture.c
>> b/src/mesa/state_tracker/st_cb_texture.c
>> index 3cea2df..80a440d 100644
>> --- a/src/mesa/state_tracker/st_cb_texture.c
>> +++ b/src/mesa/state_tracker/st_cb_texture.c
>> @@ -597,7 +597,7 @@ decompress_with_blit(struct gl_context * ctx,
>>
>>      /* Find the best match for the format+type combo. */
>>      pipe_format = st_choose_format(pipe->screen, GL_RGBA8, format,
>>      type,
>> -                                  pipe_target, 0, bind);
>> +                                  pipe_target, 0, bind, FALSE);
>>      if (pipe_format == PIPE_FORMAT_NONE) {
>>         /* unable to get an rgba format!?! */
>>         _mesa_problem(ctx, "%s: cannot find a supported format",
>>         __func__);
>> diff --git a/src/mesa/state_tracker/st_format.c
>> b/src/mesa/state_tracker/st_format.c
>> index 7ef0639..c3b286a 100644
>> --- a/src/mesa/state_tracker/st_format.c
>> +++ b/src/mesa/state_tracker/st_format.c
>> @@ -1397,19 +1397,48 @@ static const struct format_mapping
>> format_map[] = {
>>
>>
>>   /**
>> + * Is the given pipe format a DXT format?
>> + */
>> +static boolean
>> +is_dxt_format(enum pipe_format format)
>> +{
>> +   switch (format) {
>> +   case PIPE_FORMAT_DXT1_RGB:
>> +   case PIPE_FORMAT_DXT1_RGBA:
>> +   case PIPE_FORMAT_DXT3_RGBA:
>> +   case PIPE_FORMAT_DXT5_RGBA:
>> +   case PIPE_FORMAT_DXT1_SRGB:
>> +   case PIPE_FORMAT_DXT1_SRGBA:
>> +   case PIPE_FORMAT_DXT3_SRGBA:
>> +   case PIPE_FORMAT_DXT5_SRGBA:
>> +      return TRUE;
>> +   default:
>> +      return FALSE;
>> +   }
>> +}
>
> I think you could use u_format.h's util_format_is_s3tc here.

Yup, I had grepped for "dxt" instead of "s3tc".


>
>> +
>> +/**
>>    * Return first supported format from the given list.
>> + * \param allow_dxt  indicates whether it's OK to return a DXT
>> format.
>>    */
>>   static enum pipe_format
>>   find_supported_format(struct pipe_screen *screen,
>>                         const enum pipe_format formats[],
>>                         enum pipe_texture_target target,
>>                         unsigned sample_count,
>> -                      unsigned tex_usage)
>> +                      unsigned tex_usage,
>> +                      boolean allow_dxt)
>>   {
>>      uint i;
>>      for (i = 0; formats[i]; i++) {
>>         if (screen->is_format_supported(screen, formats[i], target,
>>                                         sample_count, tex_usage)) {
>> +         if (!allow_dxt&&  is_dxt_format(formats[i])) {
>> +            /* we can't return a dxt format, continue searching */
>> +            continue;
>> +         }
>> +
>>            return formats[i];
>>         }
>>      }
>> @@ -1519,7 +1548,7 @@ enum pipe_format
>>   st_choose_format(struct pipe_screen *screen, GLenum internalFormat,
>>                    GLenum format, GLenum type,
>>                    enum pipe_texture_target target, unsigned
>>                    sample_count,
>> -                 unsigned bindings)
>> +                 unsigned bindings, boolean allow_dxt)
>
> It looks like parameter is serving two duties:
>
> - some cases it is set to FALSE because we don't have the DXTn
> compression library available
>
> - some cases it is set to FALSE because we don't want compressed
> textures, but truth is that setting FALSE will only black lists DXT
> and happily return non-DXT compressed libraries (KTX, etc).

The cases which I think you're looking at (st_cb_drawpixels.c, 
decompress_with_blit(), st_choose_renderbuffer_format(), etc) will 
never choose a compressed format so the allow_dxt argument's value 
doesn't matter.  I didn't feel like making allow_dxt a tri-valued 
(true/false/dont-care) parameter.

Remember, the only time we choose a compressed format is when the 
"internalFormat" parameter is a generic or specific compressed format. 
  That can only happen when we're called via glTexImage().

I can improve the comments to explain this.


> If so, then I'd suggest rename allow_dxt to allow_compressed, and
> check ctx->Mesa_DXTn when is_dxt_format() is true.
>
> Otherwise the series looks good.

Thanks for reviewing!

-Brian



More information about the mesa-dev mailing list