Bug 3165 - texImage.IsCompressed and texImage.CompressedSize issues
texImage.IsCompressed and texImage.CompressedSize issues
Status: NEW
Product: Mesa
Classification: Unclassified
Component: Mesa core
git
x86 (IA32) Linux (All)
: high normal
Assigned To: mesa-dev
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-04-30 05:39 UTC by Felix Kühling
Modified: 2011-08-01 09:18 UTC (History)
1 user (show)

See Also:


Attachments
First attempte at making texture formats self contained (79.01 KB, patch)
2005-05-20 17:57 UTC, Felix Kühling
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Felix Kühling 2005-04-30 05:39:46 UTC
texImage.IsCompressed and texImage.CompressedSize are set in
_mesa_init_teximage_fields based on the texture's internalFormat specified by
the application. However, the driver may want to choose an uncompressed format
instead of a compressed one or it may want to compress textures with e.g.
internalFormat=GL_COMPRESSED_RGBA, which is not recognized as compressed right
now. In such cases IsCompressed ends up being clearly wrong and leads to weird
crashes later on. I suspect that wrong texture image sizes and/or row strides in
_mesa_store_teximage_... are responsible.

I believe texImage.IsCompressed and texImage.CompressedSize should be
initialized after the driver has chosen a hardware texture format and based on
that hardware format instead of the internalFormat specified by the application.
Does that make sense or am I trying to do something stupid?
Comment 1 Brian Paul 2005-04-30 12:46:30 UTC
I guess I was expecting that the driver itself would plug in the correct
InternalFormat and IsCompressed values when needed.  In practice, I believe this
would only be needed when the user's internalFormat was one of the six generic
compressed formats.  In that case, the driver chooses the best specific
compressed format, or falls back to an uncompressed format.  The spec says that
the GL then replaces internalFormat's value with the actual chosen format.

If you think calling _mesa_init_teximage_fields() later in the glTexImage
process would be better, that's OK with me.  Maybe it should go in the
_mesa_texstore() routines after the actual texture format has been chosen.  And
maybe the internalFormat parameter to ctx->Driver.ChooseTextureFormat() should
be passed via a pointer so the routine can change it if needed.

Maybe you should go ahead and try that out and see how it works.
Comment 2 Felix Kühling 2005-04-30 15:35:48 UTC
(In reply to comment #1)
> I guess I was expecting that the driver itself would plug in the correct
> InternalFormat and IsCompressed values when needed.  In practice, I believe this
> would only be needed when the user's internalFormat was one of the six generic
> compressed formats.  In that case, the driver chooses the best specific
> compressed format, or falls back to an uncompressed format.  The spec says that
> the GL then replaces internalFormat's value with the actual chosen format.

Is it actually necessary to change the internalFormat? I thought it was merely a
hint to the OpenGL implementation with respect to the internal representation of
the image. For example a driver that can't do RGB can fall back to an RGBA
format without changing the internalFormat. I thought the same could work for
compressed vs. uncompressed formats.

> 
> If you think calling _mesa_init_teximage_fields() later in the glTexImage
> process would be better, that's OK with me.

I'm not sure if the fields set in _mesa_init_teximage_fields aren't needed
earlier, so I'd try to avoid moving _mesa_init_teximage_fields. I was rather
thinking to initialize IsCompressed and CompressedSize outside that function.
And is_compressed_format should decide by the chosen gl_texture_format instead
of the internalFormat.

>  Maybe it should go in the
> _mesa_texstore() routines after the actual texture format has been chosen.

Yes.

>  And
> maybe the internalFormat parameter to ctx->Driver.ChooseTextureFormat() should
> be passed via a pointer so the routine can change it if needed.

I don't think this is needed. See above.

> 
> Maybe you should go ahead and try that out and see how it works.
> 

I think I'd like to take this a bit further. Can we add an IsCompressed field to
the gl_texture_format and move function CompressedTextureSize from ctx->Driver
to gl_texture_format? I'd have to check if other drivers make their own
gl_texture_formats and adjust them accordingly along with the texture formats in
core Mesa. This should make it easy to add more (driver-specific) compressed
formats in the future without any further changes in core Mesa.
Comment 3 Roland Scheidegger 2005-04-30 16:29:05 UTC
This is exactly the problem I had when I tried to implement an option to always
enable texture compression even on textures which are not compressed (not
exactly legal, but for performance reasons and commercial drivers may do it too,
results were mixed though since the cpu overhead for on-the-fly generated
textures was way too high). I considered the workarounds for it hackish though,
and dropped it in later versions of the patches. IIRC I simply added code to
reinitialize the IsCompressed field (the crashes were due to incorrect size
calculations).
Comment 4 Brian Paul 2005-04-30 16:44:13 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > I guess I was expecting that the driver itself would plug in the correct
> > InternalFormat and IsCompressed values when needed.  In practice, I believe this
> > would only be needed when the user's internalFormat was one of the six generic
> > compressed formats.  In that case, the driver chooses the best specific
> > compressed format, or falls back to an uncompressed format.  The spec says that
> > the GL then replaces internalFormat's value with the actual chosen format.
> 
> Is it actually necessary to change the internalFormat? I thought it was merely a
> hint to the OpenGL implementation with respect to the internal representation of
> the image.

Normally, the user's internalFormat is just a hint and the value is kept as-is
in the texture image's state record.  But in the case of texture compression,
when the user's internalFormat is a generic format, we want to store the actual
internal format chosen by the driver so that the user can ask OpenGL what format
it really settled on.  That's my reading of the spec.


> For example a driver that can't do RGB can fall back to an RGBA
> format without changing the internalFormat.

Yes, that's true.


> I thought the same could work for compressed vs. uncompressed formats.

I think the generic compression formats are an exception.

 
> > If you think calling _mesa_init_teximage_fields() later in the glTexImage
> > process would be better, that's OK with me.
> 
> I'm not sure if the fields set in _mesa_init_teximage_fields aren't needed
> earlier, so I'd try to avoid moving _mesa_init_teximage_fields. I was rather
> thinking to initialize IsCompressed and CompressedSize outside that function.
> And is_compressed_format should decide by the chosen gl_texture_format instead
> of the internalFormat.

I think you're right.

 
> >  Maybe it should go in the
> > _mesa_texstore() routines after the actual texture format has been chosen.
> 
> Yes.
> 
> >  And
> > maybe the internalFormat parameter to ctx->Driver.ChooseTextureFormat() should
> > be passed via a pointer so the routine can change it if needed.
> 
> I don't think this is needed. See above.
> 
> > 
> > Maybe you should go ahead and try that out and see how it works.
> > 
> 
> I think I'd like to take this a bit further. Can we add an IsCompressed field to
> the gl_texture_format and move function CompressedTextureSize from ctx->Driver
> to gl_texture_format?

That sounds good, actually.  But I'd rename CompressedTextureSize() to just
TextureSize() since the former wouldn't apply to uncompressed formats but the
later would apply to all kinds of formats.


> I'd have to check if other drivers make their own
> gl_texture_formats and adjust them accordingly along with the texture formats in
> core Mesa. This should make it easy to add more (driver-specific) compressed
> formats in the future without any further changes in core Mesa.

Right.  I don't have time to work on this but I encourage you to look into it. 
If you can provide a patch, I will review it and test it out with various tests
before it's committed.  Please take a look at the texture compression docs to
see if you agree about replacing the internalFormat value for generic formats.

Sounds like these changes would help with Roland's problem too.
Comment 5 Felix Kühling 2005-05-01 09:58:33 UTC
(In reply to comment #4)
> > Is it actually necessary to change the internalFormat? I thought it was merely a
> > hint to the OpenGL implementation with respect to the internal representation of
> > the image.
> 
> Normally, the user's internalFormat is just a hint and the value is kept as-is
> in the texture image's state record.  But in the case of texture compression,
> when the user's internalFormat is a generic format, we want to store the actual
> internal format chosen by the driver so that the user can ask OpenGL what format
> it really settled on.  That's my reading of the spec.

You're right. Reading the spec made a few things clearer.

> > I think I'd like to take this a bit further. Can we add an IsCompressed field to
> > the gl_texture_format and move function CompressedTextureSize from ctx->Driver
> > to gl_texture_format?
> 
> That sounds good, actually.  But I'd rename CompressedTextureSize() to just
> TextureSize() since the former wouldn't apply to uncompressed formats but the
> later would apply to all kinds of formats.

Ok.
 
> > I'd have to check if other drivers make their own
> > gl_texture_formats and adjust them accordingly along with the texture formats in
> > core Mesa. This should make it easy to add more (driver-specific) compressed
> > formats in the future without any further changes in core Mesa.
> 
> Right.  I don't have time to work on this but I encourage you to look into it. 
> If you can provide a patch, I will review it and test it out with various tests
> before it's committed. 

Ok, I'll be working on this.

> Please take a look at the texture compression docs to
> see if you agree about replacing the internalFormat value for generic formats.

I have read the spec now.

> 
> Sounds like these changes would help with Roland's problem too.
> 

Yep.
Comment 6 Felix Kühling 2005-05-20 17:57:13 UTC
Created attachment 2728 [details] [review]
First attempte at making texture formats self contained

This patch attempts to make texture formats self contained such that future
(compressed) texture formats can be added without changing core mesa code. Most
of the time compressed and uncompressed textures are not distinguished by the
common code now. It does not go all the way yet, though. There are still some
assumptions in core mesa (in the compressed subimage functions) about tiling of
compressed textures which happen to be true for fxt1 and s3tc. I may not have
time to work on this for a couple weeks so I put the current state here for
comments.

The patch also makes the internalFormat parameter of drivers'
chooseTextureFormat function a pointer. Now drivers can safely choose
hardware-compressed formats when the application specified a generic one. The
patch includes the necessary driver changes.

via and tdfx needed more extensive changes because they override texImage and
texSubImage functions. The via code was a bit confusing for me, so I might have
got it wrong. I can't test this myself.
Comment 7 ajax at nwnk dot net 2009-08-24 12:23:14 UTC
Mass version move, cvs -> git
Comment 8 Ian Romanick 2011-08-01 09:18:59 UTC
If I'm not mistaken, most of the issues from this bug have been fixed for some time.  The one remaining issue was returning the wrong format for GL_TEXTURE_INTERNAL_FORMAT queries.  That should be fixed by the following series on Mesa master.  If that's the case, I'll pick those patches to the 7.11 and 7.10 branches and close this old, old bug.

commit b189d1635d89cd7d900e8f9a5eed88d7dc0b46cb
Author: Ian Romanick <ian.d.romanick@intel.com>
Date:   Fri Jul 22 16:45:50 2011 -0700

    mesa: Make _mesa_get_compressed_formats match the texture compression specs
    
    The implementation deviated slightly from the GL_EXT_texture_sRGB spec
    and from other implementations.  A giant comment block was added to
    justify the somewhat odd behavior of this function.
    
    In addition, the interface had unnecessary cruft.  The 'all' parameter
    was false at all callers, so it has been removed.
    
    Reviewed-by: Brian Paul <brianp@vmware.com>

commit 143b65f7612c255f29d08392192098b1c2bf4b62
Author: Ian Romanick <ian.d.romanick@intel.com>
Date:   Fri Jul 22 15:26:24 2011 -0700

    mesa: Return the correct internal fmt when a generic compressed fmt was used
    
    If an application requests a generic compressed format for a texture
    and the driver does not pick a specific compressed format, return the
    generic base format (e.g., GL_RGBA) for the GL_TEXTURE_INTERNAL_FORMAT
    query.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=3165
    Reviewed-by: Brian Paul <brianp@vmware.com>

commit 09916e877fc14723d7950f892e181df9f7d7f36f
Author: Ian Romanick <ian.d.romanick@intel.com>
Date:   Fri Jul 22 15:25:55 2011 -0700

    mesa: Add utility function to get base format from a GL compressed format
    
    Reviewed-by: Brian Paul <brianp@vmware.com>