Implement Box blur fast filter that could approximate gaussian filter #223
Conversation
|
You mean in crates/kornia-imgproc/benches/bench_filters.rs ? Sure ok |
| @@ -92,4 +122,11 @@ mod tests { | |||
| assert_eq!(k, expected[i]); | |||
| } | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
Maybe add also some test that it’s not only ones ?
| /// * `src` - The source image with shape (H, W, C). | ||
| /// * `dst` - The destination image with shape (H, W, C). | ||
| /// * `kernel_size` - The size of the kernel (kernel_x, kernel_y). | ||
| /// * `sigma` - The sigma of the gaussian kernel. |
There was a problem hiding this comment.
Specify that it should be x-y ordered
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
I would some simple numbers test too similar to the other functions to verify that’s doing the right thing
There was a problem hiding this comment.
So I added 2 tests here: test_box_blur_fast() and test_gaussian_blur(). Both has the same input (0..25) to show that the outputs are not that much different.
I did attempt to use the same input (all 0.0, 9.0 in the middle) as test_fast_horizontal_filter(), if that's how you mean by this comment. The result was a little disappointing as there's a big difference between the outputs of the 2 methods. I figured it's because the test input was odd. It might be fitting for test_fast_horizontal_filter() but not for these. Therefore I went with something more randomized.
There was a problem hiding this comment.
Why should be different? The test you describe should give you a box of ones, right ?
| @@ -88,6 +88,72 @@ pub fn separable_filter<const C: usize>( | |||
| Ok(()) | |||
| } | |||
|
|
|||
|
|
|||
| /// Apply a fast filter horizontally, take advantage of property where all | |||
There was a problem hiding this comment.
Split header docs. Usually there’s a single line explaining in short the purpose of the function followed by end of line then you can add some clarification, formulation of anything needed
| /// * `src` - The source image with shape (H, W, C). | ||
| /// * `dst_transposed` - The destination image with shape (W, H, C). | ||
| /// * `half_kernel_x_size` - Half of the kernel at weight 1. The total size would be 2*this+1 | ||
| pub fn fast_horizontal_filter<const C: usize>( |
There was a problem hiding this comment.
| pub fn fast_horizontal_filter<const C: usize>( | |
| fn fast_horizontal_filter<const C: usize>( |
I wouldn’t make public to users, this is more a utility function
There was a problem hiding this comment.
I change it to pub(crate) because just removing pub would not expose it to ops.rs/box_blur_fast()
| /// * `half_kernel_x_size` - Half of the kernel at weight 1. The total size would be 2*this+1 | ||
| pub fn fast_horizontal_filter<const C: usize>( | ||
| src: &Image<f32, C>, | ||
| dst_transposed: &mut Image<f32, C>, |
There was a problem hiding this comment.
You're right. I was signaling that the result would be transposed. But it could be placed in the docstring.
| row_acc[ch] += src_data[kernel_pix_offset]; | ||
| } | ||
| leftmost_pixel[ch] = *source_pixel; | ||
| rightmost_pixel[ch] = src_data[pix_offset+((src.cols()-1)*C)]; |
There was a problem hiding this comment.
(src.cols()-1)*C) could be computed outside before the loops
|
|
||
| if c == 0 { | ||
| row_acc[ch] = *source_pixel * (half_kernel_x_size+1) as f32; | ||
| let mut kernel_pix_offset = pix_offset; |
There was a problem hiding this comment.
Wondering wether this offset could be precomputed beforehand as you are computing several times in the top level function
|
@johnnv1 any idea why python tests are failing (I believe it’s unrelated to this PR). Shouldn’t we be using the new just commands in https://github.com/kornia/kornia-rs/blob/main/.github/workflows/python_test.yml#L40 |
yeah, seems unrelated, but should be working |
There was a problem hiding this comment.
Can you expand this benchmark and report the numbers so that we know wether this method is really making what’s expected ?
I highly suggest once you have the benchmark setup that you play around with it and try to do micro optimisations like reusing as much as possible pre-computed variables as I suggested in the review to see how affects in the benchmarks.
|
|
||
| let mut input_img = src.clone(); | ||
| for (half_kernel_x_size, half_kernel_y_size) in half_kernel_x_sizes.iter().zip(half_kernel_y_sizes.iter()) { | ||
| let mut transposed = Image::<f32, C>::from_size_val(transposed_size, 0.0)?; |
There was a problem hiding this comment.
You can allocate once outside the loop
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
Why should be different? The test you describe should give you a box of ones, right ?
solve #168. The algorithm was derived from this blog post