Properly convert Java char to C char in inputSwizzle #876

javagl · 2024-03-22T17:28:38Z

Fixes #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.png)

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

(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.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 · 2024-03-22T17:28:43Z

All committers have signed the CLA.

MarkCallow · 2024-03-23T07:24:50Z

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 · 2024-03-23T14:30:17Z

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 · 2024-03-24T11:15:55Z

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

One question to be answered and 1 change to make.

MarkCallow · 2024-03-25T08:18:59Z

interface/java_binding/src/test/java/org/khronos/ktx/test/KtxTexture2Test.java

+		t.compressBasisEx(p);
+
+		// Obtain the texture data, and compare it to the
+		// expected data of the reference texture


Since you are creating the texture in the test, I'd prefer not to need a reference file. You can transcode the swizzled t to KTX_TTF_RGBA32 and compare those pixels with the pixels in rgba. Given the full on colors you are using I don't think compression & transcode will change them.

I'd also like to avoid that reference file. (One reason: The tests/testimages directory already contains >100 files, without any information about what these files are, where they come from, and what they are used for. I'd like to avoid throwing more images on that pile...)

And what you suggested might ... nearly work: From a quick test, converting the resulting data to KTX_TTF_RGBA32 gave the following input/output pxiel values:

Pixel 0 in input 255, 0, 0, 255 Pixel 0 in output 2, 255, 2, 255 ... Pixel 256 in input 0, 255, 0, 255 Pixel 256 in output 0, 0, 253, 255 ... Pixel 512 in input 0, 0, 255, 255 Pixel 512 in output 253, 0, 0, 255

The swizzling is applied correctly, but there is a slight deviation in the formerly "pure" colors, of +/- 2 in some components. I don't know where this might come from - maybe something about SRGB, or it's a compression artifact, or whatnot. If you have an idea what might be causing this (or how to avoid this), I could change it accordingly.

Regarldless of that, the test could be changed to not use a reference image, by comparing the result of converting the image to KTX_TTF_RGBA32 to pixel values that are created programmatically (using the same functions that are used for creating the input image).

This could then be

A fixed comparison to the values that I currently see in the output (2, 255, 2)

Or a comparison with expected=(0, 255, 0) but with some "epsilon" of maybe +/-5 to account for the slight changes...

Any preference?

I prefer the fixed comparison as the test will not then be sensitive to changes in the encoder. You could also try ktxTexture2_CompressAstcEx. Perhaps that will have less artifacts.

You could also try ktxTexture2_CompressAstcEx. Perhaps that will have less artifacts.

Actually, in order to have proper test coverage, I can not only 'try' this (as an alternative), but actually have to test this in addition to the current test. Both of them (basis and astc) are going through different code paths (even though they are doing the same sort of conversion for the swizzling).

interface/java_binding/src/main/cpp/libktx-jni.cpp

MarkCallow · 2024-03-25T08:50:03Z

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

javagl · 2024-03-25T17:09:21Z

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 · 2024-03-26T06:56:52Z

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 BinomialLLC/basis_universal#60.

javagl · 2024-03-26T20:19:46Z

@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 Expose supercompression functions via JNI #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 JNI improvements #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 · 2024-03-27T09:54:11Z

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 · 2024-03-27T12:51:51Z

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 #879 (comment) )

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 · 2024-03-27T14:48:30Z

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 #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 · 2024-04-11T08:37:37Z

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 · 2024-04-12T10:12:06Z

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 · 2024-04-13T17:54:25Z

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

When this is merged, I'd wrap up #879 (which should only be a small one then).

For the "big" one, #886 , I'll do some more testing/polishing , and ... we'll have to figure out a way how this could sensibly be "reviewed"....

This builds upon #876 , which is already merged. It exposes the `deflateZstd` and `deflateZLIB` functions of the `ktxTexture2` class to the Java `KtxTexture2` class.

Properly convert Java char to C char in inputSwizzle

5eea92e

javagl added 2 commits March 23, 2024 15:19

Add basic test for JNI input swizzle

0bedf8d

Removed unused imports

4638ca0

This was referenced Mar 23, 2024

No option to apply zstd/zlib compression via JNI #877

Closed

Expose supercompression functions via JNI #879

Merged

Compare result of swizzle to reference

0ff0788

javagl mentioned this pull request Mar 24, 2024

Improvements for the Java/JNI bindings #880

Closed

MarkCallow requested changes Mar 25, 2024

View reviewed changes

javagl added 3 commits March 26, 2024 13:09

Test swizzle without reference image

7da5422

Organize imports

89b29cb

Merge remote-tracking branch 'upstream/main' into fix-jni-input-swizzle

13a571c

javagl mentioned this pull request Mar 26, 2024

JNI improvements #886

Merged

9 tasks

Fix test for input swizzling in JNI

647a5fc

MarkCallow approved these changes Apr 14, 2024

View reviewed changes

MarkCallow merged commit cc56485 into KhronosGroup:main Apr 14, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly convert Java char to C char in inputSwizzle #876

Properly convert Java char to C char in inputSwizzle #876

javagl commented Mar 22, 2024 •

edited

Loading

CLAassistant commented Mar 22, 2024 •

edited

Loading

MarkCallow commented Mar 23, 2024

javagl commented Mar 23, 2024 •

edited

Loading

javagl commented Mar 24, 2024

MarkCallow left a comment

MarkCallow Mar 25, 2024

javagl Mar 25, 2024

MarkCallow Mar 26, 2024

javagl Mar 26, 2024 •

edited

Loading

MarkCallow commented Mar 25, 2024 •

edited

Loading

javagl commented Mar 25, 2024

MarkCallow commented Mar 26, 2024

javagl commented Mar 26, 2024

MarkCallow commented Mar 27, 2024 •

edited

Loading

javagl commented Mar 27, 2024 •

edited

Loading

javagl commented Mar 27, 2024

MarkCallow commented Apr 11, 2024

javagl commented Apr 12, 2024

javagl commented Apr 13, 2024

Properly convert Java char to C char in inputSwizzle #876

Properly convert Java char to C char in inputSwizzle #876

Conversation

javagl commented Mar 22, 2024 • edited Loading

CLAassistant commented Mar 22, 2024 • edited Loading

MarkCallow commented Mar 23, 2024

javagl commented Mar 23, 2024 • edited Loading

javagl commented Mar 24, 2024

MarkCallow left a comment

Choose a reason for hiding this comment

MarkCallow Mar 25, 2024

Choose a reason for hiding this comment

javagl Mar 25, 2024

Choose a reason for hiding this comment

MarkCallow Mar 26, 2024

Choose a reason for hiding this comment

javagl Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

MarkCallow commented Mar 25, 2024 • edited Loading

javagl commented Mar 25, 2024

MarkCallow commented Mar 26, 2024

javagl commented Mar 26, 2024

MarkCallow commented Mar 27, 2024 • edited Loading

javagl commented Mar 27, 2024 • edited Loading

javagl commented Mar 27, 2024

MarkCallow commented Apr 11, 2024

javagl commented Apr 12, 2024

javagl commented Apr 13, 2024

javagl commented Mar 22, 2024 •

edited

Loading

CLAassistant commented Mar 22, 2024 •

edited

Loading

javagl commented Mar 23, 2024 •

edited

Loading

javagl Mar 26, 2024 •

edited

Loading

MarkCallow commented Mar 25, 2024 •

edited

Loading

MarkCallow commented Mar 27, 2024 •

edited

Loading

javagl commented Mar 27, 2024 •

edited

Loading