fix(imgproc): CLAHE edge-pixel extrapolation and calc_back_project range validation - #122
Conversation
…nge 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 <noreply@anthropic.com>
PR Summary by QodoFix CLAHE edge interpolation and back-project range validation
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
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 <noreply@anthropic.com>
|
Confirmed real — reproduced it directly with Fixed in Verified: fmt/clippy clean under |
Summary
Fixes two real bugs found by Qodo's review on #121, verified against the actual
code before fixing (a third finding on that review — missing
FromPrimitivebound — is a false positive, the same recurring rule-2966076 pattern already
identified on #110: the functions only ever read pixels via
ToPrimitive::to_f32(), never constructTfrom a primitive).CLAHE edge-pixel extrapolation. Interpolation clamped the tile index
before computing the blend fraction. At
y=0(tile_rows=4),tyf=-0.5→floor=-1→ clamped to0→ya = tyf - 0 = -0.5,ya1 = 1.5. Those aren'tinterpolation weights (they don't lie in
[0,1]) — they extrapolate beyondthe edge tile's LUT instead of blending within it, on the top row and left
column of both the
u8andu16paths. Fixed by deriving the fraction fromthe unclamped floor (always in
[0,1)by construction) and clamping onlythe LUT-lookup indices separately. Extracted the duplicated
u8/u16weightmath into a single
tile_interp_weights()helper.calc_back_projectrange validation. It checked onlyranges.len() == dims, never the contents — unlikecalc_hist, which already validatesUniform(lo, hi)satisfieslo < hi(rejecting NaN) andNonUniformboundaries have the right length and strict ordering. Extracted
calc_hist'svalidation into a shared
validate_hist_ranges()helper so the two functionscan't drift apart again.
Test plan
tile_interp_weightschecking weights stay in[0,1]and sum to 1 across the actual coordinate range (several tile counts),
including the exact
y=0case from the bug report. Confirmed the oldformula fails this assertion (
ya=-0.5).calc_back_projectrejecting NaN/inverted uniform rangesand malformed non-uniform boundaries.
cargo fmt --check/clippy -D warningsclean under--all-featuresand
--no-default-features.cargo test --workspace: 345 lib tests + 40 doc-tests passing underboth default (parallel) and
--no-default-features --features std(sequential) configs, identical results.
🤖 Generated with Claude Code