deprecate heifload autorotate

heifload autorotate is now always on, and we always delete EXIF
orientation tags. If a HEIC image fails to rotate upright, at least it's
someone else's fault.

see https://github.com/libvips/libvips/pull/1680

ChangeLog

@@ -24,8 +24,9 @@
   behaviour with alpha channels
 - improve bioformats support with read and write of tiff subifd pyramids
 - thumbnail exploits subifd pyramids
-- handle all EXIF orientation cases, deprecate 
+- handle all EXIF orientation cases, deprecate vips_autorot_get_angle() 
-  vips_autorot_get_angle() [Elad-Laufer]
+  [Elad-Laufer]
+- deprecate heifload autorotate -- it's now always on
 
 24/4/20 started 8.9.3
 - better iiif tile naming [IllyaMoskvin]

configure.ac

@@ -928,18 +928,6 @@ if test x"$with_heif" = x"yes"; then
   LIBS="$save_LIBS"
 fi
 
-# fetch untransformed width/height added in 1.3.4
-if test x"$with_heif" = x"yes"; then
-  save_LIBS="$LIBS"
-  LIBS="$LIBS $HEIF_LIBS"
-  AC_CHECK_FUNCS(heif_image_handle_get_ispe_width,[
-     AC_DEFINE(HAVE_HEIF_IMAGE_HANDLE_GET_ISPE_WIDTH,1,
-	       [define if you have heif_image_handle_get_ispe_width.])
-   ],[]
-  )
-  LIBS="$save_LIBS"
-fi
-
 # heif_decoding_options.convert_hdr_to_8bit added in 1.7.0
 if test x"$with_heif" = x"yes"; then
   save_CFLAGS="$CFLAGS"

libvips/foreign/heifload.c

@@ -16,6 +16,8 @@
  * 	- restart after minimise
  * 15/3/20
  * 	- revise for new VipsSource API
+ * 10/5/20
+ * 	- deprecate autorotate -- it's too difficult to support properly
  */
 
 /*
@@ -99,6 +101,11 @@ typedef struct _VipsForeignLoadHeif {
 	gboolean thumbnail;
 
 	/* Apply any orientation tags in the header.
+	 *
+	 * This is deprecated and does nothing. Non-autorotated reads from
+	 * libheif are surprisingly hard to support well, since orientation can
+	 * be represented in several different ways in HEIC files and devices
+	 * vary in how they do this.
 	 */
 	gboolean autorotate;
 
@@ -364,7 +371,7 @@ vips_foreign_load_heif_set_header( VipsForeignLoadHeif *heif, VipsImage *out )
 #endif /*DEBUG*/
 	bands = heif->has_alpha ? 4 : 3;
 
-	/* FIXME .. need to test XMP and IPCT.
+	/* FIXME .. IPTC as well?
 	 */
 	n_metadata = heif_image_handle_get_list_of_metadata_block_IDs( 
 		heif->handle, NULL, id, VIPS_NUMBER( id ) );
@@ -414,10 +421,20 @@ vips_foreign_load_heif_set_header( VipsForeignLoadHeif *heif, VipsImage *out )
 		vips_image_set_blob( out, name, 
 			(VipsCallbackFn) NULL, data, length );
 
-		if( g_ascii_strcasecmp( type, "exif" ) == 0 ) 
+		/* image_set will automatically parse EXIF, if necessary.
-			(void) vips__exif_parse( out );
+		 */
 	}
 
+	/* We use libheif's autorotate, so we need to remove any EXIF
+	 * orientaion tags.
+	 *
+	 * According to the HEIF standard, EXIF orientation tags are only
+	 * informational and images should not be rotated because of them.
+	 * Unless we strip these tags, there's a danger downstream processing
+	 * could double-rotate.
+	 */
+	vips_autorot_remove_angle( out );
+
 #ifdef HAVE_HEIF_COLOR_PROFILE
 	enum heif_color_profile_type profile_type = 
 		heif_image_handle_get_color_profile_type( heif->handle );
