Merge pull request #1483 from libvips/check-metadata-changes

block metadata changes on shared images

.travis.yml

@@ -15,7 +15,7 @@ script:
   - LD_LIBRARY_PATH=$PWD/libvips/.libs
     DYLD_LIBRARY_PATH=$PWD/libvips/.libs
     LD_PRELOAD=$ASAN_DSO
-    $PYTHON -m pytest -v test/test-suite
+    $PYTHON -m pytest -sv --log-cli-level=WARNING test/test-suite
 
 matrix:
   allow_failures:

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

libvips/convolution/convsep.c

@@ -64,7 +64,7 @@ vips_convsep_build( VipsObject *object )
 	VipsConvolution *convolution = (VipsConvolution *) object;
 	VipsConvsep *convsep = (VipsConvsep *) object;
 	VipsImage **t = (VipsImage **) 
-		vips_object_local_array( object, 3 );
+		vips_object_local_array( object, 4 );
 
 	VipsImage *in;
 
@@ -86,19 +86,19 @@ vips_convsep_build( VipsObject *object )
 		in = t[0];
 	}
 	else { 
-		if( vips_rot( convolution->M, &t[0], VIPS_ANGLE_D90, NULL ) )
+		/* Take a copy, since we must set the offset.
-			return( -1 ); 
-
-		/* We must only add the offset once.
 		 */
-		vips_image_set_double( t[0], "offset", 0 );
+		if( vips_rot( convolution->M, &t[0], VIPS_ANGLE_D90, NULL ) ||
+			vips_copy( t[0], &t[3], NULL ) )
+			return( -1 ); 
+		vips_image_set_double( t[3], "offset", 0 );
 
 		if( vips_conv( in, &t[1], convolution->M, 
 				"precision", convsep->precision,
 				"layers", convsep->layers,
 				"cluster", convsep->cluster,
 				NULL ) ||
-			vips_conv( t[1], &t[2], t[0], 
+			vips_conv( t[1], &t[2], t[3], 
 				"precision", convsep->precision,
 				"layers", convsep->layers,
 				"cluster", convsep->cluster,

libvips/foreign/dzsave.c

@@ -590,17 +590,28 @@ write_image( VipsForeignSaveDz *dz,
 {
 	VipsObjectClass *class = VIPS_OBJECT_GET_CLASS( dz );
 
+	VipsImage *t;
 	void *buf;
 	size_t len;
 
+	/* We need to block progress signalling on individual image write, so
+	 * we need a copy of the tile in case it's shared (eg. associated
+	 * images).
+	 */
+	if( vips_copy( image, &t, NULL ) ) 
+		return( -1 );
+
 	/* We default to stripping all metadata. Only "no_strip" turns this
 	 * off. Very few people really want metadata on every tile.
 	 */
-	vips_image_set_int( image, "hide-progress", 1 );
+	vips_image_set_int( t, "hide-progress", 1 );
-	if( vips_image_write_to_buffer( image, format, &buf, &len,
+	if( vips_image_write_to_buffer( t, format, &buf, &len,
 		"strip", !dz->no_strip,
-		NULL ) )
+		NULL ) ) {
+		VIPS_UNREF( t );
 		return( -1 );
+	}
+	VIPS_UNREF( t );
 
 	/* gsf doesn't like more than one write active at once.
 	 */

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,15 @@ vips_foreign_save_heif_build( VipsObject *object )
 		build( object ) )
 		return( -1 );
 
+	/* Only rebuild exif if there's an EXIF block or we'll make a
+	 * default set of tags. EXIF is not required for heif.
+	 */
+	if( vips_copy( save->ready, &heif->image, NULL ) ) 
+		return( -1 );
+	if( vips_image_get_typeof( heif->image, VIPS_META_EXIF_NAME ) ) 
+		if( vips__exif_update( heif->image ) )
+			return( -1 );
+
 	error = heif_context_get_encoder_for_format( heif->ctx, 
 		(enum heif_compression_format) heif->compression, 
 		&heif->encoder );
@@ -328,16 +342,16 @@ vips_foreign_save_heif_build( VipsObject *object )
 	 * heif_encoder_list_parameters().
 	 */
 
-	heif->page_width = save->ready->Xsize;
+	heif->page_width = heif->image->Xsize;
-	heif->page_height = vips_image_get_page_height( save->ready );
+	heif->page_height = vips_image_get_page_height( heif->image );
-	heif->n_pages = save->ready->Ysize / heif->page_height;
+	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 +362,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 );
@@ -357,15 +371,9 @@ vips_foreign_save_heif_build( VipsObject *object )
 	heif->data = heif_image_get_plane( heif->img, 
 		heif_channel_interleaved, &heif->stride );
 
-	/* 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 ) )
-			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 );
 

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,12 @@ write_new( VipsImage *in )
 	write->eman.fp = NULL;
 	write->inverted = NULL;
 
+	if( vips_copy( in, &write->in, NULL ) ||
+		vips__exif_update( write->in ) ) { 
+		write_destroy( write );
+		return( NULL );
+	}
+
         return( write );
 }
 
@@ -322,8 +329,7 @@ write_xmp( Write *write )
 static int
 write_exif( Write *write )
 {
-	if( vips__exif_update( write->in ) ||
+	if( write_blob( write, VIPS_META_EXIF_NAME, JPEG_APP0 + 1 ) )
-		write_blob( write, VIPS_META_EXIF_NAME, JPEG_APP0 + 1 ) )
 		return( -1 );
 
 	return( 0 );

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;
 
+	/* Rebuild exif on image. We must do this on a copy. 
+	 */
+	if( vips_copy( image, &write->image, NULL ) ||
+		vips__exif_update( write->image ) ) {
+		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 )
+	if( page_height < write->image->Ysize )
-		return( write_webp_anim( write, image, page_height ) );
+		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,15 +470,10 @@ 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 ) )
-		return( -1 ); 
-
 	data.bytes = write->memory_writer.mem;
 	data.size = write->memory_writer.size;
 
@@ -480,10 +484,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 +495,8 @@ vips_webp_add_metadata( VipsWebPWrite *write, VipsImage *image, gboolean strip )
 
 	/* Add extra metadata.
 	 */
-	if( !strip &&
+	if( !write->strip &&
-		vips_webp_add_chunks( write, image ) ) 
+		vips_webp_add_chunks( write ) ) 
 		return( -1 );
 
 	if( WebPMuxAssemble( write->mux, &data ) != WEBP_MUX_OK ) {
@@ -524,12 +528,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 );
 	}

libvips/iofuncs/header.c

@@ -1020,6 +1020,13 @@ 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 );
 	(void) meta_new( image, name, value );
 
