VipsImage ref count in vips7 old style filters

[DavidStorm]

I’m updating libvips from 8.12 to last 8.15 version.
I have a lot of filters in old vips7 style like:

static int
neo_posterize_gen( VipsRegion *or, void *s, void *a, void * b, gboolean* stop )
{
    VipsRegion *ir = (VipsRegion *)s;
    VipsImage *in = (VipsImage *)a;
    st_neo_posterize * pSt = (st_neo_posterize *)b;    
	
	......

	return( 0 );
}

int
neo_posterize( VipsImage *in, VipsImage *out, double levels )
{
	st_neo_posterize * pSt=NULL;
		
	if( vips_image_pio_input( in ) )
		return( -1 );
	
	if( im_cp_desc( out, in ) ) 
		return( -1 );
	
	if( !(pSt = VIPS_NEW( out, st_neo_posterize )) )
		return( -1 );
	
	pSt->levels = levels;
	
	if( neovips_demand_hint( out, VIPS_DEMAND_STYLE_THINSTRIP, in, NULL ) )
		return( -1 );
	
	if( vips_image_generate( out, vips_start_one, neo_posterize_gen, vips_stop_one, in, pSt ) )
		return( -1 );
	
	return( 0 );
}

but we don’t connect both images and reference count as new style vips8.

In this sequence:

void main()
{
	VipsImage* out = vips_image_new();
	....

	VipsImage* in = vips_image_new_from_file("Sample.png", nullptr);
	neo_posterize(in, out, 2.0);
	g_object_unref(in);

	....

	vips_image_write_to_file(out, "Post.png", nullptr);
	g_object_unref(out);
}

function vips_image_write_to_file() crashes because in and out are not connected.

How can I connect ‘in' image to ‘out’ image and ref count to process or save ‘out’ in other part of my application?.

I saw functions as vips__object_set_member() or vips_operation_set_valist_required() used to connect in-out images in vips filters, but is not clear how to use.

[jcupitt]

Hi @DavidStorm,

We had to change this bit of API, unfortunately. The new thing is vips_image_pipeline_array().

https://www.libvips.org/API/current/libvips-generate.html#vips-image-pipeline-array

With vips_image_pipelinev() as the varargs convenience function. Call that instead of neovips_demand_hint() and you should be OK (I think).

This new call does a load of things:

• constructs the demand hints
• copies metadata correctly down the pipeline
• sorts image dependencies
• builds the tree of up/down connections in the pipeline

[DavidStorm]

Hi @jcupitt

I tried to use vips_image_pipeline_array(), but seems not enough. Some other filters uses im_wrapmany() that calls the same connection functions and does not work.

This is a sample code:

int
neo_LabQ2LabS( VipsImage *labim, VipsImage *outim )
{
	if( im_cp_desc( outim, labim ) )
		return( -1 );
	outim->Bands = labim->Bands-1;
	outim->Type = VIPS_INTERPRETATION_LABS;
	outim->BandFmt = VIPS_FORMAT_SHORT;
	outim->Bbits = IM_BBITS_SHORT;
	outim->Coding = VIPS_CODING_NONE;
    VIPS_SETSTR( outim->filename, labim->filename );

	if( im_wrapone( labim, outim, (im_wrapone_fn)(outim->Bands == 3 ? _labQ2LabS : _labQ2LabS_withalpha ),
		labim, outim) )
		return( -1 );
	 
	return( 0 );
}

The function im_wrapone() uses im_wrapmany():

int
im_wrapmany( IMAGE **in, IMAGE *out, im_wrapmany_fn fn, void *a, void *b )
{
	...

	vips__demand_hint_array( out, VIPS_DEMAND_STYLE_THINSTRIP, in );
	if( vips__reorder_set_input( out, in ) )
		return( -1 ); 
	...
}

a similar execution as vips_image_pipeline_array() without copy fields:

int 
vips_image_pipeline_array( VipsImage *image, 
	VipsDemandStyle hint, VipsImage **in )
{
	vips__demand_hint_array( image, hint, in );

	if( in[0] && 
		vips__image_copy_fields_array( image, in ) )
		return( -1 ); 

	if( vips__reorder_set_input( image, in ) )
		return( -1 ); 

	return( 0 );
}

Is it possible the problem is use VImage objets as wrapper of this filters?

VImage neoLabQ2LabPS(VImage* input) noexcept(false)
{
	VImage output;

	int errorCode = neo_LabQ2LabPS(input->image(), output.image());
	THROW_ERROR_IF_FAIL(errorCode);

	return( output );
}

void main()
{
	VImage in(“Sample.png");
	in = neoLabQ2LabPS(&in);
}

Thanks in advanced.

[jcupitt]

I'd think that should work, but it's hard to tell from snippets of code, perhaps something else is happening.

Could you make a complete, runnable example that shows the problem? Then I can test it instead of trying to fix it by looking at it.

[DavidStorm]

Sure.

#include <vips/vips8>
#include <vips/vips7compat.h>

void
_Q2S(unsigned char* in, signed short* out, int n, void* ap, void* bp)
{
		unsigned char* p = (unsigned char*)in;
		signed short* q = (signed short*)out;

		int i;
		unsigned char ext;
		signed short l, a, b;

		for (i = 0; i < n; i++) {
			/* Get most significant 8 bits of lab.
			 */
			l = p[0] << 7;
			a = p[1] << 8;
			b = p[2] << 8;

			/* Get x-tra bits.
			 */
			ext = p[3];
			p += 4;

			/* Shift and mask in to lab.
			 */
			l |= (unsigned char)(ext & 0xc0) >> 1;
			a |= (ext & 0x38) << 2;
			b |= (ext & 0x7) << 5;

			/* Write!
			 */
			q[0] = l;
			q[1] = a;
			q[2] = b;
			q += 3;
		}
}

int
_labQ2LabS(VipsImage* labim, VipsImage* outim)
{
	if (labim->Coding != VIPS_CODING_LABQ) {
		vips_error("neo_LabQ2LabS", " not a packed Lab image");
		return(-1);
	}

	if (im_cp_desc(outim, labim))
		return(-1);
	outim->Bands = labim->Bands - 1;
	outim->Type = VIPS_INTERPRETATION_LABS;
	outim->BandFmt = VIPS_FORMAT_SHORT;
	outim->Bbits = IM_BBITS_SHORT;
	outim->Coding = VIPS_CODING_NONE;
	VIPS_SETSTR(outim->filename, labim->filename);

	if (im_wrapone(labim, outim, (im_wrapone_fn)(_Q2S),
		labim, outim))
		return(-1);

	return(0);
}

int
main( int argc, char **argv )
{
	int errorCode;
	char srcPath[512] = "../Common/UnitTests/Images/ImageLABQ.tif";
	char dstPath[512] = "../Common/UnitTests/Output/ImageLABS.tif";

	vips::VImage dst = vips_image_new();
	
	try
	{
		vips::VImage src = vips_image_new();
		if (vips_format_read(srcPath, src.get_image()))
			throw(-1);

		errorCode = _labQ2LabS(src.get_image(), dst.get_image());
		if (errorCode)
			throw(-1);
	}
	catch(...)
	{
	}

	errorCode = vips_image_write_to_file(dst.get_image(), dstPath, nullptr);
	if (errorCode)
		;
}

ImageLABQ.tif.zip

[jcupitt]

I think you just need to call vips_init(). I changed your main() to:

int
main( int argc, char **argv )
{
        int errorCode;

        if (VIPS_INIT(argv[0]))
                throw(-1);

        vips::VImage src = vips_image_new();
        if (vips_format_read(argv[1], src.get_image()))
                throw(-1);

        vips::VImage dst = vips_image_new();
        errorCode = _labQ2LabS(src.get_image(), dst.get_image());
        if (errorCode)
                throw(-1);

        errorCode = vips_image_write_to_file(dst.get_image(), argv[2], nullptr);
        if (errorCode)
                throw(-1);

        return(0);
}

and it seems to work for me.

[DavidStorm]

Yes, I forget vips_init(), but this is a small sample. Our VImage objects comes from other functions in a more expense code.
Please, try with this small change:

int
main( int argc, char **argv )
{
        int errorCode;

        if (VIPS_INIT(argv[0]))
                return(-1);

        vips::VImage dst = vips_image_new();

        if (true)
        {
                vips::VImage src = vips_image_new();
                if (vips_format_read(argv[1], src.get_image()))
                        return(-1);

                errorCode = _labQ2LabS(src.get_image(), dst.get_image());
                if (errorCode)
                        return(-1);
        }

        errorCode = vips_image_write_to_file(dst.get_image(), argv[2], nullptr);
        if (errorCode)
                throw(-1);

        return(0);
}

After execute the conditional, src image is destroyed but must be alive a refcount result of the filter _labQ2LabS, but is not like this. The function vips__demand_hint_array don’t increment the refcount and it’s not possible write the image.

[jcupitt]

I'll have a look.

(you need to mark your code up like this:

    ```C
    int
    main( int argc, char **argv )
    {
        int errorCode;

        if (VIPS_INIT(argv[0]))
            throw(-1);
    etc.
    ```

to get it to display correctly ... a single back tick won't work)

[jcupitt]

I don't think vips7 ever did refcounting, did it? That's one of the things we added in vips8 when we moved everything on top of gobject.

You're mixing the vips8 C++ API (which uses vips8 refcounts) with the old vips7 API (no refcounts) and it's getting tangled up :(

I would just use vips7 or just use vips8 and not mix them.

[jcupitt]

Oh, or use the vips7 C++ API, but we don't support that any more, unfortunately.

So ... I think I would suggest sticking with C++, but just just using the vips7 C API.

[DavidStorm]

Ok. It seems clear we have to update our filters from vips7 to vips8.

Thanks for your help.

[jcupitt]

vips7 stuff should still work -- nip2 (for example) has some built in vips7 filters and they work fine. You just can't mix the vips7 and vuips8 APIs (unless you are extremely careful).

[jcupitt]

If I change your main() to be:

int
main( int argc, char **argv )
{
        int errorCode;

        if (VIPS_INIT(argv[0]))
                return(-1);

        IMAGE *src = im_open("temp", "p");

        if (vips_format_read(argv[1], src))
                return(-1);

        IMAGE *dst = im_open("temp", "p");

        errorCode = _labQ2LabS(src, dst);
        if (errorCode)
                return(-1);

        im_close(src);

        errorCode = vips_image_write_to_file(dst, argv[2], nullptr);
        if (errorCode)
                throw(-1);

        im_close(dst);

        return(0);
}

It still works. It can write dst to the output, even after src has been closed.

[DavidStorm]

Good to know. Thanks.