Gaussian blur #2496
Gaussian blur #2496
Conversation
# Conflicts: # src/imageops/sample.rs # src/images/dynimage.rs
# Conflicts: # benches/blur.rs # src/imageops/sample.rs # src/images/dynimage.rs
|
Thank you! I wonder, are the benchmark numbers for libblur from single-threaded or multi-threaded execution? |
|
Single threaded as far as I can tell. |
| @@ -854,8 +855,8 @@ impl DynamicImage { | |||
| /// This method typically assumes that the input is scene-linear light. | |||
| /// If it is not, color distortion may occur. | |||
| #[must_use] | |||
| pub fn blur(&self, sigma: f32) -> DynamicImage { | |||
| dynamic_map!(*self, ref p => imageops::blur(p, sigma)) | |||
| pub fn blur(&self, kernel_size: usize, sigma: f32) -> DynamicImage { | |||
There was a problem hiding this comment.
We'll need to make a decision on whether we want to do this API change. If so, this PR cannot be merged until we start working on the next major release.
Do other library generally require that you specify the kernel size alongside the blur amount?
There was a problem hiding this comment.
Pure analytical gaussian filter often assumes that you want full control on it. libblur and OpenCV require kernel size and also support assymetry.
See here.
I'm ok to remove kernel size, and compute correct kernel size from sigma if you think it's better fit. But I'm not ok to return old behaviour, what is wrong.
There was a problem hiding this comment.
If it needs to wait something it's better to me to go ahead and close this PR. I don't have any plans to support rotting PRs
There was a problem hiding this comment.
An easy-to-use wrapper that only requires the user to specify the sigma and computes the kernel size by itself would be nice. So it'd be two functions, e.g. blur() and blur_advanced().
There was a problem hiding this comment.
We're already preparing a major version bump on the main branch so I'd do the change now. However, another idea is given by your question regarding kernel sizes. Small kernel sizes are faster (just see our cutoff point for ring queue in this). Considering most of our users we should at least suggest the common choices.
What if we were to introduce a 'BlurKernel' type that wraps those choices and gives a few constructors / static constants of common cases? Then indeed blur_{by,with}(BlurKernel) may make sense with blur(f32) yielding a somewhat common default.
There was a problem hiding this comment.
I was actually confused by the sigma vs kernel size distinction.
I just want a high-level API that I can call that roughly matches what I'd get from "blur radius" in GIMP, but I also recognize that there are use cases for more direct control of the parameters. So I'd prefer an easy-to-use version as blur() and a more advanced API for people who need it.
There was a problem hiding this comment.
I did in libblur such parameters builder. However, it might still be overkill, but I don't expect that someone plugging in 70K lanes of SIMD code wouldn't at least do a bit of investigation into how to use the API.
For more general purpose implementation I agree that GIMP style "blur radius" is preferred.
There was a problem hiding this comment.
I'm in favor of a single argument that behaves like blur radius. That's what I've always assumed that sigma did
There was a problem hiding this comment.
What if we were to introduce a 'BlurKernel' type that wraps those choices and gives a few constructors / static constants of common cases? Then indeed blur_{by,with}(BlurKernel) may make sense with blur(f32) yielding a somewhat common default.
I think providing something like image.blur(3) is enough for general-purpose use.
Adding methods like image.blur3, image.blur5, image.blur7 gives a strong impression of API bloat.
It might make some sense if we were doing something truly interesting in these implementations, just to mark them that they may yield results are different from what you'd expect. But for now all implementations are the same, even when you hit a ring queue path it gives you the same result, but using a different way to get it.
There was a problem hiding this comment.
The argument radius/sigma makes perfect sense to me, too. So agreed, the simple function might as well accept a simple integer argument. I'm still not entirely sure we should remove the interaction entirely though for an advanced function.. Deriving sigma from the radius doesn't make much sense either. For small values (3, 5, 7) the influence of normalization is quite high, which people will have different preferences for.
But also importantly, imagmagick has it as an option(https://imagemagick.org/script/command-line-options.php#blur). I'm thinking if we had a struct and dispatched internally:
struct BlurKernel {
size: (u32, u32),
sigma: (f32, f32),
}
impl BlurKernel {
// Corresponds to calling the simple function with (3).
pub const THREE: … // Due to float-const this must be filled manually..
/// The isotropic case.
pub fn from_radius(sz: u32) -> Self { … }
/// Document the (1.0, 1.0) default and what anisotropy refers to.
pub fn with_sigma(self, (x, y): (f32, 32)) -> Self { … }
}
I think that is straightforward enough, but please do argue if you think this is API bloat. The anisotropy case in particular, I think there's enough reason to support it if it is one additional parameter to an intermediate/expert-level function call.
There was a problem hiding this comment.
Do you want fixed point implementations for u8 or u16 instead of f32? Or just pure f32 convolution is needed?
You matched the style in as discussed in the codec case and to me that is neat enough.
But also this seems to be an implementation choice, rather than an API choice. At least when we cast back to the underlying storage type I'd expect that what you're referring to as fixed point is actually exact with regards to rounding ("as-if floating point"). Then it should not matter to the user and the choice should be as fast as possible. However, that is then also a discussion point for the future instead as we can dispatch on those specific types (I::Pixel: 'static allows TypeId).
If special cases is wanted then do you expect them to be bit exact?
Not sure. It doesn't seem necessary but appealing for the special casing of types / fixed-point. Less so for special casing for kernel sizes but also see discussion on those below.
|
|
||
| let mut start_ky = column_kernel_len / 2 + 1; | ||
|
|
||
| start_ky %= column_kernel_len; |
There was a problem hiding this comment.
This one is odd. It just catches the case of column_kernel_len == 1 but that rather seems like a very special case on its own which doesn't need column buffers at all to be honest.
There was a problem hiding this comment.
Currently, the implementation assumes that anisotropy is an acceptable condition, and there is no special implementation case that handles column as identity and row as convolution. So implementation assumes that column_kernel_len == 1 and row_kernel_len == 5 is just fine.
I technically could drop any possibility of anisotropy if you see a better fit.
There was a problem hiding this comment.
I was just confused by the way those lines are written. It suggests that column_kernel_len == 1 is an even more special case than anisotropy itself since no other case required the modulos operation and the length of 1 does not really require any intermediate buffers.
There was a problem hiding this comment.
That said, a comment is a fine resolution to this to me. There are bigger fish to fry.
src/imageops/filter_1d.rs
Outdated
| if scanned_row_kernel.is_empty() || scanned_column_kernel.is_empty() { | ||
| for (dst, src) in destination.iter_mut().zip(image.iter()) { | ||
| *dst = *src; | ||
| } | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
An empty kernel is an error for convolution, the no-op case is a [1.0] kernel. So this is a leniency contract, right? Should be documented in the function signature.
There was a problem hiding this comment.
There is a mistake here — this is a no-op only when both kernels are [1.0]. Currently, the implementation assumes that anisotropy is an acceptable condition.
| @@ -854,8 +855,8 @@ impl DynamicImage { | |||
| /// This method typically assumes that the input is scene-linear light. | |||
| /// If it is not, color distortion may occur. | |||
| #[must_use] | |||
| pub fn blur(&self, sigma: f32) -> DynamicImage { | |||
| dynamic_map!(*self, ref p => imageops::blur(p, sigma)) | |||
| pub fn blur(&self, kernel_size: usize, sigma: f32) -> DynamicImage { | |||
There was a problem hiding this comment.
We're already preparing a major version bump on the main branch so I'd do the change now. However, another idea is given by your question regarding kernel sizes. Small kernel sizes are faster (just see our cutoff point for ring queue in this). Considering most of our users we should at least suggest the common choices.
What if we were to introduce a 'BlurKernel' type that wraps those choices and gives a few constructors / static constants of common cases? Then indeed blur_{by,with}(BlurKernel) may make sense with blur(f32) yielding a somewhat common default.
awxkee
force-pushed
the
gauss_b
branch
3 times, most recently
from
6f1ac04 to
6dc369e
Compare
|
A ton of WebP tests started failing today. I'm not sure if this PR is related to that. |
|
That should be the fixes shipped in https://crates.io/crates/image-webp v0.2.3 altering the enshrined hashes. In the long run something like image-webp's pixel difference threshold should be implemented, or image-rs/image-webp#146 should be fixed at which point we could enshrine hashes again. But for now they should probably just be regenerated. I'll open a PR with that against the main branch. |
awxkee
force-pushed
the
gauss_b
branch
3 times, most recently
from
338e71a to
b564431
Compare
|
I realized that I benchmarked the Benchmark |
Closes #986
Benchmarks
My working laptop has 10-15% noise, so numbers just to understand the magnitude of the difference. As well as comparison to libblur, because it actually is completely different execution class.