From 801111a2fa98690cfb41106ab890a79e307b713a Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Tue, 8 Sep 2020 13:50:14 +0100 Subject: [PATCH 1/2] better dint rules We had some special cases coded for dhint inheritance, but they could fail in some edge cases. Revert to something simpler and more predictable. see https://github.com/libvips/libvips/issues/1810 --- ChangeLog | 1 + libvips/iofuncs/generate.c | 29 +++++++++++------------------ 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/ChangeLog b/ChangeLog index f5181adb..eb41e6d2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,6 @@ 6/9/20 started 8.10.2 - update magicksave/load profile handling [kelilevi] +- better demand hint rules [kaas3000] 9/8/20 started 8.10.1 - fix markdown -> xml conversion in doc generation diff --git a/libvips/iofuncs/generate.c b/libvips/iofuncs/generate.c index 8e8e53a4..575c0f98 100644 --- a/libvips/iofuncs/generate.c +++ b/libvips/iofuncs/generate.c @@ -297,25 +297,18 @@ vips__demand_hint_array( VipsImage *image, if( in[i]->dhint == VIPS_DEMAND_STYLE_ANY ) nany++; + /* Find the most restrictive of all the hints available to us. + * + * We have tried to be smarter about this in the past -- for example, + * detecting all ANY inputs and ignoring the hint in this case, but + * there are inevitably odd cases which cause problems. For example, + * new_from_memory, resize, affine, write_to_memory would run with + * FATSTRIP. + */ set_hint = hint; - if( len == 0 ) - /* No input images? Just set the requested hint. We don't - * force ANY, since the operation might be something like - * tiled read of an EXR image, where we certainly don't want - * ANY. - */ - ; - else if( nany == len ) - /* Special case: if all the inputs are ANY, then output can - * be ANY regardless of what this function wants. - */ - set_hint = VIPS_DEMAND_STYLE_ANY; - else - /* Find the most restrictive of all the hints available to us. - */ - for( i = 0; i < len; i++ ) - set_hint = (VipsDemandStyle) VIPS_MIN( - (int) set_hint, (int) in[i]->dhint ); + for( i = 0; i < len; i++ ) + set_hint = (VipsDemandStyle) VIPS_MIN( + (int) set_hint, (int) in[i]->dhint ); image->dhint = set_hint; From 348e5e1523b2e33e78ed2bd391ee27442351962d Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Mon, 14 Sep 2020 12:33:31 +0100 Subject: [PATCH 2/2] don't set JFIF res if we will set EXIF res Some JPEG loaders give priority to JFIF resolution over EXIF resolution tags. This patch makes libvips not write the JFIF res tags if it will be writing the EXIF res tags. See https://github.com/libvips/ruby-vips/issues/247 --- ChangeLog | 1 + libvips/foreign/jpeg2vips.c | 19 +++++++--- libvips/foreign/vips2jpeg.c | 73 ++++++++++++++++++++++++++++++++++--- 3 files changed, 81 insertions(+), 12 deletions(-) diff --git a/ChangeLog b/ChangeLog index eb41e6d2..6603f05a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,6 +1,7 @@ 6/9/20 started 8.10.2 - update magicksave/load profile handling [kelilevi] - better demand hint rules [kaas3000] +- in jpegsave, don't set JFIF resolution if we set EXIF resolution 9/8/20 started 8.10.1 - fix markdown -> xml conversion in doc generation diff --git a/libvips/foreign/jpeg2vips.c b/libvips/foreign/jpeg2vips.c index 80964ecc..08d8e7c3 100644 --- a/libvips/foreign/jpeg2vips.c +++ b/libvips/foreign/jpeg2vips.c @@ -108,6 +108,8 @@ * - revise for source IO * 5/5/20 angelmixu * - better handling of JFIF res unit 0 + * 13/9/20 + * - set resolution unit from JFIF */ /* @@ -506,6 +508,13 @@ read_jpeg_header( ReadJpeg *jpeg, VipsImage *out ) break; } +#ifdef DEBUG + if( cinfo->saw_JFIF_marker ) + printf( "read_jpeg_header: jfif _density %d, %d, unit %d\n", + cinfo->X_density, cinfo->Y_density, + cinfo->density_unit ); +#endif /*DEBUG*/ + /* Get the jfif resolution. exif may overwrite this later. Default to * 72dpi (as EXIF does). */ @@ -514,12 +523,6 @@ read_jpeg_header( ReadJpeg *jpeg, VipsImage *out ) if( cinfo->saw_JFIF_marker && cinfo->X_density != 1U && cinfo->Y_density != 1U ) { -#ifdef DEBUG - printf( "read_jpeg_header: jfif _density %d, %d, unit %d\n", - cinfo->X_density, cinfo->Y_density, - cinfo->density_unit ); -#endif /*DEBUG*/ - switch( cinfo->density_unit ) { case 0: /* X_density / Y_density gives the pixel aspect ratio. @@ -535,6 +538,8 @@ read_jpeg_header( ReadJpeg *jpeg, VipsImage *out ) */ xres = cinfo->X_density / 25.4; yres = cinfo->Y_density / 25.4; + vips_image_set_string( out, + VIPS_META_RESOLUTION_UNIT, "in" ); break; case 2: @@ -542,6 +547,8 @@ read_jpeg_header( ReadJpeg *jpeg, VipsImage *out ) */ xres = cinfo->X_density / 10.0; yres = cinfo->Y_density / 10.0; + vips_image_set_string( out, + VIPS_META_RESOLUTION_UNIT, "cm" ); break; default: diff --git a/libvips/foreign/vips2jpeg.c b/libvips/foreign/vips2jpeg.c index c748fad8..83495072 100644 --- a/libvips/foreign/vips2jpeg.c +++ b/libvips/foreign/vips2jpeg.c @@ -94,6 +94,8 @@ * - revise for target IO * 18/2/20 Elad-Laufer * - add subsample_mode, deprecate no_subsample + * 13/9/20 + * - only write JFIF resolution if we don't have EXIF */ /* @@ -412,6 +414,59 @@ write_profile_data (j_compress_ptr cinfo, } } +#ifndef HAVE_EXIF +/* Set the JFIF resolution from the vips xres/yres tags. + */ +static void +vips_jfif_resolution_from_image( struct jpeg_compress_struct *cinfo, + VipsImage *image ) +{ + int xres, yres; + const char *p; + int unit; + + /* Default to inches, more progs support it. + */ + unit = 1; + if( vips_image_get_typeof( image, VIPS_META_RESOLUTION_UNIT ) && + !vips_image_get_string( image, + VIPS_META_RESOLUTION_UNIT, &p ) ) { + if( vips_isprefix( "cm", p ) ) + unit = 2; + else if( vips_isprefix( "none", p ) ) + unit = 0; + } + + switch( unit ) { + case 0: + xres = VIPS_RINT( image->Xres ); + yres = VIPS_RINT( image->Yres ); + break; + + case 1: + xres = VIPS_RINT( image->Xres * 25.4 ); + yres = VIPS_RINT( image->Yres * 25.4 ); + break; + + case 2: + xres = VIPS_RINT( image->Xres * 10.0 ); + yres = VIPS_RINT( image->Yres * 10.0 ); + break; + + default: + g_assert_not_reached(); + break; + } + + VIPS_DEBUG_MSG( "vips_jfif_resolution_from_image: " + "setting xres = %d, yres = %d, unit = %d\n", xres, yres, unit ); + + cinfo->density_unit = unit; + cinfo->X_density = xres; + cinfo->Y_density = yres; +} +#endif /*HAVE_EXIF*/ + /* Write an ICC Profile from a file into the JPEG stream. */ static int @@ -643,18 +698,24 @@ write_vips( Write *write, int qfac, const char *profile, } } - /* Don't write the APP0 JFIF headers if we are stripping. + /* Only write the JFIF headers if we are not stripping and we have no + * EXIF. Some readers get confused if you set both. */ - if( strip ) - write->cinfo.write_JFIF_header = FALSE; + write->cinfo.write_JFIF_header = FALSE; +#ifndef HAVE_EXIF + if( !strip ) { + vips_jfif_resolution_from_image( &write->cinfo, write->in ); + write->cinfo.write_JFIF_header = TRUE; + } +#endif /*HAVE_EXIF*/ - /* Build compress tables. + /* Write app0 and build compress tables. */ jpeg_start_compress( &write->cinfo, TRUE ); - /* Write any APP markers we need. + /* All the other APP chunks come next. */ - if( !strip ) { + if( !strip ) { /* We need to rebuild the exif data block from any exif tags * on the image. */