-
Notifications
You must be signed in to change notification settings - Fork 226
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
JNI improvements #886
base: main
Are you sure you want to change the base?
JNI improvements #886
Conversation
# Conflicts: # interface/java_binding/src/test/java/org/khronos/ktx/test/KtxTexture2Test.java
I locally started a few updates, including some basic error handling, so that things like One thing that could/should be done next: The /**
* Destroy the KTX texture and free memory image resources
*
* NOTE: If you try to use a {@link KTXTexture} after it's destroyed, that will cause a segmentation
* fault (the texture pointer is set to NULL).
*/
public native void destroy(); Now, the goal is: No segmentation faults. And we'll probably not be able to completely avoid this I think that trying to use a |
There currently are two functions in the
(The latter has been added as part of this PR) Many libraries that are using JNI expect direct On the Java side, the byte array[] = ...;
t = KtxTexture2.createFromMemory(ByteBuffer.wrap(array), ...); But as a middle-ground, I'd suggest to...
An aside: The documentation of the
This statement is not true, and will be updated accordingly. Remotely related: There currently is a function |
What does "passed data" refer to here? Is it the data at the pointer location that was passed to setImageFromMemory? Not that it matters since you say it isn't true. Indeed the data is copied into the ktxTexture object. |
There are multiple "layers" here, and this revolves exactly about the question where data is copied. Imagine the following Java pseudo code, similar to what could appear in the KTX JNI bindings: byte input[] = new byte[10];
// Create a texture from the input
KtxTexture2 texture = KtxTexture2.createFrom(input);
// XXX Will this affect the result?
input[5] = 123;
// Encode the input data that was given earlier
texture.encode(); The comment suggested that this |
A detail @ShukantPal : Currently, the bindings are set up for Java 11. I don't see a strong reason for that. Unless there is a reason, I'd suggest lowering the version requirement to "the lowest version that works" (and I think that 8 should do it). |
@javagl, PR #874 extends the Javascript binding to provide full libktx functionality. As part of it I went over the naming. I made the names consistent with the libktx names and I ended up removing all the I suggest asking @ShukantPal to review this as he is the author of the original binding. The bullet point list at the top is a good guide. Obviously there are items to be completed. I think a short call would be good but I want to first familiarize myself with the code. I'll let you know when I've done that and we can then plan the call. |
Thanks @MarkCallow , I'll try to allocate some time looking over the changes from the linked PR. This PR should be a step in the direction of aligning the JNI bindings with KTX itself. We can decide later whether some missing parts (like bindings for the
I'm in favor of aligning names insofar that they should be recognizable as their native counterpart. There are some conventions in the respective target languages, though. For example, I wouldn't use something else than a |
@javagl, I've been reviewing the discussion here and some of the code including the commits you specifically called out in comment of April 18th. The code I've seen looks very good. I added a note to the description suggesting following the JS binding handling of metadata queries.
Can't you call the native function(s) from the stringFor function to get the elaborate description. I'd particularly like to have the full descriptions of the error codes. Are there any beyond I am happy you have replaced the crashes with exceptions.
Any plan to release this? Does the preview show you both pre- and post-compression images so you can easily compare them?
Which implementations?
I agree a version that accepts ByteBuffer is needed.
Yes.
There is certainly enough here to review. I want to spent a little more time familiarizing myself with the code before we arrange that call. Note that all GitHub Actions Windows builds are currently broken due a bad GHA runner image. Many tests, including the Java binding tests fail with segmentation violations. |
Thanks for the detailed review.
I haven't looked at the detals of the hash-related functions. If it's "only" about exposing 3 functions, then it might be added here, to be on par with other bindings, but if it's too much, it might make more sense to carve it out into a follow-up PR.
There are two levels here:
Until now, I only addressed 1.: Each "enum" has a Now, I saw that there is the
Until now, this was mainly for internal experiments and testing. I think that it might eventually have some aspects that are "useful", and that it could be worth being published. We'll probably have to sort out some of the "Maven Release"-related questions before this can be done, but maybe there can be an "early sneak preview" release. The goal was to have some sort of "interactive preview". (As 'interactive' as it can be, when encoding an image takes 5 seconds...). I did create a short screengrab of that a while ago (yeah... as a GIF... but), maybe it shows the overall idea:
I did try to keep an eye on that for every function that I "touched", but did not systematically go through all function from top to bottom. It boils down to the following pattern: When any JNI function that is called with (The only functions that may be called with a pending exception are the ones like To check this manually, I'd have to read every function and see whether it exposes this pattern (calling a function when an exception might be pending). It can be automated to some extent, with the I still have to look closer at the other PR, but will try to at least pick up the work here (syncing with |
I am now back to familiarizing myself with the more of this code. Sorry for the looooong delay. Can we arrange a call towards the end of next week, FaceTime or Skype? I want to get this wrapped up. Regarding constant naming. both the JS and Python bindings remove KTX_ and all other parts of a name that are reflected in the class or enum name. (The sole slight exception I've spotted is e.g. Another aspect of naming is the enum or class name. In the JS binding I have used the enum or struct name from the C API minus the Please merge the latest main into this branch. You will almost certainly get a failure in the first Travis-CI job of a build which is a Mac build. This is due to now using vcpkg for dependencies for the load test apps. The dependencies will not be cached in the initial build. The resulting extended log length from building them seems to be hitting a Travis-CI limit. |
I have updated with the For the constants: I'm OK with either pattern.
As long as it is consistent, I don't have a strong opinion. It can either be aligned with the C name (i.e. the long form), or the short name that is used in other bindings. Java does not differentiate between "struct" and "class". Java does have
If you don't mind, I'll send you a mail to arrange the details. |
Certainly. I did not intend to arrange that through this public forum. |
The |
The last sequence of commits are...
The last one appears to be related to the (open) question about how to handle the absence of Visual Studio even points this out: So I changed that check to
|
I think that the build failure should already have been sorted out - right now, it marks some actions as 'failed' because they have been cancelled (maybe because I edited the first post while the build was running?). In doubt, I could push ... "some change" ... to trigger a re-run (because I apparently cannot request a re-run manually). Apart from that, this PR should be 'Ready for review' (which might be short, because intermediate states have already been reviewed and discussed). Miscellaneous:
|
In GitHub Actions when a job in a workflow fails the remaining jobs are cancelled. The VS2019 job in the Windows workflow is failing. I just tried re-running this job but it still fails in the same place: problems creating the Javadoc. I will try rerunning the cancelled jobs. Some of the Linux build jobs are failing for a similar reason. I did not try to re-run them. I'm don't know yet why some macOS jobs are failing. When you add more commits to a PR, existing queued builds are cancelled, running builds may be cancelled. A new build is started for the new commit when an existing job is cancelled or finishes. All this means in this case that the errors remain in the code of the most recent commit because a build was done using it. |
The failures in the Emscripten jobs look to be due to an updated clang in the latest Emscripten Docker image. Research is needed. |
Addresses #880
I know, the title is too generic. It would be better to clearly lay out a work plan and create a clean sequence of self-contained, specific PRs. But there are just too many (unrelated) things to do. "You can't cross a chasm in two small jumps". The first pass has to be tackled somewhat holistically. The result may not be as 'good' as I'd like it to be, but I hope that it will be 'better' along many dimensions. If this is not considered to be a viable approach, then that's OK. Then I'll just create my own repo with JNI bindings for KTX, with blackjack and error handling.
The preliminary task list that I tried to create in the linked issue is now replicated here, and I'll try to update it and extend it as we move on:
CreateFromMemory
stringFor
... methods to convert them to stringsAligned the names of all constants with the native versionAligned the names of constants with the other bindings (see JNI improvements #886 (comment) )
Added documentation and string functions for all constants, except for...
KtxInternalformat
VkFormat
(See Synchronizing auto-generated files with bindings #887 )
NOTES:
ktxTexture2.supercompressionScheme
does not appear in the native documentationKtxPackAstcEncoderMode
are not documented on the native sideKtxTestLibraryLoader
to also work on Windowsenv->GetFieldID(ktx_texture_class, "instance", "J");
are called in each function. ThejfieldID
should be obtained once initially.null
to any method should throw aNullPointerException
.destroy()
was called should throw anIllegalStateException
.create...
methods) now generally throw aKtxException
when the implementation caused an error code that was notKTX_SUCCESS
ktxErrorString
Not (yet) supposed to be addressed here:
Try to find a clean solution for JAR release to Maven Central #624
There are a few cases where we might have to think about how to handle the absence of
unsigned
types in Java. For example, when callingcreateInfo.setBaseWidth(-123); // INVALID!
, then this value is implicitly converted into aktx_uint32_t
.Update the documentation ofdeflateZstd
anddeflateZLIB
EDIT: This was now done as part of Expose supercompression functions via JNI #879, but only for the native side and for the Java bindings (other bindings are still missing the doc update or the functions)
deflateZLIB
function is still missing in the Python binding (and thedeflate*
functions are just about to be added to the Java binding), so ... this may have to be tracked and addressed separatelyImprove thoroughness of
deflate*
tests by comparing the deflated valuesdeflate*
functions should compare the result of inflating the data again to the original input. Theinflate*
methods are not yet exposed (anywhere), so this may have to be deferred for nowAdd test for input swizzling in ASTC
Consider consistent formatting. I know, it's a detail, but just auto-formatting everything with the Eclipse default could already be an improvement. I do have strong preferences for my own projects, but here, the main point is that the formatting should be consistent...
The ktxHash... functionalities are not yet mapped through from the native side.
findKeyValue
,addKVPair
anddeleteKVPair
methods to the ktxTexture2 object rather than exposingktxHashList
.