Add force subsample argument to JPEG save #1552

Elad-Laufer · 2020-02-16T16:58:04Z

If Q >= 90 then subsampling is disabled, this argument forces it

Merge from main

Merge master

… directive

jcupitt · 2020-02-16T17:33:58Z

We already have no-subsample and Q. Perhaps we should deprecate no-subsample and have a three way switch: force on, force off, automatic (set by Q), with auto as the default.

…ample

lovell · 2020-02-17T12:50:14Z

If there's a plan to offer more fine-grained control over subsampling, perhaps we should consider allowing 4:2:2 as well as 4:4:4 ("no-subsample") and 4:2:0 ("force-subsample")?

jcupitt · 2020-02-17T16:35:57Z

I looked into this when we added jpeg-chroma-subsample to jpegload: the only two supported settings for JPEG are 4:4:4 (no subsampling) and 4:2:0 (subsample both Cb and Cr by 2). It's very confusing :(

http://poynton.ca/PDFs/Chroma_subsampling_notation.pdf

edit: of course there are CMYK JPG too, but they never have subsampled K, so the full set of chroma subsample settings are:

4:2:0:4
4:4:4:4
4:2:0
4:4:4

jcupitt · 2020-02-17T16:36:38Z

(or that's my understanding from that useful PDF)

lovell · 2020-02-17T20:20:19Z

@jcupitt Thanks for the info, no worries, let's keep this as simple as possible.

…EIGN_JPEG_SUBSAMPLE_OFF when no_subsample=1

jcupitt

It looks great! Thank you very much for doing this work.

I think we should deprecate no_subsample, otherwise it's just nitpicks.

libvips/foreign/jpegsave.c

jcupitt · 2020-02-18T10:44:51Z

libvips/foreign/jpegsave.c

@@ -230,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,


Let's not pass no_subsample. Instead, a little higher up, have something like:

if arg-set("no_subsample") && !arg_set("subsample_mode") subsample_mode = no_subsample ? OFF : ON

ie. old code using the no_subsample arg is able to set the default for the new subsample_mode value.

Should I add the check to each _build(...) function? currently I only check once at vips__jpeg_write_target(...).

Is there a way to perform the check in single point without code duplication?

Yes, add a build method to the base class for jpegsave and do the test after chaining up.

tiffsave has something a little like this:

https://github.com/libvips/libvips/blob/master/libvips/foreign/tiffsave.c#L153

A _build method on the base class and use of isset to set defaults.

jcupitt · 2020-02-18T10:45:37Z

libvips/foreign/jpegsave.c

@@ -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,


Again, remove no_subsample.

jcupitt · 2020-02-18T10:45:58Z

libvips/foreign/jpegsave.c

@@ -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,


Remove 'no_subsample`.

jcupitt · 2020-02-18T10:46:12Z

libvips/foreign/jpegsave.c

@@ -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,


jcupitt · 2020-02-18T10:48:52Z

libvips/foreign/vips2jpeg.c

+            break;
+        case VIPS_FOREIGN_JPEG_SUBSAMPLE_AUTO:
+            /* Turn off chroma subsampling. Follow IM and do it automatically for
+            * high Q.


Asterisks should line up.

/* Like * this. */

jcupitt · 2020-02-18T10:50:04Z

libvips/foreign/vips2jpeg.c

+                break;
+            }
+        case VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF:
+        default: {


Newline before {, like a func definition, and for formatted as:

for( i = 0; i < xxx; i++ ) {

jcupitt · 2020-02-18T10:50:59Z

libvips/foreign/vips2jpeg.c

+    if( no_subsample ) {
+        subsample_mode = VIPS_FOREIGN_JPEG_SUBSAMPLE_OFF;
+    }
+


This if() can go if we have the extra logic in _build().

jcupitt · 2020-02-18T10:52:27Z

test/test-suite/test_foreign.py

+
+        # higher Q should mean a bigger buffer
+        q10 = im.jpegsave_buffer(Q=10)
+        q10_subsample_off = im.jpegsave_buffer(Q=10, subsample_mode=2)


I would use the string value here, ie. "off".

jcupitt · 2020-02-18T10:52:59Z

test/test-suite/test_foreign.py

+        # 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)


I don't think we should have deprecated API being tested in the test suite, let's remove this.

Same for the case below. Just test on, off, auto.

- styling

jcupitt

Look great! Thanks again, I'll merge.

fix up some formatting from #1552 plus some other small changes

jcupitt · 2020-02-18T17:38:56Z

I revised the formatting slightly and added some docs.

Elad Laufer added 3 commits February 10, 2020 11:59

Merge pull request #2 from libvips/master

2b6cc42

Merge from main

Merge pull request #3 from libvips/master

71e9071

Merge master

add force subsample argument that overrides the Q deduced subsampling…

c626c9d

… directive

Elad-Laufer requested a review from jcupitt February 16, 2020 16:58

- use a single enum and switch that replaces no_subsample, force_subs…

d2b794e

…ample

elad laufer added 2 commits February 17, 2020 17:13

- fix comment

00bf326

- don't skimp on names

494d887

elad laufer added 5 commits February 18, 2020 10:54

- simplified subsampling selection by setting subsample_mode=VIPS_FOR…

408f3b0

…EIGN_JPEG_SUBSAMPLE_OFF when no_subsample=1

- styling

c866cf4

- add test case

65b1a3e

- add test case

838b5e7

Use a single enum and switch that replaces no_subsample, force_subsample

4c6df5b

jcupitt reviewed Feb 18, 2020

View reviewed changes

elad laufer added 3 commits February 18, 2020 13:31

- argument deprecation flag

41d7941

- styling

- use enum string

22b3a0d

- remove no_subsample from inner calls

8749871

Elad-Laufer requested a review from jcupitt February 18, 2020 16:06

- fix comment

1db2f47

jcupitt approved these changes Feb 18, 2020

View reviewed changes

jcupitt merged commit fefefdf into libvips:master Feb 18, 2020

jcupitt added a commit that referenced this pull request Feb 18, 2020

revise formatting, add docs

0a23bf3

fix up some formatting from #1552 plus some other small changes

lovell mentioned this pull request Mar 24, 2020

ChromaSubsampling parameter ignored when converting to JPEG output lovell/sharp#2135

Closed

alon-ne mentioned this pull request Apr 30, 2020

unsharp-mask wix-playground/libvips#6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add force subsample argument to JPEG save #1552

Add force subsample argument to JPEG save #1552

Elad-Laufer commented Feb 16, 2020

jcupitt commented Feb 16, 2020

lovell commented Feb 17, 2020

jcupitt commented Feb 17, 2020 •

edited

jcupitt commented Feb 17, 2020

lovell commented Feb 17, 2020

jcupitt left a comment

jcupitt Feb 18, 2020

Elad-Laufer Feb 18, 2020

jcupitt Feb 18, 2020

jcupitt Feb 18, 2020

Elad-Laufer Feb 18, 2020

jcupitt Feb 18, 2020

Elad-Laufer Feb 18, 2020

jcupitt Feb 18, 2020

Elad-Laufer Feb 18, 2020

jcupitt Feb 18, 2020

Elad-Laufer Feb 18, 2020

jcupitt Feb 18, 2020

Elad-Laufer Feb 18, 2020

jcupitt Feb 18, 2020

Elad-Laufer Feb 18, 2020

jcupitt Feb 18, 2020

Elad-Laufer Feb 18, 2020

jcupitt Feb 18, 2020

Elad-Laufer Feb 18, 2020

jcupitt Feb 18, 2020

Elad-Laufer Feb 18, 2020

jcupitt left a comment

jcupitt commented Feb 18, 2020

		@@ -230,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,

		@@ -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,

		@@ -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,

		@@ -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,

Add force subsample argument to JPEG save #1552

Add force subsample argument to JPEG save #1552

Conversation

Elad-Laufer commented Feb 16, 2020

jcupitt commented Feb 16, 2020

lovell commented Feb 17, 2020

jcupitt commented Feb 17, 2020 • edited

jcupitt commented Feb 17, 2020

lovell commented Feb 17, 2020

jcupitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcupitt left a comment

Choose a reason for hiding this comment

jcupitt commented Feb 18, 2020

jcupitt commented Feb 17, 2020 •

edited