Properly convert Java char to C char in inputSwizzle

javagl commented 5 months ago

Fixes https://github.com/KhronosGroup/KTX-Software/issues/875

As detailed in the issue: The Java char[] array for the input swizzle was casted to a jbyteArray in the JNI layer, in order to write it to the C/C++ char[4] array. This cast was invalid and caused data corruption.

There could be two ways of tackling this:

One could change the type of the inputSwizzle in the Java layer to byte[] (which has 8 bits, and would make the cast valid). This would be a breaking change, and may require the inconvenient casting at the call site like p.setInputSwizzle(new byte[]{ (byte)'b', (byte)'r', (byte)'g', (byte)'a' });
One could fetch the Java char[] array as a jcharArray, and cast the elements to (C/C++) char individually

This PR takes the second option (but we could do this either way, depending on the preferences from others).

With this change, running the test/example program that is given in the issue produces the following results (showing the PNG files here - the KTX ones are attached below)

Without calling setInputSwizzle, the result is this:

output_swizzle_unswizzled

(output_swizzle_unswizzled.png)

With setInputSwizzle(new char[]{ 'r', 'g', 'b', 'a' }); (which should not change anything), the result is this:

output_swizzle_rgba_fixed

(output_swizzle_rgba_fixed.png)

With setInputSwizzle(new char[]{ 'b', 'r', 'g', 'a' });, actually swizzling things around, the result is this:

output_swizzle_brga-fixed

(output_swizzle_brga-fixed.png)

The KTX and PNG files are attached here:

KTX swizzle results 2024-03-22.zip

This PR does not involve any unit tests. I wouldn't know how to add one exactly - maybe just comparing the output to a "golden" reference file?

CLAassistant commented 5 months ago

All committers have signed the CLA.

MarkCallow commented 5 months ago

Thank you for this. I will review it as soon as I can. I have other PRs to review first.

In the meantime, please add a test for swizzle to interface/java_binding/src/test/java/org/khronos/ktx/test.

javagl commented 5 months ago

I have added a test that calls setInputSwizzle (basically the same as the snippet that was posted in the issue).

EDIT: The following is obsolete. See the comment below for details.

But calling the function did alread work before this fix. I don't see a way to check that the output is the desired result. I created a "reference image", and tried to compare the image that is generated in the test to the reference image, but ... what exactly should be compared there? The file contents differs, because the file contains version information like 4.3.0-alpha3~2 and so. Calling getData returns ... something, but ... I don't know what it is, and apparently, it cannot be compared (doing the same thing twice causes different return values for getData...).

There does not seem to be a way to check whether the result image is the "broken" one,

or the one where the RGBA -> BRGA swizzle has properly been applied...

javagl commented 5 months ago

I have added a test to compare the result of getData() to the data that is obtained from a reference texture, stored as /tests/testImages/swizzle-brga-reference.ktx.

(I tried this initially, and it didn't work as expected, but this was caused by an issue in the KtxTextLibraryLoader, mentioned in this comment. I'll try to keep track of that and see whether it can easily be resolved)

MarkCallow commented 5 months ago

The test failed on GNU/Linux and macOS but passes on Windows. I'm surprised by that.

javagl commented 4 months ago

The test failed on GNU/Linux and macOS but passes on Windows. I'm surprised by that.

I'm a little bit surprised by that as well. Looking at the failed build output:

[ERROR] Tests run: 11, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.248 s <<< FAILURE! - in org.khronos.ktx.test.KtxTexture2Test
[ERROR] testInputSwizzleBasisEx  Time elapsed: 0.014 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: array contents differ at index [0], expected: <49> but was: <77>
    at org.khronos.ktx.test.KtxTexture2Test.testInputSwizzleBasisEx(KtxTexture2Test.java:278)

