From fde23c93ef39fd708b4e53e8a9c1378225f816d9 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Sun, 17 Jul 2011 14:36:57 +0100 Subject: [PATCH] vipsobject cleanups vipsobject always goes via set_prop so it can work with subclasses which override these funcs (eg. wrap7) --- TODO | 18 ++++ libvips/deprecated/wrapvips7.c | 184 +++++++++++++-------------------- libvips/include/vips/object.h | 2 + libvips/iofuncs/object.c | 115 ++++++++++++++------- libvips/iofuncs/operation.c | 2 +- 5 files changed, 169 insertions(+), 152 deletions(-) diff --git a/TODO b/TODO index 2b2bd681..4027afec 100644 --- a/TODO +++ b/TODO @@ -1,3 +1,21 @@ +- vips_wrap7_object_set_property() needs to do the thing with reffing the + object or the operation, depending on the param direction + + also watch close signal + +- vips_object_free_argument() uses offset directly to free strings, disaster + with wrap7 + + instead, it should use g_object_set() to set the vars to NULL, that way + it'll work for wrap7 as well + + check for other uses of offset + + also vips_object_dispose_argument() + + + + - revisit orc conv diff --git a/libvips/deprecated/wrapvips7.c b/libvips/deprecated/wrapvips7.c index ea45e320..5c43a2ac 100644 --- a/libvips/deprecated/wrapvips7.c +++ b/libvips/deprecated/wrapvips7.c @@ -382,14 +382,9 @@ vips_wrap7_object_set_property( GObject *gobject, case VIPS_WRAP7_IMAGE: case VIPS_WRAP7_INTERPOLATE: - /* This does not add a ref to object. - */ - wrap7->vargv[i] = g_value_get_object( value ); - - /* Now ref the object this operation refs. Drop this ref in - * _dispose(), see above. - */ - g_object_ref( g_value_get_object( value ) ); + vips__object_set_member( object, pspec, + (GObject **) &wrap7->vargv[i], + g_value_get_object( value ) ); break; default: @@ -464,6 +459,66 @@ vips_wrap7_object_get_property( GObject *gobject, } } +/* Init an output slot in vargv. + */ +static void * +vips_wrap7_build_output( VipsObject *object, + GParamSpec *pspec, + VipsArgumentClass *argument_class, + VipsArgumentInstance *argument_instance, + void *a, void *b ) +{ + VipsWrap7 *wrap7 = VIPS_WRAP7( object ); + VipsWrap7Class *class = VIPS_WRAP7_GET_CLASS( wrap7 ); + int i = argument_class->offset; + im_arg_desc *arg = &class->fn->argv[i]; + im_type_desc *type = arg->desc; + im_arg_type vt = type->type; + + /* We want required, construct-time, unassigned output args. + */ + if( !(argument_class->flags & VIPS_ARGUMENT_REQUIRED) || + !(argument_class->flags & VIPS_ARGUMENT_CONSTRUCT) || + argument_instance->assigned || + !(argument_class->flags & VIPS_ARGUMENT_OUTPUT) ) + return( NULL ); + + /* Provide output objects for the operation to write to. + */ + switch( vips_wrap7_lookup_type( vt ) ) { + case VIPS_WRAP7_DOUBLE: + case VIPS_WRAP7_INT: + case VIPS_WRAP7_COMPLEX: + case VIPS_WRAP7_STRING: + break; + + case VIPS_WRAP7_IMAGE: + g_object_set( object, arg->name, vips_image_new(), NULL ); + break; + + case VIPS_WRAP7_DMASK: + case VIPS_WRAP7_IMASK: + break; + + case VIPS_WRAP7_GVALUE: + { + GValue *value = wrap7->vargv[i]; + + memset( value, 0, sizeof( GValue ) ); + + break; + } + + case VIPS_WRAP7_DOUBLEVEC: + case VIPS_WRAP7_INTVEC: + default: + wrap7->error = TRUE; + break; + } + + return( NULL ); +} + static int vips_wrap7_build( VipsObject *object ) { @@ -484,6 +539,12 @@ vips_wrap7_build( VipsObject *object ) return( -1 ); } + /* Init all the output args. + */ + (void) vips_argument_map( VIPS_OBJECT( wrap7 ), + vips_wrap7_build_output, + NULL, NULL ); + if( VIPS_OBJECT_CLASS( vips_wrap7_parent_class )->build( object ) ) return( -1 ); @@ -500,9 +561,9 @@ vips_wrap7_print_class( VipsObjectClass *oclass, VipsBuf *buf ) im_function *fn = class->fn; if( fn ) - vips_buf_appendf( buf, "%s, ", fn->name ); + vips_buf_appendf( buf, "%s ", fn->name ); else - vips_buf_appendf( buf, "%s, ", G_OBJECT_CLASS_NAME( class ) ); + vips_buf_appendf( buf, "%s ", G_OBJECT_CLASS_NAME( class ) ); if( oclass->nickname ) vips_buf_appendf( buf, "(%s), ", oclass->nickname ); @@ -708,103 +769,6 @@ vips_wrap7_subclass_class_init( VipsWrap7Class *class ) } } -static void -vips_wrap7_arg_close( GObject *argument, - VipsArgumentInstance *argument_instance ) -{ - VipsObject *object = argument_instance->object; - - g_object_unref( object ); -} - -/* Init an output slot in vargv. - */ -static void * -vips_wrap7_build_output( VipsObject *object, - GParamSpec *pspec, - VipsArgumentClass *argument_class, - VipsArgumentInstance *argument_instance, - void *a, void *b ) -{ - VipsWrap7 *wrap7 = VIPS_WRAP7( object ); - VipsWrap7Class *class = VIPS_WRAP7_GET_CLASS( wrap7 ); - int i = argument_class->offset; - im_arg_desc *arg = &class->fn->argv[i]; - im_type_desc *type = arg->desc; - im_arg_type vt = type->type; - - /* We want required, construct-time, unassigned output args. - */ - if( !(argument_class->flags & VIPS_ARGUMENT_REQUIRED) || - !(argument_class->flags & VIPS_ARGUMENT_CONSTRUCT) || - argument_instance->assigned || - !(argument_class->flags & VIPS_ARGUMENT_OUTPUT) ) - return( NULL ); - - /* Provide output objects for the operation to write to. - */ - switch( vips_wrap7_lookup_type( vt ) ) { - case VIPS_WRAP7_DOUBLE: - case VIPS_WRAP7_INT: - case VIPS_WRAP7_COMPLEX: - case VIPS_WRAP7_STRING: - break; - - case VIPS_WRAP7_IMAGE: - /* Output objects ref this operation. - */ - wrap7->vargv[i] = vips_image_new(); - g_object_ref( wrap7 ); - - /* vipsobject will handle close_id disconnect for us. - */ - argument_instance->close_id = - g_signal_connect( wrap7->vargv[i], "close", - G_CALLBACK( vips_wrap7_arg_close ), - argument_instance ); - break; - - case VIPS_WRAP7_DMASK: - case VIPS_WRAP7_IMASK: - { - im_mask_object *mo = wrap7->vargv[i]; - - mo->mask = NULL; - mo->name = im_strdup( NULL, "" ); - - break; - } - - case VIPS_WRAP7_GVALUE: - { - GValue *value = wrap7->vargv[i]; - - memset( value, 0, sizeof( GValue ) ); - - break; - } - - case VIPS_WRAP7_DOUBLEVEC: - case VIPS_WRAP7_INTVEC: - { - /* intvec is also int + pointer. - */ - im_doublevec_object *dv = wrap7->vargv[i]; - - dv->n = 0; - dv->vec = NULL; - - break; - } - - default: - wrap7->error = TRUE; - break; - } - - return( NULL ); -} - static void vips_wrap7_subclass_init( VipsWrap7 *wrap7 ) { @@ -816,12 +780,6 @@ vips_wrap7_subclass_init( VipsWrap7 *wrap7 ) wrap7->error = TRUE; return; } - - /* Init all the output args. - */ - (void) vips_argument_map( VIPS_OBJECT( wrap7 ), - vips_wrap7_build_output, - NULL, NULL ); } static GType diff --git a/libvips/include/vips/object.h b/libvips/include/vips/object.h index a2f91eee..0b50de9e 100644 --- a/libvips/include/vips/object.h +++ b/libvips/include/vips/object.h @@ -156,6 +156,8 @@ VipsArgumentInstance *vips__argument_get_instance( VipsArgumentClass *, VipsObject *); VipsArgument *vips__argument_table_lookup( VipsArgumentTable *, GParamSpec *); +void vips__object_set_member( VipsObject *object, GParamSpec *pspec, + GObject **member, GObject *argument ); typedef void *(*VipsArgumentMapFn)( VipsObject *, GParamSpec *, VipsArgumentClass *, VipsArgumentInstance *, void *a, void *b ); void *vips_argument_map( VipsObject *object, diff --git a/libvips/iofuncs/object.c b/libvips/iofuncs/object.c index 4e997e88..264c7ce3 100644 --- a/libvips/iofuncs/object.c +++ b/libvips/iofuncs/object.c @@ -333,20 +333,19 @@ vips__argument_get_instance( VipsArgumentClass *argument_class, } static void -vips_object_clear_object( VipsObject *object, GParamSpec *pspec ) +vips_object_clear_member( VipsObject *object, GParamSpec *pspec, + GObject **member ) { VipsObjectClass *class = VIPS_OBJECT_GET_CLASS( object ); VipsArgumentClass *argument_class = (VipsArgumentClass *) vips__argument_table_lookup( class->argument_table, pspec ); VipsArgumentInstance *argument_instance = vips__argument_get_instance( argument_class, object ); - GObject **member = &G_STRUCT_MEMBER( GObject *, object, - argument_class->offset ); if( *member ) { if( argument_class->flags & VIPS_ARGUMENT_INPUT ) { #ifdef DEBUG_REF - printf( "vips_object_clear_object: vips object: " ); + printf( "vips_object_clear_member: vips object: " ); vips_object_print_name( object ); printf( " no longer refers to gobject %s (%p)\n", G_OBJECT_TYPE_NAME( *member ), *member ); @@ -360,7 +359,7 @@ vips_object_clear_object( VipsObject *object, GParamSpec *pspec ) } else if( argument_class->flags & VIPS_ARGUMENT_OUTPUT ) { #ifdef DEBUG_REF - printf( "vips_object_clear_object: gobject %s (%p)\n", + printf( "vips_object_clear_member: gobject %s (%p)\n", G_OBJECT_TYPE_NAME( *member ), *member ); printf( " no longer refers to vips object: " ); vips_object_print_name( object ); @@ -378,7 +377,6 @@ vips_object_clear_object( VipsObject *object, GParamSpec *pspec ) g_signal_handler_disconnect( object, argument_instance->close_id ); argument_instance->close_id = 0; - *member = NULL; g_object_unref( object ); } @@ -387,6 +385,19 @@ vips_object_clear_object( VipsObject *object, GParamSpec *pspec ) } } +/* Clear a member variable, where the member has an offset. + */ +static void +vips_object_clear_object( VipsObject *object, GParamSpec *pspec ) +{ + VipsObjectClass *class = VIPS_OBJECT_GET_CLASS( object ); + VipsArgumentClass *argument_class = (VipsArgumentClass *) + vips__argument_table_lookup( class->argument_table, pspec ); + + vips_object_clear_member( object, pspec, + &G_STRUCT_MEMBER( GObject *, object, argument_class->offset ) ); +} + /* Free any args which are holding resources. */ static void * @@ -398,23 +409,17 @@ vips_object_dispose_argument( VipsObject *object, GParamSpec *pspec, g_assert( ((VipsArgument *) argument_class)->pspec == pspec ); g_assert( ((VipsArgument *) argument_instance)->pspec == 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 ); - + if( G_IS_PARAM_SPEC_OBJECT( pspec ) || + G_IS_PARAM_SPEC_BOXED( pspec ) ) { #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 ); - *member = NULL; - } + g_object_set( object, + g_param_spec_get_name( pspec ), NULL, + NULL ); } return( NULL ); @@ -446,16 +451,15 @@ vips_object_free_argument( VipsObject *object, GParamSpec *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 ); + g_object_set( object, + g_param_spec_get_name( pspec ), NULL, + NULL ); } return( NULL ); @@ -503,15 +507,16 @@ vips_object_dispose( GObject *gobject ) /* Clear all our arguments: they may be holding refs we should drop. */ vips_argument_dispose_all( object ); - VIPS_FREEF( vips_argument_table_destroy, object->argument_table ); vips_object_close( object ); - G_OBJECT_CLASS( vips_object_parent_class )->dispose( gobject ); - vips_object_postclose( object ); vips_argument_free_all( object ); + + VIPS_FREEF( vips_argument_table_destroy, object->argument_table ); + + G_OBJECT_CLASS( vips_object_parent_class )->dispose( gobject ); } static void @@ -548,21 +553,24 @@ vips_object_arg_close( GObject *argument, vips_object_clear_object( object, pspec ); } -static void -vips_object_set_object( VipsObject *object, GParamSpec *pspec, - GObject *argument ) +/* Set a member to an object. Handle the ref counts and signal + * connect/disconnect. + */ +void +vips__object_set_member( VipsObject *object, GParamSpec *pspec, + GObject **member, GObject *argument ) { VipsObjectClass *class = VIPS_OBJECT_GET_CLASS( object ); VipsArgumentClass *argument_class = (VipsArgumentClass *) vips__argument_table_lookup( class->argument_table, pspec ); VipsArgumentInstance *argument_instance = vips__argument_get_instance( argument_class, object ); - GObject **member = &G_STRUCT_MEMBER( GObject *, object, - argument_class->offset ); g_assert( argument_instance ); - g_assert( !*member ); + vips_object_clear_member( object, pspec, member ); + + g_assert( !*member ); *member = argument; if( *member ) { @@ -593,6 +601,7 @@ vips_object_set_object( VipsObject *object, GParamSpec *pspec, /* The argument reffs us. */ g_object_ref( object ); + g_assert( !argument_instance->close_id ); argument_instance->close_id = g_signal_connect( *member, "close", G_CALLBACK( vips_object_arg_close ), @@ -601,6 +610,40 @@ vips_object_set_object( VipsObject *object, GParamSpec *pspec, } } +static void +vips_object_set_object( VipsObject *object, GParamSpec *pspec, + GObject *argument ) +{ + VipsObjectClass *class = VIPS_OBJECT_GET_CLASS( object ); + VipsArgumentClass *argument_class = (VipsArgumentClass *) + vips__argument_table_lookup( class->argument_table, pspec ); + GObject **member = &G_STRUCT_MEMBER( GObject *, object, + argument_class->offset ); + + vips__object_set_member( object, pspec, member, argument ); +} + +/* Is a value NULL? We allow multiple sets of NULL so props can be cleared. + */ +static gboolean +vips_pspec_value_is_null( GParamSpec *pspec, const GValue *value ) +{ + if( G_IS_PARAM_SPEC_STRING( pspec ) && + !g_value_get_string( value ) ) + return( TRUE ); + if( G_IS_PARAM_SPEC_OBJECT( pspec ) && + !g_value_get_object( value ) ) + return( TRUE ); + if( G_IS_PARAM_SPEC_POINTER( pspec ) && + !g_value_get_pointer( value ) ) + return( TRUE ); + if( G_IS_PARAM_SPEC_BOXED( pspec ) && + !g_value_get_boxed( value ) ) + return( TRUE ); + + return( FALSE ); +} + /* Also used by subclasses, so not static. */ void @@ -641,7 +684,8 @@ vips_object_set_property( GObject *gobject, * built. */ if( argument_class->flags & VIPS_ARGUMENT_CONSTRUCT && - object->constructed ) { + object->constructed && + !vips_pspec_value_is_null( pspec, value ) ) { g_warning( "%s: %s can't assign '%s' after construct", G_STRLOC, G_OBJECT_TYPE_NAME( gobject ), @@ -652,7 +696,8 @@ vips_object_set_property( GObject *gobject, /* If this is a set-once argument, check we've not set it before. */ if( argument_class->flags & VIPS_ARGUMENT_SET_ONCE && - argument_instance->assigned ) { + argument_instance->assigned && + !vips_pspec_value_is_null( pspec, value ) ) { g_warning( "%s: %s can only assign '%s' once", G_STRLOC, G_OBJECT_TYPE_NAME( gobject ), @@ -667,12 +712,6 @@ vips_object_set_property( GObject *gobject, VIPS_SETSTR( *member, g_value_get_string( value ) ); } else if( G_IS_PARAM_SPEC_OBJECT( pspec ) ) { - /* Remove any old object. - */ - vips_object_clear_object( object, pspec ); - - /* Install the new object. - */ vips_object_set_object( object, pspec, g_value_get_object( value ) ); } diff --git a/libvips/iofuncs/operation.c b/libvips/iofuncs/operation.c index 4386b153..2744dd6f 100644 --- a/libvips/iofuncs/operation.c +++ b/libvips/iofuncs/operation.c @@ -28,8 +28,8 @@ */ /* -#define VIPS_DEBUG */ +#define VIPS_DEBUG #ifdef HAVE_CONFIG_H #include