harvard-lts / kakadu-vips

Kakadu JP2 reader and writer for libvips
Apache License 2.0
3 stars 0 forks source link

`kakadusave_buffer` method never completes #21

Closed quoideneuf closed 4 weeks ago

quoideneuf commented 1 month ago

It appears that the tests are currently failing at this line:

https://github.com/harvard-lts/kakadu-vips/blob/main/test/test_kakadusave.py#L45

The tests do not fail, but the process fails to complete after this line and must be killed. If this line is commented out, the same thing will happen at this line:

https://github.com/harvard-lts/kakadu-vips/blob/main/test/test_kakadusave.py#L65

So possibly there is some issue with calling the method more than once time with the Qfactor option?

quoideneuf commented 1 month ago

@jcupitt I wonder if the issue is here: https://github.com/harvard-lts/kakadu-vips/blob/8addb12c243501fbb499b6136ebab96feb9ada53/src/kakadusave.cpp#L417

From the inline docs for parse_string:

           In several respects, attribute parsing is unlike the other two
           methods provided for setting parameter attributes (i.e., the `set'
           function and marker segment translation via the
           `translate_marker_segment' function).  Firstly, it is illegal to
           parse the same attribute twice -- an appropriate error message
           will be generated through `kdu_error' if information has already
           been parsed into the attribute (but not if it has been set using
           `set' or `translate_marker_segment').  The only exception to this
           rule applies to objects which support multiple instances, in which
           case a new instance of the object will be created automatically to
           accommodate multiple occurrences of the same attribute string.

However, no error is raised when code like this appears in a test:

q1 = self.ppm.kakadusave_buffer(options="Qfactor=10")
q2 = self.ppm.kakadusave_buffer(options="Qfactor=20")

The process simply freezes when it hits the second line.

scossu commented 1 month ago

@jcupitt did you have a chance to look into this issue? Thanks.

jcupitt commented 4 weeks ago

Sorry for the delay in responding, I've been trapped in a big deadline on another project.

I'll look at this in the next day or so.

jcupitt commented 4 weeks ago

This (huge) commit seems to be the cause:

https://github.com/harvard-lts/kakadu-vips/commit/8addb12c243501fbb499b6136ebab96feb9ada53

I made a small C test program to make this easier to debug:

// compile with
//  gcc -g -Wall kakadusave_options.c `pkg-config vips --cflags --libs`

#include <vips/vips.h>

// basic C wrapper for kakdusave_buffer
static int
vips_kakadusave_buffer(VipsImage *in, void **buf, size_t *len, ...)
{
    va_list ap;
    VipsArea *area;
    int result;

    area = NULL;

    va_start(ap, len);
    result = vips_call_split("kakadusave_buffer", ap, in, &area);
    va_end(ap);

    if (!result &&
        area) {
        if (buf) {
            *buf = area->data;
            area->free_fn = NULL;
        }
        if (len)
            *len = area->length;

        vips_area_unref(area);
    }

    return result;
}

int
main(int argc, char **argv)
{   
    if (VIPS_INIT(argv[0]))
        vips_error_exit(NULL);

    VipsImage *image;
    if (!(image = vips_image_new_from_file(argv[1], NULL)))
        vips_error_exit(NULL);

    void *buf;
    size_t len;

    printf("kakdusave_buffer, no options ...\n");
    if (vips_kakadusave_buffer(image, &buf, &len, NULL))
        vips_error_exit(NULL);
    printf("\t(saved as %zd bytes of jp2k)\n", len);
    g_free(buf);

    printf("kakdusave_buffer, options=\"Qfactor=45\" ...\n");
    if (vips_kakadusave_buffer(image, &buf, &len, 
        "options", "Qfactor=45",
        NULL))
        vips_error_exit(NULL);
    printf("vips_kakadusave_buffer: saved as %zd bytes of jp2k\n", len);
    g_free(buf);

    printf("kakdusave_buffer, options=\"Qfactor=45\" ...\n");
    if (vips_kakadusave_buffer(image, &buf, &len,
        "options", "Qfactor=45",
        NULL))
        vips_error_exit(NULL);
    printf("vips_kakadusave_buffer: saved as %zd bytes of jp2k\n", len);
    g_free(buf);

    g_object_unref(image);

    return 0;
}

I see:

$ ./a.out ~/pics/k2.jpg
kakdusave_buffer, no options ...
    (saved as 1050903 bytes of jp2k)
kakdusave_buffer, options="Qfactor=45" ...

** (process:103496): WARNING **: 15:43:17.904: Kakadu Core Warning:
The `Qweights' parameter values supplied are not a close match to the
square-root of the synthesis energy gains of the prevailing multi-component
transform, if any, in tile 0.  Use the following parameters: 1.732051,
1.805108, 1.573402.

vips_kakadusave_buffer: saved as 84268 bytes of jp2k
kakdusave_buffer, options="Qfactor=45" ...

And it gets stuck, I'll investigate.

Thanks for the report, @quoideneuf!

jcupitt commented 4 weeks ago

I think I found it -- we were not chaining up in message flush, so the message mutex was not being released, leading to a deadlock.

I've made a PR, let's continue any discussion there.