-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BC1 and BC4 rounding behavior doesn't match DirectXTex #17
Comments
RenderDoc on my PC is showing the colors for that DDS as 46, 47, 46 in RGB. The preview app on my MacBook Air shows 47, 47, 47. I don't believe the older compression modes require bit accurate implementations based on the discussion in iOrange/bcdec#12. Your solution looks reasonable, but I'm not sure if it's worth breaking the test suite for a small difference in pixel values. |
I would get in contact with the image-rs maintainers if you haven't already to see if they have any opinions on how they want to approach DDS. |
Thanks for the quick response! As for correctness and bit accuracy, I outlined my reasoning for what I see as correct in the BC1 issue I opened on the bcdec repo: iOrange/bcdec#15. To quote it here:
I hope this makes it clear why I see the current behavior of
I already made a PR draft: image-rs/image#2258. |
There are multiple implementations of the older compression formats. The specifications for GPU implementations I can find only require perfect bit accuracy for bc6h and bc7. A difference of 1/255 is allowed for bc1.
I would recommend just using bcdec_rs in its entirety in image-rs. It doesn't seem worth it to maintain a completely separate decoder implementation for legacy formats. I would like bcdec_rs to match the C implementation due to how the test suite is implemented. If your change doesn't compromise the benchmarking performance, I may consider patching the C code in the sys crate. This would also require additional tests to make sure nothing else has regressed in terms of the output. |
I think this is a weird thing to say. I understand that the spec may allow for some error to allow optimizations in use cases where perfect color accuracy isn't necessary, but isn't bcdec_rs is a general-purpose DDS decoder? As I see it, a general-purpose decoder can't just say "a bit of error is fine" and then leave the users to deal with it. This just means that bcdec_rs can't be used when color accuracy is required. This is also why I cannot use bcdec_rs in its current form for BC1-5 in my PR for the
Aside from color accuracy, there's also the problem that bcdec_rs doesn't support signed BC4 and BC5. If you want to, we can upstream my code and integrate it into bcdec_rs. Would that be something you'd be interested in?
That would be great. As for performance: there should be any perf difference for BC4 since we just changed a constant. BC1 should be a tiny bit slower since we'll need 1 or 2 instructions more to calculate each channel of |
That's the goal of image_dds. Not all DDS files use BCN compression and some use cases need BCN decoding/encoding without DDS. bcdec_rs is just a safe Rust port of the original C implementation.
This should be a separate issue. The signed formats are almost never used from my experience.
I'm all for adding bit fiddling to get slightly more precision but not without the proper testing infrastructure first. I've outlined some ideas I may explore later in #4. The image crate is designed around standard 2D images. DDS is meant store GPU textures and contains a lot of features that aren't supported by the image crate like 3D textures, cube maps, array layers, mipmaps, etc. Adding better decoding support to image-rs would be great, but the design of image-rs limits its usefulness as a DDS converter. |
I worded that badly. I meant to express that "
The author of the
Certainly. Even for single-surface images, there are pixel formats that can't be supported, like the However, I see DDS support for the |
Yes. I'll update bcndecode-sys and bdec_rs with the new changes soon. I've also made good progress on implementing #4 for testing the decoding process end to end in image_dds with special test images for BC1-BC5. You can generate the DDS test files on this repo with |
bcdec_rs 0.1.2 is now available with the new rounding behavior. |
Hi! I'm currently implementing a new DDS decoder for the
image
crate and wanted to usebcdec_rs
for decoding BC1-7 images. Unfortunately,bcdec_rs
does not decode BC1-5 images correctly. Due to rounding errors, colors can be off by up to one for both BC1 and BC4.Example
While the error is only a single bit, it is noticeable. In BC1, the error affects G differently than R and B. This can lead to situations where R is one more than it should be and R and B are one less than they should be, resulting in a noticeable green tint. Here's an example BC1 image that should have the color RGB=47 47 47 everywhere (this is also what image programs like Paint.net and Gimp will show). Using
bcdec_rs
to decode this image will yield the color RGB=46 48 46 instead.BC1_UNORM_SRGB-47.zip
I also want to point out that these off-by-one errors aren't rare. I magnified the error of the following image, so we can see it:
Image:
![image](https://private-user-images.githubusercontent.com/20878432/338274586-e79beb82-ea3d-4a15-9438-27bda3d8e1b4.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU1OTQwOTgsIm5iZiI6MTcyNTU5Mzc5OCwicGF0aCI6Ii8yMDg3ODQzMi8zMzgyNzQ1ODYtZTc5YmViODItZWEzZC00YTE1LTk0MzgtMjdiZGEzZDhlMWI0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MDYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTA2VDAzMzYzOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTE5OTM0Mzk0Zjc4M2RmN2VkNWFmMmM5NjMwNjk2MTgyNjBhNjhmZThkZDc4MGEwNjYyNzM0MmVmZDFkNjYzNmEmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.fQJMYpZDIIByFTzVqz0d-5R311OEopblRcdeOjXn_gA)
Error:
![image](https://private-user-images.githubusercontent.com/20878432/338274612-e26bd004-404d-4e6a-93a3-d8936fc43be4.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU1OTQwOTgsIm5iZiI6MTcyNTU5Mzc5OCwicGF0aCI6Ii8yMDg3ODQzMi8zMzgyNzQ2MTItZTI2YmQwMDQtNDA0ZC00ZTZhLTkzYTMtZDg5MzZmYzQzYmU0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MDYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTA2VDAzMzYzOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWMwN2U4ZTNlN2M4OTIxMzc3OWRlZGYzYjg1Njc2N2ZkYTA0NGM4OWZmNTBkODk1Zjg3Nzg1Y2Y0MWIzOGE2ZTMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.z0xe5fgwYh-iq7RgN7hXCrB7YL-WHZleT6ExuS-gzTo)
BC1
Before I explain the cause of the issue, I quickly want to point out that this bug is not with the source translation of bcdec. The original bcdec C library has the same bug. bcdec_rs simply faithfully reproduces the bug.
The bug itself is actually quite simple: when calculating
color2
andcolor3
, bcdec is interpolating the already rounded 8-bit values ofcolor0
andcolor1
instead of the original 5/6-bit values.Using the 8-bit values for color0/1 is incorrect, because the conversion from 5/6 bits to 8 bits adds a bit of rounding error (e.g. 30 (5 bit) converted to 8 bit is 246.774 exactly but rounded 247) that is then passed along to color2/3 which are rounded again causing even more rounding error.
The fix is to do the interpolation with the original values and then convert to 8 bit. In code, this can be done like this:
In case you're curious about the crazy conversions like
(r * 351 + 61) >> 7
: they use the same trick as bcdec for its 5/6-bit number to 8-bit number conversion. E.g.(r * 351 + 61) >> 7
is equivalent to(r as f64 / (3 * 63) * 255).round()
for values0 <= r <= 3*63
. I got all of these constants using a brute force script that verified that it correctly maps the entire range of inputs.BC4
Here the bug is simpler: the rounding is wrong. E.g.
That
+ 1
should have been+ 3
.Similarly:
That
+ 1
should have been+ 2
.Why is
+1
wrong for the/7
values? E.g. if the interpolated alpha value turns out to be 5, then 5/7 should be rounded to 1. But(5+1) / 7 == 0
because integer division is floor division.In general, if you have 2 unsigned integers
x
andn
and want to find the rounded division ofx/n
, then it can be calculated as(x + (n>>1)) / n
. This is why the small number we have to add to the interpolated value is 3 for/7
and2
for/5
.Would you like me to make a PR?
The text was updated successfully, but these errors were encountered: