From c626c9de14d474d7620c8a2fedf701d2b770e23a Mon Sep 17 00:00:00 2001
From: elad laufer <elad@wix.com>
Date: Sun, 16 Feb 2020 18:37:32 +0200
Subject: [PATCH 01/13] add force subsample argument that overrides the Q
 deduced subsampling directive

---
 libvips/foreign/jpegsave.c      | 18 ++++++++++++++----
 libvips/foreign/pforeign.h      |  2 +-
 libvips/foreign/vips2jpeg.c     | 13 ++++++++-----
 test/test-suite/test_foreign.py | 14 ++++++++++++++
 4 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/libvips/foreign/jpegsave.c b/libvips/foreign/jpegsave.c
index 3deea400..672c2394 100644
--- a/libvips/foreign/jpegsave.c
+++ b/libvips/foreign/jpegsave.c
@@ -77,6 +77,10 @@ typedef struct _VipsForeignSaveJpeg {
 	 */
 	gboolean no_subsample;
 
+    /* Force chroma subsampling, if Q >= 90 then subsampling is disabled, use this flag to force it
+    */
+    gboolean force_subsample;
+
 	/* Apply trellis quantisation to each 8x8 block.
 	 */
 	gboolean trellis_quant;
@@ -194,6 +198,12 @@ 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_ARGUMENT_OPTIONAL_INPUT,
+            G_STRUCT_OFFSET( VipsForeignSaveJpeg, force_subsample ),
+            FALSE );
 }
 
 static void
@@ -230,7 +240,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->optimize_scans, jpeg->quant_table, jpeg->force_subsample ) )
 		return( -1 );
 
 	return( 0 );
@@ -297,7 +307,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->optimize_scans, jpeg->quant_table, jpeg->force_subsample ) ) {
 		VIPS_UNREF( target );
 		return( -1 );
 	}
@@ -367,7 +377,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->optimize_scans, jpeg->quant_table, jpeg->force_subsample ) ) {
 		VIPS_UNREF( target );
 		return( -1 );
 	}
@@ -440,7 +450,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->optimize_scans, jpeg->quant_table, jpeg->force_subsample ) ) {
 		VIPS_UNREF( target );
 		return( -1 );
 	}
diff --git a/libvips/foreign/pforeign.h b/libvips/foreign/pforeign.h
index 77381f47..8d536329 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 );
+	int quant_table, gboolean force_subsample );
 
 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 103deff0..97f8a3a9 100644
--- a/libvips/foreign/vips2jpeg.c
+++ b/libvips/foreign/vips2jpeg.c
@@ -480,7 +480,8 @@ 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 overshoot_deringing, gboolean optimize_scans, int quant_table,
+	gboolean force_subsample)
 {
 	VipsImage *in;
 	J_COLOR_SPACE space;
@@ -630,8 +631,9 @@ 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( !force_subsample && (
+	        no_subsample ||
+	        qfac >= 90 )) {
 		int i;
 
 		for( i = 0; i < in->Bands; i++ ) { 
@@ -775,7 +777,8 @@ vips__jpeg_write_target( VipsImage *in, VipsTarget *target,
 	int Q, 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 overshoot_deringing, gboolean optimize_scans, int quant_table,
+	gboolean force_subsample)
 {
 	Write *write;
 
@@ -802,7 +805,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 ) ) {
+		quant_table, force_subsample ) ) {
 		write_destroy( write );
 		return( -1 );
 	}
diff --git a/test/test-suite/test_foreign.py b/test/test-suite/test_foreign.py
index cb4b4b21..9dbc1e6b 100644
--- a/test/test-suite/test_foreign.py
+++ b/test/test-suite/test_foreign.py
@@ -254,6 +254,20 @@ class TestForeign:
             # format area at the end
             assert y.startswith("hello world")
 
+    @skip_if_no("jpegload")
+    def test_jpegsave(self):
+        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)
+
+        # 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)
+
     @skip_if_no("jpegload")
     def test_truncated(self):
         # This should open (there's enough there for the header)

