Fast and correct resizing for DynamicImage

[Shnatsel]

This PR migrates resizing DynamicImage from the built-in resizing impl that's hamstrung by the limitations of our generic APIs to the pic-scale-safe crate by @awxkee. This code is ported from the one I had in wondermagick for months now.

Performance

This implementation is ~5x faster than the previous one when running single-threaded. It also has optional parallelism gated by the rayon feature that takes performance even further.

I had to do an unnecessary copy to adhere to the existing API; we can eliminate it in the next semver-breaking release.

Fixes #2223

Correctness

Unlike the previous implementation, this one automatically premultiplies the pixels by alpha before resizing, but this is done only when necessary to avoid precision loss. Premultiplication is then reversed after resizing. This happens entirely transparently for the user. This API just automatically does the right thing for images with alpha.

Fixes #2422

Future work

Not part of this PR to make it easier to review:

1. Wire up resizing via pic-scale-safe to ImageBuffer wrapping concrete types such as Rgb, Rgba and even some types that aren't in DynamicImage like Brg or LumaF32
2. Change the thumbnail methods to produce high-quality thumbnails by resizing down to 5x the using nearest-neighbor and then downscaling properly from there, like imagemagick does
3. Use the same alpha premultiplication trick to address Blur for images with alpha is incorrect, bleeds color from transparent pixels #2324

Gamma and CICP are not taken into account. But the only reasonable way to do that is convert to a 16-bit linear colorspace and do the resizing there, which is too expensive to do transparently.

It is theoretically possible to make pic-scale-safe code entirely generic and fold it into image codebase, but it will require making nontrivial and semver-breaking improvements to our generic APIs first, and I don't think they should block this work.

[awxkee]

I want to share an important point about using Rayon for heavy workloads.

Rayon uses some sort of global scheduler see docs that shared between all tasks.

Consider the following pseudocode:

[array_of_many_images_about_80-100].par_iter()
.for_each(|image| {
do_resize_with_rayoun_enabled()
});

Due to this rayon rules this code is vulnerable to DDoS attacks since it quickly exhaust RAM because rayon schedules all images at once where other ones are still not finished. I discovered this surprising behavior in libblur on my Mac M3, processing around 80–100 images is enough to knock the system out. That's a fairly small number. Image resizing is a bit less greedy, but it still shows the same characteristics. For some typical shared, or small server with 1-2GB RAM and 10GB of disk and 1-2 vCPU this amount I think the value is 10 times lower.

Even to my low security standards I had to remove rayon from pic-scale, libblur etc and make more classic solution (https://crates.io/crates/novtb), that's not counting it is actually 10% because it's planning heavy jobs better.

Even if we won't actually fix it here, I think it worth document that this API is subject to vulnerability to DDoS attacks with rayon enabled and require some additional attention to it.

[Shnatsel]

Thanks for letting me know! I've disabled rayon to sidestep this issue.

We can flip it on and update the documentation in the next semver-breaking release.

Separately, it is indeed very odd that rayon doesn't let us limit the amount of items concurrently in flight.

[awxkee]

I thought it would end up being upstreamed rather than included :)

What do you think about transferring pic-scale-safe to image-rs org? I’m not quite sure how it would work on crates.io though, since the naming rules may require the crate to remain under my namespace 🤔

[Shnatsel]

That sounds good to me but I'd like to hear from the other maintainers first. I don't think I have the required Github permissions in any case.

[fintelia]

We received feedback that adding new default dependencies in patch releases complicates things for distros. Unfortunately, we can't make minor non-breaking changes until doing a 1.0 release.

On a somewhat related note, I've really been wanting to fix the inconsistency between DynamicImage::resize and imageops::resize (the former keeps the aspect ratio while the latter behaves like DynamicImage::resize_exact). Probably requires deprecating+removing DynamicImage::resize so that there isn't a silent behavior change

[Shnatsel]

On a somewhat related note, I've really been wanting to fix the inconsistency between DynamicImage::resize and imageops::resize

Yes, I think imageops::resize needs to be deprecated and replaced with resize_exact and resize_proportional implemented on buffer types that pic-scale-safe supports.

We received feedback that adding new default dependencies in patch releases #2626. Unfortunately, we can't make minor non-breaking changes until doing a 1.0 release

Distros also take issue with us upgrading major versions of crates, so us bumping tiff crate was also a problem. And we've already bumped zune-jpeg from 0.4 to 0.5 and ravif from 0.11 to 0.12, so this next release isn't making its way into distros anyway, with or without pic-scale-safe.

But I do believe it's time to start merging breaking changes, now that we know 0.25.8 doesn't need any urgent fixes. I'd love to make this into a breaking change and eliminate the extra copy, and also start merging things like #2636

[Shnatsel]

Now that API breaks are allowed, I've eliminated the mandatory copy. The API user can now choose to explicitly clone the image to hold on to an older version of it, or let it be overwritten.

I'm not sure if resizing should operate in-place or if it should accept an owned DynamicImage as input and consume it. I went with the in-place variant for now but I'm open to changing that, now or after this PR.

[awxkee]

Since there is an API break anyway, I'd suggest renaming the Triangle filter to Bilinear or Linear. While Triangle is mathematically correct definition, it's much less commonly used, especially in image processing. I believe most users don’t actually understand what it means. This likely leads to higher use of Lanczos, which is more expensive and in practice not needed for most applications.

Perhaps Mitchell-Netravali filter could be added as well, since it's the most commonly used filter nowadays.

[fintelia]

If I'm understanding correctly, the reason for the API change is that pic-scale-safe uses the input as a scratch buffer, while the old implementation allocates a dedicated scratch buffer?

My instinct is that in the future we may want to again change around how scratch buffers are handled. For instance, nearest sampling shouldn't need a scratch buffer and in other cases we might be able to use a scratch buffer that's much smaller than the input image. In which case, perhaps it would be better to keep the existing API?

[Shnatsel]

No, pic-scale-safe does not touch the original buffer.

The original buffer is premultiplied by alpha channel in place, which is done by the code added in this PR. This is necessary to handle resizing images with alpha correctly, see #2422.

Nearest neighbour is indeed a special case that does not need premultiplying by alpha. I think it can be exposed separately as sample() or some such and avoid the complications of buffer management.

Whether to keep the original buffer around and make a copy internally (but only for images with alpha), or whether to always consume the buffer and have the API user make a copy explicitly (wasteful for images without alpha), comes down to what we expect the common case to be. In wondermagick I need to resize the image and don't need to hold on to the original, so I went with that. If we expect holding on to the original to be the default, then we can accept &DynamicImage instead and make a copy internally.

Or I guess there's also a third option of making a more complicated API that surfaces these differences.

[fintelia]

Ah, using the original input to apply premultiplication makes sense. In that case I'm in favor of taking &mut self. The other option is consuming self, but there doesn't seem to be any precedent for that on the other image processing methods of DynamicImage.

I also noticed that there's two thumbnail methods on DynamicImage based on the doc comment seem to be shorthand for calling resize with nearest filtering? If so, perhaps we should remove them to avoid confusion over the differences

[Shnatsel]

I'm going to replace thumbnail with fast and clever thumbnailing that imagemagick does: first use nearest-neighbor to scale down to 5x the target size, then use high-quality scaling the rest of the way.

I was going to do that in a follow-up PR, but if you'd like me to include it here I'm open to that.

[fintelia]

That sounds good for a separate PR!

[Shnatsel]

So this one's good to go?

I'd like to get this wrapped up before I move on to thumbnailing, etc.

[Shnatsel]

I'm going to go ahead and merge this, and then build other changes outlined in the PR description on top of this.

[laundmo]

FYI: I think the "implementation is ~5x faster" estimate is underselling it a bit.

For my usecase, a small GrayImage, i benchmarked 1.4787 ms for Lanczos3 using the old image-rs resize, and 68.273 µs for Lanczos3 using the pic_scale_safe resize.

Heres some benchmarks on various sizes of GrayImage:

[Shnatsel]

You probably have the rayon feature enabled, so it runs on all cores instead of just one like the old implementation did.

[laundmo]

Nope! No rayon feature, during the benchmark only 1 core at a time is maxed. It truly is just that much better. Oh, no target-cpu=native either.