Conversation
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoAdd ExecutionStrategy support to separable filters, blur operations, and threshold functions
WalkthroughsDescription• Add ExecutionStrategy parameter to separable filter and blur operations - Enables serial, parallel elements, auto rows, and fixed thread pool execution modes - Consolidates execution strategies using global macros for efficient convolution • Implement parallel execution paths with proper error handling - Adds Parallel error variant to ImageError for execution failures - Validates thread counts and image sizes before processing • Extend ExecutionStrategy support to threshold operations - Introduces ExecuteExt trait for flexible strategy-based execution - Updates threshold_binary to accept execution strategy parameter • Add comprehensive benchmarks and tests for parallel strategies - New bench_threshold.rs benchmark suite comparing execution strategies - Tests verify consistency across Serial, Fixed, AutoRows, and ParallelElements strategies Diagramflowchart LR
A["ExecutionStrategy API"] --> B["Separable Filter"]
A --> C["Blur Operations"]
A --> D["Threshold Operations"]
B --> E["Serial Execution"]
B --> F["Parallel Strategies"]
C --> E
C --> F
D --> E
D --> F
F --> G["Fixed Thread Pool"]
F --> H["AutoRows"]
F --> I["ParallelElements"]
File Changes1. crates/kornia-image/src/error.rs
|
There was a problem hiding this comment.
Pull request overview
This PR extends the ExecutionStrategy API introduced in #600 to separable filter operations (box_blur, gaussian_blur, sobel), enabling runtime control over parallelization. The implementation adds flexible serial and parallel execution modes with strategies including Serial, ParallelElements, AutoRows, and Fixed thread pools.
Changes:
- Implemented
SeparableFilterstruct with execution strategy support and optimized convolution macros - Added
ExecutionStrategyparameter toseparable_filter()and createdseparable_filter_serial()for non-thread-safe types - Updated
box_blur,gaussian_blur, andsobelwith_with_strategyvariants while maintaining backward compatibility through Serial defaults - Added comprehensive tests verifying consistency across all execution strategies
- Updated examples and benchmarks to use the new API
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/kornia-imgproc/src/parallel.rs | Adds ExecutionStrategy enum, ParallelError types, and ExecuteExt trait (from PR #600) |
| crates/kornia-imgproc/src/filter/separable_filter.rs | Core implementation with execution strategies, macros for convolution, tests for strategy consistency |
| crates/kornia-imgproc/src/filter/ops.rs | Updates box_blur, gaussian_blur, sobel with _with_strategy variants and Serial defaults |
| crates/kornia-imgproc/src/threshold.rs | Updates threshold_binary to use ExecutionStrategy (from PR #600) |
| crates/kornia-imgproc/benches/bench_filters.rs | Adds benchmarks for Serial, ParallelElements, and AutoRows strategies |
| crates/kornia-imgproc/benches/bench_threshold.rs | New benchmark file for threshold operations with different strategies |
| crates/kornia-imgproc/Cargo.toml | Adds bench_threshold benchmark configuration |
| crates/kornia-image/src/error.rs | Adds Parallel error variant for execution errors |
| examples/features/src/main.rs | Updates threshold_binary calls with ExecutionStrategy::Serial |
| examples/binarize/src/main.rs | Updates threshold_binary calls with ExecutionStrategy::Serial |
| dst: &mut Image<T, C, A2>, | ||
| kernel_x: &[f32], | ||
| kernel_y: &[f32], | ||
| strategy: ExecutionStrategy, | ||
| ) -> Result<(), ImageError> | ||
| where | ||
| T: FloatConversion + Clone + Zero + Send + Sync, | ||
| { | ||
| if kernel_x.is_empty() || kernel_y.is_empty() { | ||
| return Err(ImageError::InvalidKernelLength( | ||
| kernel_x.len(), | ||
| kernel_y.len(), | ||
| )); | ||
| } | ||
|
|
||
| if src.size() != dst.size() { | ||
| return Err(ImageError::InvalidImageSize( | ||
| src.cols(), | ||
| src.rows(), | ||
| dst.cols(), | ||
| dst.rows(), | ||
| )); | ||
| } | ||
|
|
||
| let filter = SeparableFilter::new(kernel_x, kernel_y); | ||
| filter.apply(src, dst, strategy) | ||
| } |
There was a problem hiding this comment.
Breaking API change: The separable_filter function signature has been modified to require a strategy: ExecutionStrategy parameter. This breaks backward compatibility for any code using separable_filter directly. Consider either: (1) keeping the original separable_filter signature defaulting to ExecutionStrategy::Serial and adding separable_filter_with_strategy for the parameterized version (consistent with the pattern used for box_blur, gaussian_blur, and sobel), or (2) clearly documenting this as a breaking change in the PR description and considering a major version bump.
| &mut dst_auto, | ||
| &kernel_x, | ||
| &kernel_y, | ||
| ExecutionStrategy::AutoRows(0), |
There was a problem hiding this comment.
AutoRows(0) is being tested with a stride of 0, which should trigger an InvalidRowStride error in the ExecuteExt trait implementation. However, the separable_filter implementation ignores the stride parameter for AutoRows and uses cols * C instead. This creates an inconsistency: the ExecuteExt::execute_with would reject stride=0, but separable_filter's AutoRows branch doesn't validate or use the provided stride value at all. Either the test should use a valid stride (like AutoRows(10) for width 10), or the AutoRows implementation in separable_filter should validate the stride parameter.
| ExecutionStrategy::AutoRows(0), | |
| ExecutionStrategy::AutoRows(1), |
| &mut dst_auto, | ||
| &kernel_x, | ||
| &kernel_y, | ||
| ExecutionStrategy::AutoRows(0), |
There was a problem hiding this comment.
AutoRows(0) is being tested with a stride of 0, which should trigger an InvalidRowStride error in the ExecuteExt trait implementation. However, the separable_filter implementation ignores the stride parameter for AutoRows and uses cols * C instead. This creates an inconsistency: the ExecuteExt::execute_with would reject stride=0, but separable_filter's AutoRows branch doesn't validate or use the provided stride value at all. Either the test should use a valid stride (like AutoRows(8) for width 8), or the AutoRows implementation in separable_filter should validate the stride parameter.
| ExecutionStrategy::AutoRows(0), | |
| ExecutionStrategy::AutoRows(1), |
| ExecutionStrategy::AutoRows(_) => { | ||
| // Horizontal | ||
| temp.par_chunks_mut(cols * C) | ||
| .enumerate() | ||
| .for_each(|(r, row)| run_horizontal!(self, r, row, src_data, cols, C)); | ||
|
|
||
| // Vertical | ||
| dst_data | ||
| .par_chunks_mut(cols * C) |
There was a problem hiding this comment.
The AutoRows execution strategy ignores the stride parameter provided by the user. The stride value in ExecutionStrategy::AutoRows(_) is not validated or used; instead, the implementation hardcodes cols * C as the chunk size. This is inconsistent with the ExecutionStrategy API design where AutoRows(usize) should allow the user to specify the row stride. Either remove the parameter and make it AutoRows (auto-computing the stride), or use the provided stride value.
| ExecutionStrategy::AutoRows(_) => { | |
| // Horizontal | |
| temp.par_chunks_mut(cols * C) | |
| .enumerate() | |
| .for_each(|(r, row)| run_horizontal!(self, r, row, src_data, cols, C)); | |
| // Vertical | |
| dst_data | |
| .par_chunks_mut(cols * C) | |
| ExecutionStrategy::AutoRows(row_stride) => { | |
| if row_stride == 0 { | |
| return Err(ImageError::Parallel("row_stride must be > 0".to_string())); | |
| } | |
| let row_width = cols * C; | |
| let chunk_size = row_width | |
| .checked_mul(row_stride) | |
| .ok_or_else(|| ImageError::Parallel("row_stride too large".to_string()))?; | |
| // Horizontal | |
| temp.par_chunks_mut(chunk_size) | |
| .enumerate() | |
| .for_each(|(r, row)| run_horizontal!(self, r, row, src_data, cols, C)); | |
| // Vertical | |
| dst_data | |
| .par_chunks_mut(chunk_size) |
| dst: &mut Image<T, C, A2>, | ||
| kernel_x: &[f32], | ||
| kernel_y: &[f32], | ||
| strategy: ExecutionStrategy, |
There was a problem hiding this comment.
The documentation for separable_filter is incomplete. The strategy parameter is not documented in the doc comment. Add documentation for the strategy parameter explaining what execution strategy to use.
| /// | ||
| /// * `src` - Source image | ||
| /// * `dst` - Destination image (must have same size as source) | ||
| /// * `kernel_x` - Horizontal filter kernel |
There was a problem hiding this comment.
Trailing whitespace detected at the end of this line. Remove the trailing spaces.
| /// * `kernel_x` - Horizontal filter kernel | |
| /// * `kernel_x` - Horizontal filter kernel |
Code Review by Qodo
✅ 1.
|
| let img = Image::<f32, 1, _>::from_size_val(size, 0.5, CpuAllocator).unwrap(); | ||
| let mut dst = Image::<f32, 1, _>::from_size_val(size, 0.0, CpuAllocator).unwrap(); | ||
| let kernel = vec![1.0]; | ||
|
|
||
| // Fixed(0) should error | ||
| let result = separable_filter( | ||
| &img, | ||
| &mut dst, | ||
| &kernel, | ||
| &kernel, | ||
| ExecutionStrategy::Fixed(0), | ||
| ); | ||
| assert!(result.is_err()); | ||
| let err_msg = result.unwrap_err().to_string(); | ||
| assert!( |
There was a problem hiding this comment.
1. test_fixed_threadpool_validation unwraps 📘 Rule violation ⛯ Reliability
The newly added test uses unwrap() and unwrap_err() on fallible image creation and error extraction, which can panic and obscures error handling intent. This violates the compliance requirement to avoid unchecked assumptions and propagate/handle errors using Result/?.
Agent Prompt
## Issue description
The new test `test_fixed_threadpool_validation` uses `unwrap()`/`unwrap_err()` on fallible calls, which can panic and violates the no-unwrap compliance rule.
## Issue Context
This test already uses assertions on `Result`; it can be made panic-free by returning `Result` and matching on `Err(e)`.
## Fix Focus Areas
- crates/kornia-imgproc/src/filter/separable_filter.rs[755-778]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ExecutionStrategy::AutoRows(_) => { | ||
| // Horizontal | ||
| temp.par_chunks_mut(cols * C) | ||
| .enumerate() | ||
| .for_each(|(r, row)| run_horizontal!(self, r, row, src_data, cols, C)); | ||
|
|
||
| // Vertical | ||
| dst_data | ||
| .par_chunks_mut(cols * C) | ||
| .enumerate() | ||
| .for_each(|(r, row)| run_vertical!(self, r, row, temp, rows, cols, C, T)); | ||
| } |
There was a problem hiding this comment.
2. autorows stride unused 📘 Rule violation ✓ Correctness
In SeparableFilter::apply, the ExecutionStrategy::AutoRows(stride) parameter is ignored (_), so callers cannot control/validate row stride as documented. This creates misleading behavior and misses validation of boundary values like stride == 0.
Agent Prompt
## Issue description
`ExecutionStrategy::AutoRows(stride)` is implemented as `AutoRows(_)` in `SeparableFilter::apply`, so the provided `stride` is ignored and `stride == 0` is not validated/handled. This makes the API misleading and breaks boundary/edge-case expectations.
## Issue Context
`ExecutionStrategy::AutoRows(usize)` is documented as requiring a row stride. The implementation should either (a) honor the stride, or (b) redefine the variant to not accept a stride and update call sites/tests accordingly.
## Fix Focus Areas
- crates/kornia-imgproc/src/filter/separable_filter.rs[203-241]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| dst: &mut Image<T, C, A2>, | ||
| threshold: T, | ||
| max_value: T, | ||
| strategy: ExecutionStrategy, | ||
| ) -> Result<(), ImageError> |
There was a problem hiding this comment.
3. Morphology example compile break 🐞 Bug ✓ Correctness
threshold_binary() now requires an ExecutionStrategy argument, but examples/morphology still calls the old 4-argument signature. Since examples/* are workspace members, this will break building the workspace examples.
Agent Prompt
### Issue description
`threshold_binary` now requires a new `ExecutionStrategy` argument, but `examples/morphology` still calls the old signature. This causes a compile failure for the `examples/morphology` workspace member.
### Issue Context
Other examples were updated in this PR, but `examples/morphology/src/main.rs` still uses the old 4-arg call.
### Fix Focus Areas
- examples/morphology/src/main.rs[44-51]
- crates/kornia-imgproc/src/threshold.rs[42-48]
### Suggested change
Pass a strategy argument (likely `ExecutionStrategy::Serial` for deterministic/small example runs), e.g.:
```rust
use kornia::imgproc::parallel::ExecutionStrategy;
...
threshold::threshold_binary(&gray_single, &mut binary, 128u8, 255u8, ExecutionStrategy::Serial)?;
```
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Execution Strategy Performance Comparison: 
Small Image (256x224), Kernel Size 5:
Medium Image (512x448), Kernel Size 5:
Medium Image (512x448), Kernel Size 11:
Large Image (1024x896), Kernel Size 17:
Mostly, AutoRows is giving better 5.6x speedup on large images. Note: AutoRows uses 1 row per parallel chunk for optimal granularity. |
edgarriba
left a comment
There was a problem hiding this comment.
good progress on the execution strategy for separable filters.
some concerns:
-
unsafe everywhere: the macros
run_horizontal!andrun_vertical!useget_unchecked— i understand the perf motivation but we should minimize unsafe. can you benchmark with bounds-checked access first? if the difference is negligible, prefer safe code. if it matters, at least add// SAFETY:comments explaining the invariants. -
widedependency: i seewide 1.1.1added to Cargo.lock — is this actually used in your changes? i don't see it in the separable filter code. if it's from another PR leaking into your branch, rebase. -
macro duplication:
run_horizontal!andrun_vertical!have very similar structure. consider a single generic approach or at least extracting the inner loop. -
benchmarks: good that you added strategy benchmarks. can you share actual numbers? specifically: for a 1920x1080 image, what's the speedup of ParallelElements vs Serial for gaussian_blur?
-
the
apply_serialvsapplysplit is clean. good that serial doesn't requireSend + Sync. -
the
_with_strategysuffix pattern works but makes the API a bit verbose. consider making strategy the default parameter and having the no-strategy version as the convenience wrapper (which you did — that's fine).
@sidd-27 can you cross-review the parallel execution parts?
…080 and added missing benchmark for fixed strategy
|
Benchmark for 1920x1080: strategy comparison (kernel size 5) at full HD resolution.
I currently kept unsafe indexing for performance but will change if required. |
|
@sidd-27 can you check once, I ran the benchmarks for all 4 strategies giving similar results. Once approved, could continue it to other ops. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs within 7 days. Thank you for your contributions! |
📝 Description
Applies
ExecutionStrategysupport to separable_filter and operations (box_blur, gaussian_blur, sobel), enabling flexible serial and parallel execution modes. The implementation consolidates execution strategies using global macros and improves parallelization options.Fixes/Relates to: #600
🛠️ Changes Made
ExecutionStrategyparameter to separable_filter() function signaturerun_horizontalandrun_verticalmacros for efficient convolutionSend + Synctrait bounds)ParallelElements,AutoRows, andFixedstrategiesExecutionStrategyExecutionStrategyfor deterministic results and verification of value match.ExecutionStrategyargument for examples for api change🧪 How Was This Tested?
Unit Tests: All existing 95 tests in
kornia-imgprocpassedManual Verification:
cargo check --package kornia-imgprocandcargo check --package feature.Performance/Edge Cases:
Send + Syncbounds ensure thread safe pixel type and avoided in serial for non thread safe pixel type.🕵️ AI Usage Disclosure
Check one of the following:
🚦 Checklist
💭 Additional Context