From d2b794ec359cd61cfbb01d1db466105c322c8f71 Mon Sep 17 00:00:00 2001
From: elad laufer <elad@wix.com>
Date: Mon, 17 Feb 2020 11:29:57 +0200
Subject: [PATCH 02/13] - 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 <elad@wix.com>
Date: Mon, 17 Feb 2020 17:13:01 +0200
Subject: [PATCH 03/13] - 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 <elad@wix.com>
Date: Mon, 17 Feb 2020 17:15:43 +0200
Subject: [PATCH 04/13] - 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 <elad@wix.com>
Date: Tue, 18 Feb 2020 10:54:46 +0200
Subject: [PATCH 05/13] - 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 <elad@wix.com>
Date: Tue, 18 Feb 2020 11:05:16 +0200
Subject: [PATCH 06/13] - 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 <elad@wix.com>
Date: Tue, 18 Feb 2020 11:09:18 +0200
Subject: [PATCH 07/13] - 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 <elad@wix.com>
Date: Tue, 18 Feb 2020 11:33:29 +0200
Subject: [PATCH 08/13] - 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)

From 41d79415b96c6ae25edd54d548c740687987f3d4 Mon Sep 17 00:00:00 2001
From: elad laufer <elad@wix.com>
Date: Tue, 18 Feb 2020 13:31:58 +0200
Subject: [PATCH 09/13] - argument deprecation flag - styling

---
 libvips/foreign/jpegsave.c  |  2 +-
 libvips/foreign/vips2jpeg.c | 20 ++++++++++----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/libvips/foreign/jpegsave.c b/libvips/foreign/jpegsave.c
index 9fe952e5..d5978cad 100644
--- a/libvips/foreign/jpegsave.c
+++ b/libvips/foreign/jpegsave.c
@@ -166,7 +166,7 @@ vips_foreign_save_jpeg_class_init( VipsForeignSaveJpegClass *class )
 	VIPS_ARG_BOOL( class, "no_subsample", 14,
 		_( "No subsample" ),
 		_( "Disable chroma subsample" ),
-		VIPS_ARGUMENT_OPTIONAL_INPUT,
+		VIPS_ARGUMENT_OPTIONAL_INPUT | VIPS_ARGUMENT_DEPRECATED,
 		G_STRUCT_OFFSET( VipsForeignSaveJpeg, no_subsample ),
 		FALSE );
 
diff --git a/libvips/foreign/vips2jpeg.c b/libvips/foreign/vips2jpeg.c
index 4b90a0ba..8f165ad8 100644
--- a/libvips/foreign/vips2jpeg.c
+++ b/libvips/foreign/vips2jpeg.c
@@ -628,20 +628,20 @@ write_vips( Write *write, int qfac, const char *profile,
 	if( progressive ) 
 		jpeg_simple_progression( &write->cinfo ); 
 
-	switch (subsample_mode) {
-        case VIPS_FOREIGN_JPEG_SUBSAMPLE_ON:
+	switch ( subsample_mode ) {
+    case VIPS_FOREIGN_JPEG_SUBSAMPLE_ON:
             break;
-        case VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO:
+    case VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO:
             /* Turn off chroma subsampling. Follow IM and do it automatically for
-            * high Q.
-            */
-            if( qfac < 90 ) {
+             * high Q.
+             */
+            if( qfac < 90 )
                 break;
-            }
-        case VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF:
-        default: {
+    case VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF:
+    default:
+        {
             int i;
-            for (i = 0; i < in->Bands; 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;
             }

From 22b3a0d161f27c8bc209922613cbbc8f2436a465 Mon Sep 17 00:00:00 2001
From: elad laufer <elad@wix.com>
Date: Tue, 18 Feb 2020 13:43:02 +0200
Subject: [PATCH 10/13] - use enum string

---
 test/test-suite/test_foreign.py | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/test/test-suite/test_foreign.py b/test/test-suite/test_foreign.py
index 2f17cc2c..8d24ca2c 100644
--- a/test/test-suite/test_foreign.py
+++ b/test/test-suite/test_foreign.py
@@ -260,18 +260,14 @@ 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)
-        q10_subsample_no_and_subsample_mode_on = im.jpegsave_buffer(Q=10, no_subsample=1, subsample_mode=1)
+        q10_subsample_off = im.jpegsave_buffer(Q=10, subsample_mode="off")
         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)
-        q90_subsample_auto = im.jpegsave_buffer(Q=90, subsample_mode=0)
+        q90_subsample_on = im.jpegsave_buffer(Q=90, subsample_mode="on")
+        q90_subsample_auto = im.jpegsave_buffer(Q=90, subsample_mode="auto")
         assert len(q90) > len(q90_subsample_on)
         assert len(q90) == len(q90_subsample_auto)
 

From 8749871c71e9f7909ce675d372cfaa7ca4b77d6d Mon Sep 17 00:00:00 2001
From: elad laufer <elad@wix.com>
Date: Tue, 18 Feb 2020 18:04:29 +0200
Subject: [PATCH 11/13] - remove no_subsample from inner calls

---
 libvips/foreign/jpegsave.c      | 43 ++++++++++++++++++++++++---------
 libvips/foreign/pforeign.h      |  6 ++---
 libvips/foreign/vips2jpeg.c     | 12 +++------
 test/test-suite/test_foreign.py | 24 ++++++++++++------
 4 files changed, 53 insertions(+), 32 deletions(-)

diff --git a/libvips/foreign/jpegsave.c b/libvips/foreign/jpegsave.c
index d5978cad..358b6702 100644
--- a/libvips/foreign/jpegsave.c
+++ b/libvips/foreign/jpegsave.c
@@ -114,6 +114,24 @@ static int bandfmt_jpeg[10] = {
    UC, UC, UC, UC, UC, UC, UC, UC, UC, UC
 };
 
+static int
+vips_foreign_save_jpeg_build( VipsObject *object )
+{
+    VipsForeignSaveJpeg *jpeg = (VipsForeignSaveJpeg *) object;
+
+    if( VIPS_OBJECT_CLASS( vips_foreign_save_jpeg_parent_class )->
+        build( object ) )
+        return( -1 );
+
+    /* no_subsample is deprecated, but we retain backwards compatibility
+     * new code should use subsample_mode
+     */
+    if( vips_object_argument_isset(object, "no_subsample") )
+        jpeg->subsample_mode = jpeg->no_subsample ? VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF : VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO;
+
+    return( 0 );
+}
+
 static void
 vips_foreign_save_jpeg_class_init( VipsForeignSaveJpegClass *class )
 {
@@ -127,6 +145,7 @@ vips_foreign_save_jpeg_class_init( VipsForeignSaveJpegClass *class )
 
 	object_class->nickname = "jpegsave_base";
 	object_class->description = _( "save jpeg" );
+    object_class->build = vips_foreign_save_jpeg_build;
 
 	foreign_class->suffs = vips__jpeg_suffs;
 
@@ -239,9 +258,9 @@ vips_foreign_save_jpeg_target_build( VipsObject *object )
 
 	if( vips__jpeg_write_target( save->ready, target->target,
 		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->subsample_mode ) )
+		jpeg->interlace, save->strip, jpeg->trellis_quant,
+		jpeg->overshoot_deringing, jpeg->optimize_scans,
+		jpeg->quant_table, jpeg->subsample_mode ) )
 		return( -1 );
 
 	return( 0 );
@@ -305,9 +324,9 @@ vips_foreign_save_jpeg_file_build( VipsObject *object )
 		return( -1 );
 	if( vips__jpeg_write_target( save->ready, target,
 		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->subsample_mode ) ) {
+		jpeg->interlace, save->strip, jpeg->trellis_quant,
+		jpeg->overshoot_deringing, jpeg->optimize_scans,
+		jpeg->quant_table, jpeg->subsample_mode ) ) {
 		VIPS_UNREF( target );
 		return( -1 );
 	}
@@ -375,9 +394,9 @@ vips_foreign_save_jpeg_buffer_build( VipsObject *object )
 
 	if( vips__jpeg_write_target( save->ready, target,
 		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->subsample_mode ) ) {
+		jpeg->interlace, save->strip, jpeg->trellis_quant,
+		jpeg->overshoot_deringing, jpeg->optimize_scans,
+		jpeg->quant_table, jpeg->subsample_mode ) ) {
 		VIPS_UNREF( target );
 		return( -1 );
 	}
