diff --git a/ChangeLog b/ChangeLog index 01432673..85eeb952 100644 --- a/ChangeLog +++ b/ChangeLog @@ -7,6 +7,10 @@ - libtiff LOGLUV images load and save as libvips XYZ - add gifload_source +31/1/19 started 8.9.2 +- fix a deadlock with --vips-leak [DarthSim] +- better gifload behaviour for DISPOSAL_UNSPECIFIED [DarthSim] + 20/6/19 started 8.9.1 - don't use the new source loaders for new_from_file or new_from_buffer, it will break the loader priority system diff --git a/libvips/foreign/gifload.c b/libvips/foreign/gifload.c index a4a6b3ac..151c6af6 100644 --- a/libvips/foreign/gifload.c +++ b/libvips/foreign/gifload.c @@ -37,6 +37,9 @@ * 30/1/19 * - rework on top of VipsSource * - add gifload_source + * 31/1/20 + * - treat DISPOSAL_UNSPECIFIED as _DO_NOT, since that's what many GIFs + * in the wild appear to do */ /* @@ -847,8 +850,12 @@ vips_foreign_load_gif_render_line( VipsForeignLoadGif *gif, /* In DISPOSE_DO_NOT mode, the previous frame shows * through (ie. we do nothing). In all other modes, * it's just transparent. + * + * Many GIFs use DISPOSAL_UNSPECIFIED to mean DO_NOT, + * so use that for previous frame as well. */ - if( gif->dispose != DISPOSE_DO_NOT ) + if( gif->dispose != DISPOSE_DO_NOT && + gif->dispose != DISPOSAL_UNSPECIFIED ) iq[x] = 0; } else @@ -971,6 +978,20 @@ vips_foreign_load_gif_render( VipsForeignLoadGif *gif ) return( 0 ); } +#ifdef VIPS_DEBUG +static const char * +dispose2str( int dispose ) +{ + switch( dispose ) { + case DISPOSAL_UNSPECIFIED: return( "DISPOSAL_UNSPECIFIED" ); + case DISPOSE_DO_NOT: return( "DISPOSE_DO_NOT" ); + case DISPOSE_BACKGROUND: return( "DISPOSE_BACKGROUND" ); + case DISPOSE_PREVIOUS: return( "DISPOSE_PREVIOUS" ); + default: return( "" ); + } +} +#endif /*VIPS_DEBUG*/ + static int vips_foreign_load_gif_extension( VipsForeignLoadGif *gif ) { @@ -993,15 +1014,18 @@ vips_foreign_load_gif_extension( VipsForeignLoadGif *gif ) * is being set. */ gif->transparency = -1; - if( extension[1] & 0x1 ) + if( extension[1] & 0x1 ) { gif->transparency = extension[4]; + VIPS_DEBUG_MSG( "vips_foreign_load_gif_extension: " + "transparency = %d\n", gif->transparency ); + } /* Set the current dispose mode. This is read during frame load * to set the meaning of background and transparent pixels. */ gif->dispose = (extension[1] >> 2) & 0x7; VIPS_DEBUG_MSG( "vips_foreign_load_gif_extension: " - "dispose = %d\n", gif->dispose ); + "dispose = %s\n", dispose2str( gif->dispose ) ); } while( extension != NULL ) diff --git a/libvips/include/vips/private.h b/libvips/include/vips/private.h index e28735fc..8716cf36 100644 --- a/libvips/include/vips/private.h +++ b/libvips/include/vips/private.h @@ -192,6 +192,8 @@ int vips__view_image( struct _VipsImage *image ); */ extern int _vips__argument_id; +void vips__meta_init( void ); + #ifdef __cplusplus } #endif /*__cplusplus*/ diff --git a/libvips/iofuncs/header.c b/libvips/iofuncs/header.c index 6a6347a3..dba1c767 100644 --- a/libvips/iofuncs/header.c +++ b/libvips/iofuncs/header.c @@ -36,6 +36,8 @@ * - add vips_image_get_n_pages() * 20/6/19 * - add vips_image_get/set_array_int() + * 31/1/19 + * - lock for metadata changes */ /* @@ -130,6 +132,11 @@ * these types, it can be copied between images efficiently. */ +/* Use in various small places where we need a mutex and it's not worth + * making a private one. + */ +static GMutex *vips__meta_lock = NULL; + /* We have to keep the gtype as a string, since we statically init this. */ typedef struct _HeaderField { @@ -934,10 +941,10 @@ meta_cp( VipsImage *dst, const VipsImage *src ) /* We lock with vips_image_set() to stop races in highly- * threaded applications. */ - g_mutex_lock( vips__global_lock ); + g_mutex_lock( vips__meta_lock ); vips_slist_map2( src->meta_traverse, (VipsSListMap2Fn) meta_cp_field, dst, NULL ); - g_mutex_unlock( vips__global_lock ); + g_mutex_unlock( vips__meta_lock ); } return( 0 ); @@ -1025,13 +1032,6 @@ vips_image_set( VipsImage *image, const char *name, GValue *value ) g_assert( name ); g_assert( value ); - /* If this image is shared, block metadata changes. - */ - if( G_OBJECT( image )->ref_count > 1 ) { - g_warning( "can't set metadata \"%s\" on shared image", name ); - return; - } - meta_init( image ); /* We lock between modifying metadata and copying metadata between @@ -1041,9 +1041,9 @@ vips_image_set( VipsImage *image, const char *name, GValue *value ) * metadata copy on another -- this can lead to crashes in * highly-threaded applications. */ - g_mutex_lock( vips__global_lock ); + g_mutex_lock( vips__meta_lock ); (void) meta_new( image, name, value ); - g_mutex_unlock( vips__global_lock ); + g_mutex_unlock( vips__meta_lock ); /* If we're setting an EXIF data block, we need to automatically expand * out all the tags. This will set things like xres/yres too. @@ -1240,14 +1240,6 @@ vips_image_remove( VipsImage *image, const char *name ) result = FALSE; - /* If this image is shared, block metadata changes. - */ - if( G_OBJECT( image )->ref_count > 1 ) { - g_warning( "can't remove metadata \"%s\" on shared image", - name ); - return( result ); - } - if( image->meta ) { /* We lock between modifying metadata and copying metadata * between images, see meta_cp(). @@ -1256,9 +1248,9 @@ vips_image_remove( VipsImage *image, const char *name ) * racing with metadata copy on another -- this can lead to * crashes in highly-threaded applications. */ - g_mutex_lock( vips__global_lock ); + g_mutex_lock( vips__meta_lock ); result = g_hash_table_remove( image->meta, name ); - g_mutex_unlock( vips__global_lock ); + g_mutex_unlock( vips__meta_lock ); } return( result ); @@ -2027,3 +2019,12 @@ vips_image_get_history( VipsImage *image ) return( image->Hist ? image->Hist : "" ); } + +/* Called during vips_init(). + */ +void +vips__meta_init( void ) +{ + if( !vips__meta_lock ) + vips__meta_lock = vips_g_mutex_new(); +} diff --git a/libvips/iofuncs/init.c b/libvips/iofuncs/init.c index bd5a90cc..20ed972d 100644 --- a/libvips/iofuncs/init.c +++ b/libvips/iofuncs/init.c @@ -382,6 +382,7 @@ vips_init( const char *argv0 ) vips__threadpool_init(); vips__buffer_init(); + vips__meta_init(); /* This does an unsynchronised static hash table init on first call -- * we have to make sure we do this single-threaded. See: