Discuss about other bindings implementation

[dbouron]

Hello,

The goal is to implement missing bindings. This non exhaustive list aims to report the axis of improvement, conventions and technical choices for the project. I will update this task list once we agree on a solution:

Propositions

1. Naming convention (see also: https://github.com/criteo/JVips/issues/40)

• variable, method and function names in camel case
• class names in pascal case

2. Stateless VipsImage Actually, VipsImage object is a stateful object. It has a mutable state when some methods are invoked (e.g.: resize(), pad(), crop(), ...). The .NET wrapper has a different approach. Every image method returns a new image. Should we keep the actual implementation or move to a stateless VipsImage?

3. How to handle the optional input arguments of some vips operation in java?

• create a <OperationName>OptionalArguments class with default values and primitive type. Use builder pattern.
• add non primitive type (e.g.: Integer) but nullable and append only non-nullable parameter to vips_operation (possible [non-]primitive type mixing in the method signature -> ugly 🤮)
• use default value annotation

4. How to handle optional output in java?

• return a <OperationName>Result struct
• pass a <OperationName>ResultRef struct as parameter which wraps and packages required fields into a ByteBuffer. Thus we share a memory space which can be read/written by both C and Java code (not thread safe ⛐)

5. max() is a kind of union result using optional output It returns either a single maximum or a array of maximum values (with the coordinates in both cases)

• split this in function into two functions: max1() and max() (some function actually do this in libvips)

Status

• [x] Naming convention
• [ ] VipsImage statelessness
• [x] Optional arguments
• [ ] Optional results
• [x] max variadic result

[warrenseine]

My take:

1. Ok
2. Yes. We probably need copy-on-write for non-mutating functions, but that's just an optimisation.
3. The idiomatic pattern in Java is to overload a method and call the original by passing the default value. Do you see any problem with this approach?
4. Optional results is a really weird pattern. Do you have an example in Vips API which returns "optional output"?
5. From what I read, max() always returns an array. Its size varies depending on the size argument. I don't see why we should make a specific case here. Let's do exactly what the C function does.

[dbouron]

3. Ok, so what about using the "default annotation"? Ideally, it would be worth to have named parameters in Java 4 . Sure:

• https://github.com/libvips/libvips/blob/981d5c4b1695de0e984bb1ef7472b35759b36c8d/libvips/conversion/autorot.c#L237
• https://github.com/libvips/libvips/blob/10c4831a709063189d67b7d6076b1b26bf62b2b2/libvips/iofuncs/system.c#L329

5. According to the doc:

You can read out the position of the maximum with x and y . You can read out arrays of the values and positions of the top size maxima with out_array , x_array and y_array . These values are returned sorted from largest to smallest.

If size is not specified or equals to 1, output values are stored in x and y, in x/y_array otherwise

[warrenseine]

3. This seems to require preprocessing, as annotations in Java can't inject arguments. That's complex, adds a new dependency, and isn't idiomatic anyway. I wouldn't recommend it.

4. Ok, that's an unusual pattern. I don't think we'll get anything better than a ResultRef, but none of these approaches seem idiomatic.

5. In that case, maybe an Either-like structure would be a good pattern?

[dbouron]

3. You're right, patching the lack of named parameter with annotation is not the best approach.
4. Ok, let's s do that. It may be a source of leak with forgotten references which are not garbage collected.
5. +1

[karlvr]

Pardon me for jumping on in on this issue, but this has piqued my interest. I've built an experimental TypeScript program to parse the output of vips -l and vips <op> and to generate Java native stubs and JNI code using the g_blah approach from https://www.libvips.org/API/current/binding.html and it appears promising.

This is an example of the first function I've tried generating:

JNIEXPORT void JNICALL
Java_com_criteo_vips_VipsImage_embed(JNIEnv *env, jobject in, jint x, jint y, jint width, jint height, jobject options)
{
        GValue gvalue = { 0 };

        VipsOperation *op = vips_operation_new("embed");

        // in
        g_value_init( &gvalue, VIPS_TYPE_IMAGE );
        g_value_set_object(&gvalue, (VipsImage *) (*env)->GetLongField(env, in, handle_fid));
        g_object_set_property(G_OBJECT(op), "in", &gvalue);
        g_value_unset(&gvalue);

        // x
        g_value_init(&gvalue, G_TYPE_INT );
        g_value_set_int(&gvalue, x);
        g_object_set_property(G_OBJECT(op), "x", &gvalue);
        g_value_unset(&gvalue);

        // y
        g_value_init(&gvalue, G_TYPE_INT );
        g_value_set_int(&gvalue, y);
        g_object_set_property(G_OBJECT(op), "y", &gvalue);
        g_value_unset(&gvalue);

        // width
        g_value_init(&gvalue, G_TYPE_INT );
        g_value_set_int(&gvalue, width);
        g_object_set_property(G_OBJECT(op), "width", &gvalue);
        g_value_unset(&gvalue);

        // height
        g_value_init(&gvalue, G_TYPE_INT );
        g_value_set_int(&gvalue, height);
        g_object_set_property(G_OBJECT(op), "height", &gvalue);
        g_value_unset(&gvalue);

        // Operation
        VipsOperation *new_op;
        if (!(new_op = vips_cache_operation_build(op))) {
                g_object_unref(op);
                vips_error_exit(NULL); 
        }
        g_object_unref(op);
        op = new_op;

        // Image result
        g_value_init(&gvalue, VIPS_TYPE_IMAGE);
        g_object_get_property(G_OBJECT(op), "out", &gvalue);
        VipsImage *_out = VIPS_IMAGE(g_value_get_object(&gvalue));
        g_object_ref(_out); 
        g_value_unset(&gvalue);
        g_object_unref((VipsImage *) (*env)->GetLongField(env, in, handle_fid));
        (*env)->SetLongField(env, in, handle_fid, (jlong) _out);

        // Free the operation
        vips_object_unref_outputs(VIPS_OBJECT(op)); 
        g_object_unref(op);

}

I'd love to collaborate with you both on this if it piques your interest too. At the moment I think it's quite likely that we can automatically generate most of the implementation.

[karlvr]

A complete thumbnail_image:

JNIEXPORT void JNICALL
Java_com_criteo_vips_VipsImage_thumbnailImageNative(JNIEnv *env, jobject in, jint width, jobject options)
{
        GValue gvalue = { 0 };

        VipsOperation *op = vips_operation_new("thumbnail_image");

        // in
        if (in != NULL) {
                g_value_init(&gvalue, VIPS_TYPE_IMAGE);
                g_value_set_object(&gvalue, (VipsImage *) (*env)->GetLongField(env, in, handle_fid));
                g_object_set_property(G_OBJECT(op), "in", &gvalue);
                g_value_unset(&gvalue);
        }

        // width
        g_value_init(&gvalue, G_TYPE_INT);
        g_value_set_int(&gvalue, width);
        g_object_set_property(G_OBJECT(op), "width", &gvalue);
        g_value_unset(&gvalue);

        // Optionals
        if (options != NULL) {
                jclass optionsCls = (*env)->GetObjectClass(env, options);

                // height
                jfieldID heightFid = (*env)->GetFieldID(env, optionsCls, "height", "Ljava/lang/Integer;");
                jobject heightObjectValue = (*env)->GetObjectField(env, options, heightFid);
                if (heightObjectValue != NULL) {
                        jint height = (*env)->CallIntMethod(env, heightObjectValue, intValue_mid);
                        g_value_init(&gvalue, G_TYPE_INT);
                        g_value_set_int(&gvalue, height);
                        g_object_set_property(G_OBJECT(op), "height", &gvalue);
                        g_value_unset(&gvalue);
                }

                // size
                jfieldID sizeFid = (*env)->GetFieldID(env, optionsCls, "size", "Lcom/criteo/vips/enums/VipsSize;");
                jobject size = (*env)->GetObjectField(env, options, sizeFid);
                if (size != NULL) {
                        jclass sizeCls = (*env)->GetObjectClass(env, size);
                        jfieldID sizeValueFid = (*env)->GetFieldID(env, sizeCls, "value", "I");
                        jint sizeValue = (*env)->GetIntField(env, size, sizeValueFid);
                        g_value_init(&gvalue, G_TYPE_INT);
                        g_value_set_int(&gvalue, sizeValue);
                        g_object_set_property(G_OBJECT(op), "size", &gvalue);
                        g_value_unset(&gvalue);
                }

                // no-rotate
                jfieldID noRotateFid = (*env)->GetFieldID(env, optionsCls, "noRotate", "Ljava/lang/Boolean;");
                jobject noRotateObjectValue = (*env)->GetObjectField(env, options, noRotateFid);
                if (noRotateObjectValue != NULL) {
                        jboolean noRotate = (*env)->CallBooleanMethod(env, noRotateObjectValue, booleanValue_mid);
                        g_value_init(&gvalue, G_TYPE_BOOLEAN);
                        g_value_set_boolean(&gvalue, noRotate);
                        g_object_set_property(G_OBJECT(op), "no-rotate", &gvalue);
                        g_value_unset(&gvalue);
                }

                // crop
                jfieldID cropFid = (*env)->GetFieldID(env, optionsCls, "crop", "Lcom/criteo/vips/enums/VipsInteresting;");
                jobject crop = (*env)->GetObjectField(env, options, cropFid);
                if (crop != NULL) {
                        jclass cropCls = (*env)->GetObjectClass(env, crop);
                        jfieldID cropValueFid = (*env)->GetFieldID(env, cropCls, "value", "I");
                        jint cropValue = (*env)->GetIntField(env, crop, cropValueFid);
                        g_value_init(&gvalue, G_TYPE_INT);
                        g_value_set_int(&gvalue, cropValue);
                        g_object_set_property(G_OBJECT(op), "crop", &gvalue);
                        g_value_unset(&gvalue);
                }

                // linear
                jfieldID linearFid = (*env)->GetFieldID(env, optionsCls, "linear", "Ljava/lang/Boolean;");
                jobject linearObjectValue = (*env)->GetObjectField(env, options, linearFid);
                if (linearObjectValue != NULL) {
                        jboolean linear = (*env)->CallBooleanMethod(env, linearObjectValue, booleanValue_mid);
                        g_value_init(&gvalue, G_TYPE_BOOLEAN);
                        g_value_set_boolean(&gvalue, linear);
                        g_object_set_property(G_OBJECT(op), "linear", &gvalue);
                        g_value_unset(&gvalue);
                }

                // import-profile
                jfieldID importProfileFid = (*env)->GetFieldID(env, optionsCls, "importProfile", "Ljava/lang/String;");
                jstring importProfile = (jstring) (*env)->GetObjectField(env, options, importProfileFid);
                if (importProfile != NULL) {
                        const char *importProfileChars = (*env)->GetStringUTFChars(env, importProfile, NULL);
                        g_value_init(&gvalue, G_TYPE_STRING);
                        g_value_set_string(&gvalue, importProfileChars);
                        (*env)->ReleaseStringUTFChars(env, importProfile, importProfileChars);
                        g_object_set_property(G_OBJECT(op), "import-profile", &gvalue);
                        g_value_unset(&gvalue);
                }

                // export-profile
                jfieldID exportProfileFid = (*env)->GetFieldID(env, optionsCls, "exportProfile", "Ljava/lang/String;");
                jstring exportProfile = (jstring) (*env)->GetObjectField(env, options, exportProfileFid);
                if (exportProfile != NULL) {
                        const char *exportProfileChars = (*env)->GetStringUTFChars(env, exportProfile, NULL);
                        g_value_init(&gvalue, G_TYPE_STRING);
                        g_value_set_string(&gvalue, exportProfileChars);
                        (*env)->ReleaseStringUTFChars(env, exportProfile, exportProfileChars);
                        g_object_set_property(G_OBJECT(op), "export-profile", &gvalue);
                        g_value_unset(&gvalue);
                }

                // intent
                jfieldID intentFid = (*env)->GetFieldID(env, optionsCls, "intent", "Lcom/criteo/vips/enums/VipsIntent;");
                jobject intent = (*env)->GetObjectField(env, options, intentFid);
                if (intent != NULL) {
                        jclass intentCls = (*env)->GetObjectClass(env, intent);
                        jfieldID intentValueFid = (*env)->GetFieldID(env, intentCls, "value", "I");
                        jint intentValue = (*env)->GetIntField(env, intent, intentValueFid);
                        g_value_init(&gvalue, G_TYPE_INT);
                        g_value_set_int(&gvalue, intentValue);
                        g_object_set_property(G_OBJECT(op), "intent", &gvalue);
                        g_value_unset(&gvalue);
                }

        }
        // Operation
        VipsOperation *new_op;
        if (!(new_op = vips_cache_operation_build(op))) {
                g_object_unref(op);
                throwVipsException(env, "thumbnail_image failed");
                return;
        }
        g_object_unref(op);
        op = new_op;

        // Mutating image result
        g_value_init(&gvalue, VIPS_TYPE_IMAGE);
        g_object_get_property(G_OBJECT(op), "out", &gvalue);
        VipsImage *_out = VIPS_IMAGE(g_value_get_object(&gvalue));
        g_object_ref(_out); 
        g_value_unset(&gvalue);
        g_object_unref((VipsImage *) (*env)->GetLongField(env, in, handle_fid));
        (*env)->SetLongField(env, in, handle_fid, (jlong) _out);

        // Free the operation
        vips_object_unref_outputs(VIPS_OBJECT(op)); 
        g_object_unref(op);

}