search

KhronosGroup / KTX-Software

KTX (Khronos Texture) Library and Tools
Other
830 stars 221 forks source link

Improvements for the Java/JNI bindings #880

Open javagl opened 3 months ago

javagl commented 3 months ago

The tl;dr of this issue could be:

I'd try to allocate some time to clean up and refactor the JNI part in some ways.

Anybody opposed?

Details:

I recently wanted to use the Java bindings that are offered via https://github.com/KhronosGroup/KTX-Software/tree/main/interface/java_binding . I noticed some issues ( https://github.com/KhronosGroup/KTX-Software/issues/624, https://github.com/KhronosGroup/KTX-Software/issues/875 , https://github.com/KhronosGroup/KTX-Software/issues/877 ...), and started fixing some of them ( https://github.com/KhronosGroup/KTX-Software/pull/876 , https://github.com/KhronosGroup/KTX-Software/pull/879 ...). But while diving into the implementation and the JNI part, I noticed that there are some more general questions that might warrant a dedicated discussion. Some of this might have to be broken down into dedicated issues, but I'll start with an overview here.

The current approach for the JNI bindings aims at offering the functionality of the KTX-Software - mainly, the ktxTexture2 class - with a 1:1 mapping of the original API. This is a reasonable approach. Any attempt to introduce a "convenience layer", abstractions, or functionality that is more "Java-idiomatic" raises a bunch of questions, and bears the risk of introducing bugs in the translation layer or limitations compared to the original API. However, there are some questions that have to be answered even for the 1:1 translation (and I think that they should be solved differently than they are right now).

Some questions are fine-grained and specific. For example: I think that the createFromNamedFile and writeToNamedFile methods should not be offered as part of the KtxTexture2 class in the current form. File systems and IO are too language- and platform-specific. (There may be convenience methods for stuff like that, but the underlying C/C++-functions should not be routed through via JNI). Some of these points may have to be discussed on a case-by-case basis, though.

One broader, general point is that of ...

Error handling

To get this straight: Error handling in JNI is a pain in the back. It bloats the code by a factor of 5, and is "frustrating" insofar that the developer knows in 99% of the cases that the error will not happen 99% of the time.

But right now, there is zero error handling for the Java bindings.

A call like KtxTexture2 t = KtxTexture2.create(null, KtxCreateStorage.ALLOC); will crash the JVM.

A call like t.setImageFromMemory(0, 0, 0, null); will crash the JVM.

A call like t.writeToNamedFile(null); will crash the JVM.

Whatever we are doing: It should be impossible to provoke a JVM crash (unless the crash is directly caused by the native implementation). And all the above cases could trivially be avoided, and this wouldn't even have to happen in the JNI layer, but could just be an Objecs.requireNonNull call in the Java layer itself. (Whether or not the JNI layer should then still check for null is up for debate - because, ... "We know that this cannot happen", right? 😶 )

Other error cases are more subtle.

For example, t.writeToNamedFile("C:/Does/Not/Exist.ktx"); will do ... nothing. Sure, it will return 3. And one can look this up, and see that this means FILE_OPEN_FAILED. But whether or not this method should exist in the first place, or how to handle this error, is up for debate.

More generally: The underlying C functions usually return an error code. (The documentation talks about @exception, but given that this is supposed to be plain C (not C++), I assume that this refers to "non-KTX_SUCCESS error codes", and not to any form of throw...). This is handled somewhat inconsistently in the JNI layer. For example, in the create... family of methods, these cases are handled by printing an error message to cout, and returning NULL (for example, in createFromNamedFile). In other cases, the error code is returned. Maybe this could be made more consistent. And we could also (very carefully!) consider to actually throw a (Java) exception from the JNI layer in some cases.

Convenience

There are some aspects that fell out of the 1:1 translation, and that are OK. For example, the concept of "flag bits" like KtxTextureCreateFlagBits.LOAD_IMAGE_DATA_BIT or enum-emulations like the KtxPackAstcEncoderMode.LDR. However, one should/could consider offering some convenience functionality here, to convert these into human-readable strings. This is particularly important for the KtxErrorCode (but may find its limit at VkFormat, which seems to be auto-generated during the build).

Documentation

