From 3920f5dc7e3d4f5d65cefa318d340a40065095a9 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 16 Jun 2011 13:54:13 +0100 Subject: [PATCH] get im_system_image() working again chop stuff about to get system image working again. --- TODO | 1 - libvips/conversion/im_system_image.c | 14 +++--- libvips/include/vips/image.h | 14 +++++- libvips/include/vips/object.h | 2 +- libvips/iofuncs/image.c | 49 ++++++++++++++------ libvips/iofuncs/object.c | 68 ++++++++++++++++++++++------ libvips/iofuncs/operation.c | 4 +- 7 files changed, 109 insertions(+), 43 deletions(-) diff --git a/TODO b/TODO index 22c3e6c1..6dc81085 100644 --- a/TODO +++ b/TODO @@ -1,5 +1,4 @@ - - revisit orc conv use an 8.8 accumulator ... build the scale into the 8.8 coeffs ... no div at diff --git a/libvips/conversion/im_system_image.c b/libvips/conversion/im_system_image.c index f1cfeca6..0c505e69 100644 --- a/libvips/conversion/im_system_image.c +++ b/libvips/conversion/im_system_image.c @@ -75,7 +75,9 @@ system_image( IMAGE *im, if( !vips_buf_appends( &buf, line ) ) break; - result = pclose( fp ); + if( (result = pclose( fp )) ) + im_error( "im_system_image", + _( "command failed: \"%s\"" ), cmd_format ); if( log ) *log = im_strdup( NULL, vips_buf_all( &buf ) ); @@ -152,6 +154,7 @@ im_system_image( IMAGE *im, if( system_image( im, in_image, out_name, cmd_format, log ) ) { im_close( in_image ); g_free( out_name ); + g_unlink( out_name ); return( NULL ); } @@ -159,18 +162,13 @@ im_system_image( IMAGE *im, if( !(out = im_open( out_name, "r" )) ) { g_free( out_name ); - - return( NULL ); - } - if( im_add_postclose_callback( out, - (im_callback_fn) unlink, out->filename, NULL ) ) { - g_free( out_name ); - im_close( out ); g_unlink( out_name ); return( NULL ); } g_free( out_name ); + vips_image_set_delete_on_close( out, TRUE ); + return( out ); } diff --git a/libvips/include/vips/image.h b/libvips/include/vips/image.h index 5ef80d4a..af90fd74 100644 --- a/libvips/include/vips/image.h +++ b/libvips/include/vips/image.h @@ -197,7 +197,7 @@ typedef struct _VipsImage { size_t length; /* size of mmap area */ guint32 magic; /* magic from header, endian-ness of image */ - /* Partial image stuff. All private! All these fields are initialised + /* Partial image stuff. All these fields are initialised * to NULL and ignored unless set by vips_image_generate() etc. */ void *(*start)(); /* user-supplied start function */ @@ -264,6 +264,16 @@ typedef struct _VipsImage { */ gboolean hint_set; + /* Delete-on-close is hard to do with signals and callbacks since we + * really need to do this in finalize after the fd has been closed, + * but you can't emit signals then. + * + * Also keep a private copy of the filename string to be deleted, + * since image->filename will be freed in _dispose(). + */ + gboolean delete_on_close; + char *delete_on_close_filename; + } VipsImage; typedef struct _VipsImageClass { @@ -363,6 +373,8 @@ VipsImage *vips_image_new_from_file_raw( const char *filename, int xsize, int ysize, int bands, int offset ); VipsImage *vips_image_new_from_memory( void *buffer, int xsize, int ysize, int bands, VipsBandFormat bandfmt ); +void vips_image_set_delete_on_close( VipsImage *image, + gboolean delete_on_close ); VipsImage *vips_image_new_disc_temp( const char *format ); int vips_image_write( VipsImage *image, const char *filename ); diff --git a/libvips/include/vips/object.h b/libvips/include/vips/object.h index 547bcb56..81f04cfe 100644 --- a/libvips/include/vips/object.h +++ b/libvips/include/vips/object.h @@ -160,7 +160,7 @@ typedef void *(*VipsArgumentMapFn)( VipsObject *, GParamSpec *, VipsArgumentClass *, VipsArgumentInstance *, void *a, void *b ); void *vips_argument_map( VipsObject *object, VipsArgumentMapFn fn, void *a, void *b ); -void vips_argument_free_all( VipsObject *object ); +void vips_argument_dispose_all( VipsObject *object ); /* We have to loop over an objects args in several places, and we can't always * use vips_argument_map(), the preferred looper. Have the loop code as a diff --git a/libvips/iofuncs/image.c b/libvips/iofuncs/image.c index fe6d9f8b..7f6341c2 100644 --- a/libvips/iofuncs/image.c +++ b/libvips/iofuncs/image.c @@ -346,8 +346,7 @@ vips_image_finalize( GObject *gobject ) (void) vips__writehist( image ); if( close( image->fd ) == -1 ) vips_error( "VipsImage", - _( "unable to close fd for %s" ), - image->filename ); + "%s", _( "unable to close fd" ) ); image->fd = -1; } @@ -366,8 +365,15 @@ vips_image_finalize( GObject *gobject ) image->data = NULL; } - VIPS_FREE( image->filename ); - VIPS_FREE( image->mode ); + if( image->delete_on_close ) { + g_assert( image->delete_on_close_filename ); + + VIPS_DEBUG_MSG( "vips_image_finalize: removing temp %s\n", + image->delete_on_close_filename ); + g_unlink( image->delete_on_close_filename ); + VIPS_FREE( image->delete_on_close_filename ); + image->delete_on_close = FALSE; + } VIPS_FREEF( g_mutex_free, image->sslock ); @@ -542,7 +548,7 @@ vips_image_rewind( VipsObject *object ) char *filename; char *mode; - /* The old values for filename and mode become the new defaults. + /* This triggers a dispose. Copy filename/mode across the dispose. */ filename = g_strdup( vips_image_get_filename( image ) ); mode = g_strdup( vips_image_get_mode( image ) ); @@ -1454,7 +1460,7 @@ vips_image_set_kill( VipsImage *image, gboolean kill ) image->kill = kill; } -/* Make a name for a filename-less image. Use immediately, don'#t free the +/* Make a name for a filename-less image. Use immediately, don't free the * result. */ static const char * @@ -1747,12 +1753,29 @@ vips_image_new_from_memory( void *buffer, return( image ); } -static void -vips_image_new_temp_cb( VipsImage *image ) +/** + * vips_image_set_delete_on_close: + * @image: image to set + * @delete_on_close: format of file + * + * Sets the delete_on_close flag for the image. If this flag is set, when + * @image is finalized the filename held in @image->filename at the time of + * this call is unlinked. + * + * This function is clearly extremely dangerous, use with great caution. + * + * See also: vips__temp_name(), vips_image_new_disc_temp(). + */ +void +vips_image_set_delete_on_close( VipsImage *image, gboolean delete_on_close ) { - g_assert( image->filename ); + VIPS_DEBUG_MSG( "vips_image_set_delete_on_close: %d %s\n", + delete_on_close, image->filename ); - g_unlink( image->filename ); + image->delete_on_close = delete_on_close; + VIPS_FREE( image->delete_on_close_filename ); + if( delete_on_close ) + VIPS_SETSTR( image->delete_on_close_filename, image->filename ); } /** @@ -1783,11 +1806,7 @@ vips_image_new_disc_temp( const char *format ) } g_free( name ); - /* Needs to be postclose so we can rewind after write without - * deleting the file. - */ - g_signal_connect( image, "postclose", - G_CALLBACK( vips_image_new_temp_cb ), NULL ); + vips_image_set_delete_on_close( image, TRUE ); return( image ); } diff --git a/libvips/iofuncs/object.c b/libvips/iofuncs/object.c index 72ab9282..252bab22 100644 --- a/libvips/iofuncs/object.c +++ b/libvips/iofuncs/object.c @@ -395,27 +395,21 @@ vips_object_dispose_argument( VipsObject *object, GParamSpec *pspec, VipsArgumentInstance *argument_instance, void *a, void *b ) { -#ifdef DEBUG - printf( "vips_object_dispose_argument: " ); - vips_object_print_name( object ); - printf( ".%s\n", g_param_spec_get_name( pspec ) ); -#endif /*DEBUG*/ - g_assert( ((VipsArgument *) argument_class)->pspec == pspec ); g_assert( ((VipsArgument *) argument_instance)->pspec == pspec ); - if( G_IS_PARAM_SPEC_STRING( pspec ) ) { - char **member = &G_STRUCT_MEMBER( char *, object, - argument_class->offset ); - - VIPS_FREE( *member ); - } - else if( G_IS_PARAM_SPEC_OBJECT( pspec ) ) + if( G_IS_PARAM_SPEC_OBJECT( pspec ) ) vips_object_clear_object( object, pspec ); else if( G_IS_PARAM_SPEC_BOXED( pspec ) ) { gpointer *member = &G_STRUCT_MEMBER( gpointer, object, argument_class->offset ); +#ifdef DEBUG + printf( "vips_object_dispose_argument: " ); + vips_object_print_name( object ); + printf( ".%s\n", g_param_spec_get_name( pspec ) ); +#endif /*DEBUG*/ + if( *member ) { g_boxed_free( G_PARAM_SPEC_VALUE_TYPE( pspec ), *member ); @@ -429,6 +423,48 @@ vips_object_dispose_argument( VipsObject *object, GParamSpec *pspec, /* Free all args on this object which may be holding resources. */ void +vips_argument_dispose_all( VipsObject *object ) +{ +#ifdef DEBUG + printf( "vips_argument_dispose_all: " ); + vips_object_print_name( object ); + printf( "\n" ); +#endif /*DEBUG*/ + + vips_argument_map( object, vips_object_dispose_argument, NULL, NULL ); +} + +/* Free any args which are holding memory. + */ +static void * +vips_object_free_argument( VipsObject *object, GParamSpec *pspec, + VipsArgumentClass *argument_class, + VipsArgumentInstance *argument_instance, + void *a, void *b ) +{ + g_assert( ((VipsArgument *) argument_class)->pspec == pspec ); + g_assert( ((VipsArgument *) argument_instance)->pspec == pspec ); + + if( G_IS_PARAM_SPEC_STRING( pspec ) ) { + char **member = &G_STRUCT_MEMBER( char *, object, + argument_class->offset ); + +#ifdef DEBUG + printf( "vips_object_free_argument: " ); + vips_object_print_name( object ); + printf( ".%s\n", g_param_spec_get_name( pspec ) ); +#endif /*DEBUG*/ + + VIPS_FREE( *member ); + } + + return( NULL ); +} + +/* Free args which hold memory. Things like strings need to be freed right at + * the end in case anyone is still using them. + */ +static void vips_argument_free_all( VipsObject *object ) { #ifdef DEBUG @@ -437,7 +473,7 @@ vips_argument_free_all( VipsObject *object ) printf( "\n" ); #endif /*DEBUG*/ - vips_argument_map( object, vips_object_dispose_argument, NULL, NULL ); + vips_argument_map( object, vips_object_free_argument, NULL, NULL ); } static void @@ -466,7 +502,7 @@ vips_object_dispose( GObject *gobject ) /* Clear all our arguments: they may be holding refs we should drop. */ - vips_argument_free_all( object ); + vips_argument_dispose_all( object ); VIPS_FREEF( vips_argument_table_destroy, object->argument_table ); vips_object_close( object ); @@ -474,6 +510,8 @@ vips_object_dispose( GObject *gobject ) G_OBJECT_CLASS( vips_object_parent_class )->dispose( gobject ); vips_object_postclose( object ); + + vips_argument_free_all( object ); } static void diff --git a/libvips/iofuncs/operation.c b/libvips/iofuncs/operation.c index cd235b48..8e7fd200 100644 --- a/libvips/iofuncs/operation.c +++ b/libvips/iofuncs/operation.c @@ -359,7 +359,7 @@ vips_call( const char *operation_name, ... ) /* Build failed: junk args and back out. */ if( result ) { - vips_argument_free_all( VIPS_OBJECT( operation ) ); + vips_argument_dispose_all( VIPS_OBJECT( operation ) ); g_object_unref( operation ); return( -1 ); @@ -406,7 +406,7 @@ vips_call_split( const char *operation_name, va_list optional, ... ) /* Build failed: junk args and back out. */ if( result ) { - vips_argument_free_all( VIPS_OBJECT( operation ) ); + vips_argument_dispose_all( VIPS_OBJECT( operation ) ); g_object_unref( operation ); return( -1 );