From 971615d6ae2d52fe147c8cb7e725870b3ce1ff86 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 2 Jun 2011 12:23:56 +0100 Subject: [PATCH] back to fixed refs, add a pool system instead floating refs were very confusing and annoying, instead use simple fixed refs and add a pool system to track refs for you also fixed a couple of dumb errors in close callbacks --- ChangeLog | 1 + TODO | 64 ---------- libvips/convolution/im_aconv.c | 11 ++ libvips/deprecated/vips7compat.c | 5 - libvips/include/vips/object.h | 4 +- libvips/include/vips/pool.h | 88 ++++++++++++++ libvips/include/vips/vips.h | 1 + libvips/iofuncs/Makefile.am | 1 + libvips/iofuncs/image.c | 9 +- libvips/iofuncs/object.c | 6 +- libvips/iofuncs/operation.c | 4 - libvips/iofuncs/pool.c | 203 +++++++++++++++++++++++++++++++ libvips/iofuncs/region.c | 4 - libvips/iofuncs/sinkscreen.c | 2 +- 14 files changed, 316 insertions(+), 87 deletions(-) create mode 100644 libvips/include/vips/pool.h create mode 100644 libvips/iofuncs/pool.c diff --git a/ChangeLog b/ChangeLog index 076b639c..822ddf07 100644 --- a/ChangeLog +++ b/ChangeLog @@ -65,6 +65,7 @@ - laplacian generator lost -ve lobes for large sigma - added im_aconv(), approximate convolution - bumped smalltile to 512x512 for testing +- added VipsPool, got rid of floating refs again, argh 30/11/10 started 7.24.0 - bump for new stable diff --git a/TODO b/TODO index 9f9c06b6..82fbfc18 100644 --- a/TODO +++ b/TODO @@ -1,6 +1,3 @@ -- leak on exit, try: - - vips im_aconv img_0075.jpg x.v gmask_201.con 10 - also VipsFormat ... could this replace vips_image_new_from_string()? or @@ -13,67 +10,6 @@ -- perhaps we should have hard refs everywhere? it's very confusing having a - mixture :-( use vips_object_local() to make hard refs autounref - - what would the API look like? - - - VipsImage * - thing( VipsImage *in1, VipsImage *in2 ) - { - VipsImage *t; - VipsImage *out; - - if( vips_add( in1, in2, &t, NULL ) ) - return( NULL ); - if( vips_add( in1, t, &out, NULL ) ) { - g_object_unref( t ); - return( NULL ); - } - g_object_unref( t ); - - return( out ); - } - - yuk! how can we simplify this? keep refs in an array? - - VipsImage * - thing( VipsImage *in1, VipsImage *in2 ) - { - VipsImage *t[5] = { NULL }; - VipsImage *out; - - if( vips_add( in1, in2, &t[0], NULL ) || - vips_add( in1, t[0], &out, NULL ) ) { - vips_unref_array( t, 5 ); - return( NULL ); - } - vips_unref_array( t, 5 ); - - return( out ); - } - - vips_unref_array() just unrefs all non-NULL pointers in the array - - not too bad! and getting rid of floating refs is a win - - - - - - - - - -- try out: - - http://incubator.quasimondo.com/processing/stackblur.pde - - - - - - make something for Python as well diff --git a/libvips/convolution/im_aconv.c b/libvips/convolution/im_aconv.c index 2a011c2a..82d3cd23 100644 --- a/libvips/convolution/im_aconv.c +++ b/libvips/convolution/im_aconv.c @@ -35,6 +35,17 @@ */ +/* + + See: + + http://incubator.quasimondo.com/processing/stackblur.pde + + This thing is a little like stackblur, but generalised to any separable + mask. + + */ + /* Show sample pixels as they are transformed. #define DEBUG_PIXELS */ diff --git a/libvips/deprecated/vips7compat.c b/libvips/deprecated/vips7compat.c index ddd810ee..ff2f080f 100644 --- a/libvips/deprecated/vips7compat.c +++ b/libvips/deprecated/vips7compat.c @@ -60,11 +60,6 @@ im_open( const char *filename, const char *mode ) if( !(image = vips_image_new_mode( filename, mode )) ) return( NULL ); - /* We have to refsink since the im_open() result is used like a hard - * reference. - */ - g_object_ref_sink( image ); - return( image ); } diff --git a/libvips/include/vips/object.h b/libvips/include/vips/object.h index 084a4d51..547bcb56 100644 --- a/libvips/include/vips/object.h +++ b/libvips/include/vips/object.h @@ -201,7 +201,7 @@ void vips_argument_free_all( VipsObject *object ); (G_TYPE_INSTANCE_GET_CLASS( (obj), VIPS_TYPE_OBJECT, VipsObjectClass )) struct _VipsObject { - GInitiallyUnowned parent_object; + GObject parent_object; gboolean constructed; /* Construct done and checked */ @@ -223,7 +223,7 @@ struct _VipsObject { }; struct _VipsObjectClass { - GInitiallyUnownedClass parent_class; + GObjectClass parent_class; /* Build the object ... all argument properties have been set, * now build the thing. diff --git a/libvips/include/vips/pool.h b/libvips/include/vips/pool.h new file mode 100644 index 00000000..b6fa4285 --- /dev/null +++ b/libvips/include/vips/pool.h @@ -0,0 +1,88 @@ +/* manage pools of objects for unreffing + */ + +/* + + Copyright (C) 1991-2003 The National Gallery + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 2 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + + */ + +/* + + These files are distributed with VIPS - http://www.vips.ecs.soton.ac.uk + + */ + +#ifndef VIPS_POOL_H +#define VIPS_POOL_H + +#ifdef __cplusplus +extern "C" { +#endif /*__cplusplus*/ + +/* A pool context ... just an array of GObject pointers. + */ +typedef GPtrArray VipsPoolContext; + +/* A VipsPool is really just a hash table, but we make it a VipsObject so we + * can put pools into other pools. + */ + +#define VIPS_TYPE_POOL (vips_pool_get_type()) +#define VIPS_POOL( obj ) \ + (G_TYPE_CHECK_INSTANCE_CAST( (obj), VIPS_TYPE_POOL, VipsPool )) +#define VIPS_POOL_CLASS( klass ) \ + (G_TYPE_CHECK_CLASS_CAST( (klass), VIPS_TYPE_POOL, VipsPoolClass)) +#define VIPS_IS_POOL( obj ) \ + (G_TYPE_CHECK_INSTANCE_TYPE( (obj), VIPS_TYPE_POOL )) +#define VIPS_IS_POOL_CLASS( klass ) \ + (G_TYPE_CHECK_CLASS_TYPE( (klass), VIPS_TYPE_POOL )) +#define VIPS_POOL_GET_CLASS( obj ) \ + (G_TYPE_INSTANCE_GET_CLASS( (obj), VIPS_TYPE_POOL, VipsPoolClass )) + +typedef struct _VipsPool { + VipsObject parent_object; + + /* A table of all the contexts we've seen. + */ + GHashTable *contexts; +} VipsPool; + +typedef struct _VipsPoolClass { + VipsObjectClass parent_class; +} VipsPoolClass; + +GType vips_pool_get_type( void ); + +VipsPool *vips_pool_new( const char *name ); +VipsPoolContext *vips_pool_context_new( VipsPool *pool ); +GObject **vips_pool_context_object( VipsPoolContext *context, int n ); + +/* Save some typing. + */ +#define VIPS_VAR_IMAGE_REF( N ) \ + ((VipsImage **) vips_pool_context_object( context, (N) )) +#define VIPS_VAR_IMAGE( N ) \ + (*((VipsImage **) vips_pool_context_object( context, (N) ))) + +#ifdef __cplusplus +} +#endif /*__cplusplus*/ + +#endif /*VIPS_POOL_H*/ + + diff --git a/libvips/include/vips/vips.h b/libvips/include/vips/vips.h index b62c2ad9..9106c0a8 100644 --- a/libvips/include/vips/vips.h +++ b/libvips/include/vips/vips.h @@ -97,6 +97,7 @@ extern "C" { #include #include #include +#include #include #include diff --git a/libvips/iofuncs/Makefile.am b/libvips/iofuncs/Makefile.am index 9332a161..dc846646 100644 --- a/libvips/iofuncs/Makefile.am +++ b/libvips/iofuncs/Makefile.am @@ -3,6 +3,7 @@ noinst_LTLIBRARIES = libiofuncs.la libiofuncs_la_SOURCES = \ enumtypes.c \ object.c \ + pool.c \ base64.h \ base64.c \ error.c \ diff --git a/libvips/iofuncs/image.c b/libvips/iofuncs/image.c index ce36ad94..a4bea8a9 100644 --- a/libvips/iofuncs/image.c +++ b/libvips/iofuncs/image.c @@ -566,8 +566,10 @@ typedef struct { } Lazy; static void -lazy_free_cb( Lazy *lazy ) +lazy_free_cb( VipsImage *image, Lazy *lazy ) { + VIPS_DEBUG_MSG( "lazy_free: %p \"%s\"\n", lazy, lazy->filename ); + VIPS_FREE( lazy->filename ); VIPS_UNREF( lazy->real ); } @@ -578,16 +580,15 @@ lazy_new( VipsImage *image, { Lazy *lazy; - VIPS_DEBUG_MSG( "lazy_new: \"%s\"\n", filename ); - if( !(lazy = VIPS_NEW( image, Lazy )) ) return( NULL ); + VIPS_DEBUG_MSG( "lazy_new: %p \"%s\"\n", lazy, filename ); lazy->image = image; lazy->format = format; lazy->filename = NULL; lazy->disc = disc; lazy->real = NULL; - g_signal_connect( image, "close", G_CALLBACK( lazy_free_cb ), NULL ); + g_signal_connect( image, "close", G_CALLBACK( lazy_free_cb ), lazy ); if( !(lazy->filename = vips_strdup( NULL, filename )) ) return( NULL ); diff --git a/libvips/iofuncs/object.c b/libvips/iofuncs/object.c index fe5484b4..9823ff99 100644 --- a/libvips/iofuncs/object.c +++ b/libvips/iofuncs/object.c @@ -534,7 +534,7 @@ vips_object_set_object( VipsObject *object, GParamSpec *pspec, /* Ref the argument. */ - g_object_ref_sink( *member ); + g_object_ref( *member ); } else if( argument_class->flags & VIPS_ARGUMENT_OUTPUT ) { #ifdef DEBUG_REF @@ -548,7 +548,7 @@ vips_object_set_object( VipsObject *object, GParamSpec *pspec, /* The argument reffs us. */ - g_object_ref_sink( object ); + g_object_ref( object ); argument_instance->close_id = g_signal_connect( *member, "close", G_CALLBACK( vips_object_arg_close ), @@ -1586,7 +1586,7 @@ vips_type_find( const char *basename, const char *nickname ) return( 0 ); /* FIXME ... we've not supposed to use G_TYPE_FROM_CLASS(), I think. - * I'm not * sure what the alternative is. + * I'm not sure what the alternative is. */ return( G_TYPE_FROM_CLASS( class ) ); } diff --git a/libvips/iofuncs/operation.c b/libvips/iofuncs/operation.c index 7f198ee0..cd235b48 100644 --- a/libvips/iofuncs/operation.c +++ b/libvips/iofuncs/operation.c @@ -199,10 +199,6 @@ vips_operation_new( const char *name ) return( NULL ); operation = VIPS_OPERATION( g_object_new( type, NULL ) ); - /* Clear the initial floating ref, return a real ref. - */ - g_object_ref_sink( operation ); - return( operation ); } diff --git a/libvips/iofuncs/pool.c b/libvips/iofuncs/pool.c new file mode 100644 index 00000000..19543f36 --- /dev/null +++ b/libvips/iofuncs/pool.c @@ -0,0 +1,203 @@ +/* manage pools of objects for unreffing + */ + +/* + + Copyright (C) 1991-2003 The National Gallery + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 2 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + + */ + +/* + + These files are distributed with VIPS - http://www.vips.ecs.soton.ac.uk + + */ + +/* +#define DEBUG +#define VIPS_DEBUG +#define DEBUG_REF + */ + +#ifdef HAVE_CONFIG_H +#include +#endif /*HAVE_CONFIG_H*/ +#include + +#include +#include + +#include +#include +#include + +#ifdef WITH_DMALLOC +#include +#endif /*WITH_DMALLOC*/ + +/* + + Here's how to handle ref counts when calling vips operations: + + VipsImage * + thing( VipsImage *in1, VipsImage *in2 ) + { + VipsImage *t; + VipsImage *out; + + if( vips_add( in1, in2, &t, NULL ) ) + return( NULL ); + if( vips_add( in1, t, &out, NULL ) ) { + g_object_unref( t ); + return( NULL ); + } + g_object_unref( t ); + + return( out ); + } + + The first vips_add() call returns (via the reference argument) a new + VipsImage in variable t. The second vips_add() uses this as an input and + takes the ref count up to two. After calling the second vips_add() we have + to drop t to avoid leaks. We also have to drop t if the second vips_add() + fails. + + VipsPool provides a nicer way to track the objects that you create and free + them safely. The above function would become: + + VipsImage * + thing( VipsPool *pool, VipsImage *in1, VipsImage *in2 ) + { + VipsPoolContext *context = vips_pool_context_new( pool ); + + VipsImage *out; + + if( vips_add( in1, in2, VIPS_VAR_IMAGE_REF( 1 ), NULL ) || + vips_add( in1, VIPS_VAR_IMAGE( 1 ), &out, NULL ) ) + return( NULL ); + + return( out ); + } + + vips_pool_context_new() creates a new context to hold a set of temporary + objects. You can get a reference to a temporary image object with the + macro VIPS_VAR_IMAGE_REF(), and get the object with VIPS_VAR_IMAGE(). + Temporary objects are numbered from zero. + + Our caller will (eventually) call g_object_unref() on the pool and this + will in turn unref all objects in the pool. + + */ + +G_DEFINE_TYPE( VipsPool, vips_pool, VIPS_TYPE_POOL ); + +static void +vips_pool_dispose( GObject *gobject ) +{ + VipsPool *pool = VIPS_POOL( gobject ); + +#ifdef VIPS_DEBUG + VIPS_DEBUG_MSG( "vips_pool_dispose: " ); + vips_object_print( VIPS_OBJECT( gobject ) ); +#endif /*VIPS_DEBUG*/ + + VIPS_FREEF( g_hash_table_unref, pool->contexts ); + + G_OBJECT_CLASS( vips_pool_parent_class )->dispose( gobject ); +} + +static void +vips_pool_context_print( VipsPoolContext *context, VipsBuf *buf ) +{ + vips_buf_appendf( buf, "VipsPoolContext %p, %d objects\n", + context, context->len ); +} + +static void +vips_pool_print( VipsObject *object, VipsBuf *buf ) +{ + VipsPool *pool = VIPS_POOL( object ); + + if( pool->contexts ) { + int size = g_hash_table_size( pool->contexts ); + + vips_buf_appendf( buf, "%d contexts\n", size ); + if( size > 0 ) + g_hash_table_foreach( pool->contexts, + (GHFunc) vips_pool_context_print, buf ); + } + + VIPS_OBJECT_CLASS( vips_pool_parent_class )->print( object, buf ); +} + +static void +vips_pool_class_init( VipsPoolClass *class ) +{ + GObjectClass *gobject_class = G_OBJECT_CLASS( class ); + VipsObjectClass *vobject_class = VIPS_OBJECT_CLASS( class ); + + gobject_class->dispose = vips_pool_dispose; + + vobject_class->print = vips_pool_print; +} + +static void +vips_pool_init( VipsPool *pool ) +{ + pool->contexts = g_hash_table_new_full( g_direct_hash, g_direct_equal, + NULL, (GDestroyNotify) g_ptr_array_unref ); +} + +VipsPool * +vips_pool_new( const char *name ) +{ + VipsPool *pool; + + pool = VIPS_POOL( g_object_new( VIPS_TYPE_POOL, NULL ) ); + + g_object_set( pool, "name", name, NULL ); + + if( vips_object_build( VIPS_OBJECT( pool ) ) ) { + VIPS_UNREF( pool ); + return( NULL ); + } + + return( pool ); +} + +VipsPoolContext * +vips_pool_context_new( VipsPool *pool ) +{ + VipsPoolContext *context; + + context = g_ptr_array_new_with_free_func( g_object_unref ); + g_hash_table_insert( pool->contexts, context, context ); + + return( context ); +} + +GObject ** +vips_pool_context_object( VipsPoolContext *context, int n ) +{ + g_assert( n >= 0 ); + + if( n >= context->len ) + g_ptr_array_set_size( context, n + 10 ); + + return( (GObject **) &g_ptr_array_index( context, n ) ); +} + diff --git a/libvips/iofuncs/region.c b/libvips/iofuncs/region.c index af3ddbde..0eb144b4 100644 --- a/libvips/iofuncs/region.c +++ b/libvips/iofuncs/region.c @@ -442,10 +442,6 @@ vips_region_new( VipsImage *image ) return( NULL ); } - /* We return a hard reference, so unfloat it. - */ - g_object_ref_sink( region ); - return( region ); } diff --git a/libvips/iofuncs/sinkscreen.c b/libvips/iofuncs/sinkscreen.c index 14af7bab..88522890 100644 --- a/libvips/iofuncs/sinkscreen.c +++ b/libvips/iofuncs/sinkscreen.c @@ -568,7 +568,7 @@ tile_equal( gconstpointer a, gconstpointer b ) } static int -render_close_cb( Render *render ) +render_close_cb( VipsImage *image, Render *render ) { VIPS_DEBUG_MSG_AMBER( "render_close_cb\n" );