There is basically zero documentation in the Java part. Collecting the documentation from ktx.h (and the documentation that is smeared over the implementation files), and "translating" it into JavaDoc (with proper cross-linking like The create flag {@link KtxTextureCreateFlagBits#LOAD_IMAGE_DATA_BIT} ... is a tedious, boring, and thankless task. But ... I'm a useful idiot, so I'd do some of that, at least...

MarkCallow commented 3 months ago

Thank you for starting this discussion and most especially for your willingness to improve the JNI.

Re. NamedFile stuff, the native functions accept UTF-8 file names which just work on macOS and Linux. On Windows the functions convert to the proper wide-chars before attempting file open so they work there too. Given this is should be possible for the JNI functions to work. If JNI filenames are 16-bit then a conversion to UTF-8 will be needed.

Re @exception in the documentation, it is used because that is the only way I found document the errors in Doxygen. Since this is a C API there are clearly no real exceptions happening. If JavaI supports exceptions, though the wrapper should probably check all return codes that throw if it is not KTX_SUCCESS.

There are functions in libktx for converting VkFormats to human readable strings and vice-versa. (Not sure if the latter is in libktx or just in some of the commands.)

javagl commented 3 months ago

Trying to pull together a few "TODOs" from related issues and this one.

(EDIT: A summary of the tasks is now in https://github.com/KhronosGroup/KTX-Software/pull/886 )

In recent comments here and here, you mentioned

I agree CreateFromMemory should be exposed.

The JNI binding should absolutely expose ktxTexture2_GLUpload and, if there is a Java Vulkan wrapper, ktxTexture2_VkUpload.

I'll start with CreateFromMemory,because it is crucial and less controversial. For the other functions, @ShukantPal voiced some concerns, so I'd probably defer that until we have a clear idea whether this will work exactly as intended.

In another recent comment, you mentioned

You can transcode the swizzled t to KTX_TTF_RGBA32

In that context, I noticed that the KTX_TTF_RGBA32 constant is not yet defined in the Java KtxTranscodeFormat class. I'll add this, as part of the ~"general cleanup" (and look for other missing constants there as well...)

Somewhat related: The current documentation at https://github.khronos.org/KTX-Software/libktx/group__reader.html#ga90cc33928a2dae962fb94b3fa2f6575d says

The following transcodeFlags are available.

That's it. It doesn't say what they are. This might be caused by some renaming, related to ktx_texture_decode_flags and ktx_transcode_flag_bits, related to https://github.com/KhronosGroup/KTX-Software/blob/5414bd0ebfdfdf4ecb8c613a6edb21372303c2b9/include/ktx.h#L1731 , which might break the documentation, but I haven't looked at the details.

In https://github.com/KhronosGroup/KTX-Software/pull/879/files/a512cc5373931777c9bf3f4d831148f770dd784a#r1537201514 you mentioned that a part of the documentation should be updated, even on the native side. This will probably affect the documentation of the JS/Python/Java bindings, so I'll have to review that carefully.

In https://github.com/KhronosGroup/KTX-Software/pull/879#discussion_r1537209791 , you mentioned that one of the new tests could also compare the image data. And I agree that the tests should be more thorough (and many of the existing tests could be improved in this regard). But it's sometimes hard to know what can reasonably be tested, and how exactly the required data can be obtained. The improvements here will therefore (have) to be iterative.

In the same comment ( https://github.com/KhronosGroup/KTX-Software/pull/879#discussion_r1537209791 ) you mentioned

To make my suggestion work you will have to call t.writeToMemory then CreateFromMemory but didn't you tell me those functions aren't exposed in JNI. They need to be.

I'll have to look at the details of the 'inflate...' functions. I already started implementing the createFromMemory function locally (and will have to see into which PR this should go. This is more than a "trivial bugfix", or "routing throgh a function". This already does raise a bunch of questions that I'll try to summarize a bit later).

The writeToMemory function already does exist, in the KtxTexture class, carrying the comment

    /**
     * This **might** not work (INVALID_OPERATION for some reason)
     */

Yeah. I'm not sure what to do with that. I'll consider it broadly as "one of these things that should be part of an overall improvement"....

There are functions in libktx for converting VkFormats to human readable strings and vice-versa.

Regardless of where this is offered in the native part: It would very likely make sense to offer that translation directly on the Java side. Having a switch (c) { case 123: return "FOO"; } is orders of magnitude simpler and easier to maintain than trying to call some native function there.

Some tasks - preliminary:

(EDIT: These have been moved to https://github.com/KhronosGroup/KTX-Software/pull/886 to better keep track of them in the context of a PR)

MarkCallow commented 3 months ago

For the other functions (GLUpload and VkUpload)

At the native level there is only one VulkanSDK which reduces the issues considerably compared to OpenGL. In #877 you mentioned "mix(ing) different JNI libraries that only communicated via some GLuint someOpenGLObject". I think that is the major issue. If that works then the upload functions should be usable in Java. For GL #708 will likely need to be fixed. The fix involves adding a function to set the address of the GetProcAddress function to use.

This might not work (INVALID_OPERATION for some reason)

Need @ShukantPal's help re. this.

There are functions in libktx for converting VkFormats to human readable strings and vice-versa.

Regardless of where this is offered in the native part: It would very likely make sense to offer that translation directly on the Java side.

Any such file should be generated as we do with ktxVkFormat.java to avoid headaches keeping it up-to-date over the long term.

Thanks for the task list.

javagl commented 3 months ago

Any such file should be generated as we do with ktxVkFormat.java to avoid headaches keeping it up-to-date over the long term.

I fully agree for the VkFormats. There are many of them. But this referred to other classes that emulate 'enums'/flags via public static final int variables. Sure, the question about updating them applies there as well (and the missing KtxTranscodeFormat.KTX_TTF_RGBA32 is one example). But except for the VkFormats class, none of these classes is currently auto-generated. So unless we completely refactor the code and build process, creating these functions manually should be a reasonable start.