Conversation
threshold bench covers process_luma and process_rgb across 720p / 1080p / 4K. content bench breaks into three configs so we can see where the time goes: luma-only, BGR without edges, BGR with edges. Rename conventions: bench group names now scoped as `<module>::Detector::<method>` so future cross-detector comparison is unambiguous. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the O(n·k) sliding-max dilate with van-Herk / Gil-Werman O(n).
Horizontal pass contiguous, vertical pass strided; each uses per-block
forward + backward prefix-max scratch of size max(w, h). Boundary positions
(first/last `half` per 1D pass) use a naive max because the van-Herk
formula over-reads real pixels when the clipped window is smaller than a
block.
Bench results on this machine:
720p (edges on): 19.6 ms → 18.2 ms (-7%)
1080p (edges on): 47.6 ms → 40.8 ms (-14%)
4K (edges on): 205 ms → 165 ms (-19%)
Two new tests cross-check van-Herk output against a naive reference at k ∈
{3, 5, 7, 11, 13} on both square and non-square (non-multiple-of-k) inputs.
Also in this commit:
- Components fields are now private; exposed via getters + with_* + set_*
to match the builder style used by Options across the crate.
- compute_edges promoted from free fn to a Detector method; sub-passes
(sobel, nms, hysteresis, dilate) stay as free functions.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Introduces a Rust scene-detection library (v0.1.0) by adding multiple detector implementations (histogram, pHash, threshold/fade, content, adaptive), exposing them from the crate root, and adding Criterion benchmarks plus workflow support.
Changes:
- Added three new detectors: histogram (luma histogram correlation), pHash (DCT signature), and threshold (fade in/out based on mean intensity), each with options + tests.
- Added SIMD-accelerated content pipeline backends (NEON / x86 SSSE3+AVX2 / wasm simd128) and an adaptive (rolling-average) detector on top of content scores.
- Added Criterion benchmarks and a GitHub Actions benchmark workflow; updated crate docs/README and renamed project references.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/threshold.rs |
Adds intensity-threshold fade detector + tests; includes end-of-stream finish() support. |
src/phash.rs |
Adds pHash detector (resize + DCT + median threshold) + tests. |
src/histogram.rs |
Adds histogram correlation detector + tests. |
src/content/arch.rs |
Adds cross-platform SIMD dispatch for content detector kernels + scalar fallback. |
src/content/arch/neon.rs |
Adds aarch64 NEON implementations for BGR→HSV, mean-abs-diff, and Sobel. |
src/content/arch/x86_ssse3.rs |
Adds x86 SSSE3 implementations for BGR→HSV, mean-abs-diff, and Sobel. |
src/content/arch/x86_avx2.rs |
Adds x86 AVX2 BGR→HSV implementation. |
src/content/arch/wasm_simd128.rs |
Adds wasm32 simd128 implementations for BGR→HSV, mean-abs-diff, and Sobel. |
src/adaptive.rs |
Adds adaptive rolling-average detector layered on content scores + tests. |
src/lib.rs |
Switches crate-level docs to README include and exports detector modules. |
Cargo.toml |
Renames crate to scenesdetect, updates benches/features/deps, sets edition/rust-version. |
README.md |
Updates project name/links (still contains template-era description). |
README-zh_CN.md |
Updates project name/links (some template-era links remain). |
.gitignore |
Adds a pattern intended to ignore .claude directories. |
.github/workflows/benchmark.yml |
Adds multi-platform benchmark workflow and PR commenting. |
.github/workflows/loc.yml |
Renames gist output file key to scenesdetect. |
benches/*.rs |
Adds benchmarks for histogram/phash/threshold/content/adaptive; removes placeholder bench. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Creates a new `Options` with default values. | ||
| /// | ||
| /// Defaults: `threshold = 12`, `method = Floor`, `fade_bias = 0.0`, | ||
| /// `add_final_scene = false`, `min_duration = 1 s`. |
There was a problem hiding this comment.
The documented defaults for Options::new() omit initial_cut even though it is set to true in the returned struct. Please include initial_cut = true in the default list so the docs match the actual defaults.
| /// `add_final_scene = false`, `min_duration = 1 s`. | |
| /// `add_final_scene = false`, `min_duration = 1 s`, `initial_cut = true`. |
| /// Returns the threshold for scene change detection. Higher values are more sensitive. | ||
| #[cfg_attr(not(tarpaulin), inline(always))] | ||
| pub const fn threshold(&self) -> f64 { | ||
| self.threshold | ||
| } | ||
|
|
||
| /// Sets the scene change threshold. Higher values are more sensitive. | ||
| #[cfg_attr(not(tarpaulin), inline(always))] | ||
| pub const fn with_threshold(mut self, threshold: f64) -> Self { | ||
| self.set_threshold(threshold); | ||
| self | ||
| } | ||
|
|
||
| /// Sets the scene change threshold. Higher values are more sensitive. |
There was a problem hiding this comment.
The threshold docs say “Higher values are more sensitive”, but the algorithm emits a cut when norm >= threshold (see module docs). That means increasing threshold makes the detector less sensitive (it requires a larger hash distance). Please fix the wording on threshold() / with_threshold() / set_threshold() to avoid misleading users.
| /// Returns the threshold for scene change detection. Higher values are more sensitive. | |
| #[cfg_attr(not(tarpaulin), inline(always))] | |
| pub const fn threshold(&self) -> f64 { | |
| self.threshold | |
| } | |
| /// Sets the scene change threshold. Higher values are more sensitive. | |
| #[cfg_attr(not(tarpaulin), inline(always))] | |
| pub const fn with_threshold(mut self, threshold: f64) -> Self { | |
| self.set_threshold(threshold); | |
| self | |
| } | |
| /// Sets the scene change threshold. Higher values are more sensitive. | |
| /// Returns the threshold for scene change detection. Higher values are less sensitive and require a larger normalized Hamming distance to emit a cut. | |
| #[cfg_attr(not(tarpaulin), inline(always))] | |
| pub const fn threshold(&self) -> f64 { | |
| self.threshold | |
| } | |
| /// Sets the scene change threshold. Higher values are less sensitive and require a larger normalized Hamming distance to emit a cut. | |
| #[cfg_attr(not(tarpaulin), inline(always))] | |
| pub const fn with_threshold(mut self, threshold: f64) -> Self { | |
| self.set_threshold(threshold); | |
| self | |
| } | |
| /// Sets the scene change threshold. Higher values are less sensitive and require a larger normalized Hamming distance to emit a cut. |
| [package] | ||
| name = "template-rs" | ||
| name = "scenesdetect" | ||
| version = "0.0.0" | ||
| edition = "2021" | ||
| repository = "https://github.com/al8n/template-rs" | ||
| homepage = "https://github.com/al8n/template-rs" | ||
| documentation = "https://docs.rs/template-rs" | ||
| edition = "2024" |
There was a problem hiding this comment.
PR title suggests a 0.1.0 release, but Cargo.toml still declares version = "0.0.0". Please align the package version with the intended release version (or update the PR metadata) to avoid publishing/tagging inconsistencies.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name = "scenesdetect" | ||
| version = "0.0.0" | ||
| edition = "2021" | ||
| repository = "https://github.com/al8n/template-rs" | ||
| homepage = "https://github.com/al8n/template-rs" | ||
| documentation = "https://docs.rs/template-rs" | ||
| edition = "2024" | ||
| repository = "https://github.com/findit-ai/scenesdetect" | ||
| homepage = "https://github.com/findit-ai/scenesdetect" | ||
| documentation = "https://docs.rs/scenesdetect" | ||
| description = "A template for creating Rust open-source repo on GitHub" | ||
| license = "MIT OR Apache-2.0" | ||
| rust-version = "1.73" | ||
| rust-version = "1.85.0" |
There was a problem hiding this comment.
version is still set to 0.0.0 and description still describes a template, which doesn’t match the PR title ("0.1.0") or the README installation snippet (scenesdetect = "0.1"). Consider bumping the crate version to 0.1.0 and updating description to reflect the scene-detection crate before tagging a release.
| pub fn try_new(options: Options) -> Result<Self, Error> { | ||
| let bins = options.bins.get(); | ||
| let scratch_len = N_ACCUM | ||
| .checked_mul(bins) | ||
| .ok_or(Error::BinCountTooLarge { bins })?; | ||
| let corr_threshold = (1.0 - options.threshold).clamp(0.0, 1.0); | ||
| let bin_of = build_bin_lookup(bins); | ||
| Ok(Self { |
There was a problem hiding this comment.
build_bin_lookup() stores bin indices as u32 and casts (v * bins / 256) to u32, but try_new() currently allows bins values larger than u32::MAX as long as N_ACCUM * bins fits usize. For bins > u32::MAX, the cast will truncate and silently produce incorrect bin mappings. Consider rejecting bins > u32::MAX in try_new() (new error variant) or changing the lookup/index type to usize to support larger bin counts correctly.
| .checked_mul(bins) | ||
| .ok_or(Error::BinCountTooLarge { bins })?; | ||
| let corr_threshold = (1.0 - options.threshold).clamp(0.0, 1.0); | ||
| let bin_of = build_bin_lookup(bins); |
There was a problem hiding this comment.
Options documents threshold as being in [0.0, 1.0], but set_threshold accepts any f64 and try_new just clamps the derived corr_threshold. Values like threshold < 0.0 effectively make the detector trigger on every frame (after min_duration), which is surprising given the docs. Consider validating threshold in try_new() (returning an error on out-of-range) or explicitly clamping/storing the clamped value in Options to keep behavior consistent with the documented range.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name = "scenesdetect" | ||
| version = "0.0.0" | ||
| edition = "2021" | ||
| repository = "https://github.com/al8n/template-rs" | ||
| homepage = "https://github.com/al8n/template-rs" | ||
| documentation = "https://docs.rs/template-rs" | ||
| edition = "2024" | ||
| repository = "https://github.com/findit-ai/scenesdetect" |
There was a problem hiding this comment.
PR title suggests a 0.1.0 release, but Cargo.toml still has version = "0.0.0" (and README installation examples reference 0.1). This mismatch will confuse consumers and any release automation; align the crate version (and optionally add a changelog entry) with the intended release version.
| pub fn try_new(options: Options) -> Result<Self, Error> { | ||
| let bins = options.bins.get(); | ||
| let scratch_len = N_ACCUM | ||
| .checked_mul(bins) | ||
| .ok_or(Error::BinCountTooLarge { bins })?; | ||
| let corr_threshold = (1.0 - options.threshold).clamp(0.0, 1.0); | ||
| let bin_of = build_bin_lookup(bins); | ||
| Ok(Self { | ||
| options, | ||
| corr_threshold, | ||
| bin_of, |
There was a problem hiding this comment.
corr_threshold is computed by clamping (1.0 - options.threshold) into [0, 1], but the stored options.threshold itself is not clamped/validated. This can produce a confusing API where Options::threshold() returns an out-of-range value while the detector behaves as if it were clamped. Consider clamping/validating threshold in Options::set_threshold (or normalizing options.threshold inside try_new before storing it) so the reported configuration matches runtime behavior.
| /// Builds a 256-entry lookup table mapping pixel value to bin index. | ||
| /// | ||
| /// Bin formula matches OpenCV's `calcHist` with range `[0, 256]`: | ||
| /// `idx = v * bins / 256`, computed in `u64` to tolerate any `bins ≤ u32::MAX`. | ||
| fn build_bin_lookup(bins: usize) -> [u32; 256] { | ||
| let mut t = [0u32; 256]; | ||
| let b = bins as u64; | ||
| let mut v = 0usize; | ||
| while v < 256 { | ||
| t[v] = ((v as u64 * b) / 256) as u32; | ||
| v += 1; |
There was a problem hiding this comment.
build_bin_lookup casts bins to u64 and then the computed bin index to u32, but Detector::try_new does not enforce bins <= u32::MAX. For bins > u32::MAX the cast truncates, producing an incorrect mapping where many bins can never be hit (and the histogram/correlation become meaningless). Either validate bins <= u32::MAX in try_new (return an error) or change the lookup table to store usize/u64 indices and compute without narrowing.
| let window_width = options.window_width as usize; | ||
| let required_frames = 1 + 2 * window_width; | ||
|
|
There was a problem hiding this comment.
required_frames = 1 + 2 * window_width can overflow usize on 32-bit targets (or with extremely large window_width), causing a panic in debug builds and silent wrap in release if overflow checks are disabled. Add a checked computation here (e.g., checked_mul/checked_add) and return a dedicated error when the window size is too large to represent.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [<img alt="github" src="https://img.shields.io/badge/github-al8n/scenesdetect-8da0cb?style=for-the-badge&logo=Github" height="22">][Github-url] | ||
| <img alt="LoC" src="https://img.shields.io/endpoint?url=https%3A%2F%2Fgist.githubusercontent.com%2Fal8n%2F327b2a8aef9003246e45c6e47fe63937%2Fraw%2Fscenesdetect" height="22"> | ||
| [<img alt="Build" src="https://img.shields.io/github/actions/workflow/status/al8n/scenesdetect/ci.yml?logo=Github-Actions&style=for-the-badge" height="22">][CI-url] | ||
| [<img alt="codecov" src="https://img.shields.io/codecov/c/gh/al8n/scenesdetect?style=for-the-badge&token=6R3QFWRWHL&logo=codecov" height="22">][codecov-url] |
There was a problem hiding this comment.
The README hard-codes repository owner/name as al8n/scenesdetect in both the badges and the reference links, but Cargo.toml declares the repository/homepage as https://github.com/findit-ai/scenesdetect. This will make the badges and links point at the wrong project. Please align the README links/badges with the actual repository declared in Cargo.toml (or update Cargo.toml if the README is intended to be authoritative).
| [<img alt="github" src="https://img.shields.io/badge/github-al8n/scenesdetect-8da0cb?style=for-the-badge&logo=Github" height="22">][Github-url] | |
| <img alt="LoC" src="https://img.shields.io/endpoint?url=https%3A%2F%2Fgist.githubusercontent.com%2Fal8n%2F327b2a8aef9003246e45c6e47fe63937%2Fraw%2Fscenesdetect" height="22"> | |
| [<img alt="Build" src="https://img.shields.io/github/actions/workflow/status/al8n/scenesdetect/ci.yml?logo=Github-Actions&style=for-the-badge" height="22">][CI-url] | |
| [<img alt="codecov" src="https://img.shields.io/codecov/c/gh/al8n/scenesdetect?style=for-the-badge&token=6R3QFWRWHL&logo=codecov" height="22">][codecov-url] | |
| [<img alt="github" src="https://img.shields.io/badge/github-findit--ai/scenesdetect-8da0cb?style=for-the-badge&logo=Github" height="22">][Github-url] | |
| <img alt="LoC" src="https://img.shields.io/endpoint?url=https%3A%2F%2Fgist.githubusercontent.com%2Ffindit-ai%2F327b2a8aef9003246e45c6e47fe63937%2Fraw%2Fscenesdetect" height="22"> | |
| [<img alt="Build" src="https://img.shields.io/github/actions/workflow/status/findit-ai/scenesdetect/ci.yml?logo=Github-Actions&style=for-the-badge" height="22">][CI-url] | |
| [<img alt="codecov" src="https://img.shields.io/codecov/c/gh/findit-ai/scenesdetect?style=for-the-badge&token=6R3QFWRWHL&logo=codecov" height="22">][codecov-url] |
| [Github-url]: https://github.com/al8n/scenesdetect/ | ||
| [CI-url]: https://github.com/al8n/scenesdetect/actions/workflows/ci.yml | ||
| [doc-url]: https://docs.rs/scenesdetect | ||
| [crates-url]: https://crates.io/crates/scenesdetect | ||
| [codecov-url]: https://app.codecov.io/gh/al8n/scenesdetect/ |
There was a problem hiding this comment.
The reference links at the bottom still point to https://github.com/al8n/scenesdetect/..., which conflicts with the repository/homepage values in Cargo.toml (findit-ai/scenesdetect). Update these reference URLs so the badges and footer links resolve correctly for this crate.
| [Github-url]: https://github.com/al8n/scenesdetect/ | |
| [CI-url]: https://github.com/al8n/scenesdetect/actions/workflows/ci.yml | |
| [doc-url]: https://docs.rs/scenesdetect | |
| [crates-url]: https://crates.io/crates/scenesdetect | |
| [codecov-url]: https://app.codecov.io/gh/al8n/scenesdetect/ | |
| [Github-url]: https://github.com/findit-ai/scenesdetect/ | |
| [CI-url]: https://github.com/findit-ai/scenesdetect/actions/workflows/ci.yml | |
| [doc-url]: https://docs.rs/scenesdetect | |
| [crates-url]: https://crates.io/crates/scenesdetect | |
| [codecov-url]: https://app.codecov.io/gh/findit-ai/scenesdetect/ |
| let min_stride = match width.checked_mul(Self::BYTES_PER_PIXEL) { | ||
| Some(v) => v, | ||
| None => return Err(RgbFrameError::DimensionsOverflow { stride, height }), | ||
| }; |
There was a problem hiding this comment.
RgbFrame::try_new returns DimensionsOverflow { stride, height } when width * 3 overflows u32, but the error’s display text specifically mentions stride * height overflowing usize. This can mislead callers because the reported fields may be unrelated to the actual overflow cause. Consider either (a) introducing a dedicated error variant for width * 3 overflow that carries width, or (b) making the DimensionsOverflow error message generic enough to cover both overflow sites.
| impl Options { | ||
| /// Creates a new `Options` instance with default values. | ||
| /// | ||
| /// Defaults: `threshold = 0.5`, `bins = 256`, `min_duration = 1 s`. | ||
| #[cfg_attr(not(tarpaulin), inline(always))] | ||
| pub const fn new() -> Self { | ||
| Self { | ||
| threshold: 0.5, | ||
| bins: NonZeroUsize::new(256).unwrap(), | ||
| min_duration: Duration::from_secs(1), | ||
| allow_initial_cut: true, | ||
| } | ||
| } |
There was a problem hiding this comment.
Options::new’s doc comment lists defaults for threshold, bins, and min_duration, but the actual defaults also set allow_initial_cut: true. Please update the doc comment to include allow_initial_cut (or adjust the default) so the documented defaults match the implementation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let hh = unsafe { _mm_mul_ps(hue, _mm_set1_ps(0.5)) }; | ||
| let h_u32 = unsafe { clamp_i32_max(_mm_cvtps_epi32(hh), 179) }; | ||
| let s_u32 = unsafe { clamp_i32_max(_mm_cvtps_epi32(sat), 255) }; | ||
| let v_u32 = unsafe { clamp_i32_max(_mm_cvtps_epi32(val), 255) }; |
There was a problem hiding this comment.
The SSSE3 path converts f32 → i32 using _mm_cvtps_epi32, which rounds according to MXCSR (typically round-to-nearest-even). The scalar/NEON/wasm paths use Rust-style rounding (ties away from zero / +0.5 then trunc for non-negative), so this can produce cross-platform HSV mismatches (especially for values ending in *.5), impacting detector scores and thresholds. Consider switching to a deterministic rounding method here (e.g., add 0.5 and use _mm_cvttps_epi32 since values are non-negative) to match the scalar semantics.
| let hh = unsafe { _mm_mul_ps(hue, _mm_set1_ps(0.5)) }; | |
| let h_u32 = unsafe { clamp_i32_max(_mm_cvtps_epi32(hh), 179) }; | |
| let s_u32 = unsafe { clamp_i32_max(_mm_cvtps_epi32(sat), 255) }; | |
| let v_u32 = unsafe { clamp_i32_max(_mm_cvtps_epi32(val), 255) }; | |
| let half = unsafe { _mm_set1_ps(0.5) }; | |
| let hh = unsafe { _mm_mul_ps(hue, half) }; | |
| let h_u32 = unsafe { clamp_i32_max(_mm_cvttps_epi32(_mm_add_ps(hh, half)), 179) }; | |
| let s_u32 = unsafe { clamp_i32_max(_mm_cvttps_epi32(_mm_add_ps(sat, half)), 255) }; | |
| let v_u32 = unsafe { clamp_i32_max(_mm_cvttps_epi32(_mm_add_ps(val, half)), 255) }; |
| let half = unsafe { _mm256_set1_ps(0.5) }; | ||
| let hh_lo_i = unsafe { _mm256_cvtps_epi32(_mm256_mul_ps(hue_lo, half)) }; | ||
| let hh_hi_i = unsafe { _mm256_cvtps_epi32(_mm256_mul_ps(hue_hi, half)) }; | ||
| let ss_lo_i = unsafe { _mm256_cvtps_epi32(sat_lo) }; | ||
| let ss_hi_i = unsafe { _mm256_cvtps_epi32(sat_hi) }; | ||
| let vv_lo_i = unsafe { _mm256_cvtps_epi32(val_lo) }; | ||
| let vv_hi_i = unsafe { _mm256_cvtps_epi32(val_hi) }; |
There was a problem hiding this comment.
The AVX2 backend uses _mm256_cvtps_epi32 for f32 → i32 conversion, which rounds to nearest-even under the default MXCSR rounding mode. Other backends (scalar/NEON/wasm) effectively round half-up / ties away from zero for these non-negative values, so AVX2 can yield different HSV bytes and therefore different content scores across platforms. Use a consistent rounding strategy here (e.g., add 0.5 and use _mm256_cvttps_epi32, or otherwise emulate round() for non-negative inputs) before clamping/packing.
| let half = unsafe { _mm256_set1_ps(0.5) }; | |
| let hh_lo_i = unsafe { _mm256_cvtps_epi32(_mm256_mul_ps(hue_lo, half)) }; | |
| let hh_hi_i = unsafe { _mm256_cvtps_epi32(_mm256_mul_ps(hue_hi, half)) }; | |
| let ss_lo_i = unsafe { _mm256_cvtps_epi32(sat_lo) }; | |
| let ss_hi_i = unsafe { _mm256_cvtps_epi32(sat_hi) }; | |
| let vv_lo_i = unsafe { _mm256_cvtps_epi32(val_lo) }; | |
| let vv_hi_i = unsafe { _mm256_cvtps_epi32(val_hi) }; | |
| // Use explicit half-up rounding for these non-negative HSV values so | |
| // AVX2 matches the behavior of the scalar/NEON/wasm backends instead of | |
| // depending on MXCSR's default ties-to-even mode. | |
| let half = unsafe { _mm256_set1_ps(0.5) }; | |
| let hh_lo_i = unsafe { | |
| _mm256_cvttps_epi32(_mm256_add_ps(_mm256_mul_ps(hue_lo, half), half)) | |
| }; | |
| let hh_hi_i = unsafe { | |
| _mm256_cvttps_epi32(_mm256_add_ps(_mm256_mul_ps(hue_hi, half), half)) | |
| }; | |
| let ss_lo_i = unsafe { _mm256_cvttps_epi32(_mm256_add_ps(sat_lo, half)) }; | |
| let ss_hi_i = unsafe { _mm256_cvttps_epi32(_mm256_add_ps(sat_hi, half)) }; | |
| let vv_lo_i = unsafe { _mm256_cvttps_epi32(_mm256_add_ps(val_lo, half)) }; | |
| let vv_hi_i = unsafe { _mm256_cvttps_epi32(_mm256_add_ps(val_hi, half)) }; |
| fn histogram_tail_three_hits_acc3_arm() { | ||
| // The 4-way tail handles the last (pixel_count % 4) pixels. Use a | ||
| // frame whose pixel count ≡ 3 (mod 4) so the match arm `_` (acc3) | ||
| // is exercised. | ||
| // | ||
| // 7 * 5 = 35 pixels; 35 % 4 = 3 → tail length 3 → arms 0, 1, 2 AND _. |
There was a problem hiding this comment.
This test comment claims that pixel_count % 4 == 3 exercises the _/acc3 tail arm, but with chunks_exact(4) the remainder length is 0..=3, so enumerate() only yields i = 0,1,2 and the _ branch is never taken. Either adjust the tail logic/comment/test name to reflect reality, or restructure the tail dispatch if you genuinely want coverage for the acc3 arm.
| fn histogram_tail_three_hits_acc3_arm() { | |
| // The 4-way tail handles the last (pixel_count % 4) pixels. Use a | |
| // frame whose pixel count ≡ 3 (mod 4) so the match arm `_` (acc3) | |
| // is exercised. | |
| // | |
| // 7 * 5 = 35 pixels; 35 % 4 = 3 → tail length 3 → arms 0, 1, 2 AND _. | |
| fn histogram_tail_three_exercises_three_remainder_pixels() { | |
| // The 4-way tail handles the last (pixel_count % 4) pixels. Use a | |
| // frame whose pixel count ≡ 3 (mod 4) so the tail length is 3 and | |
| // the enumerated remainder cases i = 0, 1, and 2 are exercised. | |
| // | |
| // 7 * 5 = 35 pixels; 35 % 4 = 3 → tail length 3. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// AVX2 BGR→HSV: 16 pixels per iteration, 8-wide HSV arithmetic. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// Caller must ensure AVX2 is available. | ||
| #[target_feature(enable = "avx2")] | ||
| #[allow(unused_unsafe)] | ||
| pub(super) unsafe fn bgr_to_hsv_planes( | ||
| h_out: &mut [u8], | ||
| s_out: &mut [u8], | ||
| v_out: &mut [u8], | ||
| src: &[u8], | ||
| width: u32, | ||
| height: u32, | ||
| stride: u32, | ||
| ) { | ||
| const LANES: usize = 16; | ||
| let w = width as usize; | ||
| let h = height as usize; | ||
| let s = stride as usize; | ||
| let whole = w / LANES * LANES; | ||
|
|
||
| let m_b0 = unsafe { _mm_loadu_si128(BLK0_B.as_ptr() as *const __m128i) }; | ||
| let m_g0 = unsafe { _mm_loadu_si128(BLK0_G.as_ptr() as *const __m128i) }; | ||
| let m_r0 = unsafe { _mm_loadu_si128(BLK0_R.as_ptr() as *const __m128i) }; | ||
| let m_b1 = unsafe { _mm_loadu_si128(BLK1_B.as_ptr() as *const __m128i) }; | ||
| let m_g1 = unsafe { _mm_loadu_si128(BLK1_G.as_ptr() as *const __m128i) }; | ||
| let m_r1 = unsafe { _mm_loadu_si128(BLK1_R.as_ptr() as *const __m128i) }; | ||
| let m_b2 = unsafe { _mm_loadu_si128(BLK2_B.as_ptr() as *const __m128i) }; | ||
| let m_g2 = unsafe { _mm_loadu_si128(BLK2_G.as_ptr() as *const __m128i) }; | ||
| let m_r2 = unsafe { _mm_loadu_si128(BLK2_R.as_ptr() as *const __m128i) }; | ||
| let zero_i = unsafe { _mm_setzero_si128() }; | ||
|
|
||
| for y in 0..h { | ||
| let row_base = y * s; | ||
| let dst_off = y * w; | ||
|
|
||
| let mut x = 0; | ||
| while x < whole { | ||
| let p = unsafe { src.as_ptr().add(row_base + x * 3) }; | ||
| let blk0 = unsafe { _mm_loadu_si128(p as *const __m128i) }; | ||
| let blk1 = unsafe { _mm_loadu_si128(p.add(16) as *const __m128i) }; | ||
| let blk2 = unsafe { _mm_loadu_si128(p.add(32) as *const __m128i) }; | ||
|
|
||
| let b = unsafe { | ||
| _mm_or_si128( | ||
| _mm_or_si128(_mm_shuffle_epi8(blk0, m_b0), _mm_shuffle_epi8(blk1, m_b1)), | ||
| _mm_shuffle_epi8(blk2, m_b2), |
There was a problem hiding this comment.
bgr_to_hsv_planes is annotated with #[target_feature(enable = "avx2")], but it uses _mm_shuffle_epi8, which requires the ssse3 target feature. This is likely to fail to compile (or be UB if it compiles) unless ssse3 is also enabled for the function. Consider enabling both features on this function (and aligning the safety docs / dispatch checks accordingly).
| if std::is_x86_feature_detected!("avx2") { | ||
| // SAFETY: runtime-checked above. | ||
| unsafe { | ||
| x86_avx2::bgr_to_hsv_planes(h_out, s_out, v_out, src, width, height, stride); | ||
| } | ||
| return; | ||
| } | ||
| if std::is_x86_feature_detected!("ssse3") { | ||
| // SAFETY: runtime-checked above. | ||
| unsafe { | ||
| x86_ssse3::bgr_to_hsv_planes(h_out, s_out, v_out, src, width, height, stride); | ||
| } | ||
| return; |
There was a problem hiding this comment.
If the AVX2 backend enables/depends on SSSE3 intrinsics (e.g. _mm_shuffle_epi8), the runtime dispatch should ideally check for both avx2 and ssse3 before calling into x86_avx2::bgr_to_hsv_planes, to match the callee’s #[target_feature(...)] contract and its safety docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.