@@ -448,9 +467,9 @@ vips_foreign_save_jpeg_mime_build( VipsObject *object )
 
 	if( vips__jpeg_write_target( save->ready, target,
 		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->subsample_mode ) ) {
+		jpeg->interlace, save->strip, jpeg->trellis_quant,
+		jpeg->overshoot_deringing, 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 069013cf..791d26e3 100644
--- a/libvips/foreign/pforeign.h
+++ b/libvips/foreign/pforeign.h
@@ -164,9 +164,9 @@ extern const char *vips__jpeg_suffs[];
 int vips__jpeg_write_target( VipsImage *in, VipsTarget *target,
 	int Q, 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, VipsForeignJpegSubsample subsample_mode );
+	gboolean trellis_quant, gboolean overshoot_deringing,
+	gboolean optimize_scans, 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 8f165ad8..62c1cbbf 100644
--- a/libvips/foreign/vips2jpeg.c
+++ b/libvips/foreign/vips2jpeg.c
@@ -782,9 +782,9 @@ int
 vips__jpeg_write_target( VipsImage *in, VipsTarget *target,
 	int Q, 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,
-	VipsForeignJpegSubsample subsample_mode)
+	gboolean strip, gboolean trellis_quant,
+    gboolean overshoot_deringing, gboolean optimize_scans,
+    int quant_table, VipsForeignJpegSubsample subsample_mode)
 {
 	Write *write;
 
@@ -806,12 +806,6 @@ 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, 
diff --git a/test/test-suite/test_foreign.py b/test/test-suite/test_foreign.py
index 8d24ca2c..b70dd68d 100644
--- a/test/test-suite/test_foreign.py
+++ b/test/test-suite/test_foreign.py
@@ -258,18 +258,26 @@ class TestForeign:
     def test_jpegsave(self):
         im = pyvips.Image.new_from_file(JPEG_FILE)
 
-        # higher Q should mean a bigger buffer
         q10 = im.jpegsave_buffer(Q=10)
+        q10_subsample_auto = im.jpegsave_buffer(Q=10, subsample_mode="auto")
+        q10_subsample_on = im.jpegsave_buffer(Q=10, subsample_mode="on")
         q10_subsample_off = im.jpegsave_buffer(Q=10, subsample_mode="off")
+        
         q90 = im.jpegsave_buffer(Q=90)
-        assert len(q90) > len(q10)
-        assert len(q10_subsample_off) > len(q10)
-
-        # force subsampling should result in smaller buffer
-        q90_subsample_on = im.jpegsave_buffer(Q=90, subsample_mode="on")
         q90_subsample_auto = im.jpegsave_buffer(Q=90, subsample_mode="auto")
-        assert len(q90) > len(q90_subsample_on)
-        assert len(q90) == len(q90_subsample_auto)
+        q90_subsample_on = im.jpegsave_buffer(Q=90, subsample_mode="on")
+        q90_subsample_off = im.jpegsave_buffer(Q=90, subsample_mode="off")
+
+        # higher Q should mean a bigger buffer
+        assert len(q90) > len(q10)
+        
+        assert len(q10_subsample_auto) == len(q10) 
+        assert len(q10_subsample_on) == len(q10_subsample_auto)
+        assert len(q10_subsample_off) > len(q10)    
+        
+        assert len(q90_subsample_auto) == len(q90) 
+        assert len(q90_subsample_on) < len(q90) 
+        assert len(q90_subsample_off) == len(q90_subsample_auto)
 
     @skip_if_no("jpegload")
     def test_truncated(self):

From 1db2f4731c2ba7d38aa7b1a024507dccde0b3e06 Mon Sep 17 00:00:00 2001
From: elad laufer <elad@wix.com>
Date: Tue, 18 Feb 2020 18:21:21 +0200
Subject: [PATCH 12/13] - fix comment

---
 libvips/foreign/jpegsave.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/libvips/foreign/jpegsave.c b/libvips/foreign/jpegsave.c
index 358b6702..a138e564 100644
--- a/libvips/foreign/jpegsave.c
+++ b/libvips/foreign/jpegsave.c
@@ -73,12 +73,15 @@ typedef struct _VipsForeignSaveJpeg {
 	 */
 	gboolean interlace;
 
-	/* Disable chroma subsampling. 
+	/* Deprecated: Disable chroma subsampling. Use subsample_mode instead.
 	 */
 	gboolean no_subsample;
 
-    /* Force chroma subsampling, if Q >= 90 then subsampling is disabled, use this flag to force it
-    */
+    /* Select chroma subsampling mode:
+     * auto will disable subsampling for Q >= 90
+     * on will always enable subsampling
+     * off will always disable subsampling
+     */
     VipsForeignJpegSubsample subsample_mode;
 
 	/* Apply trellis quantisation to each 8x8 block.

From 0a23bf35780f409c7f7d59bd4ea969345c9327b1 Mon Sep 17 00:00:00 2001
From: John Cupitt <jcupitt@gmail.com>
Date: Tue, 18 Feb 2020 17:37:56 +0000
Subject: [PATCH 13/13] revise formatting, add docs

fix up some formatting from https://github.com/libvips/libvips/pull/1552

plus some other small changes
---
 ChangeLog                      |  1 +
 libvips/foreign/jpegsave.c     | 71 ++++++++++++++++++----------------
 libvips/foreign/pforeign.h     |  2 +-
 libvips/foreign/vips2jpeg.c    | 35 +++++++----------
 libvips/include/vips/foreign.h |  9 +++--
 libvips/iofuncs/enumtypes.c    |  1 +
 6 files changed, 61 insertions(+), 58 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c23df862..7359d1d4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -11,6 +11,7 @@
 - add vips_pipe_read_limit_set(), --vips-pipe-read-limit,
   VIPS_PIPE_READ_LIMIT
 - revise gifload to fix BACKGROUND and PREVIOUS dispose [alon-ne]
+- add subsample_mode, deprecate no_subsample in jpegsave [Elad-Laufer]
 
 31/1/19 started 8.9.2
 - fix a deadlock with --vips-leak [DarthSim]
diff --git a/libvips/foreign/jpegsave.c b/libvips/foreign/jpegsave.c
index a138e564..e1cca6c1 100644
--- a/libvips/foreign/jpegsave.c
+++ b/libvips/foreign/jpegsave.c
@@ -2,6 +2,8 @@
  *
  * 24/11/11
  * 	- wrap a class around the jpeg writer
+ * 18/2/20 Elad-Laufer
+ * 	- add subsample_mode, deprecate no_subsample
  */
 
 /*
@@ -77,12 +79,12 @@ typedef struct _VipsForeignSaveJpeg {
 	 */
 	gboolean no_subsample;
 
-    /* Select chroma subsampling mode:
-     * auto will disable subsampling for Q >= 90
-     * on will always enable subsampling
-     * off will always disable subsampling
-     */
-    VipsForeignJpegSubsample subsample_mode;
+	/* Select chroma subsampling mode:
+	 * auto will disable subsampling for Q >= 90
+	 * on will always enable subsampling
+	 * off will always disable subsampling
+	 */
+	VipsForeignJpegSubsample subsample_mode;
 
 	/* Apply trellis quantisation to each 8x8 block.
 	 */
@@ -113,26 +115,28 @@ G_DEFINE_ABSTRACT_TYPE( VipsForeignSaveJpeg, vips_foreign_save_jpeg,
 /* Type promotion for save ... just always go to uchar.
  */
 static int bandfmt_jpeg[10] = {
-/* UC  C   US  S   UI  I   F   X   D   DX */
-   UC, UC, UC, UC, UC, UC, UC, UC, UC, UC
+     /* UC  C   US  S   UI  I   F   X   D   DX */
+	UC, UC, UC, UC, UC, UC, UC, UC, UC, UC
 };
 
 static int
 vips_foreign_save_jpeg_build( VipsObject *object )
 {
-    VipsForeignSaveJpeg *jpeg = (VipsForeignSaveJpeg *) object;
+	VipsForeignSaveJpeg *jpeg = (VipsForeignSaveJpeg *) object;
 
-    if( VIPS_OBJECT_CLASS( vips_foreign_save_jpeg_parent_class )->
-        build( object ) )
-        return( -1 );
+	if( VIPS_OBJECT_CLASS( vips_foreign_save_jpeg_parent_class )->
+		build( object ) )
+		return( -1 );
 
-    /* no_subsample is deprecated, but we retain backwards compatibility
-     * new code should use subsample_mode
-     */
-    if( vips_object_argument_isset(object, "no_subsample") )
-        jpeg->subsample_mode = jpeg->no_subsample ? VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF : VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO;
+	/* no_subsample is deprecated, but we retain backwards compatibility
+	 * new code should use subsample_mode
+	 */
+	if( vips_object_argument_isset( object, "no_subsample" ) )
+		jpeg->subsample_mode = jpeg->no_subsample ? 
+			VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF : 
+			VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO;
 
-    return( 0 );
+	return( 0 );
 }
 
 static void
@@ -148,7 +152,7 @@ vips_foreign_save_jpeg_class_init( VipsForeignSaveJpegClass *class )
 
 	object_class->nickname = "jpegsave_base";
 	object_class->description = _( "save jpeg" );
-    object_class->build = vips_foreign_save_jpeg_build;
+	object_class->build = vips_foreign_save_jpeg_build;
 
 	foreign_class->suffs = vips__jpeg_suffs;
 
@@ -220,19 +224,20 @@ vips_foreign_save_jpeg_class_init( VipsForeignSaveJpegClass *class )
 		G_STRUCT_OFFSET( VipsForeignSaveJpeg, quant_table ),
 		0, 8, 0 );
 
-    VIPS_ARG_ENUM( class, "subsample_mode", 19,
-            _( "Subsample mode" ),
-            _( "Select chroma subsample operation mode" ),
-            VIPS_ARGUMENT_OPTIONAL_INPUT,
-            G_STRUCT_OFFSET( VipsForeignSaveJpeg, subsample_mode ),
-            VIPS_TYPE_FOREIGN_JPEG_SUBSAMPLE,
-            VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO );
+	VIPS_ARG_ENUM( class, "subsample_mode", 19,
+		_( "Subsample mode" ),
+		_( "Select chroma subsample operation mode" ),
+		VIPS_ARGUMENT_OPTIONAL_INPUT,
+		G_STRUCT_OFFSET( VipsForeignSaveJpeg, subsample_mode ),
+		VIPS_TYPE_FOREIGN_JPEG_SUBSAMPLE,
+		VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO );
 }
 
 static void
 vips_foreign_save_jpeg_init( VipsForeignSaveJpeg *jpeg )
 {
 	jpeg->Q = 75;
+	jpeg->subsample_mode = VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO;
 }
 
 typedef struct _VipsForeignSaveJpegTarget {
@@ -524,7 +529,7 @@ vips_foreign_save_jpeg_mime_init( VipsForeignSaveJpegMime *mime )
  * * @optimize_coding: %gboolean, compute optimal Huffman coding tables
  * * @interlace: %gboolean, write an interlaced (progressive) jpeg
  * * @strip: %gboolean, remove all metadata from image
- * * @no_subsample: %gboolean, disable chroma subsampling
+ * * @subsample_mode: #VipsForeignJpegSubsample, chroma subsampling mode
  * * @trellis_quant: %gboolean, apply trellis quantisation to each 8x8 block
  * * @overshoot_deringing: %gboolean, overshoot samples with extreme values
  * * @optimize_scans: %gboolean, split DCT coefficients into separate scans
@@ -550,10 +555,10 @@ vips_foreign_save_jpeg_mime_init( VipsForeignSaveJpegMime *mime )
  * conection, but need much more memory to encode and decode. 
  *
  * If @strip is set, no EXIF data, IPTC data, ICC profile or XMP metadata is 
- * written into the output file. 
+ * written into the output file.
  *
- * If @no_subsample is set, chrominance subsampling is disabled. This will 
- * improve quality at the cost of larger file size. Useful for high Q factors. 
+ * Chroma subsampling is normally automatically disabled for Q > 90. You can
+ * force the subsampling mode with @@subsample_mode.
  *
  * If @trellis_quant is set and the version of libjpeg supports it
  * (e.g. mozjpeg >= 3.0), apply trellis quantisation to each 8x8 block.
@@ -640,7 +645,7 @@ vips_jpegsave( VipsImage *in, const char *filename, ... )
  * * @optimize_coding: %gboolean, compute optimal Huffman coding tables
  * * @interlace: %gboolean, write an interlaced (progressive) jpeg
  * * @strip: %gboolean, remove all metadata from image
- * * @no_subsample: %gboolean, disable chroma subsampling
+ * * @subsample_mode: #VipsForeignJpegSubsample, chroma subsampling mode
  * * @trellis_quant: %gboolean, apply trellis quantisation to each 8x8 block
  * * @overshoot_deringing: %gboolean, overshoot samples with extreme values
  * * @optimize_scans: %gboolean, split DCT coefficients into separate scans
@@ -679,7 +684,7 @@ vips_jpegsave_target( VipsImage *in, VipsTarget *target, ... )
  * * @optimize_coding: %gboolean, compute optimal Huffman coding tables
  * * @interlace: %gboolean, write an interlaced (progressive) jpeg
  * * @strip: %gboolean, remove all metadata from image
- * * @no_subsample: %gboolean, disable chroma subsampling
+ * * @subsample_mode: #VipsForeignJpegSubsample, chroma subsampling mode
  * * @trellis_quant: %gboolean, apply trellis quantisation to each 8x8 block
  * * @overshoot_deringing: %gboolean, overshoot samples with extreme values
  * * @optimize_scans: %gboolean, split DCT coefficients into separate scans
@@ -735,7 +740,7 @@ vips_jpegsave_buffer( VipsImage *in, void **buf, size_t *len, ... )
  * * @optimize_coding: %gboolean, compute optimal Huffman coding tables
  * * @interlace: %gboolean, write an interlaced (progressive) jpeg
  * * @strip: %gboolean, remove all metadata from image
- * * @no_subsample: %gboolean, disable chroma subsampling
+ * * @subsample_mode: #VipsForeignJpegSubsample, chroma subsampling mode
  * * @trellis_quant: %gboolean, apply trellis quantisation to each 8x8 block
  * * @overshoot_deringing: %gboolean, overshoot samples with extreme values
  * * @optimize_scans: %gboolean, split DCT coefficients into separate scans
diff --git a/libvips/foreign/pforeign.h b/libvips/foreign/pforeign.h
index 791d26e3..92b32cc4 100644
--- a/libvips/foreign/pforeign.h
+++ b/libvips/foreign/pforeign.h
@@ -49,7 +49,7 @@ void vips__tiff_init( void );
 
 int vips__tiff_write( VipsImage *in, const char *filename, 
 	VipsForeignTiffCompression compression, int Q, 
-		VipsForeignTiffPredictor predictor,
+	VipsForeignTiffPredictor predictor,
 	char *profile,
 	gboolean tile, int tile_width, int tile_height,
 	gboolean pyramid,
diff --git a/libvips/foreign/vips2jpeg.c b/libvips/foreign/vips2jpeg.c
index 62c1cbbf..cd11e453 100644
--- a/libvips/foreign/vips2jpeg.c
+++ b/libvips/foreign/vips2jpeg.c
@@ -92,6 +92,8 @@
  * 	- ignore large XMP
  * 14/10/19
  * 	- revise for target IO
+ * 18/2/20 Elad-Laufer
+ * 	- add subsample_mode, deprecate no_subsample
  */
 
 /*
@@ -481,7 +483,7 @@ write_vips( Write *write, int qfac, const char *profile,
 	gboolean optimize_coding, gboolean progressive, gboolean strip, 
 	gboolean trellis_quant, gboolean overshoot_deringing,
 	gboolean optimize_scans, int quant_table,
-    VipsForeignJpegSubsample subsample_mode)
+	VipsForeignJpegSubsample subsample_mode )
 {
 	VipsImage *in;
 	J_COLOR_SPACE space;
@@ -628,24 +630,15 @@ write_vips( Write *write, int qfac, const char *profile,
 	if( progressive ) 
 		jpeg_simple_progression( &write->cinfo ); 
 
-	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( 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;
-            }
-        }
+	if( subsample_mode == VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF ||
+		(subsample_mode == VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO && 
+			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;
+		}
 	}
 
 	/* Don't write the APP0 JFIF headers if we are stripping.
@@ -783,8 +776,8 @@ vips__jpeg_write_target( VipsImage *in, VipsTarget *target,
 	int Q, const char *profile, 
 	gboolean optimize_coding, gboolean progressive,
 	gboolean strip, gboolean trellis_quant,
-    gboolean overshoot_deringing, gboolean optimize_scans,
-    int quant_table, VipsForeignJpegSubsample subsample_mode)
+	gboolean overshoot_deringing, gboolean optimize_scans,
+	int quant_table, VipsForeignJpegSubsample subsample_mode)
 {
 	Write *write;
 
diff --git a/libvips/include/vips/foreign.h b/libvips/include/vips/foreign.h
index 0f25c52a..da9a82a2 100644
--- a/libvips/include/vips/foreign.h
+++ b/libvips/include/vips/foreign.h
@@ -369,11 +369,14 @@ int vips_openslideload( const char *filename, VipsImage **out, ... )
  * @VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO: default preset
  * @VIPS_FOREIGN_JPEG_SUBSAMPLE_ON: always perform subsampling
  * @VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF: never perform subsampling
+ *
+ * Set jpeg subsampling mode.
  */
 typedef enum {
-    VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO,
-    VIPS_FOREIGN_JPEG_SUBSAMPLE_ON,
-    VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF
+	VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO,
+	VIPS_FOREIGN_JPEG_SUBSAMPLE_ON,
+	VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF,
+	VIPS_FOREIGN_JPEG_SUBSAMPLE_LAST
 } VipsForeignJpegSubsample;
 
 int vips_jpegload( const char *filename, VipsImage **out, ... )
diff --git a/libvips/iofuncs/enumtypes.c b/libvips/iofuncs/enumtypes.c
index e9069cd2..8a748a7d 100644
--- a/libvips/iofuncs/enumtypes.c
+++ b/libvips/iofuncs/enumtypes.c
@@ -508,6 +508,7 @@ vips_foreign_jpeg_subsample_get_type( void )
 			{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"},
+			{VIPS_FOREIGN_JPEG_SUBSAMPLE_LAST, "VIPS_FOREIGN_JPEG_SUBSAMPLE_LAST", "last"},
 			{0, NULL, NULL}
 		};