iOrange / bcdec

Small header-only C library to decompress any BC compressed image
Other
143 stars 12 forks source link

BC1 colors are not rounded correctly #15

Closed RunDevelopment closed 3 months ago

RunDevelopment commented 3 months ago

Hi! While analyzing the Rust source port (bcdec_rs) of this library, I discovered that BC1 and BC4 are not rounded correctly compared to doing all calculations in float32 and converting the image to 8 bit ints at the very end. I then confirmed that the Rust source port simply faithfully recreated the issues from the original bcdec C library. While there's already an issue for BC4 (#12), there wasn't one for BC1 yet. (Here's the issue I made for the Rust source port for reference: https://github.com/ScanMountGoat/image_dds/issues/17)

What's the issue?

Colors are not rounded correctly. I define "correct" here are doing all calculations in float32 and then converting the decoded float32 image to uint8 at the very end. I'm using this definition, because that's how DirectXTex is implemented, and it's how Paint.net and GIMP decode DDS images. So this definition of correct is widely-used in modern software.

How is it incorrect: the R, G, and B color channels can be off by to +-1. The error is only a single bit. While small, it's still noticeable. The problem is that the error is not the same between channels. So e.g. the R and B channels can have an error of -1 while the G channel can have an error of +1, which will cause a noticeable green tint.

Here's an example image showing this exact problem. All pixels in this image are supposed to have the color RGB=47 47 47, as is the case when opened in Paint.net and Gimp, but bcdec will decode all pixels as RGB=46 48 46. BC1_UNORM_SRGB-47.zip

These rounding errors are also not rare. Here's a test image and all error magnified so we can see them: image image

The cause of the issue

The cause is that the issue is that bcdec uses the already-rounded 8-bit values for color0 and color1 to calculate the values for color2 and color3.

E.g. if the 5-bit value of r is 29, then the 8-bit value is 238.55 which is rounded to 239. That's not a problem for color0 and color1, because that's just want correct rounding does, but it is a problem for color2 and color3. This rounding for color0/1 created an error of 0.45 which is then multiplied by 2 when calculating color2/3 giving us an error of 0.9. This error is capable of changing the rounding of color2 and color3.

The fix

The fix is to use the original 5/6-bit values for interpolation instead of the rounded 8-bit values. E.g. if r0_5 and r1_5 are the 5-bit values of color0 and color1 respectively, then the RGB values of color_2 = 2/3*color_0 + 1/3*color_1 can be calculated as

r = ((2 * r0_5 + r1_5) * 351 + 61) >> 7;
g = ((2 * g0_6 + g1_6) * 2763 + 1039) >> 11;
b = ((2 * b0_5 + b1_5) * 351 + 61) >> 7;

(Note that g must be calculated with 32-bit integers, while 16-bit integers are enough for r and b).

I got these constants using a script I made that uses a brute-forces search. This script also found the same constants bcdec uses for the 5/6 bit to 8 bit conversion used for color0 and color1.

Note that BC1A mode requires different constants. Here color_2 = 1/2*color_0 + 1/2*color_1 can be calculated as

r = ((r0_5 + r1_5) * 1053 + 125) >> 8
g = ((g0_6 + g1_6) * 4145 + 1019) >> 11;;
b = ((b0_5 + b1_5) * 1053 + 125) >> 8;

(Just as before, 32 bit for g and 16 bit for r and b.)

iOrange commented 3 months ago

Hi!

Thanks for submitting the issue. Yeah, I agree that interpolating the already rounded values will add more error to the output. I'll try to experiment with the values you've provided.

iOrange commented 3 months ago

@RunDevelopment so I tested your "magic" values and they match the floating-point interpolation perfectly! Also because we now avoid integer division by 3 - your version is even slightly faster. I'm going to adopt your changes, just wanted to ask you - would you mind if I add you to the CREDITS: section of the header next to Aras Pranckevicius ? Also are these proper credentials to put there ? Michael Schmidt (@RunDevelopment)

RunDevelopment commented 3 months ago

Thank you! Yes, those credentials are correct. Just put me wherever you see fit :)

Also because we now avoid integer division by 3 - your version is even slightly faster.

Nice 🎉

iOrange commented 3 months ago

3b71535 is in, thanks for your help!