@@ -480,13 +497,6 @@ vips_foreign_load_heif_set_header( VipsForeignLoadHeif *heif, VipsImage *out )
 	}
 #endif /*HAVE_HEIF_COLOR_PROFILE*/
 
-	/* If we are using libheif's autorotate, remove the exif one. 
-	 */
-#ifdef HAVE_HEIF_IMAGE_HANDLE_GET_ISPE_WIDTH
-	if( heif->autorotate )
-		vips_autorot_remove_angle( out );
-#endif /*HAVE_HEIF_IMAGE_HANDLE_GET_ISPE_WIDTH*/
-
 	vips_image_set_int( out, "heif-primary", heif->primary_page );
 	vips_image_set_int( out, "n-pages", heif->n_top );
 	if( vips_object_argument_isset( VIPS_OBJECT( heif ), "n" ) )
@@ -508,40 +518,6 @@ vips_foreign_load_heif_set_header( VipsForeignLoadHeif *heif, VipsImage *out )
 	return( 0 );
 }
 
-static int
-vips_foreign_load_heif_get_width( VipsForeignLoadHeif *heif, 
-	struct heif_image_handle *handle )
-{
-	int width;
-
-	/* _get_ipse_width() fetches the untransformed dimension, but was only
-	 * added in 1.3.4. Without it, we just use the transformed dimension
-	 * and have to autorotate.
-	 */
-	width = heif_image_handle_get_width( handle );
-#ifdef HAVE_HEIF_IMAGE_HANDLE_GET_ISPE_WIDTH
-	if( !heif->autorotate ) 
-		width = heif_image_handle_get_ispe_width( handle );
-#endif /*HAVE_HEIF_IMAGE_HANDLE_GET_ISPE_WIDTH*/
-
-	return( width );
-}
-
-static int
-vips_foreign_load_heif_get_height( VipsForeignLoadHeif *heif,
-	struct heif_image_handle *handle )
-{
-	int height;
-
-	height = heif_image_handle_get_height( handle );
-#ifdef HAVE_HEIF_IMAGE_HANDLE_GET_ISPE_WIDTH
-	if( !heif->autorotate )
-		height = heif_image_handle_get_ispe_height( handle );
-#endif /*HAVE_HEIF_IMAGE_HANDLE_GET_ISPE_WIDTH*/
-
-	return( height );
-}
-
 static int
 vips_foreign_load_heif_header( VipsForeignLoad *load )
 {
@@ -585,10 +561,6 @@ vips_foreign_load_heif_header( VipsForeignLoad *load )
 	}
 
 #ifdef DEBUG
-#ifdef HAVE_HEIF_IMAGE_HANDLE_GET_ISPE_WIDTH
-	if( !heif->autorotate )
-		printf( "using _get_ispe_width() / _height()\n" );
-#endif /*HAVE_HEIF_IMAGE_HANDLE_GET_ISPE_WIDTH*/
 	for( i = heif->page; i < heif->page + heif->n; i++ ) {
 		heif_item_id thumb_ids[1];
 		int n_items;
@@ -619,11 +591,9 @@ vips_foreign_load_heif_header( VipsForeignLoad *load )
 
 			printf( "  thumb %d\n", j );
 			printf( "    width = %d\n", 
-				vips_foreign_load_heif_get_width( heif, 
+				heif_image_handle_get_width( thumb_handle ) );
-					thumb_handle ) );
 			printf( "    height = %d\n", 
-				vips_foreign_load_heif_get_height( heif, 
+				heif_image_handle_get_height( thumb_handle ) );
-					thumb_handle ) );
 		}
 	}
 #endif /*DEBUG*/
@@ -633,18 +603,16 @@ vips_foreign_load_heif_header( VipsForeignLoad *load )
 	if( vips_foreign_load_heif_set_page( heif, 
 		heif->page, heif->thumbnail ) )
 		return( -1 );
