From b491acfe8309e3e1bfbef48bc72787b5ba1c2918 Mon Sep 17 00:00:00 2001 From: Walter Perdan Date: Wed, 2 Sep 2026 18:46:38 +0200 Subject: [PATCH 1/2] fix(imgproc): CLAHE edge-pixel extrapolation and calc_back_project range validation Two real bugs found by Qodo's review on #121, verified before fixing (a third finding on the same review - missing FromPrimitive bound - is a false positive, same recurring rule-2966076 pattern already identified on #110: the functions only ever read pixels via ToPrimitive::to_f32(), never construct T from a primitive). - CLAHE interpolation clamped the tile index *before* computing the blend fraction. At y=0 (tile_rows=4), tyf=-0.5 -> floor=-1 -> clamped to 0 -> ya = tyf - 0 = -0.5, ya1 = 1.5. Those aren't interpolation weights (they don't lie in [0,1]) - they extrapolate beyond the edge tile's LUT instead of blending within it, on the top row and left column of both the u8 and u16 paths. Fixed by deriving the fraction from the *unclamped* floor (always in [0,1) by construction) and clamping only the LUT-lookup indices separately. Extracted the duplicated u8/u16 weight math into a single tile_interp_weights() helper so both paths share one implementation and a direct unit test can check the invariant (weights in [0,1], sum to 1) across the actual coordinate range, including the exact y=0 case from the bug report. Confirmed the old formula fails that assertion (ya=-0.5). - calc_back_project checked only ranges.len() == dims, never the contents - unlike calc_hist, which already validates Uniform(lo, hi) satisfies lo < hi (rejecting NaN via partial_cmp) and NonUniform boundaries have the right length and strict ordering. A malformed range (NaN bounds, wrong-length or unsorted boundaries) silently produced a plausible-but-wrong back-projection instead of an error. Extracted calc_hist's validation into a shared validate_hist_ranges() helper (parameterized by the caller's name for error messages) so calc_hist and calc_back_project can't drift apart again, per Qodo's own suggestion. Verified: cargo fmt --check and clippy -D warnings clean under --all-features and --no-default-features; cargo test --workspace 345 lib tests + 40 doc-tests (342 + 3 new regression tests) passing under both the default (parallel) and --no-default-features --features std (sequential) configs, identical results. Co-Authored-By: Claude Sonnet 5 --- src/imgproc/histogram.rs | 176 ++++++++++++++++++++++----------------- src/imgproc/tests.rs | 98 ++++++++++++++++++++++ 2 files changed, 198 insertions(+), 76 deletions(-) diff --git a/src/imgproc/histogram.rs b/src/imgproc/histogram.rs index 49bff93..cfdda4e 100644 --- a/src/imgproc/histogram.rs +++ b/src/imgproc/histogram.rs @@ -171,6 +171,76 @@ fn map_bin(val: f32, range: &RangeSpec, hist_size: usize) -> Option { } } +/// Validates that `hist_size` and `ranges` describe a well-formed set of +/// histogram dimensions, shared by `calc_hist` and `calc_back_project` so +/// the two can't drift: `hist_size[d] > 0`, `Uniform(lo, hi)` satisfies +/// `lo < hi` (also rejects NaN, since `partial_cmp` returns `None` for it), +/// and `NonUniform(boundaries)` has exactly `hist_size[d] + 1` strictly +/// increasing entries. +/// +/// `fn_name` is used as the error-message prefix so callers keep their own +/// identity in the message (e.g. `"calc_hist: ..."` vs +/// `"calc_back_project: ..."`). +fn validate_hist_ranges(fn_name: &str, hist_size: &[usize], ranges: &[RangeSpec]) -> Result<()> { + for (d, &sz) in hist_size.iter().enumerate() { + if sz == 0 { + cv_bail!( + tags::IMGPROC, + InvalidInput, + "{}: hist_size[{}] must be > 0 (got 0)", + fn_name, + d + ); + } + match &ranges[d] { + RangeSpec::Uniform(lo, hi) => { + if lo.partial_cmp(hi) != Some(Ordering::Less) { + cv_bail!( + tags::IMGPROC, + InvalidInput, + "{}: Uniform range [{}] must satisfy lo < hi (got {} >= {})", + fn_name, + d, + lo, + hi + ); + } + } + RangeSpec::NonUniform(boundaries) => { + if boundaries.len() != sz + 1 { + cv_bail!( + tags::IMGPROC, + InvalidInput, + "{}: NonUniform boundaries[{}] length {} must be hist_size[{}]+1 ({})", + fn_name, + d, + boundaries.len(), + d, + sz + 1 + ); + } + for k in 0..boundaries.len() - 1 { + if boundaries[k].partial_cmp(&boundaries[k + 1]) != Some(Ordering::Less) { + cv_bail!( + tags::IMGPROC, + InvalidInput, + "{}: NonUniform boundaries[{}][{}] ({}) must be < boundaries[{}][{}] ({})", + fn_name, + d, + k, + boundaries[k], + d, + k + 1, + boundaries[k + 1] + ); + } + } + } + } + } + Ok(()) +} + // --------------------------------------------------------------------------- // calc_hist // --------------------------------------------------------------------------- @@ -269,58 +339,7 @@ pub fn calc_hist( } // Validate hist_size and ranges (prevents panics in bin mapping) - for (d, &sz) in hist_size.iter().enumerate() { - if sz == 0 { - cv_bail!( - tags::IMGPROC, - InvalidInput, - "calc_hist: hist_size[{}] must be > 0 (got 0)", - d - ); - } - match &ranges[d] { - RangeSpec::Uniform(lo, hi) => { - if lo.partial_cmp(hi) != Some(Ordering::Less) { - cv_bail!( - tags::IMGPROC, - InvalidInput, - "calc_hist: Uniform range [{}] must satisfy lo < hi (got {} >= {})", - d, - lo, - hi - ); - } - } - RangeSpec::NonUniform(boundaries) => { - if boundaries.len() != sz + 1 { - cv_bail!( - tags::IMGPROC, - InvalidInput, - "calc_hist: NonUniform boundaries[{}] length {} must be hist_size[{}]+1 ({})", - d, - boundaries.len(), - d, - sz + 1 - ); - } - for k in 0..boundaries.len() - 1 { - if boundaries[k].partial_cmp(&boundaries[k + 1]) != Some(Ordering::Less) { - cv_bail!( - tags::IMGPROC, - InvalidInput, - "calc_hist: NonUniform boundaries[{}][{}] ({}) must be < boundaries[{}][{}] ({})", - d, - k, - boundaries[k], - d, - k + 1, - boundaries[k + 1] - ); - } - } - } - } - } + validate_hist_ranges("calc_hist", hist_size, ranges)?; let total_bins: usize = hist_size.iter().product(); @@ -477,6 +496,7 @@ pub fn calc_back_project( dims ); } + validate_hist_ranges("calc_back_project", hist_size, ranges)?; let rows = images[0].rows; let cols = images[0].cols; @@ -1199,6 +1219,30 @@ fn clip_and_redistribute(tile_hist: &mut [i32], clip_limit: i32, hist_size: usiz } } +/// Computes the pair of tile indices and blend weight for CLAHE's bilinear +/// tile interpolation along one axis. +/// +/// `coord_f` is the pixel's tile-space coordinate, already offset by -0.5 so +/// tile centers land on integers. Returns `(idx1, idx2, weight1, weight2)` +/// where `weight1 + weight2 == 1.0` and both weights are in `[0, 1]`. +/// +/// The fraction is derived from the *unclamped* floor of `coord_f` so it +/// always lands in `[0, 1)` - a genuine convex blend. Only the indices used +/// for LUT lookup are clamped to the valid tile range; clamping the index +/// *before* computing the fraction (as an earlier version of this code did) +/// produces fractions outside `[0, 1]` at the first/last tile, extrapolating +/// beyond that tile's LUT instead of blending within it. +#[inline(always)] +pub(crate) fn tile_interp_weights(coord_f: f64, num_tiles: usize) -> (usize, usize, f64, f64) { + let idx1_raw = coord_f.floor() as i32; + let weight2 = coord_f - idx1_raw as f64; + let weight1 = 1.0 - weight2; + let max_idx = num_tiles as i32 - 1; + let idx1 = idx1_raw.clamp(0, max_idx) as usize; + let idx2 = (idx1_raw + 1).clamp(0, max_idx) as usize; + (idx1, idx2, weight1, weight2) +} + #[allow(clippy::too_many_arguments)] fn interpolate_tiles_u8( src: &Matrix, @@ -1219,21 +1263,11 @@ fn interpolate_tiles_u8( let process_row = |y: usize, dst_row: &mut [u8]| { let tyf = y as f64 * inv_th - 0.5; - let ty1 = (tyf.floor() as i32).max(0); - let ty2 = (ty1 + 1).min(tiles_y as i32 - 1); - let ya = tyf - ty1 as f64; - let ya1 = 1.0 - ya; - let ty1 = ty1 as usize; - let ty2 = ty2 as usize; + let (ty1, ty2, ya1, ya) = tile_interp_weights(tyf, tiles_y); for (x, out_pixel) in dst_row.iter_mut().enumerate() { let txf = x as f64 * inv_tw - 0.5; - let tx1 = (txf.floor() as i32).max(0); - let tx2 = (tx1 + 1).min(tiles_x as i32 - 1); - let xa = txf - tx1 as f64; - let xa1 = 1.0 - xa; - let tx1 = tx1 as usize; - let tx2 = tx2 as usize; + let (tx1, tx2, xa1, xa) = tile_interp_weights(txf, tiles_x); let src_val = *src.get(y, x, 0).unwrap_or(&0) as usize; let bin = (src_val >> bit_shift).min(hist_size - 1); @@ -1286,21 +1320,11 @@ fn interpolate_tiles_u16( let process_row = |y: usize, dst_row: &mut [u16]| { let tyf = y as f64 * inv_th - 0.5; - let ty1 = (tyf.floor() as i32).max(0); - let ty2 = (ty1 + 1).min(tiles_y as i32 - 1); - let ya = tyf - ty1 as f64; - let ya1 = 1.0 - ya; - let ty1 = ty1 as usize; - let ty2 = ty2 as usize; + let (ty1, ty2, ya1, ya) = tile_interp_weights(tyf, tiles_y); for (x, out_pixel) in dst_row.iter_mut().enumerate() { let txf = x as f64 * inv_tw - 0.5; - let tx1 = (txf.floor() as i32).max(0); - let tx2 = (tx1 + 1).min(tiles_x as i32 - 1); - let xa = txf - tx1 as f64; - let xa1 = 1.0 - xa; - let tx1 = tx1 as usize; - let tx2 = tx2 as usize; + let (tx1, tx2, xa1, xa) = tile_interp_weights(txf, tiles_x); let src_val = *src.get(y, x, 0).unwrap_or(&0) as usize; let bin = (src_val >> bit_shift).min(hist_size - 1); diff --git a/src/imgproc/tests.rs b/src/imgproc/tests.rs index 6c97aaa..65ed16b 100644 --- a/src/imgproc/tests.rs +++ b/src/imgproc/tests.rs @@ -1556,6 +1556,66 @@ mod imgproc_tests { .is_err()); } + #[test] + fn test_calc_back_project_invalid_uniform_range_error() { + // Regression test: calc_back_project used to skip the range + // validation calc_hist already had (lo < hi, NaN rejection, + // non-uniform boundary shape/ordering), silently producing a + // plausible-but-wrong projection instead of an error. + let img = Matrix::from_vec(1, 4, 1, vec![0u8, 1, 2, 3]); + let hist = Matrix::from_vec(4, 1, 1, vec![10.0, 20.0, 30.0, 40.0]); + + // NaN bounds: lo.partial_cmp(hi) is None, must be rejected. + assert!(calc_back_project( + &[&img], + &[0], + &[4], + &hist, + &[RangeSpec::Uniform(f32::NAN, f32::NAN)], + 1.0, + ) + .is_err()); + + // lo >= hi. + assert!(calc_back_project( + &[&img], + &[0], + &[4], + &hist, + &[RangeSpec::Uniform(4.0, 0.0)], + 1.0, + ) + .is_err()); + } + + #[test] + fn test_calc_back_project_invalid_nonuniform_boundaries_error() { + let img = Matrix::from_vec(1, 4, 1, vec![0u8, 1, 2, 3]); + let hist = Matrix::from_vec(4, 1, 1, vec![10.0, 20.0, 30.0, 40.0]); + + // Wrong length: needs hist_size[0] + 1 = 5 boundaries, only 3 given. + assert!(calc_back_project( + &[&img], + &[0], + &[4], + &hist, + &[RangeSpec::NonUniform(vec![0.0, 2.0, 4.0])], + 1.0, + ) + .is_err()); + + // Not strictly increasing. + assert!(calc_back_project( + &[&img], + &[0], + &[4], + &hist, + &[RangeSpec::NonUniform(vec![0.0, 2.0, 1.0, 3.0, 4.0])], + 1.0, + ) + .is_err()); + } + #[test] fn test_compare_hist_correl_identical() { let h1 = Matrix::from_vec(4, 1, 1, vec![1.0, 2.0, 3.0, 4.0]); @@ -1968,4 +2028,42 @@ mod imgproc_tests { let dst = clahe.apply_u8(&img).unwrap(); assert_eq!(dst.data.len(), 256); } + + #[test] + fn test_clahe_tile_interp_weights_are_bounded() { + // Regression test: the interpolation fraction used to be derived + // *after* clamping the tile index, which produced weights outside + // [0, 1] (extrapolation) at the top/left edges - e.g. at y=0 with + // tile_rows=4, tyf=-0.5 used to yield ya=-0.5, ya1=1.5 instead of a + // convex blend. Check the invariant holds across the coordinate + // range that actually occurs (coord_f = pixel*inv_tile_extent - 0.5, + // for pixel in 0..src_extent), for several tile counts. + use crate::imgproc::histogram::tile_interp_weights; + + for num_tiles in [1usize, 2, 3, 5] { + let tile_extent = 4usize; // pixels per tile + let inv = 1.0 / tile_extent as f64; + let src_extent = num_tiles * tile_extent; + + for pixel in 0..src_extent { + let coord_f = pixel as f64 * inv - 0.5; + let (idx1, idx2, w1, w2) = tile_interp_weights(coord_f, num_tiles); + + assert!( + (0.0..=1.0).contains(&w1) && (0.0..=1.0).contains(&w2), + "weights out of [0,1] at pixel={pixel}, num_tiles={num_tiles}: w1={w1}, w2={w2}" + ); + assert!( + (w1 + w2 - 1.0).abs() < 1e-12, + "weights don't sum to 1 at pixel={pixel}: w1={w1}, w2={w2}" + ); + assert!(idx1 < num_tiles && idx2 < num_tiles); + } + } + + // The exact case from the bug report: y=0, tile_rows=4 -> tyf=-0.5. + let (idx1, idx2, w1, w2) = tile_interp_weights(-0.5, 2); + assert_eq!((idx1, idx2), (0, 0)); // both clamp to the first tile + assert!((w1 - 0.5).abs() < 1e-12 && (w2 - 0.5).abs() < 1e-12); + } } From f5da49781ce5fb09cc54a358d4b1533783804939 Mon Sep 17 00:00:00 2001 From: Walter Perdan Date: Wed, 2 Sep 2026 18:54:55 +0200 Subject: [PATCH 2/2] fix(imgproc): guard validate_hist_ranges against usize overflow Qodo's review on #122 found that validate_hist_ranges computed hist_size[d] + 1 unchecked, so hist_size[d] == usize::MAX panicked with "attempt to add with overflow" (in debug/test builds) instead of returning the InvalidInput error the function exists to produce - violating the project's own "never panic! in library code" rule. Confirmed by reproducing the panic directly (std::panic::catch_unwind) before applying the fix. The empty-boundaries underflow Qodo also flagged (boundaries.len() - 1 on an empty Vec) is a consequence of the same root cause: it's only reachable when sz + 1 has already wrapped to 0 in a release build without overflow checks, matching an empty boundaries.len(). Guarding the addition with checked_add and returning InvalidInput on overflow closes both paths at once - for any hist_size that doesn't overflow, sz + 1 >= 1, so boundaries.len() == sz + 1 can never be 0. Verified: cargo fmt --check and clippy -D warnings clean under --all-features and --no-default-features; new regression test (hist_size = usize::MAX, empty non-uniform boundaries) confirms calc_back_project now returns Err instead of panicking; cargo test --workspace 346 lib tests + 40 doc-tests passing under both the default (parallel) and --no-default-features --features std (sequential) configs, identical results. Co-Authored-By: Claude Sonnet 5 --- src/imgproc/histogram.rs | 16 ++++++++++++++-- src/imgproc/tests.rs | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/imgproc/histogram.rs b/src/imgproc/histogram.rs index cfdda4e..0fc2e7a 100644 --- a/src/imgproc/histogram.rs +++ b/src/imgproc/histogram.rs @@ -47,6 +47,7 @@ use crate::core::types::{BorderTypes, Size2i}; use crate::core::utils::border_interpolate; use crate::core::Matrix; use crate::cv_bail; +use crate::cv_err; #[cfg(feature = "parallel")] use rayon::prelude::*; @@ -207,7 +208,18 @@ fn validate_hist_ranges(fn_name: &str, hist_size: &[usize], ranges: &[RangeSpec] } } RangeSpec::NonUniform(boundaries) => { - if boundaries.len() != sz + 1 { + let expected_len = sz.checked_add(1).ok_or_else(|| { + cv_err!( + tags::IMGPROC, + InvalidInput, + "{}: hist_size[{}] ({}) is too large (hist_size[{}]+1 overflows)", + fn_name, + d, + sz, + d + ) + })?; + if boundaries.len() != expected_len { cv_bail!( tags::IMGPROC, InvalidInput, @@ -216,7 +228,7 @@ fn validate_hist_ranges(fn_name: &str, hist_size: &[usize], ranges: &[RangeSpec] d, boundaries.len(), d, - sz + 1 + expected_len ); } for k in 0..boundaries.len() - 1 { diff --git a/src/imgproc/tests.rs b/src/imgproc/tests.rs index 65ed16b..58fc9da 100644 --- a/src/imgproc/tests.rs +++ b/src/imgproc/tests.rs @@ -1616,6 +1616,25 @@ mod imgproc_tests { .is_err()); } + #[test] + fn test_calc_back_project_hist_size_overflow_error() { + // Regression test: validate_hist_ranges computed hist_size[d] + 1 + // unchecked, so hist_size[d] == usize::MAX panicked with "attempt to + // add with overflow" instead of returning an InvalidInput error. + // Confirmed the pre-fix code panics here via std::panic::catch_unwind. + let img = Matrix::from_vec(1, 1, 1, vec![0u8]); + let hist = Matrix::from_vec(1, 1, 1, vec![1.0f32]); + let result = calc_back_project( + &[&img], + &[0], + &[usize::MAX], + &hist, + &[RangeSpec::NonUniform(vec![])], + 1.0, + ); + assert!(result.is_err()); + } + #[test] fn test_compare_hist_correl_identical() { let h1 = Matrix::from_vec(4, 1, 1, vec![1.0, 2.0, 3.0, 4.0]);