Fix/app captured scan fixtures#35
Conversation
📝 WalkthroughWalkthroughThis PR enhances the GlyphNet encode/scan Expo UI with media-library export and debug bundle capture, expands the Rust decode backend with suspect-byte APIs and CRC-resilient header recovery, strengthens Reed-Solomon error recovery with multi-erasure candidates, refines scanner candidate generation with ranking and plausibility filtering, adds normalized ribbon decoding with confidence-driven suspect-byte integration, supplies an optional OpenCV fallback decoder, and instruments the CLI with comprehensive diagnostic capture infrastructure. ChangesExpo app export and debug features
Decode and recovery backend enhancements
Scanner decoding and candidate generation enhancements
CLI diagnostic artifact capture
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
TriForMine
force-pushed
the
fix/app-captured-scan-fixtures
branch
from
25bc7dd to
4206cd3
Compare
Burst reliability (loss sweep)✅ PR gate status: PASS
|
Scanner perf (ribbon + matrix fixtures)
Matrix subset
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/expo-glyphnet/src/features/scan/ScanPanel.tsx (1)
150-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait both file copies before sharing or reporting success.
File.copy(...)is async in the currentexpo-file-systemAPI, but both copies here are fire-and-forget. That letsshareAsync(roiOut.uri)and the success message run beforecapture.jpg/roi.jpghave finished copying. Await these calls or switch tocopySync(...). (docs.expo.dev)Proposed fix
const captureOut = new File(outDir, "capture.jpg"); captureOut.create({ overwrite: true, intermediates: true }); - new File(lastCaptureUri).copy(captureOut, { overwrite: true }); + await new File(lastCaptureUri).copy(captureOut, { + overwrite: true, + }); @@ const roiOut = new File(outDir, "roi.jpg"); roiOut.create({ overwrite: true, intermediates: true }); - new File(cropped.uri).copy(roiOut, { overwrite: true }); + await new File(cropped.uri).copy(roiOut, { + overwrite: true, + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/expo-glyphnet/src/features/scan/ScanPanel.tsx` around lines 150 - 176, The two File.copy(...) calls for captureOut and roiOut are fired without awaiting, so downstream code (e.g., shareAsync(roiOut.uri) or success reporting) may run before the copies finish; update the code around captureOut.copy(...) and new File(cropped.uri).copy(...) to await them (either await each call or await Promise.all([...])) so both copies complete before proceeding (references: captureOut, roiOut, new File(...).copy, ImageManipulator.manipulateAsync, cropped.uri, lastCaptureUri).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/expo-glyphnet/src/features/encode/EncodePanel.tsx`:
- Around line 48-53: The saveSvg flow writes an SVG file and then calls
MediaLibrary.saveToLibraryAsync on its URI, but MediaLibrary targets
images/videos and may not support SVGs; update the saveSvg logic (the function
that creates exportDir, file, and uses file.uri and
MediaLibrary.saveToLibraryAsync) to detect SVG output and avoid calling
MediaLibrary.saveToLibraryAsync for .svg files—either convert the SVG to a
raster image (PNG/JPEG) before calling MediaLibrary.saveToLibraryAsync, or skip
the media-library path and fall back directly to the share or
app-storage/save-to-documents flow for SVGs; ensure you reference the created
File (file / file.uri) and the MediaLibrary.saveToLibraryAsync call when making
the change.
In `@crates/glyphnet-cli/src/main.rs`:
- Around line 1012-1038: write_binary_grid_image_named can allocate very large
buffers for full-resolution inputs; add a safeguard so dump_base_steps doesn't
OOM when calling it on huge images. In write_binary_grid_image_named (or where
dump_base_steps calls it) check the source binary dimensions and if
width*saturating_mul(height) exceeds a reasonable threshold (e.g. ~50_000_000
pixels) either skip saving the full-image grid (early return Ok(())) or create a
downscaled copy (resize/thumbnail) before composing the CELL+GRID grid; keep
current behavior for smaller images and keep dump_attempt_regions unchanged so
per-attempt crops still emit full grids. Ensure the check references
write_binary_grid_image_named and dump_base_steps so reviewers can find the
change.
- Around line 694-712: The debug artifact writes on the success paths are
currently propagated with ? which can turn a successful scan into failure; make
those writes best-effort like the failure path by swallowing errors instead of
returning them. Specifically, in the block around scan_still_with_diagnostics,
change uses of debug.dump_success(...) and debug.dump_robust_success(...) (and
the earlier dump_base_steps call) so they do not propagate errors: call them and
ignore the Result (e.g., let _ = debug.dump_success(...); let _ =
debug.dump_robust_success(...)), ensuring scan_still_with_diagnostics and
scan_still_robust results remain the decisive outcome.
In `@crates/glyphnet-ecc/src/lib.rs`:
- Around line 605-621: The loop currently only tests the prefix
suspect_pool[..count] for each count, so for count >= 3 you must instead
enumerate all combinations of suspect_pool elements of size count and try each
candidate; modify the block using rs.recover_data_shards(&normalized, data_len,
&candidate_indices_or_slices) for each combination, call rs.verify(&candidate,
data_len) and Frame::decode(&candidate).is_ok() the same way, and increment
attempts per candidate (checking attempts > max_attempts and setting
telemetry.attempts and telemetry.max_attempts_exceeded before returning).
Implement a small local combinations iterator/helper to produce each k-sized
subset from suspect_pool, and on a successful recovery set telemetry.recovered,
telemetry.attempts, telemetry.method = RecoveryMethod::ReedSolomonPair and
return Some(to_encoded_layout(candidate)) as before.
In `@crates/glyphnet-scanner/src/candidates.rs`:
- Around line 279-290: The binding `let mut seeds: Vec<DarkComponent>` is never
mutated; remove the `mut` so it becomes `let seeds: Vec<DarkComponent>` to
satisfy the Clippy `unused_mut` warning; update the declaration in the block
where `dark_components(binary).into_iter().filter(...).take(120).collect()` is
used (referencing the `seeds` variable and the `dark_components` call) and run
`cargo clippy` to confirm the warning is resolved.
- Around line 458-474: The current ranking uses region_dark_pixels (returning
raw dark-pixel counts) which biases larger regions; change ranking to use
dark-pixel density instead. Modify region_dark_pixels (or add a new
region_dark_density) to compute dark as f32 divided by region.area
(width.saturating_mul(height) as f32, guarding against zero area) and then sort
candidates by that density (use candidates.sort_by(|a,b|
b.partial_cmp(&a).unwrap()) or equivalent) so candidates are ordered by
dark-pixel density (higher first) rather than absolute count; reference
region_dark_pixels (or new region_dark_density) and the candidates.sort_by_key
call to locate where to change.
- Around line 208-214: The code subtracts from score using saturating_sub but
score is declared as 0u64 so those subtractions are no-ops; change the type of
the score variable from an unsigned integer to a signed integer (e.g., i64) and
initialize it as 0 (e.g., 0i64), then perform the subtracts for the
CandidateDetector::RibbonWeave branch (the candidate.stage "border-trim" and
"roi-group" adjustments) using normal subtraction or signed saturating logic so
the penalties reduce the score as intended; also update any downstream uses
(comparisons, sorting keys, or vector elements) that assume score is u64 to
accept the signed type.
In `@crates/glyphnet-scanner/src/lib.rs`:
- Around line 161-162: In the contour loops inside
opencv_fallback::detect_symbol_rois, opencv_fallback::page_rectified_variant,
and opencv_fallback::quad_warp_variant replace uses of .ok()? that short-circuit
the whole helper with logic that skips the failing contour and continues (e.g.,
if imgproc::bounding_rect(...) returns Err/None, log/ignore and continue to next
contour rather than returning None). In scan_still_with_diagnostics_inner, don't
finalize timings.total_micros before calling
opencv_fallback::try_decode_with_opencv; instead measure/record the start time
immediately before try_decode_with_opencv and add its elapsed duration to
timings.total_micros (or finalize timings after the call) so successful OpenCV
fallback includes its runtime in the reported timings.
- Around line 1037-1049: Recompute the timing fields right before returning from
the OpenCV fallback success path so the OpenCV decode cost is included: update
timings.total_micros (and timings.decode_attempts_micros for consistency)
immediately before the Ok(StillScanResult { ... timings }) return inside the
opencv_fallback::try_decode_with_opencv branch by re-measuring elapsed time
since the original scan start (same reference used when timings were set) and by
including the latest decode attempt duration(s) derived from attempts so the
returned StillScanResult reflects the full cost of the OpenCV decode.
---
Outside diff comments:
In `@apps/expo-glyphnet/src/features/scan/ScanPanel.tsx`:
- Around line 150-176: The two File.copy(...) calls for captureOut and roiOut
are fired without awaiting, so downstream code (e.g., shareAsync(roiOut.uri) or
success reporting) may run before the copies finish; update the code around
captureOut.copy(...) and new File(cropped.uri).copy(...) to await them (either
await each call or await Promise.all([...])) so both copies complete before
proceeding (references: captureOut, roiOut, new File(...).copy,
ImageManipulator.manipulateAsync, cropped.uri, lastCaptureUri).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b991eb01-64bd-4179-9a6a-087789f4a108
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockcrates/glyphnet-testkit/fixtures/corpus/real/app/print-clean-001.jpgis excluded by!**/*.jpgcrates/glyphnet-testkit/fixtures/corpus/real/app/print-clean-002.jpgis excluded by!**/*.jpg
📒 Files selected for processing (11)
apps/expo-glyphnet/src/features/encode/EncodePanel.tsxapps/expo-glyphnet/src/features/scan/ScanPanel.tsxcrates/glyphnet-cli/Cargo.tomlcrates/glyphnet-cli/src/main.rscrates/glyphnet-decode/src/lib.rscrates/glyphnet-decode/src/recovery.rscrates/glyphnet-ecc/src/lib.rscrates/glyphnet-scanner/Cargo.tomlcrates/glyphnet-scanner/src/candidates.rscrates/glyphnet-scanner/src/decode_paths.rscrates/glyphnet-scanner/src/lib.rs
| const exportDir = new Directory(Paths.document, "glyphnet-exports"); | ||
| exportDir.create({ idempotent: true, intermediates: true }); | ||
| const file = new File(exportDir, `glyphnet-${Date.now()}.svg`); | ||
| file.create({ overwrite: true, intermediates: true }); | ||
| file.write(svgPreview, { encoding: "utf8" }); | ||
| const uri = file.uri; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant section around the export + media-library save call
FILE="apps/expo-glyphnet/src/features/encode/EncodePanel.tsx"
echo "== File = $FILE =="
test -f "$FILE" && nl -ba "$FILE" | sed -n '1,140p' || { echo "File not found"; exit 1; }
# Specifically extract the export block + any nearby MediaLibrary usage
nl -ba "$FILE" | sed -n '35,120p' | sed -n '1,120p' >/tmp/snippet_35_120.txt
echo "== Extract export/media-library vicinity (lines 35-120) =="
cat /tmp/snippet_35_120.txtRepository: TriForMine/glyphnet
Length of output: 205
🌐 Web query:
expo-media-library saveToLibraryAsync localUri accepted file types image video svg
💡 Result:
The expo-media-library saveToLibraryAsync function is designed to save images and videos to the user's device media library [1][2]. Regarding the file types: Images and Videos: The function natively supports standard image and video file formats, such as JPEG, PNG, and MP4 [3][4][5]. Ensure your localUri points to a valid file path, typically starting with file:// on Android [5]. SVG Files: The MediaLibrary API does not support saving SVG files directly to the device's photo/media library. Standard mobile operating system media galleries (iOS Photos and Android Gallery/Google Photos) are designed for raster images and video files [6]. If you need to save an SVG to the media library, you must first convert or export it to a supported raster format (like PNG or JPEG) within your application using a library like expo-image-manipulator or an image processing tool [6]. Common Troubleshooting: - Ensure you have the necessary permissions requested and granted (MEDIA_LIBRARY/CAMERA_ROLL) [3][4][5]. - Verify that your Info.plist (iOS) includes the required NSPhotoLibraryAddUsageDescription key [5]. - If you encounter a "This file type is not supported yet" error, ensure you are using a recent version of the expo-media-library package, as past issues related to file type recognition (especially for videos on iOS) have been addressed in recent updates [4].
Citations:
- 1: https://docs.expo.dev/versions/latest/sdk/media-library-next/
- 2: https://docs.expo.dev/versions/latest/sdk/media-library/
- 3: https://stackoverflow.com/questions/67889902/react-native-expo-media-library-this-file-type-is-not-supported-yet-for-png-an
- 4: MediaLibrary.saveToLibraryAsync throw This file type is not supported yet for any video file expo/expo#33537
- 5: https://docs.expo.dev/versions/v53.0.0/sdk/media-library/
- 6: https://dev.to/aitranxuan/automating-app-icon-generation-how-to-export-ios-android-and-responsive-favicons-from-a-single-28i
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find MediaLibrary.saveToLibraryAsync occurrences in the repo to understand branching/usage patterns
rg -n "MediaLibrary\.saveToLibraryAsync|saveToLibraryAsync\(" apps/expo-glyphnet/src -SRepository: TriForMine/glyphnet
Length of output: 175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if EncodePanel.tsx does any rasterization/conditional handling for svg vs png/jpg
FILE="apps/expo-glyphnet/src/features/encode/EncodePanel.tsx"
rg -n "svg|png|jpeg|jpg|raster|rasterize|MediaLibrary" "$FILE" -SRepository: TriForMine/glyphnet
Length of output: 1035
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/expo-glyphnet/src/features/encode/EncodePanel.tsx"
echo "== Head (imports/state hooks) =="
awk 'NR<=120 {printf "%4d:%s\n", NR, $0}' "$FILE" | sed -n '1,120p'
echo
echo "== Export/save section (approx 35-105) =="
awk 'NR>=35 && NR<=115 {printf "%4d:%s\n", NR, $0}' "$FILE"
echo
echo "== Search nearby for fallback APIs =="
rg -n "shareAsync|Sharing|FileSystem|expo-sharing|writeAsStringAsync|DocumentDirectory|documentDirectory|saveToLibraryAsync|catch|mimeType" "$FILE" -n -SRepository: TriForMine/glyphnet
Length of output: 8143
🌐 Web query:
site:docs.expo.dev media-library saveToLibraryAsync svg
💡 Result:
In Expo/React Native, MediaLibrary.saveToLibraryAsync(localUri) saves a file (image or video) from a local URI into the user’s media library, but it is not documented as supporting SVG directly. What the official docs require 1) The argument must be an image/video file URI and “must contain an extension” [1]. 2) On Android, localUri “must be a local path” and “must start with file:///” [1]. 3) The saveToLibraryAsync API expects an image or video file: it is described as saving “the image or video file” at localUri to the user’s media library [1]. Implication for SVG - Because the API is documented specifically for image/video files (and does not call out SVG as a supported file type for saving), you should not assume that passing a.svg URI will work reliably. The docs only guarantee support based on being an image/video file with an extension [1]. - If your goal is to persist an SVG “drawing” to the media library, a robust approach is to render the SVG to a raster format (e.g., PNG) and then save that raster file via saveToLibraryAsync. If you’re using the newer expo-media-library API - Expo notes that saveToLibraryAsync is not available in the “new API”; the replacement is Asset.create(localUri), which saves the file and returns an Asset instance [2]. You’d still apply the same basic rule: provide a local URI to the file you want saved, with an extension as required by the underlying file-based approach [1][2]. Summary - Use MediaLibrary.saveToLibraryAsync(localUri) with a local file URI (Android: file:///...) that points to an image/video file with an extension [1]. - SVG is not explicitly documented as supported for saving via saveToLibraryAsync, so treat it as unsupported/uncertain and convert SVG to PNG/JPEG first if you need it saved to the gallery.
Citations:
- 1: https://docs.expo.dev/versions/v53.0.0/sdk/media-library/
- 2: https://docs.expo.dev/guides/sdk-libraries-migration/media-library/
Avoid calling MediaLibrary.saveToLibraryAsync with the generated .svg file
saveSvg writes glyphnet-${Date.now()}.svg to the document directory, then (when permission is granted) calls MediaLibrary.saveToLibraryAsync(uri) on that .svg URI. saveToLibraryAsync is intended for image/video files, and SVG support for direct media-library saving isn’t documented—so the “saved to library” path is unreliable. Convert the SVG to PNG/JPEG before saving, or skip the media-library step for SVG and go straight to the share/app-storage fallback. (docs.expo.dev)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/expo-glyphnet/src/features/encode/EncodePanel.tsx` around lines 48 - 53,
The saveSvg flow writes an SVG file and then calls
MediaLibrary.saveToLibraryAsync on its URI, but MediaLibrary targets
images/videos and may not support SVGs; update the saveSvg logic (the function
that creates exportDir, file, and uses file.uri and
MediaLibrary.saveToLibraryAsync) to detect SVG output and avoid calling
MediaLibrary.saveToLibraryAsync for .svg files—either convert the SVG to a
raster image (PNG/JPEG) before calling MediaLibrary.saveToLibraryAsync, or skip
the media-library path and fall back directly to the share or
app-storage/save-to-documents flow for SVGs; ensure you reference the created
File (file / file.uri) and the MediaLibrary.saveToLibraryAsync call when making
the change.
| let scanned = match scan_still_with_diagnostics(&image, mode) { | ||
| Ok(scanned) => { | ||
| if let Some(debug) = &debug { | ||
| debug.dump_success(&image, mode, &scanned)?; | ||
| } | ||
| scanned | ||
| } | ||
| Err(failed) => { | ||
| if let Some(debug) = &debug { | ||
| let _ = debug.dump_failure(&image, mode, &failed); | ||
| } | ||
| let robust = scan_still_robust(&image, mode) | ||
| .map_err(|_| anyhow::anyhow!("failed to scan image: {}", failed.error))?; | ||
| if let Some(debug) = &debug { | ||
| debug.dump_robust_success(&image, mode, &robust)?; | ||
| } | ||
| robust | ||
| } | ||
| }; |
There was a problem hiding this comment.
Debug-dump failures shouldn't change the scan command's outcome.
On the success path, dump_base_steps (Line 691), dump_success (Line 697) and dump_robust_success (Line 708) propagate with ?, so a debug artifact write failure (e.g. disk full, unwritable dir) turns an otherwise-successful scan into a non-zero exit. The failure path already treats this as best-effort via let _ = debug.dump_failure(...) (Line 703). Make the success-path dumps best-effort too for consistency so enabling GLYPHNET_SCAN_DEBUG never alters the primary result.
🛠️ Proposed best-effort dumps
let debug = ScanDebug::from_env(&input);
if let Some(debug) = &debug {
- debug.dump_base_steps(&image, mode)?;
+ let _ = debug.dump_base_steps(&image, mode);
}
let detached_signature = auth::load_detached_verification_input(detached_auth_file.as_ref())?;
let scanned = match scan_still_with_diagnostics(&image, mode) {
Ok(scanned) => {
if let Some(debug) = &debug {
- debug.dump_success(&image, mode, &scanned)?;
+ let _ = debug.dump_success(&image, mode, &scanned);
}
scanned
}
Err(failed) => {
if let Some(debug) = &debug {
let _ = debug.dump_failure(&image, mode, &failed);
}
let robust = scan_still_robust(&image, mode)
.map_err(|_| anyhow::anyhow!("failed to scan image: {}", failed.error))?;
if let Some(debug) = &debug {
- debug.dump_robust_success(&image, mode, &robust)?;
+ let _ = debug.dump_robust_success(&image, mode, &robust);
}
robust
}
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/glyphnet-cli/src/main.rs` around lines 694 - 712, The debug artifact
writes on the success paths are currently propagated with ? which can turn a
successful scan into failure; make those writes best-effort like the failure
path by swallowing errors instead of returning them. Specifically, in the block
around scan_still_with_diagnostics, change uses of debug.dump_success(...) and
debug.dump_robust_success(...) (and the earlier dump_base_steps call) so they do
not propagate errors: call them and ignore the Result (e.g., let _ =
debug.dump_success(...); let _ = debug.dump_robust_success(...)), ensuring
scan_still_with_diagnostics and scan_still_robust results remain the decisive
outcome.
| fn write_binary_grid_image_named(&self, binary: &image::GrayImage, name: &str) -> Result<()> { | ||
| const CELL: u32 = 4; | ||
| const GRID: u32 = 1; | ||
|
|
||
| let width = binary.width(); | ||
| let height = binary.height(); | ||
| let out_width = width.saturating_mul(CELL + GRID).saturating_add(GRID); | ||
| let out_height = height.saturating_mul(CELL + GRID).saturating_add(GRID); | ||
| let mut out = image::GrayImage::from_pixel(out_width, out_height, image::Luma([220])); | ||
|
|
||
| for y in 0..height { | ||
| for x in 0..width { | ||
| let pixel = binary.get_pixel(x, y).0[0]; | ||
| let start_x = GRID + x * (CELL + GRID); | ||
| let start_y = GRID + y * (CELL + GRID); | ||
| for dy in 0..CELL { | ||
| for dx in 0..CELL { | ||
| out.put_pixel(start_x + dx, start_y + dy, image::Luma([pixel])); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| out.save(self.dir.join(name)) | ||
| .context("failed to save debug binary grid image")?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Grid render can allocate a huge buffer for full-resolution inputs.
Each source pixel becomes a (CELL + GRID) = 5px cell in both dimensions, i.e. ~25× the pixel count. dump_base_steps calls write_binary_grid_image on the full-image binary, so a 12 MP photo yields a ~300 MP grayscale buffer (~300 MB), which can OOM the debug run. Consider skipping/limiting the full-image grid above some dimension threshold (the per-cell text/grid is most useful on cropped attempt regions, which are already emitted in dump_attempt_regions).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/glyphnet-cli/src/main.rs` around lines 1012 - 1038,
write_binary_grid_image_named can allocate very large buffers for
full-resolution inputs; add a safeguard so dump_base_steps doesn't OOM when
calling it on huge images. In write_binary_grid_image_named (or where
dump_base_steps calls it) check the source binary dimensions and if
width*saturating_mul(height) exceeds a reasonable threshold (e.g. ~50_000_000
pixels) either skip saving the full-image grid (early return Ok(())) or create a
downscaled copy (resize/thumbnail) before composing the CELL+GRID grid; keep
current behavior for smaller images and keep dump_attempt_regions unchanged so
per-attempt crops still emit full grids. Ensure the check references
write_binary_grid_image_named and dump_base_steps so reviewers can find the
change.
| for count in 3..=suspect_pool.len().min(rs.parity_shards) { | ||
| attempts += 1; | ||
| if attempts > max_attempts { | ||
| telemetry.attempts = attempts; | ||
| telemetry.max_attempts_exceeded = true; | ||
| return (None, telemetry); | ||
| } | ||
| if let Some(candidate) = | ||
| rs.recover_data_shards(&normalized, data_len, &suspect_pool[..count]) | ||
| && rs.verify(&candidate, data_len) | ||
| && Frame::decode(&candidate).is_ok() | ||
| { | ||
| telemetry.recovered = true; | ||
| telemetry.attempts = attempts; | ||
| telemetry.method = RecoveryMethod::ReedSolomonPair; | ||
| return (Some(to_encoded_layout(candidate)), telemetry); | ||
| } |
There was a problem hiding this comment.
Enumerate actual k-byte suspect combinations here.
This only tests suspect_pool[..count]. Unlike the 2-byte path above, it never tries the other combinations of ranked suspects, so 3+ byte recovery still fails unless every real erasure lands in the top-count prefix. Spend the attempt budget per candidate combination, not per group size.
💡 Fix sketch
- for count in 3..=suspect_pool.len().min(rs.parity_shards) {
- attempts += 1;
- if attempts > max_attempts {
- telemetry.attempts = attempts;
- telemetry.max_attempts_exceeded = true;
- return (None, telemetry);
- }
- if let Some(candidate) =
- rs.recover_data_shards(&normalized, data_len, &suspect_pool[..count])
- && rs.verify(&candidate, data_len)
- && Frame::decode(&candidate).is_ok()
- {
- telemetry.recovered = true;
- telemetry.attempts = attempts;
- telemetry.method = RecoveryMethod::ReedSolomonPair;
- return (Some(to_encoded_layout(candidate)), telemetry);
- }
- }
+ for count in 3..=suspect_pool.len().min(rs.parity_shards) {
+ for missing in combinations(&suspect_pool, count) {
+ attempts += 1;
+ if attempts > max_attempts {
+ telemetry.attempts = attempts;
+ telemetry.max_attempts_exceeded = true;
+ return (None, telemetry);
+ }
+ if let Some(candidate) =
+ rs.recover_data_shards(&normalized, data_len, &missing)
+ && rs.verify(&candidate, data_len)
+ && Frame::decode(&candidate).is_ok()
+ {
+ telemetry.recovered = true;
+ telemetry.attempts = attempts;
+ telemetry.method = RecoveryMethod::ReedSolomonPair;
+ return (Some(to_encoded_layout(candidate)), telemetry);
+ }
+ }
+ }You can implement combinations as a small local iterator/helper to avoid adding a dependency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/glyphnet-ecc/src/lib.rs` around lines 605 - 621, The loop currently
only tests the prefix suspect_pool[..count] for each count, so for count >= 3
you must instead enumerate all combinations of suspect_pool elements of size
count and try each candidate; modify the block using
rs.recover_data_shards(&normalized, data_len, &candidate_indices_or_slices) for
each combination, call rs.verify(&candidate, data_len) and
Frame::decode(&candidate).is_ok() the same way, and increment attempts per
candidate (checking attempts > max_attempts and setting telemetry.attempts and
telemetry.max_attempts_exceeded before returning). Implement a small local
combinations iterator/helper to produce each k-sized subset from suspect_pool,
and on a successful recovery set telemetry.recovered, telemetry.attempts,
telemetry.method = RecoveryMethod::ReedSolomonPair and return
Some(to_encoded_layout(candidate)) as before.
| if candidate.detector == CandidateDetector::RibbonWeave { | ||
| if candidate.stage == "border-trim" { | ||
| score = score.saturating_sub(140); | ||
| } | ||
| if candidate.stage == "roi-group" { | ||
| score = score.saturating_sub(80); | ||
| } |
There was a problem hiding this comment.
Use a signed priority score here.
score starts at 0u64, so both saturating_sub() calls are no-ops. "border-trim" and "roi-group" never get the intended boost before sorting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/glyphnet-scanner/src/candidates.rs` around lines 208 - 214, The code
subtracts from score using saturating_sub but score is declared as 0u64 so those
subtractions are no-ops; change the type of the score variable from an unsigned
integer to a signed integer (e.g., i64) and initialize it as 0 (e.g., 0i64),
then perform the subtracts for the CandidateDetector::RibbonWeave branch (the
candidate.stage "border-trim" and "roi-group" adjustments) using normal
subtraction or signed saturating logic so the penalties reduce the score as
intended; also update any downstream uses (comparisons, sorting keys, or vector
elements) that assume score is u64 to accept the signed type.
| let mut seeds: Vec<DarkComponent> = dark_components(binary) | ||
| .into_iter() | ||
| .filter(|c| { | ||
| let b = c.bounds; | ||
| if c.pixels < 60 || b.width < 20 || b.height < 10 { | ||
| return false; | ||
| } | ||
| let aspect = b.width as f32 / b.height.max(1) as f32; | ||
| (0.6..=12.0).contains(&aspect) | ||
| }) | ||
| .take(120) | ||
| .collect(); |
There was a problem hiding this comment.
Remove the unused mut from seeds.
This binding is never mutated, so unused_mut will fail the repo’s -D warnings clippy gate.
As per coding guidelines, **/*.rs: Rust code must pass cargo clippy --workspace --all-targets --all-features -- -D warnings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/glyphnet-scanner/src/candidates.rs` around lines 279 - 290, The
binding `let mut seeds: Vec<DarkComponent>` is never mutated; remove the `mut`
so it becomes `let seeds: Vec<DarkComponent>` to satisfy the Clippy `unused_mut`
warning; update the declaration in the block where
`dark_components(binary).into_iter().filter(...).take(120).collect()` is used
(referencing the `seeds` variable and the `dark_components` call) and run `cargo
clippy` to confirm the warning is resolved.
| candidates.sort_by_key(|candidate| std::cmp::Reverse(region_dark_pixels(binary, candidate.region))); | ||
| candidates | ||
| } | ||
|
|
||
| fn region_dark_pixels(binary: &GrayImage, region: ScanRegion) -> u32 { | ||
| let x_end = region.x.saturating_add(region.width).min(binary.width()); | ||
| let y_end = region.y.saturating_add(region.height).min(binary.height()); | ||
| let mut dark = 0u32; | ||
| for y in region.y..y_end { | ||
| for x in region.x..x_end { | ||
| if binary.get_pixel(x, y).0[0] == 0 { | ||
| dark = dark.saturating_add(1); | ||
| } | ||
| } | ||
| } | ||
| dark | ||
| } |
There was a problem hiding this comment.
Rank coarse-grid candidates by density, not raw dark-pixel count.
Using the absolute dark-pixel total biases truncation toward the largest windows, even when they are mostly background. That can push smaller true ROIs out of the fallback budget before decode ever runs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/glyphnet-scanner/src/candidates.rs` around lines 458 - 474, The
current ranking uses region_dark_pixels (returning raw dark-pixel counts) which
biases larger regions; change ranking to use dark-pixel density instead. Modify
region_dark_pixels (or add a new region_dark_density) to compute dark as f32
divided by region.area (width.saturating_mul(height) as f32, guarding against
zero area) and then sort candidates by that density (use
candidates.sort_by(|a,b| b.partial_cmp(&a).unwrap()) or equivalent) so
candidates are ordered by dark-pixel density (higher first) rather than absolute
count; reference region_dark_pixels (or new region_dark_density) and the
candidates.sort_by_key call to locate where to change.
| for contour in contours { | ||
| let rect = imgproc::bounding_rect(&contour).ok()?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '155,166p;276,286p;397,409p' crates/glyphnet-scanner/src/lib.rsRepository: TriForMine/glyphnet
Length of output: 1612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the helper signature/return type near the first loop (~155)
sed -n '120,220p' crates/glyphnet-scanner/src/lib.rs
# Show the second loop (~276-286) in full context
sed -n '240,330p' crates/glyphnet-scanner/src/lib.rs
# Show the third loop (~398-409) in full context
sed -n '360,460p' crates/glyphnet-scanner/src/lib.rs
# Inspect the OpenCV success path + timing handling (~1037-1049)
sed -n '990,1070p' crates/glyphnet-scanner/src/lib.rs
# Find the helper name mentioned in the original comment (scan_still_with_diagnostics_inner)
rg -n "scan_still_with_diagnostics_inner" crates/glyphnet-scanner/src/lib.rsRepository: TriForMine/glyphnet
Length of output: 13390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the helper functions mentioned
rg -n "fn (detect_symbol_rois|page_rectified_variant|quad_warp_variant)\b" crates/glyphnet-scanner/src/lib.rs
# Show each helper's signature + the contour loops nearby (small windows)
# (We’ll expand once we know the exact line numbers from rg)
rg -n "detect_symbol_rois" crates/glyphnet-scanner/src/lib.rs
rg -n "page_rectified_variant" crates/glyphnet-scanner/src/lib.rs
rg -n "quad_warp_variant" crates/glyphnet-scanner/src/lib.rs
# Show the full opencv_fallback module block header (for context)
rg -n "mod opencv_fallback" crates/glyphnet-scanner/src/lib.rs
sed -n '1,260p' crates/glyphnet-scanner/src/lib.rs | head -n 260
# Inspect scan_still_with_diagnostics_inner: where `started` is set and where `timings.total_micros` is finalized
rg -n "fn scan_still_with_diagnostics_inner" crates/glyphnet-scanner/src/lib.rs
sed -n '740,980p' crates/glyphnet-scanner/src/lib.rs
# Inspect around the GLYPHNET_SCAN_OPENCV fallback return and surrounding timings
sed -n '980,1120p' crates/glyphnet-scanner/src/lib.rsRepository: TriForMine/glyphnet
Length of output: 24237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show detect_symbol_rois tail (to confirm exact contour loop behavior)
sed -n '90,170p' crates/glyphnet-scanner/src/lib.rs
# Show page_rectified_variant full-ish region (focus on contour loop + any `.ok()?` inside)
sed -n '200,310p' crates/glyphnet-scanner/src/lib.rs
# Show quad_warp_variant region (focus on contour loop + any `.ok()?` inside)
sed -n '310,460p' crates/glyphnet-scanner/src/lib.rs
# Show end of scan_still_with_diagnostics_inner around opencv fallback timing assignment
sed -n '920,1065p' crates/glyphnet-scanner/src/lib.rsRepository: TriForMine/glyphnet
Length of output: 17645
Skip per-contour OpenCV failures and account for OpenCV-fallback runtime in timings
- In
opencv_fallback::{detect_symbol_rois,page_rectified_variant,quad_warp_variant}, the contour loops use.ok()?, so a single OpenCV failure aborts the whole helper by returningNone; failures shouldcontinueto the next contour instead of short-circuiting. - In
scan_still_with_diagnostics_inner,timings.total_microsis finalized before callingopencv_fallback::try_decode_with_opencv(...), so a successful OpenCV fallback returns stale timing that omits the fallback’s duration; measure/update timings around that call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/glyphnet-scanner/src/lib.rs` around lines 161 - 162, In the contour
loops inside opencv_fallback::detect_symbol_rois,
opencv_fallback::page_rectified_variant, and opencv_fallback::quad_warp_variant
replace uses of .ok()? that short-circuit the whole helper with logic that skips
the failing contour and continues (e.g., if imgproc::bounding_rect(...) returns
Err/None, log/ignore and continue to next contour rather than returning None).
In scan_still_with_diagnostics_inner, don't finalize timings.total_micros before
calling opencv_fallback::try_decode_with_opencv; instead measure/record the
start time immediately before try_decode_with_opencv and add its elapsed
duration to timings.total_micros (or finalize timings after the call) so
successful OpenCV fallback includes its runtime in the reported timings.
| #[cfg(all(not(target_arch = "wasm32"), feature = "opencv-fallback"))] | ||
| if std::env::var_os("GLYPHNET_SCAN_OPENCV").is_some() | ||
| && let Some(decoded) = opencv_fallback::try_decode_with_opencv(image) | ||
| { | ||
| return Ok(StillScanResult { | ||
| decoded, | ||
| crop: None, | ||
| quad: None, | ||
| warp_size: None, | ||
| attempts, | ||
| timings, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '1030,1049p' crates/glyphnet-scanner/src/lib.rsRepository: TriForMine/glyphnet
Length of output: 694
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/glyphnet-scanner/src/lib.rs"
# Show where `started` and `timings` are defined and where `elapsed_micros(started)` is computed.
sed -n '980,1065p' "$FILE"
# Also show the `StillScanResult` construction fields for `timings` (near by).
rg -n "StillScanResult\s*\{" "$FILE" -n
sed -n '1065,1125p' "$FILE"Repository: TriForMine/glyphnet
Length of output: 5409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/glyphnet-scanner/src/lib.rs"
# Find opencv_fallback module usage and the function signature.
rg -n "opencv_fallback::try_decode_with_opencv" "$FILE"
rg -n "mod opencv_fallback|opencv_fallback" "$FILE"
# Locate the actual definition of try_decode_with_opencv in the repo.
rg -n "fn\s+try_decode_with_opencv" -S .
# Print the definition with a bit of context.
rg -n "fn\s+try_decode_with_opencv" -n -S . | headRepository: TriForMine/glyphnet
Length of output: 522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/glyphnet-scanner/src/lib.rs"
# Inspect opencv fallback implementation.
sed -n '1,130p' "$FILE"
# Locate ScanTimings and timing fields.
rg -n "struct\s+ScanTimings|ScanTimings\s*\{" "$FILE"
rg -n "total_micros" "$FILE"
# Show the ScanTimings definition block.
# (Use the first match line numbers to keep output small.)
LINE="$(rg -n "struct\s+ScanTimings" "$FILE" | head -n1 | cut -d: -f1)"
start=$((LINE-20))
end=$((LINE+120))
sed -n "${start},${end}p" "$FILE"Repository: TriForMine/glyphnet
Length of output: 4504
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate ScanTimings definition (likely in types.rs)
rg -n "struct\s+ScanTimings" crates/glyphnet-scanner/src
# Show ScanTimings and related timing helpers.
FILE="crates/glyphnet-scanner/src/types.rs"
if [ -f "$FILE" ]; then
sed -n '1,220p' "$FILE"
rg -n "elapsed_micros|total_micros|decode_attempts_micros|ScanTimings" "$FILE"
fi
# Find the scanner function that contains the OpenCV fallback branch and locate started/decode_started.
rg -n "opencv_fallback::try_decode_with_opencv" crates/glyphnet-scanner/src/lib.rs
rg -n "decode_started|let\s+started|started\s*=" crates/glyphnet-scanner/src/lib.rs
# Show a wider slice around the fallback branch with the variable definitions.
sed -n '880,1065p' crates/glyphnet-scanner/src/lib.rsRepository: TriForMine/glyphnet
Length of output: 11823
Refresh timings when returning from the OpenCV fallback success path (crates/glyphnet-scanner/src/lib.rs).
timings.total_micros (and timings.decode_attempts_micros) are set right before the #[cfg(...opencv-fallback)] OpenCV if, so the Ok(StillScanResult { ... timings }) return inside that branch excludes the OpenCV decode cost. Recompute timings.total_micros (and likely timings.decode_attempts_micros for consistency) immediately before returning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/glyphnet-scanner/src/lib.rs` around lines 1037 - 1049, Recompute the
timing fields right before returning from the OpenCV fallback success path so
the OpenCV decode cost is included: update timings.total_micros (and
timings.decode_attempts_micros for consistency) immediately before the
Ok(StillScanResult { ... timings }) return inside the
opencv_fallback::try_decode_with_opencv branch by re-measuring elapsed time
since the original scan start (same reference used when timings were set) and by
including the latest decode attempt duration(s) derived from attempts so the
returned StillScanResult reflects the full cost of the OpenCV decode.
Summary by CodeRabbit
New Features
Improvements