-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Port vips_thumbnail
logic to sharp
#2789
Conversation
@kleisauke at #2787 you mentioned:
So taken for example this test you changed: Line 26 in 82eaa8b
What happens is:
It is explained in further detail here:
So when you redefine Lines 239 to 241 in 82eaa8b
|
This is amazing, thank you Kleis.
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; |
82eaa8b
to
1769bc8
Compare
Thanks for the analysis @ahukkanen! I tried to implement a round-up behavior in I had a go to produce "normalised" dimensions with commit 1769bc8 by recalculating DetailsTested commit: kleisauke/libvips@d7ed0dd. $ 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 The first operation resizes the third page of the pyramid TIFF from
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 |
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 |
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! |
1769bc8
to
1de39a8
Compare
My apologies for the delay.
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.
Indeed, it would probably be better to wait for the RGBX improvement in libvips first. Using SIMD intrinsics in
Thanks for testing. I'll remove the draft status once I've completed the TODO-items. |
04a736d
to
b4ca8a4
Compare
b4ca8a4
to
e76b8d4
Compare
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
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 usedCodeconst 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'))) ResultsIs this a bug or am I doing something wrong? |
@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. |
3b79d8f
to
5dc150d
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing Kleis, thank you, you've even done the multi-frame split and rejoin logic for the crop and embed paths! 🚀
src/pipeline.cc
Outdated
} else { | ||
// Reload WebP file | ||
image = VImage::webpload(const_cast<char*>(baton->input->file.data()), option); | ||
} | ||
} else if (inputImageType == sharp::ImageType::SVG) { | ||
option->set("unlimited", TRUE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to use the value of the new baton->input->unlimited
property here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Commit 70b1913 leverages this new property.
(I'm about to update the prebuilt binaries to include libspng 0.7.1 that will fix the failing ARM64 jobs.) |
@kleisauke The |
This should probably not be controlled by users, and should only change when the image is resized. This commit also changes the delay option to allow singular values (to change the delay for all frames).
This should be resolved by commit libvips/libvips@9c1003f and libvips/libvips@f92069b instead.
Inline with libvips' behaviour.
Saves a few ms.
de64844
to
70b1913
Compare
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:
|
+ add unit tests for this.
Maybe this fixes the MacStadium CI error(?).
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: Line 464 in d72f4bc
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: |
Fantastisch, as always, thank you Kleis! |
* Ports vips_thumbnail logic to sharp * Deprecates the pageHeight output option for WebP/GIF
* Ports vips_thumbnail logic to sharp * Deprecates the pageHeight output option for WebP/GIF
As discussed in #2787, this PR ports some
vips_thumbnail
logic to sharp. It implements "scale-on-load" for WebP, SVG and PDF images, which allows an image to be resized in one go without the need for an intermediate resizing step. Previously sharp uses the deprecatedshrink
option forwebpload
, so I expect WebP images to be resized a bit faster within this PR.This PR also ports the corresponding
page-height
logic ofvips_thumbnail
, allowing resizing of animated/multipage images without having to specify thepageHeight
option for the.webp()
and.gif()
methods (this covers issue #2275).Before / after examples
.gif({ pageHeight: 136 })
doesn't work..gif({ pageHeight: 136 })
isn't necessary.$ curl -LO https://github.com/lovell/sharp/raw/master/test/fixtures/circle.svg $ node -e "require('sharp')('circle.svg').resize(1024).toFile('before.png')" $ vipsthumbnail circle.svg -s 1024x -o after.png
Marked this PR as a draft due to these TODO-items:
fastShrinkOnLoad
produce the same "normalised" dimensions if it wasn't enabled?These features of
vips_thumbnail
are currently missing from sharp, but might be included later: