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?
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.
(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.
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).
(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.
(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.
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.
Mass version move, cvs -> git
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>
Closing as per Ian's comments in the previous post.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.