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

Added DDS BC6H reading #6449

Merged
merged 17 commits into from
Oct 11, 2022
Merged

Added DDS BC6H reading #6449

merged 17 commits into from
Oct 11, 2022

Conversation

ShadelessFox
Copy link
Contributor

@ShadelessFox ShadelessFox commented Jul 17, 2022

Fixes #6344

Before After

@ShadelessFox ShadelessFox force-pushed the main branch 2 times, most recently from f6bf186 to 37f766b Compare July 17, 2022 18:58
@radarhere
Copy link
Member

Thanks very much for figuring this out!

@radarhere
Copy link
Member

Our habit is to add tests with PRs. This helps prevent regressions in the future.

If you're someone who works with BC6 images, do you happen to have an image that we can add to our test suite, and distribute under the Pillow license?

@ShadelessFox
Copy link
Contributor Author

I have several images that can be used as a reference. I'm unsure about their origin. If that's okay, I can attach them.

I originally wanted to add tests, but there are no python bindings for BC6H. Are there C API tests of some kind?

@radarhere
Copy link
Member

I've created ShadelessFox#1 to add Python bindings.

Just making sure there is an explanation of the change here - you looked at the code, and as per https://docs.microsoft.com/en-us/windows/win32/direct3d11/bc6h-format#transform-inversion-for-endpoint-values, saw that not all of the deltas were being applied, yes? Previously, the B0 delta was incorrectly being applied to each of the RGB channels, right?

@ShadelessFox
Copy link
Contributor Author

Thanks, I will look into that later.

I was looking at Khronos' specs, and it says the following:

If the mode has transformed endpoints, the values from E0 are used as a base to offset all other endpoints, wrapped at the number of endpoint bits. For example, R1 = (R0 + R1) & ((1 << EPB) - 1).

Where E0 is (R0, G0, B0). The provided reference shows the operation just for a single component, and others were implied.

ShadelessFox and others added 4 commits July 18, 2022 22:24
Decoding error were caused by additional sign extend call after endpoint
transform, according to khronos documentation, you only suppose to sign
extend endpoints only once, further calls to sign extend mangles
endpoint data.
@ShadelessFox
Copy link
Contributor Author

ShadelessFox commented Jul 18, 2022

I noticed that the provided bit packing table differs from the table provided by Khronos.

You can find its textual representation here:
https://registry.khronos.org/OpenGL/extensions/ARB/ARB_texture_compression_bptc.txt

The following python script can be used to transform it into a suitable format:

def transform(data: str) -> str:
    result = []
    for elem in data.split(','):
        split = elem.index('[')
        name = elem[0:split]
        indices = elem[split + 1:len(elem) - 1]
        if ':' in indices:
            a, b = map(int, indices.split(':'))
            if a > b:
                indices = [x for x in range(b, a + 1, +1)]
            else:
                indices = [x for x in range(b, a - 1, -1)]
        else:
            indices = [int(indices)]
        for index in indices:
            result.append(endpoints[name] << 4 | index)
    return ', '.join(map(str, result))

Update: wrong spec link

src/decode.c Outdated Show resolved Hide resolved
Tests/test_file_dds.py Outdated Show resolved Hide resolved
ShadelessFox and others added 2 commits July 18, 2022 16:15
Restored unimplemented DXGI format test
@ShadelessFox
Copy link
Contributor Author

I hope you don't mind this mess. I can squash commits if you want.

@radarhere
Copy link
Member

Not a problem. It's good to be able to look back and understand where the changes came from.

@REDxEYE
Copy link
Contributor

REDxEYE commented Jul 19, 2022

All this started from an idea of previewing textures in a java application, both I and @ShadelessFox ended up implementing BC1-BC7 decoders, to see who can get it working first. @ShadelessFox found an issue with delta decoding and I found an issue with the signed format.

@ShadelessFox
Copy link
Contributor Author

Let me know if this PR requires more polishing before it can be merged.

@ShadelessFox
Copy link
Contributor Author

Hi @radarhere,

Is there anything I must do before this PR can be merged?

@radarhere
Copy link
Member

I'm not aware of any problems at the moment, and you've added tests, so it's just a matter of myself or someone else finding the time to review.

The Valgrind failure happening here isn't due to your code. I've raised a separate issue for it - #6468

@radarhere
Copy link
Member

Interestingly, I found that Table 58 at https://registry.khronos.org/DataFormat/specs/1.1/dataformat.1.1.html#_bc6h has some mistakes in it (e.g. Mode 1, Bit 4 should be G35 as per Table 57).

Everything in this PR makes sense to me, except for bc6_gamma_correct. That doesn't seem to be mentioned in the specification?

@ShadelessFox
Copy link
Contributor Author

It's not the part of the specification. I had to perform gamma correction so the output image looks fine without washed-out colors due to the latter float16 -> int8 conversion required by Pillow. AFAIK Pillow does not support floating point storage?

@radarhere radarhere changed the title Fix BC6 block decoder Added DDS BC6 reading Oct 10, 2022
@radarhere
Copy link
Member

There is an open issue for multichannel floating point images - #1888. I'm not following why that should lead to washed-out colors though?

If I remove the gamma correction, then the output matches what I get when saving the DDS image as a PNG from macOS Preview. I've created ShadelessFox#6

@ShadelessFox
Copy link
Contributor Author

Sorry, not washed-out but "deep-fried". If you're okay with the result, then me either.

@radarhere radarhere added the automerge Automatically merge PRs that are ready label Oct 10, 2022
@radarhere radarhere merged commit 28878c6 into python-pillow:main Oct 11, 2022
@radarhere radarhere changed the title Added DDS BC6 reading Added DDS BC6H reading Oct 19, 2022
radarhere added a commit to radarhere/Pillow that referenced this pull request Oct 19, 2022
hugovk added a commit that referenced this pull request Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge PRs that are ready Hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DDS BC6 decoding has a broken blue channel
4 participants