@@ -1213,6 +1220,14 @@ vips_image_get_typeof( const VipsImage *image, const char *name )
 gboolean
 vips_image_remove( VipsImage *image, const char *name )
 {
+        /* 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( FALSE );
+        }
+
 	if( image->meta && 
 		g_hash_table_remove( image->meta, name ) )
 		return( TRUE );

libvips/resample/thumbnail.c

@@ -520,7 +520,7 @@ static int
 vips_thumbnail_build( VipsObject *object )
 {
 	VipsThumbnail *thumbnail = VIPS_THUMBNAIL( object );
-	VipsImage **t = (VipsImage **) vips_object_local_array( object, 13 );
+	VipsImage **t = (VipsImage **) vips_object_local_array( object, 14 );
 	VipsInterpretation interpretation = thumbnail->linear ?
 		VIPS_INTERPRETATION_scRGB : VIPS_INTERPRETATION_sRGB; 
 
@@ -673,6 +673,9 @@ vips_thumbnail_build( VipsObject *object )
 		return( -1 );
 	in = t[4];
 
+	if( vips_copy( in, &t[13], NULL ) )
+		return( -1 );
+	in = t[13];
 	output_page_height = VIPS_RINT( preshrunk_page_height / vshrink );
 	vips_image_set_int( in, 
 		VIPS_META_PAGE_HEIGHT, output_page_height );

test/test-suite/test_foreign.py

@@ -23,10 +23,10 @@ class TestForeign:
         cls.tempdir = tempfile.mkdtemp()
 
         cls.colour = pyvips.Image.jpegload(JPEG_FILE)
-        cls.mono = cls.colour.extract_band(1)
+        cls.mono = cls.colour.extract_band(1).copy()
         # we remove the ICC profile: the RGB one will no longer be appropriate
         cls.mono.remove("icc-profile-data")
-        cls.rad = cls.colour.float2rad()
+        cls.rad = cls.colour.float2rad().copy()
         cls.rad.remove("icc-profile-data")
         cls.cmyk = cls.colour.bandjoin(cls.mono)
         cls.cmyk = cls.cmyk.copy(interpretation=pyvips.Interpretation.CMYK)
@@ -160,6 +160,7 @@ class TestForeign:
 
             # can remove orientation, save, load again, orientation
             # has reset
+            x = x.copy()
             x.remove("orientation")
 
             filename = temp_filename(self.tempdir, '.jpg')
@@ -303,6 +304,7 @@ class TestForeign:
         x = pyvips.Image.new_from_file(filename)
         y = x.get("orientation")
         assert y == 2
+        x = x.copy()
         x.remove("orientation")
 
         filename = temp_filename(self.tempdir, '.tif')
@@ -542,6 +544,7 @@ class TestForeign:
         if have("gifload"):
             x1 = pyvips.Image.new_from_file(GIF_ANIM_FILE, n=-1)
             w1 = x1.webpsave_buffer(Q=10)
+
             x2 = pyvips.Image.new_from_buffer(w1, "", n=-1)
             assert x1.width == x2.width
             assert x1.height == x2.height

test/test-suite/test_stream.py

@@ -26,10 +26,10 @@ class TestStream:
         cls.tempdir = tempfile.mkdtemp()
 
         cls.colour = pyvips.Image.jpegload(JPEG_FILE)
-        cls.mono = cls.colour.extract_band(1)
+        cls.mono = cls.colour.extract_band(1).copy()
         # we remove the ICC profile: the RGB one will no longer be appropriate
         cls.mono.remove("icc-profile-data")
-        cls.rad = cls.colour.float2rad()
+        cls.rad = cls.colour.float2rad().copy()
         cls.rad.remove("icc-profile-data")
         cls.cmyk = cls.colour.bandjoin(cls.mono)
         cls.cmyk = cls.cmyk.copy(interpretation=pyvips.Interpretation.CMYK)