It does compare the array obtained with getData there. (And I'm sure that the fact that it expected a 1 but found an M is just a coincidence). Now I'm wondering whether the contents of this data might in any form or way depend on the operating system with which the data is created.,,

(Maybe this becomes obsolete when the reference file is no longer used, but I'm still curious...)

MarkCallow commented 4 months ago

Now I'm wondering whether the contents of this data might in any form or way depend on the operating system with which the data is created.,,

Yes it does. See https://github.com/BinomialLLC/basis_universal/issues/60.

javagl commented 4 months ago

@MarkCallow The test has been updated to no longer compare to a reference image. Instead, it is converting the texture to KTX_TTF_RGBA32, and comparing the data to the expected pixels, which are created "manually" in memory. The relevant part is https://github.com/KhronosGroup/KTX-Software/pull/876/files#diff-8ed75fc18c4588264650ce9f7dd4e693adb82c64a22caee3eb1de19cba383b05R278

As you have seen, there are follow-ups for this PR:

This PR is only a "bugfix", strictly speaking
Based on that, I had created https://github.com/KhronosGroup/KTX-Software/pull/879 (so when this PR is merged, the one that adds the supercompression functions could be wrapped up and merged)
Based on both of them, the PR https://github.com/KhronosGroup/KTX-Software/pull/886 aims at addressing all the cross-cutting things that I noticed (and that you brought up) while creating the first ones: Documentation, error handling, more missing functions, more tests.... This PR will probably still take a while: There are a few questions about how exactly things will be addressed, and some of the points are nearly "open ended" (like documentation and tests), but I'll still try to bring it into a somewhat consistent state ASAP.

MarkCallow commented 4 months ago

So you want me to merge this one first, @javagl?

Please note the comment I made in PR #879 regarding the swizzle test. I commented there because I was thinking of merging that PR to get both fixes at once.

javagl commented 4 months ago

There are three PRs open right now. (And sorry for that. I certainly did not anticipate "big" changes in the beginning, and only wanted to fix that swizzle thing, but ... the requirements for further changes 'piled up' in the process).

Given that these PRs build upon one another, there are the options to

Merge OR drop the swizzle fix
Merge OR drop the supercompression functions
Merge the JNI improvements (when it's done...)

The last one will probably be 'large', and involve more changes, discussion, and reviews. So the first ones could be merged as intermediate steps, because they are still 'self-contained'. (The 'JNI improvements' will involve many changes across many files, and is harder to split into dedicated PRs...)

None of these PRs is "time-critical", so to speak. (And it's unlikely that there are conflicts with other areas - who's using JNI after all? 😁 ). So we can defer merging anything until you're back (c.f. your comment at https://github.com/KhronosGroup/KTX-Software/pull/879#issuecomment-2022094949 )

In the 'supercompression' PR, you added a comment about the test that actually refers back to a comment here, where you said

I thought the "fixed" solution you were proposing was to also encode the source without swizzling and compare the rows of the transcoded swizzled data with what are expected to be the matching rows of the transcoded unswizzled data.

What I did:

Encode the image data with Basis+Swizzle
Transcode it to RGBA32
Compare the 'pixel colors' of this result to the colors that one would expect, given the sizzling

What you now suggested, as pseudocode:

byte input[] = createInput();

// Compress to basis, once without and once with swizzling
KtxTexture2 original = compressWithoutSwizzle(input);
KtxTexture2 swizzled = compressWithSwizzle(input);

// Transcode to RGBA32
byte originalRgba[] = original.transcodeToRgba32();
byte swizzledRgba[] = swizzled.transcodeToRgba32();

// Check each pixel of the swizzled output if it is
// exactly the same as the swizzled pixel in the
// output that was created without swizzling
for (each pixel) {
    swizzledRgba[pixel] = swizzle(originalRgba[pixel]);
}

If this is correct, then I can change this accordingly. (It involves implementing a 'swizzle(pixel)' function for the test, but ... OK)

javagl commented 4 months ago

A quick test suggests that the proposed approach does not work as expected. When implemented based on the pseudocode, the output will be

At 0 unswizzled 253,   0,   0, 255
At 0 swizzled     2, 255,   2, 255
At 0 expected     0, 253,   0, 255
...
At 256 unswizzled   2, 255,   2, 255
At 256 swizzled     0,   0, 253, 255
At 256 expected     2,   2, 255, 255
...

So apparently

"compressing with swizzling" and
"compressing and then swizzling"

produce different results, exactly in ths +/- 2 range for some components.

Given that...

before this PR, swizzling did not work at all
before this PR, there was no test coverage for "pixel colors" at all

... I'm leaning towards not trying to make "this test perfect" now. Improving the tests and coverage (including the subtle questions about slight color deviations) may be part of https://github.com/KhronosGroup/KTX-Software/pull/886 .

If your concern is that this test might break unpredictably, I'll just remove the "pixel color comparison". Then we'd have a working swizzle functionality, even though it's not covered by a test, similar to a million other things that are 'working 🤞' but not covered by a test.

And by the way: All this suggests for me that there is zero test coverage for the effects of swizzling on the native side either. One could certainly argue about the responsibility of the Java bindings for "pixel-perfect comparisons" here...

MarkCallow commented 4 months ago

There are unittests for the function that performs the swizzle before submitting the data to the encoder and there are tests of the --input-swizzle option of ktx create. I think the issue you are seeing is due to how block compression works. Try this pseudo-code instead

byte input[] = createInput();
byte goldSwizzle[];
for (each pixel) {
    goldSwizzle[pixel] = swizzle(input[pixel]);
}

// Compress to basis
KtxTexture2 swizzled = compressWithSwizzle(input);
KtxTexture2  goldSwizzled = compressWithoutSwizzle(goldSwizzle);

// Transcode to RGBA32
byte swizzledRgba[] = swizzled.transcodeToRgba32();
byte goldRgba[] = goldSwizzled.transcodeToRgba32();

// Check each pixel of the swizzled output if it is
// exactly the same as the swizzled pixel in the
// output that was created without swizzling
for (each pixel) {
    swizzledRgba[pixel] = goldRgba[pixel]);
}

javagl commented 4 months ago

Thanks @MarkCallow , that seems to work (at least locally - let's see whether the build passes...)

If the number of this kind of tests (which compare actual pixel values) increases, some of the required functionality could be moved into the TestUtils class, but that can be done in follow-up PRs.

javagl commented 4 months ago

@MarkCallow I think this should be ready to merge, if you agree.

When this is merged, I'd wrap up https://github.com/KhronosGroup/KTX-Software/pull/879 (which should only be a small one then).

For the "big" one, https://github.com/KhronosGroup/KTX-Software/pull/886 , I'll do some more testing/polishing , and ... we'll have to figure out a way how this could sensibly be "reviewed"....

KhronosGroup / KTX-Software

Properly convert Java char to C char in inputSwizzle #876