Skip to content

Conversation

@jansol
Copy link
Contributor

@jansol jansol commented Oct 15, 2019

Fixes #4142

@hugovk
Copy link
Member

hugovk commented Oct 16, 2019

Please can you add unit tests which fail before and pass after, to guard against future regressions?

@hugovk
Copy link
Member

hugovk commented Oct 16, 2019

And do you have a link to a spec or some docs that define the correct behaviour?

@jansol
Copy link
Contributor Author

jansol commented Oct 16, 2019

Spec is here: https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_texture_compression_s3tc.txt . As per section "Appendix in OpenGL 1.2.1" "Appendix C.2 in OpenGL ES 3.0.2":

COMPRESSED_RGBA_S3TC_DXT3_EXT: Each 4x4 block of texels consists of 64
bits of uncompressed alpha image data followed by 64 bits of RGB image
data.
Each RGB image data block is encoded according to the
COMPRESSED_RGB_S3TC_DXT1_EXT format, with the exception that the two code
bits always use the non-transparent encodings. In other words, they are
treated as though color0 > color1, regardless of the actual values of
color0 and color1.

COMPRESSED_RGBA_S3TC_DXT5_EXT: Each 4x4 block of texels consists of 64
bits of compressed alpha image data followed by 64 bits of RGB image data.
Each RGB image data block is encoded according to the
COMPRESSED_RGB_S3TC_DXT1_EXT format, with the exception that the two code
bits always use the non-transparent encodings. In other words, they are
treated as though color0 > color1, regardless of the actual values of
color0 and color1.

I suppose it'd make sense to mention this in a comment in the code as well. I'll look into tests.

@radarhere
Copy link
Member

The table at https://en.wikipedia.org/wiki/S3_Texture_Compression#S3TC_format_comparison makes me think that it's not quite this simple, since BC2 can be DXT2/3 and BC3 can be DXT4/5, and DXT2/4 and 3/5 treat alpha differently?

@jansol
Copy link
Contributor Author

jansol commented Dec 24, 2019

As per Issues/(1) in the abovementioned spec, no. OpenGL does not directly support the premultiplied alpha variants and they only really serve as a hint for blending mode parameters. There is also no in-band data to indicate which alpha mode you are dealing with. Similarly whether BC1 has alpha at all is out-of-band information. Anyway neither of these is directly related to this PR.

Oh and in case someone was looking at MSDN docs for BC2/3, those apparently forgot to mention the different alpha handling:

Issues
[...]
  (6) Is the encoding of the RGB components for DXT3 and DXT5 formats
        correct in this spec?  MSDN documentation suggests that the RGB blocks
        for DXT3 and DXT5 are decoded as described by the DXT1 format.

        RESOLVED:  Yes -- this appears to be a bug in the MSDN documentation.
        The specification for the DXT2-DXT5 formats require decoding using the
        opaque block encoding, regardless of the relative values of "color0"
        and "color1".

@hugovk
Copy link
Member

hugovk commented Mar 31, 2020

@radarhere What do you think: ready to merge, or still some questions? Thanks!

@radarhere radarhere merged commit 1e074f5 into python-pillow:master Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Erroneous DXT5/BC3 alpha decoding

3 participants