From d2b794ec359cd61cfbb01d1db466105c322c8f71 Mon Sep 17 00:00:00 2001 From: elad laufer Date: Mon, 17 Feb 2020 11:29:57 +0200 Subject: [PATCH 1/7] - use a single enum and switch that replaces no_subsample, force_subsample --- libvips/foreign/jpegsave.c | 21 +++++++++--------- libvips/foreign/pforeign.h | 2 +- libvips/foreign/vips2jpeg.c | 37 +++++++++++++++++++------------- libvips/include/vips/enumtypes.h | 2 ++ libvips/include/vips/foreign.h | 12 +++++++++++ libvips/iofuncs/enumtypes.c | 18 ++++++++++++++++ test/test-suite/test_foreign.py | 15 +++++++------ 7 files changed, 75 insertions(+), 32 deletions(-) diff --git a/libvips/foreign/jpegsave.c b/libvips/foreign/jpegsave.c index 672c2394..4ea93c8f 100644 --- a/libvips/foreign/jpegsave.c +++ b/libvips/foreign/jpegsave.c @@ -79,7 +79,7 @@ typedef struct _VipsForeignSaveJpeg { /* Force chroma subsampling, if Q >= 90 then subsampling is disabled, use this flag to force it */ - gboolean force_subsample; + VipsForeignJpegSubsample subsample_mode; /* Apply trellis quantisation to each 8x8 block. */ @@ -198,12 +198,13 @@ vips_foreign_save_jpeg_class_init( VipsForeignSaveJpegClass *class ) G_STRUCT_OFFSET( VipsForeignSaveJpeg, quant_table ), 0, 8, 0 ); - VIPS_ARG_BOOL( class, "force_subsample", 19, - _( "Force subsample" ), - _( "Force chroma subsample" ), + VIPS_ARG_ENUM( class, "subsample_mode", 19, + _( "Subsample mode" ), + _( "Select chroma subsample operation mode" ), VIPS_ARGUMENT_OPTIONAL_INPUT, - G_STRUCT_OFFSET( VipsForeignSaveJpeg, force_subsample ), - FALSE ); + G_STRUCT_OFFSET( VipsForeignSaveJpeg, subsample_mode ), + VIPS_TYPE_FOREIGN_JPEG_SUBSAMPLE, + VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO ); } static void @@ -240,7 +241,7 @@ vips_foreign_save_jpeg_target_build( VipsObject *object ) jpeg->Q, jpeg->profile, jpeg->optimize_coding, jpeg->interlace, save->strip, jpeg->no_subsample, jpeg->trellis_quant, jpeg->overshoot_deringing, - jpeg->optimize_scans, jpeg->quant_table, jpeg->force_subsample ) ) + jpeg->optimize_scans, jpeg->quant_table, jpeg->subsample_mode ) ) return( -1 ); return( 0 ); @@ -307,7 +308,7 @@ vips_foreign_save_jpeg_file_build( VipsObject *object ) jpeg->Q, jpeg->profile, jpeg->optimize_coding, jpeg->interlace, save->strip, jpeg->no_subsample, jpeg->trellis_quant, jpeg->overshoot_deringing, - jpeg->optimize_scans, jpeg->quant_table, jpeg->force_subsample ) ) { + jpeg->optimize_scans, jpeg->quant_table, jpeg->subsample_mode ) ) { VIPS_UNREF( target ); return( -1 ); } @@ -377,7 +378,7 @@ vips_foreign_save_jpeg_buffer_build( VipsObject *object ) jpeg->Q, jpeg->profile, jpeg->optimize_coding, jpeg->interlace, save->strip, jpeg->no_subsample, jpeg->trellis_quant, jpeg->overshoot_deringing, - jpeg->optimize_scans, jpeg->quant_table, jpeg->force_subsample ) ) { + jpeg->optimize_scans, jpeg->quant_table, jpeg->subsample_mode ) ) { VIPS_UNREF( target ); return( -1 ); } @@ -450,7 +451,7 @@ vips_foreign_save_jpeg_mime_build( VipsObject *object ) jpeg->Q, jpeg->profile, jpeg->optimize_coding, jpeg->interlace, save->strip, jpeg->no_subsample, jpeg->trellis_quant, jpeg->overshoot_deringing, - jpeg->optimize_scans, jpeg->quant_table, jpeg->force_subsample ) ) { + jpeg->optimize_scans, jpeg->quant_table, jpeg->subsample_mode ) ) { VIPS_UNREF( target ); return( -1 ); } diff --git a/libvips/foreign/pforeign.h b/libvips/foreign/pforeign.h index 8d536329..069013cf 100644 --- a/libvips/foreign/pforeign.h +++ b/libvips/foreign/pforeign.h @@ -166,7 +166,7 @@ int vips__jpeg_write_target( VipsImage *in, VipsTarget *target, gboolean optimize_coding, gboolean progressive, gboolean strip, gboolean no_subsample, gboolean trellis_quant, gboolean overshoot_deringing, gboolean optimize_scans, - int quant_table, gboolean force_subsample ); + int quant_table, VipsForeignJpegSubsample subsample_mode ); int vips__jpeg_read_source( VipsSource *source, VipsImage *out, gboolean header_only, int shrink, int fail, gboolean autorotate ); diff --git a/libvips/foreign/vips2jpeg.c b/libvips/foreign/vips2jpeg.c index 97f8a3a9..0627aceb 100644 --- a/libvips/foreign/vips2jpeg.c +++ b/libvips/foreign/vips2jpeg.c @@ -481,7 +481,7 @@ write_vips( Write *write, int qfac, const char *profile, gboolean optimize_coding, gboolean progressive, gboolean strip, gboolean no_subsample, gboolean trellis_quant, gboolean overshoot_deringing, gboolean optimize_scans, int quant_table, - gboolean force_subsample) + VipsForeignJpegSubsample subsample_mode) { VipsImage *in; J_COLOR_SPACE space; @@ -628,18 +628,25 @@ write_vips( Write *write, int qfac, const char *profile, if( progressive ) jpeg_simple_progression( &write->cinfo ); - /* Turn off chroma subsampling. Follow IM and do it automatically for - * high Q. - */ - if( !force_subsample && ( - no_subsample || - qfac >= 90 )) { - int i; - - for( i = 0; i < in->Bands; i++ ) { - write->cinfo.comp_info[i].h_samp_factor = 1; - write->cinfo.comp_info[i].v_samp_factor = 1; - } + switch (subsample_mode) { + case VIPS_FOREIGN_JPEG_SUBSAMPLE_ON: + break; + case VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO: + /* Turn off chroma subsampling. Follow IM and do it automatically for + * high Q. + */ + if( !(no_subsample || + qfac >= 90) ) { + break; + } + case VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF: + default: { + int i; + for (i = 0; i < in->Bands; i++) { + write->cinfo.comp_info[i].h_samp_factor = 1; + write->cinfo.comp_info[i].v_samp_factor = 1; + } + } } /* Don't write the APP0 JFIF headers if we are stripping. @@ -778,7 +785,7 @@ vips__jpeg_write_target( VipsImage *in, VipsTarget *target, gboolean optimize_coding, gboolean progressive, gboolean strip, gboolean no_subsample, gboolean trellis_quant, gboolean overshoot_deringing, gboolean optimize_scans, int quant_table, - gboolean force_subsample) + VipsForeignJpegSubsample subsample_mode) { Write *write; @@ -805,7 +812,7 @@ vips__jpeg_write_target( VipsImage *in, VipsTarget *target, if( write_vips( write, Q, profile, optimize_coding, progressive, strip, no_subsample, trellis_quant, overshoot_deringing, optimize_scans, - quant_table, force_subsample ) ) { + quant_table, subsample_mode ) ) { write_destroy( write ); return( -1 ); } diff --git a/libvips/include/vips/enumtypes.h b/libvips/include/vips/enumtypes.h index 2de73711..bc7fbb6d 100644 --- a/libvips/include/vips/enumtypes.h +++ b/libvips/include/vips/enumtypes.h @@ -58,6 +58,8 @@ GType vips_foreign_flags_get_type (void) G_GNUC_CONST; #define VIPS_TYPE_FOREIGN_FLAGS (vips_foreign_flags_get_type()) GType vips_saveable_get_type (void) G_GNUC_CONST; #define VIPS_TYPE_SAVEABLE (vips_saveable_get_type()) +GType vips_foreign_jpeg_subsample_get_type (void) G_GNUC_CONST; +#define VIPS_TYPE_FOREIGN_JPEG_SUBSAMPLE (vips_foreign_jpeg_subsample_get_type()) GType vips_foreign_webp_preset_get_type (void) G_GNUC_CONST; #define VIPS_TYPE_FOREIGN_WEBP_PRESET (vips_foreign_webp_preset_get_type()) GType vips_foreign_tiff_compression_get_type (void) G_GNUC_CONST; diff --git a/libvips/include/vips/foreign.h b/libvips/include/vips/foreign.h index 1f84f4f9..002880c2 100644 --- a/libvips/include/vips/foreign.h +++ b/libvips/include/vips/foreign.h @@ -364,6 +364,18 @@ int vips_vipssave( VipsImage *in, const char *filename, ... ) int vips_openslideload( const char *filename, VipsImage **out, ... ) __attribute__((sentinel)); +/** + * VipsForeignJpegSubsample: + * @VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO: default preset + * @VIPS_FOREIGN_JPEG_SUBSAMPLE_ON: always perform subsampling + * @VIPS_FOREIGN_JPEG_SUBSAMPLE_ON: never perform subsampling + */ +typedef enum { + VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO, + VIPS_FOREIGN_JPEG_SUBSAMPLE_ON, + VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF +} VipsForeignJpegSubsample; + int vips_jpegload( const char *filename, VipsImage **out, ... ) __attribute__((sentinel)); int vips_jpegload_buffer( void *buf, size_t len, VipsImage **out, ... ) diff --git a/libvips/iofuncs/enumtypes.c b/libvips/iofuncs/enumtypes.c index 767a43f9..e9069cd2 100644 --- a/libvips/iofuncs/enumtypes.c +++ b/libvips/iofuncs/enumtypes.c @@ -499,6 +499,24 @@ vips_saveable_get_type( void ) return( etype ); } GType +vips_foreign_jpeg_subsample_get_type( void ) +{ + static GType etype = 0; + + if( etype == 0 ) { + static const GEnumValue values[] = { + {VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO, "VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO", "auto"}, + {VIPS_FOREIGN_JPEG_SUBSAMPLE_ON, "VIPS_FOREIGN_JPEG_SUBSAMPLE_ON", "on"}, + {VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF, "VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF", "off"}, + {0, NULL, NULL} + }; + + etype = g_enum_register_static( "VipsForeignJpegSubsample", values ); + } + + return( etype ); +} +GType vips_foreign_webp_preset_get_type( void ) { static GType etype = 0; diff --git a/test/test-suite/test_foreign.py b/test/test-suite/test_foreign.py index 9dbc1e6b..b32c1da1 100644 --- a/test/test-suite/test_foreign.py +++ b/test/test-suite/test_foreign.py @@ -259,14 +259,17 @@ class TestForeign: im = pyvips.Image.new_from_file(JPEG_FILE) # higher Q should mean a bigger buffer - b1 = im.jpegsave_buffer(Q=10) - b2 = im.jpegsave_buffer(Q=90) - assert len(b2) > len(b1) + q10 = im.jpegsave_buffer(Q=10) + q10o = im.jpegsave_buffer(Q=10, subsample_mode=2) + q90 = im.jpegsave_buffer(Q=90) + assert len(q90) > len(q10) + assert len(q10o) > len(q10) # force subsampling should result in smaller buffer - b1 = im.jpegsave_buffer(Q=90, force_subsample=True) - b2 = im.jpegsave_buffer(Q=90) - assert len(b2) > len(b1) + q90s = im.jpegsave_buffer(Q=90, subsample_mode=1) + q90a = im.jpegsave_buffer(Q=90, subsample_mode=0) + assert len(q90) > len(q90s) + assert len(q90) == len(q90a) @skip_if_no("jpegload") def test_truncated(self): From 00bf3269bd69d39716e0116ccf709ee709fc3f57 Mon Sep 17 00:00:00 2001 From: elad laufer Date: Mon, 17 Feb 2020 17:13:01 +0200 Subject: [PATCH 2/7] - fix comment --- libvips/include/vips/foreign.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libvips/include/vips/foreign.h b/libvips/include/vips/foreign.h index 002880c2..0f25c52a 100644 --- a/libvips/include/vips/foreign.h +++ b/libvips/include/vips/foreign.h @@ -368,7 +368,7 @@ int vips_openslideload( const char *filename, VipsImage **out, ... ) * VipsForeignJpegSubsample: * @VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO: default preset * @VIPS_FOREIGN_JPEG_SUBSAMPLE_ON: always perform subsampling - * @VIPS_FOREIGN_JPEG_SUBSAMPLE_ON: never perform subsampling + * @VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF: never perform subsampling */ typedef enum { VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO, From 494d8876eb4355d1051b9db04d10e1e6bc4d0c19 Mon Sep 17 00:00:00 2001 From: elad laufer Date: Mon, 17 Feb 2020 17:15:43 +0200 Subject: [PATCH 3/7] - don't skimp on names --- test/test-suite/test_foreign.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/test-suite/test_foreign.py b/test/test-suite/test_foreign.py index b32c1da1..6cc6c792 100644 --- a/test/test-suite/test_foreign.py +++ b/test/test-suite/test_foreign.py @@ -260,16 +260,16 @@ class TestForeign: # higher Q should mean a bigger buffer q10 = im.jpegsave_buffer(Q=10) - q10o = im.jpegsave_buffer(Q=10, subsample_mode=2) + q10_subsample_off = im.jpegsave_buffer(Q=10, subsample_mode=2) q90 = im.jpegsave_buffer(Q=90) assert len(q90) > len(q10) - assert len(q10o) > len(q10) + assert len(q10_subsample_off) > len(q10) # force subsampling should result in smaller buffer - q90s = im.jpegsave_buffer(Q=90, subsample_mode=1) - q90a = im.jpegsave_buffer(Q=90, subsample_mode=0) - assert len(q90) > len(q90s) - assert len(q90) == len(q90a) + q90_subsample_on = im.jpegsave_buffer(Q=90, subsample_mode=1) + q90_subsample_auto = im.jpegsave_buffer(Q=90, subsample_mode=0) + assert len(q90) > len(q90_subsample_on) + assert len(q90) == len(q90_subsample_auto) @skip_if_no("jpegload") def test_truncated(self): From 408f3b08cefd139d92d177e005543f9fcbebc5f8 Mon Sep 17 00:00:00 2001 From: elad laufer Date: Tue, 18 Feb 2020 10:54:46 +0200 Subject: [PATCH 4/7] - simplified subsampling selection by setting subsample_mode=VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF when no_subsample=1 --- libvips/foreign/jpegsave.c | 1 - libvips/foreign/vips2jpeg.c | 15 ++++++++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/libvips/foreign/jpegsave.c b/libvips/foreign/jpegsave.c index 4ea93c8f..9fe952e5 100644 --- a/libvips/foreign/jpegsave.c +++ b/libvips/foreign/jpegsave.c @@ -267,7 +267,6 @@ vips_foreign_save_jpeg_target_class_init( VIPS_ARGUMENT_REQUIRED_INPUT, G_STRUCT_OFFSET( VipsForeignSaveJpegTarget, target ), VIPS_TYPE_TARGET ); - } static void diff --git a/libvips/foreign/vips2jpeg.c b/libvips/foreign/vips2jpeg.c index 0627aceb..d31d80a6 100644 --- a/libvips/foreign/vips2jpeg.c +++ b/libvips/foreign/vips2jpeg.c @@ -479,8 +479,8 @@ write_jpeg_block( VipsRegion *region, VipsRect *area, void *a ) static int write_vips( Write *write, int qfac, const char *profile, gboolean optimize_coding, gboolean progressive, gboolean strip, - gboolean no_subsample, gboolean trellis_quant, - gboolean overshoot_deringing, gboolean optimize_scans, int quant_table, + gboolean trellis_quant, gboolean overshoot_deringing, + gboolean optimize_scans, int quant_table, VipsForeignJpegSubsample subsample_mode) { VipsImage *in; @@ -635,8 +635,7 @@ write_vips( Write *write, int qfac, const char *profile, /* Turn off chroma subsampling. Follow IM and do it automatically for * high Q. */ - if( !(no_subsample || - qfac >= 90) ) { + if(qfac < 90) { break; } case VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF: @@ -807,10 +806,16 @@ vips__jpeg_write_target( VipsImage *in, VipsTarget *target, */ target_dest( &write->cinfo, target ); + /* Retain old behavior for now + */ + if (no_subsample) { + subsample_mode = VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF; + } + /* Convert! Write errors come back here as an error return. */ if( write_vips( write, - Q, profile, optimize_coding, progressive, strip, no_subsample, + Q, profile, optimize_coding, progressive, strip, trellis_quant, overshoot_deringing, optimize_scans, quant_table, subsample_mode ) ) { write_destroy( write ); From c866cf480e7ee90e6707e904735966ed36cb54b2 Mon Sep 17 00:00:00 2001 From: elad laufer Date: Tue, 18 Feb 2020 11:05:16 +0200 Subject: [PATCH 5/7] - styling --- libvips/foreign/vips2jpeg.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libvips/foreign/vips2jpeg.c b/libvips/foreign/vips2jpeg.c index d31d80a6..4b90a0ba 100644 --- a/libvips/foreign/vips2jpeg.c +++ b/libvips/foreign/vips2jpeg.c @@ -635,7 +635,7 @@ write_vips( Write *write, int qfac, const char *profile, /* Turn off chroma subsampling. Follow IM and do it automatically for * high Q. */ - if(qfac < 90) { + if( qfac < 90 ) { break; } case VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF: @@ -808,7 +808,7 @@ vips__jpeg_write_target( VipsImage *in, VipsTarget *target, /* Retain old behavior for now */ - if (no_subsample) { + if( no_subsample ) { subsample_mode = VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF; } From 65b1a3eac2c2187035361ad53403e22aeb0ea91c Mon Sep 17 00:00:00 2001 From: elad laufer Date: Tue, 18 Feb 2020 11:09:18 +0200 Subject: [PATCH 6/7] - add test case --- test/test-suite/test_foreign.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/test-suite/test_foreign.py b/test/test-suite/test_foreign.py index 6cc6c792..f07c8812 100644 --- a/test/test-suite/test_foreign.py +++ b/test/test-suite/test_foreign.py @@ -261,9 +261,11 @@ class TestForeign: # higher Q should mean a bigger buffer q10 = im.jpegsave_buffer(Q=10) q10_subsample_off = im.jpegsave_buffer(Q=10, subsample_mode=2) + q10_subsample_no = im.jpegsave_buffer(Q=10, no_subsample=1) q90 = im.jpegsave_buffer(Q=90) assert len(q90) > len(q10) assert len(q10_subsample_off) > len(q10) + assert len(q10_subsample_off) == len(q10_subsample_no) # force subsampling should result in smaller buffer q90_subsample_on = im.jpegsave_buffer(Q=90, subsample_mode=1) From 838b5e74588e4613d8c69dc59dba6a248b40b128 Mon Sep 17 00:00:00 2001 From: elad laufer Date: Tue, 18 Feb 2020 11:33:29 +0200 Subject: [PATCH 7/7] - add test case --- test/test-suite/test_foreign.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/test-suite/test_foreign.py b/test/test-suite/test_foreign.py index f07c8812..2f17cc2c 100644 --- a/test/test-suite/test_foreign.py +++ b/test/test-suite/test_foreign.py @@ -262,10 +262,12 @@ class TestForeign: q10 = im.jpegsave_buffer(Q=10) q10_subsample_off = im.jpegsave_buffer(Q=10, subsample_mode=2) q10_subsample_no = im.jpegsave_buffer(Q=10, no_subsample=1) + q10_subsample_no_and_subsample_mode_on = im.jpegsave_buffer(Q=10, no_subsample=1, subsample_mode=1) q90 = im.jpegsave_buffer(Q=90) assert len(q90) > len(q10) assert len(q10_subsample_off) > len(q10) assert len(q10_subsample_off) == len(q10_subsample_no) + assert len(q10_subsample_no_and_subsample_mode_on) == len(q10_subsample_no) # force subsampling should result in smaller buffer q90_subsample_on = im.jpegsave_buffer(Q=90, subsample_mode=1)