Conversation
Two things bit us during v0.7.1 and neither was written down. `npm run build` regenerates crates/wasm/pkg/package.json, which is tracked but had silently drifted a full release behind (0.6.1 while 0.7.0 shipped). Merging the release PR with "Rebase and merge" replayed dev's commits as new objects on main, so dev stopped being an ancestor of main — identical content, divergent history. Repaired by resetting dev to main; documented so the next release uses a merge commit instead. Also notes the two version locations in Cargo.toml, the git-cliff invocation and its missing-blank-line quirk, and that cliff.toml skips the release commit itself — so other changes must land in their own commits first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…chunks lint clippy 1.98 stabilized clippy::chunks_exact_to_as_chunks, which fires on every constant-size chunks_exact/chunks_exact_mut call in color.rs and broke CI on `dev` (and any unrelated open PR) once the runner picked up the new stable release, since our workflows install `dtolnay/rust-toolchain@stable` with no version pin. - Pin all `dtolnay/rust-toolchain@stable` steps in ci.yml and release.yml to 1.98.0 so a future clippy release can't retroactively break CI again. - Allow the new lint at each call site (guarded with `unknown_lints` so it doesn't itself break on older toolchains). `[T]::as_chunks` isn't stable yet, so the suggested rewrite isn't available.
Qodo review on #105 flagged that #[allow(clippy::chunks_exact_to_as_chunks)] violates this repo's compliance rule against introducing new Clippy allow overrides. `[T]::as_chunks`/`as_chunks_mut` (clippy's own suggested rewrite) is already stable, so rewrite the 12 flagged sites to use it directly and drop the allow attributes instead of suppressing the lint.
Implement complete histogram processing module (src/imgproc/histogram.rs) with OpenCV parity for imgproc histogram operations. - calc_hist: generic over Matrix<T> (u8/u16/f32), supports uniform and non-uniform bins, multi-image channel indexing, mask, accumulate - calc_back_project: generic over Matrix<T>, returns Matrix<f32> - compare_hist: Correl, ChiSqr, ChiSqrAlt, Intersection, Bhattacharyya, KullbackLeibler - equalize_hist: global histogram equalization for u8 - Clahe: Contrast Limited Adaptive Histogram Equalization with apply_u8 and apply_u16 support - 23 unit tests covering all functions and error paths Closes #98
- fix no_std import: replace use core::f64 with #[allow(unused_imports)] use num_traits::Float (15 E0599 in --no-default-features) - validate calc_hist inputs: hist_size>0, Uniform lo<hi, NonUniform boundaries len==hist_size+1 and monotonic, accumulate hist length==product - harden nonuniform_bin/uniform_bin against hist_size==0 and OOB indexing - validate calc_back_project: reject empty hist instead of clamp(0, -1) panic - validate Clahe tiles (handle negative Size2i via 0 sentinel) and return InvalidInput instead of divide-by-zero / overflow - validate bit_shift (u8 0..=7, u16 0..=15) in apply impls, fix 256>>/65536>> overflow - fix CLAHE padding parity with OpenCV: pad bottom/right with BORDER_REFLECT_101 instead of discarded top/left, remove stale _padded/pad params, use border_interpolate - add tile_rows/tile_cols zero check and checked_mul overflow guards - fix compare_hist empty histogram NaN -> InvalidInput - clarify clip_and_redistribute is an exact port of OpenCV CLAHE_CalcLut_Body (clahe.cpp) - refactor: generic pad_reflect101<T>, .max(0) tile sentinel, hoisted zero-capture lut_idx closure, Ordering import, functional hist_data init - verified: cargo fmt --check ok, cargo clippy -- -D warnings and --no-default-features -- -D warnings clean, cargo test --lib histogram 29/29, cargo build --no-default-features ok
The histogram module (added in #99) had zero parallel/simd feature gating, unlike the rest of imgproc. Add Rayon-backed fast paths behind the existing parallel feature convention, with sequential fallbacks preserved for no_std / parallel-disabled builds: - calc_hist: row-chunked fold/reduce (partial per-chunk histograms merged elementwise), generic over all dims since the sequential code is already dims-agnostic. - calc_back_project: row-chunked, mirrors the pattern in color.rs. - equalize_hist: the final LUT-apply pass is now a flat par_iter_mut zip; the histogram/LUT build stays sequential (cheap, <=256 bins). - Clahe::apply_u8/apply_u16: per-tile histogram + LUT construction parallelized via par_chunks_mut over the flat lut buffer (tiles are independent), and both interpolate_tiles_u8/u16 row-chunked the same way as calc_back_project. Verified cargo fmt --check, clippy -D warnings under --all-features and --no-default-features, cargo test --workspace (parallel-on) and cargo test --lib histogram --no-default-features --features std (sequential fallback) produce identical results, and a bare-metal build (thumbv7em-none-eabihf) still compiles. Adds calc_hist/calc_back_project/equalize_hist/Clahe::apply_u8 bench functions to imgproc_bench.rs; benchmark_results.md numbers are left for a follow-up run on the reference machine. Closes #106
chunks_mut/par_chunks_mut panic on a zero chunk size regardless of slice length. calc_back_project's new row-chunked parallel/sequential dispatch used images[0].cols directly as the chunk size, so a zero-width input (previously handled fine by the old nested loops, which simply skipped the inner loop) now panicked instead of returning an empty result. Bail out with the already-correctly-shaped empty dst before reaching the chunking dispatch, matching the original behavior. Confirmed the other three functions touched in #106/#110 aren't affected: calc_hist iterates by row-index range (safe at 0), equalize_hist uses flat iter_mut/par_iter_mut (safe at 0), and Clahe already validates cols == 0 via cv_bail! before reaching its interpolation loops. Found by Qodo's review on #110.
This reverts commit eb7673f.
- Moves SIMD helper from core to imgproc/simd.rs. - Fixes numerical overflow by casting intermediate f32 to f64 for accumulations. - Fixes epsilon classification of small f32 bins by using f64::EPSILON. - Restores exact f64::min() behavior for Intersection method to handle NaNs correctly. - Adds tests.rs coverage for compare_hist to satisfy Qodo rules.
- Implements MatVector class matching OpenCV.js conventions. - Exposes calcHistUniform and calcHistNonUniform supporting u8 and f32 images. - Exposes calcBackProjectUniform and calcBackProjectNonUniform. - Exposes compareHist with HIST_CMP_* constants. - Exposes equalizeHist with note on u8 single-channel support. - Exposes Clahe class wrapper with note on u8 single-channel support. - Adds unit tests in crates/wasm/tests/web.rs and updates README.md. Closes #107
Qodo's review on this PR raised 10 findings; verified each against the actual code (including compiling for the real wasm32-unknown-unknown target and running the wasm test suite under Node) before acting. Fixed (8 real issues): - Doc comments contained raw control bytes (BEL replacing 'a' in "accumulate", tabs replacing 't' in "tile_grid_width/height"), corrupting generated API docs. - crates/wasm/tests/web.rs failed to compile: it imported `Clahe`, but the Rust type is `WasmClahe` (`js_name` only renames the JS side). This went undetected because CI's "WASM Dual Build" job only runs `wasm-pack build`, never compiles the tests/ directory. - accumulate=true was a no-op: calc_hist_uniform/calc_hist_non_uniform always passed None as the existing histogram to core, so accumulate produced identical output to accumulate=false regardless of its value. Both now take an existing_hist: Option<Mat> parameter. - Multi-dimensional back-projection silently corrupted results (not a panic, contrary to the original report - verified with a probe test): calc_back_project inferred hist_size from hist's flat bin count alone, which is ambiguous for non-perfect-power shapes (8 bins could be [8] or [2, 4]). calc_back_project now takes an explicit hist_size parameter, matching calc_hist's existing convention, and the WASM wrapper functions thread it through instead of guessing. - HIST_CMP_* comparison-method constants are unavoidably exposed as callable functions (wasm-bindgen has no support for exporting a `pub const`), matching the same pattern used throughout this file for every other constant group (MORPH_*, FAST_TYPE_*, COLOR_*, etc.) - kept as-is rather than special-cased. - calc_back_project never validated that all input images have matching dimensions or that channel indices resolve (unlike calc_hist, which already does both). Fixed in core so every caller is protected. - A multichannel mask was silently accepted by calc_hist and only its channel 0 was ever read. Core now rejects non-single-channel masks. Not applied - false positives, verified directly rather than assumed: - "&[usize] unsupported by wasm-bindgen": false. Compiled clean for wasm32-unknown-unknown directly and via the actual `wasm-pack build` CI runs. - "Histogram APIs should return PureCvError, not JsError": false. Every wasm-exposed function in this file already returns JsError - that's the entire point of this being the boundary-adapter layer, and PureCvError doesn't implement Into<JsValue> anyway. - Initial pass also "fixed" the camelCase/SCREAMING_CASE js_name overrides (calcHistUniform, HIST_CMP_*, etc.) toward snake_case, based on one README example. Reverted: an exhaustive grep of this file shows every single pre-existing export (add, cvtColor, gaussianBlur, MORPH_RECT, FAST_TYPE_5_8, ...) uses this exact pattern deliberately and consistently - the README's snake_case example was simply stale documentation, not the real convention. Verified: cargo fmt --check and clippy -D warnings clean on both crates (--all-features and --no-default-features for the core crate); core `cargo test --workspace` 342 lib tests + 40 doc-tests passing; the real `npm run build:wasm` (matching CI's WASM Dual Build job exactly) succeeds; and `wasm-pack test --node` runs all 9 wasm tests (including 3 new regression tests for accumulate, 2D back-projection shape, and mask/image-size validation) against the actual wasm32 runtime, not just a type-check - all passing.
- examples/histogram.rs: calc_hist (uniform and non-uniform bins), calc_back_project, compare_hist across all 6 HistCompMethods, equalize_hist, and Clahe::apply_u8 at two tile-grid sizes. Loads examples/data/butterfly.jpg (matching filters.rs's convention) and saves output PNGs under examples/data/out/. The back-projection scale is derived from the histogram's own peak bin (255 / max_bin) rather than a fixed guess, matching OpenCV's typical demo pattern - visually verified each output image before settling on this. - crates/wasm/www/example_histogram.html + .js: interactive demo matching the existing per-feature example_*.html/.js convention (example_pyramid, example_hough_circles, ...) and reusing cv_demo_utils.js. Shows grayscale/equalize_hist/CLAHE side by side, a live calc_hist bar chart, and compare_hist scores against a CLAHE-equalized histogram, with clip-limit and tile-grid sliders. Linked from crates/wasm/www/index.html's gallery. Verified end-to-end in a real browser (served crates/wasm/ over HTTP, built the actual wasm-pack output) including interactive slider updates re-triggering calc_hist/equalize_hist/Clahe/compare_hist and re-rendering correctly. - Root README.md: new purecv-imgproc bullet for histograms/CLAHE, added `cargo run --example histogram` to the Running Examples list, and updated the Imgproc module test-coverage bullet (308 -> 342 unit tests, current count, plus a mention of the histogram/CLAHE tests). Closes #108
Verified each of the 5 findings against the code before acting: - Missing output directory (real, confirmed): the example wrote into examples/data/out/ without creating it first. It's gitignored, so a fresh checkout doesn't have it - the first save would fail with an IO error. corner_detection.rs already handles this correctly (std::fs::create_dir_all before saving); filters.rs, which this example was copied from, has the same latent bug. Reproduced by deleting examples/data/out/ and re-running - confirmed the fix. - Deep imports (split verdict): `Size2i` is already re-exported at purecv::core::Size2i (one existing example, corner_detection.rs, already uses that path) - switched to it, harmless simplification. NOT adding a `tags` re-export or changing that import: `use purecv::core::logging::tags;` is the exhaustive, deliberate convention used in 14+ files across this crate, including 3 other pre-existing examples and the official usage examples in logging.rs's own doc comments. Changing it would make this example the only inconsistent one in the whole codebase. - WASM object leak on error path, blob URL never revoked, and drag-and-drop advertised but not implemented (all real, confirmed): none of the other 9 example_*.js files in crates/wasm/www/ handle any of these three either, so these aren't regressions from an established good pattern - genuine bugs worth fixing in this file regardless. Allocated WASM objects are now tracked in variables declared outside the try block and freed in a finally block; the file-upload blob URL is now revoked once loadImage resolves (success or failure); dragover/drop handlers now share the same load path as the click-to-upload flow. Verified: cargo fmt --check and clippy -D warnings clean; cargo test --workspace 342 lib tests + 40 doc-tests passing; re-ran the example against a deleted examples/data/out/ to confirm the directory-creation fix; and re-verified the WASM demo end-to-end in a real browser - normal rendering and slider interaction still work after the try/finally refactor (no console errors, no double-free), and a synthetic drop event (fetched the demo image, dispatched a real DragEvent at the drop zone) confirmed the new drag-and-drop path loads and processes the image correctly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI never actually executed the #[cfg(not(feature = "parallel"))] code path for any module - it was compiled (via the no_std job's cargo build/clippy) but never run under cargo test. cargo test --workspace uses the crate's default = ["std", "parallel"] feature set, so the sequential fallback in every module was untested at runtime. Add a step running cargo test --workspace --no-default-features --features std alongside the existing parallel/simd/ndarray feature runs. Verified locally first: 342 lib tests + 40 doc-tests pass under this config with no latent sequential/parallel divergence. Closes #112 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Every other imgproc submodule (color.rs, edge.rs, filter.rs, morph.rs, pyramid.rs, threshold.rs, hough.rs, geometric.rs, resize.rs, derivatives.rs, feature.rs) keeps its tests solely in the shared src/imgproc/tests.rs, per the module structure convention in CLAUDE.md. histogram.rs was the only one with its own embedded #[cfg(test)] mod tests block, left over from how #99 originally wrote it. Mechanical move, no behavior change: all 33 histogram tests already went through the public API (calc_hist, calc_back_project, compare_hist, equalize_hist, Clahe/create_clahe, RangeSpec, HistCompMethods) with no dependency on private helpers, so they drop straight into tests.rs's existing imgproc_tests module (which already glob-imports crate::core::* and crate::imgproc::*) with no import changes needed. Verified: same 342 lib test count before and after (nothing lost or duplicated), passing under both default and --no-default-features --features std, fmt/clippy clean under --all-features and --no-default-features. Closes #111 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Bump version 0.7.1 -> 0.8.0 in Cargo.toml ([package] and [workspace.package]) and root package.json. crates/wasm/Cargo.toml inherits via version.workspace = true, no separate edit needed. - Regenerate crates/wasm/pkg/package.json via npm run build. - Fix README.md's hardcoded install-snippet version strings (still said "0.6" in five places, e.g. `purecv = "0.6"` - these don't update automatically with the version bump). crates/wasm/README.md had none. - Add a note to MAINTAINERS.md's release Step 2 about checking both READMEs for hardcoded version strings, so this doesn't drift again. - Generate the v0.8.0 changelog entry via git-cliff, plus the blank line before the previous version heading that --prepend omits. Minor: this is a minor version bump (0.7.1 -> 0.8.0) rather than a patch, since this release adds a whole new feature area (the histogram/CLAHE module: calc_hist, calc_back_project, compare_hist, equalize_hist, Clahe, with parallel + SIMD support and WASM bindings) with no breaking changes to previously-released APIs, matching this project's own precedent (0.6 -> 0.7.0 was the last feature-sized bump; 0.7.0 -> 0.7.1 was CI/Miri-only). Verified: cargo build --workspace and cargo test --workspace clean (342 lib tests + 40 doc-tests) with the new version. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PR Summary by QodoAdd histogram processing and prepare PureCV 0.8.0 release
AI Description
Diagram
High-Level Assessment
Files changed (23)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
4 tasks
…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>
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>
Member
Author
|
This is a false positive, confirmed twice now — same finding as PR #110's review (rule 2966076, "read_pixel_f32 lacks required bounds") and again as finding #2 on #122's review.
The other two findings from this same review (CLAHE edge extrapolation, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request prepares the 0.8.0 release of PureCV, introducing a new histogram module with SIMD and parallel support, various bug fixes, documentation updates, and CI improvements. It also updates the release process documentation and pins the Rust toolchain to version 1.98.0 in CI and release workflows. The most important changes are summarized below.
New Features and Performance Improvements
imgproc, includingcalc_hist,calc_back_project,compare_hist(with all 6 OpenCV methods),equalize_hist, andClahefor contrast enhancement, all with SIMD acceleration and parallel support where applicable. [1] [2] [3] [4]benches/imgproc_bench.rs. [1] [2]Documentation and Examples
README.mdandcrates/wasm/README.mdto document the new histogram/CLAHE features, usage examples, and revised installation/version instructions. [1] [2] [3] [4] [5] [6] [7]CLAUDE.mdand clarified version bumping steps inMAINTAINERS.md. [1] [2]Continuous Integration and Release Process
stablefor reproducibility. [1] [2] [3] [4] [5] [6]Changelog and Versioning
Cargo.tomland workspace metadata, and added a detailed changelog entry for 0.8.0. [1] [2] [3]Bug Fixes and Refactors