diff --git a/TODO b/TODO index 3b99796e..99fa1cce 100644 --- a/TODO +++ b/TODO @@ -1,3 +1,26 @@ +- call: + + vips_object_unref_outputs( object ); + + in more places .. a good test is + + vips copy wtc.jpg x.v + + where wtc.jpg is an empty file + + we need to catch all the error paths out of VipsForeign + + would catching in vips_call_split() be enough? + + + + +- we also have vips_argument_dispose_all() and call it from vips_call() + vips_call_split(), does this do the same job? + + + + - test vips_foreign_load_vips_get_flags(), sense inverted? we have various printf()s left in the byteswap codepath diff --git a/libvips/arithmetic/arithmetic.c b/libvips/arithmetic/arithmetic.c index 8aa13847..267d0262 100644 --- a/libvips/arithmetic/arithmetic.c +++ b/libvips/arithmetic/arithmetic.c @@ -311,11 +311,12 @@ vips_arithmetic_build( VipsObject *object ) printf( "\n" ); #endif /*DEBUG*/ - g_object_set( arithmetic, "out", vips_image_new(), NULL ); - - if( VIPS_OBJECT_CLASS( vips_arithmetic_parent_class )->build( object ) ) + if( VIPS_OBJECT_CLASS( vips_arithmetic_parent_class )-> + build( object ) ) return( -1 ); + g_object_set( arithmetic, "out", vips_image_new(), NULL ); + /* No need to check input bands, bandalike will do this for us. */ if( arithmetic->n > MAX_INPUT_IMAGES ) { @@ -326,9 +327,9 @@ vips_arithmetic_build( VipsObject *object ) for( i = 0; i < arithmetic->n; i++ ) if( vips_image_pio_input( arithmetic->in[i] ) || vips_check_uncoded( "VipsArithmetic", - arithmetic->in[i] ) ) + arithmetic->in[i] ) ) return( -1 ); - if( vips_image_pio_output( arithmetic->out ) ) + if( vips_image_pio_output( arithmetic->out ) ) return( -1 ); format = (VipsImage **) @@ -343,14 +344,14 @@ vips_arithmetic_build( VipsObject *object ) if( vips__formatalike_vec( arithmetic->in, format, arithmetic->n ) || vips__bandalike_vec( "VipsArithmetic", format, band, arithmetic->n, arithmetic->base_bands ) || - vips__sizealike_vec( band, size, arithmetic->n ) ) + vips__sizealike_vec( band, size, arithmetic->n ) ) return( -1 ); /* Keep a copy of the processed images here for subclasses. */ arithmetic->ready = size; - if( vips_image_copy_fields_array( arithmetic->out, size ) ) + if( vips_image_copy_fields_array( arithmetic->out, size ) ) return( -1 ); vips_demand_hint_array( arithmetic->out, VIPS_DEMAND_STYLE_THINSTRIP, size ); @@ -360,7 +361,7 @@ vips_arithmetic_build( VipsObject *object ) if( vips_image_generate( arithmetic->out, vips_start_many, vips_arithmetic_gen, vips_stop_many, - arithmetic->ready, arithmetic ) ) + arithmetic->ready, arithmetic ) ) return( -1 ); return( 0 ); diff --git a/libvips/foreign/foreign.c b/libvips/foreign/foreign.c index 75160a87..68627f83 100644 --- a/libvips/foreign/foreign.c +++ b/libvips/foreign/foreign.c @@ -492,10 +492,8 @@ vips_foreign_load_start_cb( VipsImage *out, void *a, void *dummy ) /* Read the image in. */ if( class->load( load ) || - vips_image_pio_input( load->real ) ) { - VIPS_UNREF( load->real ); + vips_image_pio_input( load->real ) ) return( NULL ); - } } return( vips_region_new( load->real ) ); @@ -530,8 +528,6 @@ vips_foreign_load_build( VipsObject *object ) VipsForeignLoad *load = VIPS_FOREIGN_LOAD( object ); VipsForeignLoadClass *class = VIPS_FOREIGN_LOAD_GET_CLASS( object ); - g_object_set( object, "out", vips_image_new(), NULL ); - if( class->get_flags && class->get_flags( load ) ) return( -1 ); @@ -540,10 +536,12 @@ vips_foreign_load_build( VipsObject *object ) build( object ) ) return( -1 ); + g_object_set( object, "out", vips_image_new(), NULL ); + /* Read the header into @out. */ if( class->header && - class->header( load ) ) + class->header( load ) ) return( -1 ); /* If there's no ->load() method then the header read has done @@ -565,7 +563,7 @@ vips_foreign_load_build( VipsObject *object ) vips_foreign_load_start_cb, vips_foreign_load_generate_cb, vips_stop_one, - load, NULL ) ) + load, NULL ) ) return( -1 ); } diff --git a/libvips/include/vips/object.h b/libvips/include/vips/object.h index 7d5cd439..d03e58d6 100644 --- a/libvips/include/vips/object.h +++ b/libvips/include/vips/object.h @@ -483,6 +483,8 @@ void vips_object_sanity_all( void ); void vips_object_rewind( VipsObject *object ); +void vips_object_unref_outputs( VipsObject *object ); + #ifdef __cplusplus } #endif /*__cplusplus*/ diff --git a/libvips/iofuncs/object.c b/libvips/iofuncs/object.c index 524edcb5..534f2b09 100644 --- a/libvips/iofuncs/object.c +++ b/libvips/iofuncs/object.c @@ -1998,3 +1998,42 @@ vips_object_sanity_all( void ) vips_object_map( (VipsSListMap2Fn) vips_object_sanity_all_cb, NULL, NULL ); } + +static void * +vips_object_unref_outputs_sub( VipsObject *object, + GParamSpec *pspec, + VipsArgumentClass *argument_class, + VipsArgumentInstance *argument_instance, + void *a, void *b ) +{ + if( (argument_class->flags & VIPS_ARGUMENT_OUTPUT) && + G_IS_PARAM_SPEC_OBJECT( pspec ) && + argument_instance->assigned ) { + GObject *value; + + g_object_get( object, + g_param_spec_get_name( pspec ), &value, NULL ); + + /* Doing the get refs the object, so unref the get, then unref + * again since this an an output object of the operation. + */ + g_object_unref( value ); + g_object_unref( value ); + } + + return( NULL ); +} + +/* Unref all assigned output objects. + * + * After an object is built, all output args are owned by the caller. If + * something goes wrong before then, we have to unref the outputs that have + * been made so far. And this function can also be useful for callers when + * they've finished processing outputs themselves. + */ +void +vips_object_unref_outputs( VipsObject *object ) +{ + (void) vips_argument_map( object, + vips_object_unref_outputs_sub, NULL, NULL ); +} diff --git a/libvips/iofuncs/operation.c b/libvips/iofuncs/operation.c index 62da72ff..aa7c9dc7 100644 --- a/libvips/iofuncs/operation.c +++ b/libvips/iofuncs/operation.c @@ -851,36 +851,12 @@ vips_call_argv_output( VipsObject *object, return( NULL ); } -static void * -vips_call_argv_unref_output( VipsObject *object, - GParamSpec *pspec, - VipsArgumentClass *argument_class, - VipsArgumentInstance *argument_instance, - void *a, void *b ) -{ - if( (argument_class->flags & VIPS_ARGUMENT_OUTPUT) && - G_IS_PARAM_SPEC_OBJECT( pspec ) && - argument_instance->assigned ) { - GObject *value; - - g_object_get( object, - g_param_spec_get_name( pspec ), &value, NULL ); - - /* Doing the get refs the object, so unref the get, then unref - * again since this an an output object of the operation. - */ - g_object_unref( value ); - g_object_unref( value ); - } - - return( NULL ); -} - /* Our main command-line entry point. Optional args should have been set by * the GOption parser already, see above. * * We don't create the operation, so we must not unref it. The caller must - * unref on error too. + * unref on error too. The caller must also call vips_object_unref_outputs() on + * all code paths. */ int vips_call_argv( VipsOperation *operation, int argc, char **argv ) @@ -907,15 +883,8 @@ vips_call_argv( VipsOperation *operation, int argc, char **argv ) call.i = 0; if( vips_argument_map( VIPS_OBJECT( operation ), - vips_call_argv_input, &call, NULL ) ) { - /* We must unref any output objects, they are holding refs to - * the operation. - */ - (void) vips_argument_map( VIPS_OBJECT( operation ), - vips_call_argv_unref_output, NULL, NULL ); - + vips_call_argv_input, &call, NULL ) ) return( -1 ); - } /* Any unused arguments? We must fail. Consider eg. "vips bandjoin a b * c". This would overwrite b with a and ignore c, potentially @@ -924,37 +893,16 @@ vips_call_argv( VipsOperation *operation, int argc, char **argv ) if( argc > call.i ) { vips_error( VIPS_OBJECT_GET_CLASS( operation )->nickname, "%s", _( "too many arguments" ) ); - - /* We must unref any output objects, they are holding refs to - * the operation. - */ - (void) vips_argument_map( VIPS_OBJECT( operation ), - vips_call_argv_unref_output, NULL, NULL ); - return( -1 ); } - if( vips_object_build( VIPS_OBJECT( operation ) ) ) { - /* We must unref any output objects, they are holding refs to - * the operation. - */ - (void) vips_argument_map( VIPS_OBJECT( operation ), - vips_call_argv_unref_output, NULL, NULL ); - + if( vips_object_build( VIPS_OBJECT( operation ) ) ) return( -1 ); - } call.i = 0; if( vips_argument_map( VIPS_OBJECT( operation ), - vips_call_argv_output, &call, NULL ) ) { - (void) vips_argument_map( VIPS_OBJECT( operation ), - vips_call_argv_unref_output, NULL, NULL ); - + vips_call_argv_output, &call, NULL ) ) return( -1 ); - } - - (void) vips_argument_map( VIPS_OBJECT( operation ), - vips_call_argv_unref_output, NULL, NULL ); return( 0 ); } diff --git a/tools/vips.c b/tools/vips.c index b71931eb..2f42ba80 100644 --- a/tools/vips.c +++ b/tools/vips.c @@ -1075,10 +1075,13 @@ main( int argc, char **argv ) if( argc == 1 ) vips_object_print( VIPS_OBJECT( operation ) ); + vips_object_unref_outputs( VIPS_OBJECT( operation ) ); g_object_unref( operation ); + error_exit( NULL ); } + vips_object_unref_outputs( VIPS_OBJECT( operation ) ); g_object_unref( operation ); handled = TRUE;