Skip to content
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

DDS BC5_SNORM images are decoded incorrectly #7386

Closed
RunDevelopment opened this issue Sep 10, 2023 · 8 comments · Fixed by #7401 or #7413
Closed

DDS BC5_SNORM images are decoded incorrectly #7386

RunDevelopment opened this issue Sep 10, 2023 · 8 comments · Fixed by #7401 or #7413

Comments

@RunDevelopment
Copy link

What did you do?

I loaded a BC5 SNORM images.

What did you expect to happen?

For it to be decoded correctly.

image

What actually happened?

Pillow incorrectly decoded the image. It added artifacts and washed out the "colors" of the stored normals.

This is speculation on my part (I haven't read PIL's source code), but it seems like PIL uses the same algorithm for BC5_UNORM (ATI2) and BC5_SNORM.

image

What are your OS, Python and Pillow versions?

  • OS: Win 10 64 bit
  • Python: 3.8.2
  • Pillow: 9.2.0 and 10.0.0
# it's pretty much as basic as it can get
im = Image.open(dds_path)
img = np.array(im)

The DDS file in question: cables_normal.zip

@radarhere
Copy link
Member

This is speculation on my part (I haven't read PIL's source code), but it seems like PIL uses the same algorithm for BC5_UNORM (ATI2) and BC5_SNORM.

It'll take some more investigation to figure out the problem, but I can at least say that this shouldn't be the case. Here is the ATI2 code in Python,

elif fourcc in (b"ATI2", b"BC5U"):
self.pixel_format = "BC5"
n = 5
self._mode = "RGB"

and then where your image is handled.

elif dxgi_format == DXGI_FORMAT_BC5_SNORM:
self.pixel_format = "BC5S"
n = 5
self._mode = "RGB"

If you're interested, almost all of the decoding logic for this lives in our C code.

case 5:
while (bytes >= 16) {
rgba col[16];
memset(col, 0, 16 * sizeof(col[0]));
decode_bc5_block(col, ptr, strcmp(pixel_format, "BC5S") == 0 ? 1 : 0);
put_block(im, state, (const char *)col, sizeof(col[0]), C);
ptr += 16;
bytes -= 16;
if (state->y >= ymax) {
return -1;
}
}
break;

@RunDevelopment
Copy link
Author

If you're interested, almost all of the decoding logic for this lives in our C code.

In that case, the issue is most likely in decode_bc3_alpha. Its sign parameter is basically just a bool whether the current block is BC5S. So the issue is most likely in there or around it.

decode_bc3_alpha(char *dst, const UINT8 *src, int stride, int o, int sign) {

For reference: this is how DirectXTex does it.

@radarhere
Copy link
Member

What program did you use to convert the image to your "expected" PNG?

@radarhere
Copy link
Member

I've created PR #7401. With it, this is the result. What do you think?

out

@RunDevelopment
Copy link
Author

Sorry for the late response @radarhere and thank you for fixing this!

I used texconv to decode the DDS file, but other programs (e.g. PhotoShop) give the same result (no idea what they are doing/using under the hood). So I'm not sure whether the new output is 100% correct.

BC5 doesn't define a B channel (I think), so arguably the new output is technically correct, but I think the output of texconv and co makes more logical sense. These programs say that the B channel is 0. For BC5_UNORM, we actually get B=0 in the output, but for BC5_SNORM, these programs output B=128. This is because 0 (SNORM) is mapped to 128 (UNORM).

@radarhere radarhere reopened this Sep 20, 2023
@radarhere
Copy link
Member

BC5 doesn't define a B channel (I think)

Correct. See https://learn.microsoft.com/en-us/windows/win32/direct3d10/d3d10-graphics-programming-guide-resources-block-compression#bc5

I find making changes not outlined by a specification to be awkward, but the fact that texconv is developed by Microsoft, who wrote the specification, is a pretty good argument for mimicking that behaviour.

@radarhere
Copy link
Member

radarhere commented Sep 21, 2023

I've created #7413 to resolve this.

With it, Pillow now creates
out

@RunDevelopment
Copy link
Author

That looks perfect. Thank you @radarhere!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants