Resize improvements; add ceil and gap options

[kleisauke]

This PR adds a round-up option to shrink (see #2046 for context) and allows to configure the gap between vips_shrink() and vips_reduce(). The kleisauke/pyvips-issue-148 repository contains some analysis with the test image from libvips/pyvips#148 before and after this PR.

The calculation of the integer part within resize has also been slightly changed. When resizing a 2049×2047 image to 256×256 the int part is now (4, 3) times instead of (4, 4) times.

[kleisauke]

Just came across a handy utility to analyze image scaling algorithms, see: jsummers/resamplescope.

I had a try to run this on libvips, and it seems to identify a possible precision and/or rounding error within the downscaler (reduce{h,v}). I've published some test results on the kleisauke/pyvips-issue-148 repository. For example, with the most commonly used kernel, I see:

magick	VIPS
	Precision / rounding error(?)

(These graphs were produced with the vector path disabled)

[kleisauke]

It seems it wasn't entirely safe to remove the round-to-nearest behaviour in reduce{h,v}, I've reverted this with commit kleisauke@27bee3e.

I now see:

magick	Pillow	VIPS
		Precision error(?)

The dashed line might be a precision error, but I'm not sure. Will continue to investigate this.

[jcupitt]

I think perhaps the dashed line for libvips is it using fixed-point arithmetic plus a set of pre-computed masks. If you cast to float or double before reduce, it'll recompute the masks for each point and you should see smooth lines.

[kleisauke]

Ah, I think you're right. I tried this within the cast-to-double branch (see commit kleisauke/pyvips-issue-148@e387a16), but somehow it generates a second line, see for example:

Perhaps some kind of issue in the _notab implementation?

[kleisauke]

It looks like the *_notab version seems to work fine for non-adaptive downsizing. For example, this patch (which partially reverts 68ed42e):

Patch

diff --git a/libvips/resample/reduceh.cpp b/libvips/resample/reduceh.cpp
index 1111111..2222222 100644
--- a/libvips/resample/reduceh.cpp
+++ b/libvips/resample/reduceh.cpp
@@ -109,7 +109,7 @@ vips_reduce_get_points( VipsKernel kernel, double shrink )
 		return( 1 ); 
 
 	case VIPS_KERNEL_LINEAR:
-		return( 2 * rint( shrink ) + 1 ); 
+		return( 2 ); 
 
 	case VIPS_KERNEL_CUBIC:
 	case VIPS_KERNEL_MITCHELL:
@@ -140,7 +140,8 @@ vips_reduce_make_mask( double *c, VipsKernel kernel, double shrink, double x )
 		break;
 
 	case VIPS_KERNEL_LINEAR:
-		calculate_coefficients_triangle( c, shrink, x ); 
+		c[0] = 1.0 - x;
+		c[1] = x;
 		break;
 
 	case VIPS_KERNEL_CUBIC:

Outputs this reduceh graph for VIPS_KERNEL_LINEAR:

Without this patch (i.e. libvips master), I see:

[kleisauke]

I think I found it, see commit kleisauke@87043ef. I'm not quite sure why this is necessary, but it seems to fix the graphs now. 🎉

VIPS_FORMAT_UCHAR	VIPS_FORMAT_DOUBLE

[kleisauke]

I noticed that a test case fails with VIPS_FORMAT_UCHAR, but seems to work when casting this image to double (VIPS_FORMAT_DOUBLE) prior to resizing. The vector path is disabled for this test case, and it does not utilize shrink-on-load.

Test image is from lovell/sharp#2408.

Reproduction

vipsthumbnail sharp-2408.jpg -s 1400x -o tn_%s.jpg --vips-novector

Output image

(Notice that the above border is missing)

Expected output

vips cast sharp-2408.jpg x.v double
vipsthumbnail x.v -s 1400x -o tn_%s.jpg --vips-novector

Output image

I'm not sure if there is a way to fix this without increasing the precision of the pre-computed masks.

[kleisauke]

I dropped commit kleisauke@18eebf7 and kleisauke@7681da1 since they appear to be incorrect (see for example the above test case).

After further testing, the reported issues on sharp (lovell/sharp#2408 and lovell/sharp#2411) could be fixed with commit kleisauke@b1349dc. This does not cause regressions on the pyvips-issue-148 and vips-issue-703 repositories.

[markusmoormann]

any ETA on this?

[kleisauke]

@markusmoormann I can't give any dates, unfortunately. I've just moved the fixes for the rounding issues to PR #1872.

Marked this PR as draft, because I'm not happy with those double-precision xsize and ysize optional arguments in reduce{h,v}.

I still haven't found a way to avoid passing these arguments. @jcupitt Do you have any ideas for this?

[markusmoormann]

@kleisauke @jcupitt any news on this? We are still struggling with this issue

[markusmoormann]

@kleisauke @jcupitt
I know I might be annoying but we really need a fix for this. Is there anything we can do to support you?

[jcupitt]

#1872 is merged now and in 8.10.3:

https://github.com/libvips/libvips/releases/tag/v8.10.3

So if that's enough you may be OK.

I should have a bit more free time now I'm done on something else, so I'll try to catch up on libvips. Sorry for being slow.

[markusmoormann]

@jcupitt thank you very much! works fine

[kleisauke]

Rebased. Commit f9201c5 adds a unit test (based on comment lovell/sharp#3066 (comment)) to test whether we use double-precision calculations in reduce{h,v}. I now see this:

$ vips black x.jpg 1600 1000
$ vipsthumbnail x.jpg -s 10x -o vips.jpg
$ convert x.jpg -thumbnail 10x magick.jpg
$ vipsheader vips.jpg magick.jpg
vips.jpg: 10x6 uchar, 1 band, b-w, jpegload
magick.jpg: 10x6 uchar, 1 band, b-w, jpegload

Previous behaviour:

$ vipsheader vips.jpg
vips.jpg: 10x7 uchar, 1 band, b-w, jpegload

Note that this PR is still a draft because of:

Marked this PR as draft, because I'm not happy with those double-precision xsize and ysize optional arguments in reduce{h,v}.

I don't know if there's any other way to resolve that.

[kleisauke]

I finally found a way to get rid of those double-precision xsize and ysize optional arguments in reduce{h,v}. It turned out to be simple, we could just move the vips_shrink{h,h} from vips_resize to the corresponding vips_reduce* operations. To ensure backwards compatibility, a new @gap option (inspired by Pillow's reducing_gap argument) was added to configure the gap between the box filter and the final reduce. vips_resize sets this to 2.0 by default. Also, vips_shrink uses this new option for non-integer shrinks.

This is ready for review now.

[kleisauke]

FWIW, the CIFuzz and latest OSS-Fuzz build seems to fail due to https://bugs.chromium.org/p/aomedia/issues/detail?id=3274.

Edit: I triggered a re-run of the CIFuzz job, since commit https://aomedia.googlesource.com/aom/+/be05bfaceb4d3b7ed66a4d2e7a4927f484397efa ought to fix that.

[jcupitt]

I've not forgotten this, I just have a lot of work right now :( I promise I'll read it this weekend!

[jcupitt]

This looks great. I like your idea of moving the shrink into reduce. I saw a couple of tiny things, but that's all

We need an update for the ChangeLog. Maybe:

- added @gap to vips_reduce[hv]() and vips_resize()
- added @ceil to vips_shrink()
- quality improvements for image resizing

Though that seems very vague! I'm sure you can think of something better.

[kleisauke]

Your ChangeLog notes looks OK. Perhaps we should not use @ for the options (to prevent GitHub from tagging people)? For example:

- add "gap" option to vips_reduce[hv]() and vips_resize()
- add "ceil" option to vips_shrink()
- quality improvements for image resizing

Still a bit vague, but I can't think of anything better without going into too much detail.

[jcupitt]

\o/ woo!

I'll push the changelog notes.