Coverage remained the same at 100.0% when pulling d72f4bc on kleisauke:vips-thumbnail into add4c79 on lovell:main.

@kleisauke at #2787 you mentioned:

The previous implementation returned both 320x261 with fastShrinkOnLoad enabled or disabled, vipsthumbnail returns 320x262 as dimension when patching libvips like this [...]

So taken for example this test you changed:

What happens is:

• The source image is 2725x2225
• Shrink-on-load is set to 8
• After the shrink load, the image dimensions are 340x278 -- This no longer matches the original dimensions, when rounded correctly it would be 341x278
• And this causes the final image height mismatch because it is resized proportionally to the size after the shrink-on-load, not the original dimensions

It is explained in further detail here:
https://github.com/libvips/libvips/blob/757d03100566368e5d06c7355e73c1215dd34968/libvips/foreign/jpeg2vips.c#L586-L596

cinfo->output_width and cinfo->output_height round up with
shrink-on-load. For example, if the image is 1801 pixels across and
we shrink by 4, the output will be 450.25 pixels across,
cinfo->output_width with be 451, and libjpeg will write a black
column of pixels down the right.

We must strictly round down, since we don't want fractional pixels
along the bottom and right.

So when you redefine inputWidth and inputHeight the mismatch happens here in case either dimension got rounded down:

This is amazing, thank you Kleis.

Should fastShrinkOnLoad produce the same "normalised" dimensions if it wasn't enabled?

Based on the principle of "least surprise", I'd probably say yes, if we can do so whilst making this change.

Would making the following alteration, to bring the logic in sharp more in line with libvips, help prevent some rounding problems? (I know we've previously discussed doing this, and now feels like the right time.)

- if (shrink >= 8 * factor) {
+ if (shrink >= 16 * factor) {
    jpegShrinkOnLoad = 8;

Thanks for the analysis @ahukkanen! I tried to implement a round-up behavior in jpeg2vips.c a while ago, but it didn't work out.

I had a go to produce "normalised" dimensions with commit 1769bc8 by recalculating vshrink / hshrink on the main image instead of the pre-shrunk image. This seems to work in most cases, but it might misbehave with the page-based TIFF pyramid shrink-on-load logic (which is not yet implemented in this PR).

$ curl -LO http://merovingio.c2rmf.cnrs.fr/iipimage/PalaisDuLouvre.tif
$ vipsthumbnail PalaisDuLouvre.tif -s 500x103
$ vipsheader tn_PalaisDuLouvre.jpg
tn_PalaisDuLouvre.jpg: 498x103 uchar, 3 bands, srgb, jpegload
$ vips copy PalaisDuLouvre.tif[page=0] x.jpg
$ vipsthumbnail x.jpg -s 500x103
$ vipsheader tn_x.jpg
tn_x.jpg: 498x103 uchar, 3 bands, srgb, jpegload

Would making the following alteration, to bring the logic in sharp more in line with libvips, help prevent some rounding problems? (I know we've previously discussed doing this, and now feels like the right time.)

- if (shrink >= 8 * factor) {
+ if (shrink >= 16 * factor) {
    jpegShrinkOnLoad = 8;

Probably the other shrink factors will also need to be adapted to leave at least a factor of two for the final resize step (but that would deprecate fastShrinkOnLoad, so I'm not sure if that's desired).

I'm not sure about the performance implications @kleisauke but couldn't it be also solved just by cutting one extra pixel off the shrinked image when this happens?

So, for the given example above, you would cut 1px off the height and the image would become 340x277 after the shrink. Using this aspect ratio, the resulting image would then become 320x261 as it originally was.

I'm also not sure if that works perfectly in every case but at least it would be closer to the current behavior, if that's the problem.

Thanks for the updates Kleis, I'm on the fence about whether to keep or remove fastShrinkOnLoad. Perhaps we should wait until libvips supports the proposed RGBX colourspace and therefore improve vips_reduce performance first?

Any thoughts/updates on getting this merged? I've been using @kleisauke 's branch and it's been working like a charm. Thanks for everyones hard work and contributions on this wonderful project!

My apologies for the delay.

I'm not sure about the performance implications @kleisauke but couldn't it be also solved just by cutting one extra pixel off the shrinked image when this happens?

I don't know if cutting an extra pixel off the shrunk image helps here. I thought this would be simpler, but maybe I'm overlooking something.

Thanks for the updates Kleis, I'm on the fence about whether to keep or remove fastShrinkOnLoad. Perhaps we should wait until libvips supports the proposed RGBX colourspace and therefore improve vips_reduce performance first?

Indeed, it would probably be better to wait for the RGBX improvement in libvips first. Using SIMD intrinsics in vips_reduce{h,v} will also speed that up.

Any thoughts/updates on getting this merged? I've been using @kleisauke 's branch and it's been working like a charm.

Thanks for testing. I'll remove the draft status once I've completed the TODO-items.

Although some new unit tests for animated images are still missing, I think this is now ready for reviewing. Note that this PR doesn't currently implement these shrink-on-load features of vips_thumbnail:

• Page-based and ifd-based TIFF pyramids;
• HEIC/AVIF thumbnails;
• Page-based JP2K pyramids (requires vips 8.11);
• Openslide levels.

Because that will require a lot of extra code paths (I think), which makes reviewing this PR difficult.

I wanted to test this PR by resizing GIFs and converting them to WebP.

The expected output was an animated WebP image which is contained in the image body.

Images used

Landscape
Potrait

Code

const image = sharp(filename, { animated: true })
await image.resize(512, 512, { 
    fit: 'contain',
    background: { r: 0, g: 0, b: 0, alpha: 0 } 
}).webp().toFile(join('results', filename.replace('gif', 'webp')))

Results

Sharp 0.29.4

Landscape	Potrait

github:kleisauke/sharp#vips-thumbnail

Landscape	Potrait

Is this a bug or am I doing something wrong?
Thanks for your time!

@AlenSaito1 Thanks for testing! Could you re-test with commit 3b79d8f?

The reason for this is that post-resize embedding/cropping modifies the image height, which breaks multi-page images. Commit 3b79d8f ensures that it will not alter the image height. A better solution would be to split the frames, and then perform this operation per-frame, but that comes at the expense of performance.

Thanks for the fast reply @kleisauke

I tried this again with the latest commit. But now, it works only if the image is a portrait.
If the image is a landscape, only the width gets set to the given value

Results

Landscape	Potrait

(I'm about to update the prebuilt binaries to include libspng 0.7.1 that will fix the failing ARM64 jobs.)

@kleisauke The main branch now uses the latest libvips 8.12.1 binaries so you should be good to rebase. Would you like to add any more unit tests before we merge this?

I just rebased this PR, and will add some unit tests today. In addition that this PR doesn't port all shrink-on-load features, these post-resize operations are currently not supported on multi-page/animated images:

• Smart-crop (is currently skipped for multi-page images);
• Rotation (any abirtaty angle);
• Affine transformation;
• Compositing.

Thanks for the updates Kleis, this is ready to merge for v0.30.0 👍

My current thinking is that, if possible, it's worth aborting processing should the pipeline try to mix multi-page resizing with unsupported operations (smartcrop, rotation, affine, composite).

I'm happy add this logic, with unit tests, after we merge. Are there any other scenarios you can think of that should also be included?

Great! Currently smartcrop is silently ignored, see for example:

But it would indeed be better to abort processing for operations that doesn't support multi-page images. In addition to the operations mentioned above, I think pre-resize trimming is also unsupported on multi-page images.

There are also operations that can lead to surprising results. Blurring is one of them, see for example:
https://images.weserv.nl/static/banana.webp?n=-1&blur=10
(note the footprints at the top of the image)

Fantastisch, as always, thank you Kleis!