[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