-	heif->page_width = vips_foreign_load_heif_get_width( heif, 
+	heif->page_width = heif_image_handle_get_width( heif->handle );
-		heif->handle );
+	heif->page_height = heif_image_handle_get_height( heif->handle );
-	heif->page_height = vips_foreign_load_heif_get_height( heif, 
-		heif->handle );
 	for( i = heif->page + 1; i < heif->page + heif->n; i++ ) {
 		if( vips_foreign_load_heif_set_page( heif, 
 			i, heif->thumbnail ) )
 			return( -1 );
-		if( vips_foreign_load_heif_get_width( heif, 
+		if( heif_image_handle_get_width( heif->handle ) 
-				heif->handle ) != heif->page_width ||
+				!= heif->page_width ||
-			vips_foreign_load_heif_get_height( heif, 
+			heif_image_handle_get_height( heif->handle ) 
-				heif->handle ) != heif->page_height ) {
+				!= heif->page_height ) {
 			vips_error( class->nickname, "%s", 
 				_( "not all pages are the same size" ) ); 
 			return( -1 ); 
@@ -658,11 +626,9 @@ vips_foreign_load_heif_header( VipsForeignLoad *load )
 		if( vips_foreign_load_heif_set_page( heif, i, FALSE ) )
 			return( -1 );
 		printf( "    width = %d\n", 
-			vips_foreign_load_heif_get_width( heif, 
+			heif_image_handle_get_width( heif->handle ) );
-				heif->handle ) );
 		printf( "    height = %d\n", 
-			vips_foreign_load_heif_get_height( heif, 
+			heif_image_handle_get_height( heif->handle ) );
-				heif->handle ) );
 		printf( "    has_depth = %d\n", 
 			heif_image_handle_has_depth_image( heif->handle ) );
 		printf( "    has_alpha = %d\n", 
@@ -713,13 +679,7 @@ vips_foreign_load_heif_generate( VipsRegion *or,
 			heif_chroma_interleaved_RGBA :
 			heif_chroma_interleaved_RGB;
 
-		/* Only disable transforms if we have been able to fetch the
-		 * untransformed dimensions.
-		 */
 		options = heif_decoding_options_alloc();
-#ifdef HAVE_HEIF_IMAGE_HANDLE_GET_ISPE_WIDTH
-		options->ignore_transformations = !heif->autorotate;
-#endif /*HAVE_HEIF_IMAGE_HANDLE_GET_ISPE_WIDTH*/
 #ifdef HAVE_HEIF_DECODING_OPTIONS_CONVERT_HDR_TO_8BIT
 		/* VIPS_FORMAT_UCHAR is assumed so downsample HDR to 8bpc
 		 */
@@ -891,7 +851,7 @@ vips_foreign_load_heif_class_init( VipsForeignLoadHeifClass *class )
 	VIPS_ARG_BOOL( class, "autorotate", 21, 
 		_( "Autorotate" ), 
 		_( "Rotate image using exif orientation" ),
-		VIPS_ARGUMENT_OPTIONAL_INPUT,
+		VIPS_ARGUMENT_OPTIONAL_INPUT | VIPS_ARGUMENT_DEPRECATED,
 		G_STRUCT_OFFSET( VipsForeignLoadHeif, autorotate ),
 		FALSE );
 
@@ -913,9 +873,12 @@ vips_foreign_load_heif_read( void *data, size_t size, void *userdata )
 	gint64 result;
 
 	result = vips_source_read( heif->source, data, size );
+	/* On EOF, make a note of the file length.
+	 */
 	if( result == 0 &&
 		heif->length == -1 ) 
-		heif->length = vips_source_seek( heif->source, 0L, SEEK_CUR );
+		result = heif->length = 
+			vips_source_seek( heif->source, 0L, SEEK_CUR );
 	if( result < 0 ) 
 		return( -1 );
 
@@ -1211,7 +1174,6 @@ vips_foreign_load_heif_source_init( VipsForeignLoadHeifSource *source )
  * * @page: %gint, page (top-level image number) to read
  * * @n: %gint, load this many pages
  * * @thumbnail: %gboolean, fetch thumbnail instead of image
- * * @autorotate: %gboolean, rotate image upright during load 
  *
  * Read a HEIF image file into a VIPS image. 
  *
@@ -1228,17 +1190,6 @@ vips_foreign_load_heif_source_init( VipsForeignLoadHeifSource *source )
  * If @thumbnail is %TRUE, then fetch a stored thumbnail rather than the
  * image.
  *
- * Setting @autorotate to %TRUE will make the loader interpret the 
- * orientation tag and automatically rotate the image appropriately during
- * load. 
- *
- * If @autorotate is %FALSE, the metadata field #VIPS_META_ORIENTATION is set 
- * to the value of the orientation tag. Applications may read and interpret 
- * this field
- * as they wish later in processing. See vips_autorot(). Save
- * operations will use #VIPS_META_ORIENTATION, if present, to set the
- * orientation of output images. 
- *
  * See also: vips_image_new_from_file().
  *
  * Returns: 0 on success, -1 on error.
@@ -1268,7 +1219,6 @@ vips_heifload( const char *filename, VipsImage **out, ... )
  * * @page: %gint, page (top-level image number) to read
  * * @n: %gint, load this many pages
  * * @thumbnail: %gboolean, fetch thumbnail instead of image
- * * @autorotate: %gboolean, rotate image upright during load 
  *
  * Read a HEIF image file into a VIPS image. 
  * Exactly as vips_heifload(), but read from a memory buffer. 
@@ -1311,7 +1261,6 @@ vips_heifload_buffer( void *buf, size_t len, VipsImage **out, ... )
  * * @page: %gint, page (top-level image number) to read
  * * @n: %gint, load this many pages
  * * @thumbnail: %gboolean, fetch thumbnail instead of image
- * * @autorotate: %gboolean, rotate image upright during load 
  *
  * Exactly as vips_heifload(), but read from a source. 
  *

test/test-suite/helpers/helpers.py

@@ -36,7 +36,7 @@ DICOM_FILE = os.path.join(IMAGES, "dicom_test_image.dcm")
 BMP_FILE = os.path.join(IMAGES, "MARBLES.BMP")
 NIFTI_FILE = os.path.join(IMAGES, "avg152T1_LR_nifti.nii.gz")
 ICO_FILE = os.path.join(IMAGES, "favicon.ico")
-HEIC_FILE = os.path.join(IMAGES, "Example1.heic")
+HEIC_FILE = os.path.join(IMAGES, "heic-orientation-6.heic")
 
 unsigned_formats = [pyvips.BandFormat.UCHAR,
                     pyvips.BandFormat.USHORT,

test/test-suite/test_foreign.py

@@ -968,9 +968,9 @@ class TestForeign:
             a = im(10, 10)
             # different versions of HEIC decode have slightly different 
             # rounding
-            assert_almost_equal_objects(a, [75.0, 86.0, 81.0], threshold=2)
+            assert_almost_equal_objects(a, [197.0, 181.0, 158.0], threshold=2)
-            assert im.width == 4032
+            assert im.width == 3024
-            assert im.height == 3024
+            assert im.height == 4032
             assert im.bands == 3
 
         self.file_loader("heifload", HEIC_FILE, heif_valid)

test/test-suite/test_resample.py

@@ -171,9 +171,6 @@ class TestResample:
             im = pyvips.Image.new_from_file(HEIC_FILE)
             thumb = pyvips.Image.thumbnail(HEIC_FILE, 100)
 
-            # original is landscape
-            assert im.width > im.height
-
             # thumb should be portrait 
             assert thumb.width < thumb.height
             assert thumb.height == 100