From acd9101037fecc84be7888c80f00d630c2552c3f Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 28 Nov 2019 14:45:02 +0000 Subject: [PATCH] always copy before exif_update During write, we often call vips__exif_update(). This updates the exif block from the other image metadata prior to save. Always copy the image before calling this. See https://github.com/lovell/sharp/issues/1986 --- ChangeLog | 1 + libvips/foreign/heifsave.c | 43 +++++++++++++++++++++++-------------- libvips/foreign/vips2jpeg.c | 11 +++++++++- libvips/foreign/vips2webp.c | 43 ++++++++++++++++++++++--------------- libvips/iofuncs/header.c | 5 +++-- 5 files changed, 67 insertions(+), 36 deletions(-) diff --git a/ChangeLog b/ChangeLog index 56b50f81..665b3b7a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -25,6 +25,7 @@ - fix use of resolution-unit metadata on tiff save [kayarre] - support TIFF CIELAB images with alpha [angelmixu] - support TIFF with premultiplied alpha in any band +- block metadata changes on shared images [pvdz] 17/9/19 started 8.8.4 - improve compatibility with older imagemagick versions diff --git a/libvips/foreign/heifsave.c b/libvips/foreign/heifsave.c index be425d72..f22a9f35 100644 --- a/libvips/foreign/heifsave.c +++ b/libvips/foreign/heifsave.c @@ -73,6 +73,11 @@ typedef struct _VipsForeignSaveHeif { */ VipsForeignHeifCompression compression; + /* The image we save. This is a copy of save->ready since we need to + * be able to update the metadata. + */ + VipsImage *image; + int page_width; int page_height; int n_pages; @@ -107,6 +112,7 @@ vips_foreign_save_heif_dispose( GObject *gobject ) { VipsForeignSaveHeif *heif = (VipsForeignSaveHeif *) gobject; + VIPS_UNREF( heif->image ); VIPS_FREEF( heif_image_release, heif->img ); VIPS_FREEF( heif_image_handle_release, heif->handle ); VIPS_FREEF( heif_encoder_release, heif->encoder ); @@ -134,13 +140,12 @@ static int vips_foreign_save_heif_write_metadata( VipsForeignSaveHeif *heif ) { #ifdef HAVE_HEIF_CONTEXT_ADD_EXIF_METADATA - VipsForeignSave *save = (VipsForeignSave *) heif; int i; struct heif_error error; for( i = 0; i < VIPS_NUMBER( libheif_metadata ); i++ ) - if( vips_image_get_typeof( save->ready, + if( vips_image_get_typeof( heif->image, libheif_metadata[i].name ) ) { const void *data; size_t length; @@ -150,7 +155,7 @@ vips_foreign_save_heif_write_metadata( VipsForeignSaveHeif *heif ) libheif_metadata[i].name ); #endif /*DEBUG*/ - if( vips_image_get_blob( save->ready, + if( vips_image_get_blob( heif->image, VIPS_META_EXIF_NAME, &data, &length ) ) return( -1 ); @@ -176,7 +181,7 @@ vips_foreign_save_heif_write_page( VipsForeignSaveHeif *heif, int page ) #ifdef HAVE_HEIF_COLOR_PROFILE if( !save->strip && - vips_image_get_typeof( save->ready, VIPS_META_ICC_NAME ) ) { + vips_image_get_typeof( heif->image, VIPS_META_ICC_NAME ) ) { const void *data; size_t length; @@ -184,7 +189,7 @@ vips_foreign_save_heif_write_page( VipsForeignSaveHeif *heif, int page ) printf( "attaching profile ..\n" ); #endif /*DEBUG*/ - if( vips_image_get_blob( save->ready, + if( vips_image_get_blob( heif->image, VIPS_META_ICC_NAME, &data, &length ) ) return( -1 ); @@ -201,7 +206,7 @@ vips_foreign_save_heif_write_page( VipsForeignSaveHeif *heif, int page ) #ifdef HAVE_HEIF_ENCODING_OPTIONS_ALLOC options = heif_encoding_options_alloc(); - if( vips_image_hasalpha( save->ready ) ) + if( vips_image_hasalpha( heif->image ) ) options->save_alpha_channel = 1; #else /*!HAVE_HEIF_ENCODING_OPTIONS_ALLOC*/ options = NULL; @@ -223,10 +228,10 @@ vips_foreign_save_heif_write_page( VipsForeignSaveHeif *heif, int page ) } #ifdef HAVE_HEIF_CONTEXT_SET_PRIMARY_IMAGE - if( vips_image_get_typeof( save->ready, "heif-primary" ) ) { + if( vips_image_get_typeof( heif->image, "heif-primary" ) ) { int primary; - if( vips_image_get_int( save->ready, + if( vips_image_get_int( heif->image, "heif-primary", &primary ) ) return( -1 ); @@ -299,6 +304,12 @@ vips_foreign_save_heif_build( VipsObject *object ) build( object ) ) return( -1 ); + /* We must copy the input image since we will be updating the + * metadata when we write the exif. + */ + if( vips_copy( save->ready, &heif->image, NULL ) ) + return( -1 ); + error = heif_context_get_encoder_for_format( heif->ctx, (enum heif_compression_format) heif->compression, &heif->encoder ); @@ -328,16 +339,16 @@ vips_foreign_save_heif_build( VipsObject *object ) * heif_encoder_list_parameters(). */ - heif->page_width = save->ready->Xsize; - heif->page_height = vips_image_get_page_height( save->ready ); - heif->n_pages = save->ready->Ysize / heif->page_height; + heif->page_width = heif->image->Xsize; + heif->page_height = vips_image_get_page_height( heif->image ); + heif->n_pages = heif->image->Ysize / heif->page_height; /* Make a heif image the size of a page. We send sink_disc() output * here and write a frame each time it fills. */ error = heif_image_create( heif->page_width, heif->page_height, heif_colorspace_RGB, - vips_image_hasalpha( save->ready ) ? + vips_image_hasalpha( heif->image ) ? heif_chroma_interleaved_RGBA : heif_chroma_interleaved_RGB, &heif->img ); @@ -348,7 +359,7 @@ vips_foreign_save_heif_build( VipsObject *object ) error = heif_image_add_plane( heif->img, heif_channel_interleaved, heif->page_width, heif->page_height, - vips_image_hasalpha( save->ready ) ? 32 : 24 ); + vips_image_hasalpha( heif->image ) ? 32 : 24 ); if( error.code ) { vips__heif_error( &error ); return( -1 ); @@ -359,13 +370,13 @@ vips_foreign_save_heif_build( VipsObject *object ) /* Just do this once, so we don't rebuild exif on every page. */ - if( vips_image_get_typeof( save->ready, VIPS_META_EXIF_NAME ) ) - if( vips__exif_update( save->ready ) ) + if( vips_image_get_typeof( heif->image, VIPS_META_EXIF_NAME ) ) + if( vips__exif_update( heif->image ) ) return( -1 ); /* Write data. */ - if( vips_sink_disc( save->ready, + if( vips_sink_disc( heif->image, vips_foreign_save_heif_write_block, heif ) ) return( -1 ); diff --git a/libvips/foreign/vips2jpeg.c b/libvips/foreign/vips2jpeg.c index 35258dfe..963a98cb 100644 --- a/libvips/foreign/vips2jpeg.c +++ b/libvips/foreign/vips2jpeg.c @@ -211,6 +211,7 @@ write_destroy( Write *write ) jpeg_destroy_compress( &write->cinfo ); VIPS_FREE( write->row_pointer ); VIPS_UNREF( write->inverted ); + VIPS_UNREF( write->in ); g_free( write ); } @@ -223,7 +224,7 @@ write_new( VipsImage *in ) if( !(write = g_new0( Write, 1 )) ) return( NULL ); - write->in = in; + write->in = NULL; write->row_pointer = NULL; write->cinfo.err = jpeg_std_error( &write->eman.pub ); write->cinfo.dest = NULL; @@ -232,6 +233,14 @@ write_new( VipsImage *in ) write->eman.fp = NULL; write->inverted = NULL; + /* We must copy the input image since we will be updating the + * metadata when we write the exif. + */ + if( vips_copy( in, &write->in, NULL ) ) { + write_destroy( write ); + return( NULL ); + } + return( write ); } diff --git a/libvips/foreign/vips2webp.c b/libvips/foreign/vips2webp.c index 53558656..c04231f1 100644 --- a/libvips/foreign/vips2webp.c +++ b/libvips/foreign/vips2webp.c @@ -137,6 +137,7 @@ vips_webp_write_unset( VipsWebPWrite *write ) WebPMemoryWriterClear( &write->memory_writer ); VIPS_FREEF( WebPAnimEncoderDelete, write->enc ); VIPS_FREEF( WebPMuxDelete, write->mux ); + VIPS_UNREF( write->image ); } static int @@ -147,7 +148,7 @@ vips_webp_write_init( VipsWebPWrite *write, VipsImage *image, gboolean min_size, int kmin, int kmax, gboolean strip ) { - write->image = image; + write->image = NULL; write->Q = Q; write->lossless = lossless; write->preset = preset; @@ -163,6 +164,14 @@ vips_webp_write_init( VipsWebPWrite *write, VipsImage *image, write->enc = NULL; write->mux = NULL; + /* We must copy the input image since we will be updating the + * metadata when we write the exif. + */ + if( vips_copy( image, &write->image, NULL ) ) { + vips_webp_write_unset( write ); + return( -1 ); + } + if( !WebPConfigInit( &write->config ) ) { vips_webp_write_unset( write ); vips_error( "vips2webp", @@ -390,14 +399,14 @@ write_webp_anim( VipsWebPWrite *write, VipsImage *image, int page_height ) } static int -write_webp( VipsWebPWrite *write, VipsImage *image ) +write_webp( VipsWebPWrite *write ) { - int page_height = vips_image_get_page_height( image ); + int page_height = vips_image_get_page_height( write->image ); - if( page_height < image->Ysize ) - return( write_webp_anim( write, image, page_height ) ); + if( page_height < write->image->Ysize ) + return( write_webp_anim( write, write->image, page_height ) ); else - return( write_webp_single( write, image ) ); + return( write_webp_single( write, write->image ) ); } static void @@ -437,7 +446,7 @@ vips_webp_set_chunk( VipsWebPWrite *write, } static int -vips_webp_add_chunks( VipsWebPWrite *write, VipsImage *image ) +vips_webp_add_chunks( VipsWebPWrite *write ) { int i; @@ -445,11 +454,11 @@ vips_webp_add_chunks( VipsWebPWrite *write, VipsImage *image ) const char *vips_name = vips__webp_names[i].vips; const char *webp_name = vips__webp_names[i].webp; - if( vips_image_get_typeof( image, vips_name ) ) { + if( vips_image_get_typeof( write->image, vips_name ) ) { const void *data; size_t length; - if( vips_image_get_blob( image, + if( vips_image_get_blob( write->image, vips_name, &data, &length ) || vips_webp_set_chunk( write, webp_name, data, length ) ) @@ -461,13 +470,13 @@ vips_webp_add_chunks( VipsWebPWrite *write, VipsImage *image ) } static int -vips_webp_add_metadata( VipsWebPWrite *write, VipsImage *image, gboolean strip ) +vips_webp_add_metadata( VipsWebPWrite *write ) { WebPData data; /* Rebuild the EXIF block, if any, ready for writing. */ - if( vips__exif_update( image ) ) + if( vips__exif_update( write->image ) ) return( -1 ); data.bytes = write->memory_writer.mem; @@ -480,10 +489,10 @@ vips_webp_add_metadata( VipsWebPWrite *write, VipsImage *image, gboolean strip ) return( -1 ); } - if( vips_image_get_typeof( image, "gif-loop" ) ) { + if( vips_image_get_typeof( write->image, "gif-loop" ) ) { int gif_loop; - if( vips_image_get_int( image, "gif-loop", &gif_loop ) ) + if( vips_image_get_int( write->image, "gif-loop", &gif_loop ) ) return( -1 ); vips_webp_set_count( write, gif_loop ); @@ -491,8 +500,8 @@ vips_webp_add_metadata( VipsWebPWrite *write, VipsImage *image, gboolean strip ) /* Add extra metadata. */ - if( !strip && - vips_webp_add_chunks( write, image ) ) + if( !write->strip && + vips_webp_add_chunks( write ) ) return( -1 ); if( WebPMuxAssemble( write->mux, &data ) != WEBP_MUX_OK ) { @@ -524,12 +533,12 @@ vips__webp_write_stream( VipsImage *image, VipsStreamo *streamo, alpha_q, reduction_effort, min_size, kmin, kmax, strip ) ) return( -1 ); - if( write_webp( &write, image ) ) { + if( write_webp( &write ) ) { vips_webp_write_unset( &write ); return( -1 ); } - if( vips_webp_add_metadata( &write, image, strip ) ) { + if( vips_webp_add_metadata( &write ) ) { vips_webp_write_unset( &write ); return( -1 ); } diff --git a/libvips/iofuncs/header.c b/libvips/iofuncs/header.c index 449bca95..6c84d221 100644 --- a/libvips/iofuncs/header.c +++ b/libvips/iofuncs/header.c @@ -1023,7 +1023,7 @@ vips_image_set( VipsImage *image, const char *name, GValue *value ) /* If this image is shared, block metadata changes. */ if( G_OBJECT( image )->ref_count > 1 ) { - g_info( "can't set metadata \"%s\" on shared image", name ); + g_warning( "can't set metadata \"%s\" on shared image", name ); return; } @@ -1223,7 +1223,8 @@ vips_image_remove( VipsImage *image, const char *name ) /* If this image is shared, block metadata changes. */ if( G_OBJECT( image )->ref_count > 1 ) { - g_info( "can't remove metadata \"%s\" on shared image", name ); + g_warning( "can't remove metadata \"%s\" on shared image", + name ); return( FALSE ); }