feat: OCR max-dimension downscale guardrail and perf timing hooks - #10
Conversation
Closes out Phase 7 (memory limits explicitly deferred to Phase 8). Guardrail: images over [ocr] max_dimension (default 4000, 0 disables, --max-ocr-dim overrides) are fed to tesseract as a downscaled temp PNG. Pixels come from the already-decoded gdk::Texture — never an in-process re-decode of the untrusted file, which would bypass the glycin sandbox (NFR-002) and miss formats gdk-pixbuf can't load: - core ocr/downscale.rs: pure plan_downscale (aspect-preserving, min dim 1) and upscale_bboxes (per-axis factors from the actual scaled dims — per-axis rounding makes one uniform factor drift boxes on the minor axis). Bboxes return to original image space before caching/indexing, so cache hits are indistinguishable from full-res parses. - ui ocr_prep.rs: TextureDownloader on the main thread (GL/dmabuf downloads aren't reliably thread-safe pre-GTK 4.12), pixbuf scale + PNG encode on the OCR worker. Temp file is 0600 in XDG_RUNTIME_DIR (fallback cache dir, then system tmp) and deleted on every path via the NamedTempFile guard. Any prep failure degrades to full-resolution OCR — the guardrail is performance, not correctness. - Cache key gains the *effective* downscale target (full or WxH), derived after prep succeeds: below-threshold images keep their entries across max_dimension edits (ADR-0009), and a failed prep is keyed as full resolution. - max_ocr_dimension resolves CLI > config > 4000 in the invoking process and travels the single-instance argv (--max-ocr-dim=N). Timing hooks: tracing debug events under target quickview::perf (decode, OCR cache hit, downscale prep with factor/target, tesseract+parse with word count) — opt in with RUST_LOG=quickview::perf=debug.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds an OCR max-dimension guardrail that downscales oversized images before Tesseract, remaps OCR boxes back to original coordinates, keys cache entries by the effective downscale target, and threads the new ChangesOCR Downscale Guardrail
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Window
participant ViewerController
participant ocr_prep
participant Cache
participant Tesseract
Window->>ViewerController: new(max_ocr_dimension)
ViewerController->>ViewerController: finish_decode (log timing)
ViewerController->>ocr_prep: download_rgba + write_downscaled_png
ocr_prep-->>ViewerController: DownscaledImage and scale factors
ViewerController->>Cache: ocr_cache_path(downscale_target)
alt cache hit
Cache-->>ViewerController: cached OCR result
else cache miss
ViewerController->>Tesseract: run OCR on temp or original image
Tesseract-->>ViewerController: word boxes
ViewerController->>ViewerController: upscale_bboxes if downscaled
ViewerController->>Cache: store result
end
ViewerController-->>Window: render OCR results
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec3ad7364b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let prep = plan.and_then(|plan| match crate::ocr_prep::download_rgba(texture) { | ||
| Ok(pixels) => Some((pixels, plan)), |
There was a problem hiding this comment.
For images over max_ocr_dimension, this calls download_rgba on the GTK thread before the worker has a chance to check cache::load_ocr below, so even an OCR cache hit for a reopened large screenshot still performs a full RGBA texture copy on the UI thread (e.g. hundreds of MB for an 8k–10k image). That undermines the cache and can reintroduce the large-image hitch the guardrail is meant to avoid; compute/probe the planned cache entry first and only download pixels on a miss.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — a cache-hit reopen of an oversized image still paid the full main-thread RGBA copy. Fixed in 41977ca: the entry path is derived on the main thread before any pixel work (the key now uses the planned downscale target, so it's computable pre-prep; the stat is the same metadata I/O load_file already does there) and an existing entry skips download_rgba entirely. Degraded runs store full-res results under the planned key, so even they make future reopens download-free. Smoke-verified: first open of a 7000×5000 image logs downscale prep, reopen logs only the cache hit with no prep event.
A reopened large image with a cache hit still paid the full main-thread RGBA texture copy (hundreds of MB at 8-10k px) because the download happened before the worker's cache check — reintroducing the hitch the guardrail exists to prevent. The entry path is now derived on the main thread (keyed by the *planned* downscale target, so it is computable before any pixels exist; the stat matches the metadata I/O load_file already does there) and an existing entry skips the download entirely. Degraded runs keep storing full-res results under the planned key, which future opens then hit without downloading. Found by Codex on PR #10.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/quickview/src/main.rs (1)
78-105: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePrecedence logic correctly preserves an explicit
0(disable) from CLI or config.
cli_max_ocr_dim.or(cfg.ocr.max_dimension).unwrap_or(DEFAULT_MAX_OCR_DIMENSION)only falls through to the next source onNone, so--max-ocr-dim=0ormax_dimension = 0in config correctly disables the guardrail rather than being treated as absent. Logic looks correct.One gap: I don't see a unit test exercising this three-tier precedence (CLI > config > default) directly for
resolve_ocr_options, unlike the IPC round-trip tests added inipc.rs. Consider adding a small test if one doesn't already exist elsewhere in this file.🤖 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/quickview/src/main.rs` around lines 78 - 105, Add a focused unit test for resolve_ocr_options that verifies the three-level precedence between CLI, config, and default values, especially that an explicit 0 from either cli_max_ocr_dim or cfg.ocr.max_dimension is preserved rather than falling back; use resolve_ocr_options and its returned max_ocr_dimension to cover the cases.crates/quickview-core/src/ocr/downscale.rs (1)
20-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
factordoc overstates what it guarantees.The doc says "Multiply target-space lengths by this to get back to original image space," but
upscale_bboxes's own doc (Lines 56-59) explains per-axis factors are needed because independent rounding makes "one uniform factor would drift boxes on the minor axis."plan.factoris exact only for the major axis; using it uniformly (as the docstring implies) would reproduce that drift. Currently this field is only consumed for perf logging (pershared.rs'sstart_ocr), but the misleading doc could invite a future caller to misuse it for bbox math instead of the correctly-computed per-axisfactor_x/factor_y.📝 Suggested doc clarification
- /// `original / target` (> 1.0). Multiply target-space lengths by this to - /// get back to original image space. + /// `original / target` (> 1.0), based on the largest dimension. Exact + /// only for the axis that drove the scaling decision; the other axis + /// may round independently, so don't use this for bbox math — see + /// [`upscale_bboxes`], which needs separate per-axis factors. pub factor: f64,🤖 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/quickview-core/src/ocr/downscale.rs` around lines 20 - 28, The `DownscalePlan::factor` doc is too strong and implies a single uniform scale is always correct. Update the docstring on `factor` in `DownscalePlan` to clarify it is only the major-axis/original-to-target ratio and not suitable for general bbox rescaling; point readers to the per-axis scaling used by `upscale_bboxes` (`factor_x`/`factor_y`) for exact coordinate recovery. Keep the API unchanged, but make the wording explicitly warn against using `factor` for both axes.crates/quickview-ui/src/windows/shared.rs (1)
236-259: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffMain-thread
download_rgbacould stall the UI for very large images.
download_rgbaruns synchronously on the main thread (by design, per the module doc, due to GL/dmabuf thread-affinity before GTK 4.12). Since the guardrail only kicks in for images abovemax_dimension, there's no upper bound on how large the source texture can be, so the memcpy could be large enough to cause a visible stutter (and freeze the very busy-spinner shown during OCR) for extreme cases (e.g. huge multi-monitor screenshots). This is an inherent, already-documented tradeoff, not a regression — flagging for awareness/future consideration (e.g. capping applicability or revisiting once the minimum supported GTK version reaches 4.12).
🤖 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 `@crates/quickview-ui/src/windows/shared.rs`:
- Around line 260-266: The OCR cache key in shared.rs is derived from `prep`
alone, so a failed `write_downscaled_png` can still store a full-resolution OCR
result under a downscaled key. Update the `downscale_target`/`entry` selection
in the `download_rgba` + `write_downscaled_png` flow so it reflects whether the
downscaled file was actually produced, not just whether prep started. Use the
existing `prep`, `downscale_target`, `ocr_input`, and `cache::store_ocr` logic
to recompute or override the key on write failure, ensuring failed prep stores
under the full-size bucket as intended.
---
Nitpick comments:
In `@crates/quickview-core/src/ocr/downscale.rs`:
- Around line 20-28: The `DownscalePlan::factor` doc is too strong and implies a
single uniform scale is always correct. Update the docstring on `factor` in
`DownscalePlan` to clarify it is only the major-axis/original-to-target ratio
and not suitable for general bbox rescaling; point readers to the per-axis
scaling used by `upscale_bboxes` (`factor_x`/`factor_y`) for exact coordinate
recovery. Keep the API unchanged, but make the wording explicitly warn against
using `factor` for both axes.
In `@crates/quickview/src/main.rs`:
- Around line 78-105: Add a focused unit test for resolve_ocr_options that
verifies the three-level precedence between CLI, config, and default values,
especially that an explicit 0 from either cli_max_ocr_dim or
cfg.ocr.max_dimension is preserved rather than falling back; use
resolve_ocr_options and its returned max_ocr_dimension to cover the cases.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5bf422e-7510-487f-8165-978f88ef8783
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.claude/CLAUDE.mdadrs/ADR-0009-Caching.mdcrates/quickview-core/src/cache.rscrates/quickview-core/src/config.rscrates/quickview-core/src/ocr/downscale.rscrates/quickview-core/src/ocr/mod.rscrates/quickview-ui/Cargo.tomlcrates/quickview-ui/src/ipc.rscrates/quickview-ui/src/lib.rscrates/quickview-ui/src/ocr_prep.rscrates/quickview-ui/src/windows/full_viewer.rscrates/quickview-ui/src/windows/quick_preview.rscrates/quickview-ui/src/windows/shared.rscrates/quickview/src/main.rsdocs/PHASED_PLAN.mdtemplates/config.example.toml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use rustfmt and clippy for code formatting and linting, as tracked in rust-toolchain.toml
Run clippy with all targets and features enabled with -D warnings flag to treat warnings as errors
Files:
crates/quickview-core/src/config.rscrates/quickview-core/src/ocr/mod.rscrates/quickview-ui/src/windows/full_viewer.rscrates/quickview-ui/src/windows/quick_preview.rscrates/quickview-core/src/ocr/downscale.rscrates/quickview-ui/src/ipc.rscrates/quickview/src/main.rscrates/quickview-ui/src/lib.rscrates/quickview-ui/src/ocr_prep.rscrates/quickview-core/src/cache.rscrates/quickview-ui/src/windows/shared.rs
🔇 Additional comments (19)
crates/quickview/src/main.rs (2)
30-34: LGTM!
61-68: LGTM!crates/quickview-ui/src/ipc.rs (2)
10-10: LGTM!Also applies to: 36-52, 54-98
104-114: LGTM!Also applies to: 145-162, 183-210
templates/config.example.toml (1)
21-24: LGTM!.claude/CLAUDE.md (1)
64-70: LGTM!Also applies to: 79-79
docs/PHASED_PLAN.md (1)
173-175: LGTM!Also applies to: 185-193
crates/quickview-core/src/ocr/downscale.rs (1)
1-19: LGTM!Also applies to: 30-50, 52-67, 69-146
crates/quickview-core/src/ocr/mod.rs (1)
3-3: LGTM!crates/quickview-core/src/cache.rs (1)
4-7: LGTM!Also applies to: 23-36, 67-71, 178-244, 257-257, 280-280, 313-313, 323-323
adrs/ADR-0009-Caching.md (1)
56-61: LGTM!crates/quickview-core/src/config.rs (1)
46-48: LGTM!crates/quickview-ui/Cargo.toml (1)
25-25: LGTM!crates/quickview-ui/src/ocr_prep.rs (1)
1-124: LGTM!crates/quickview-ui/src/lib.rs (1)
14-14: LGTM!Also applies to: 34-36, 110-110
crates/quickview-ui/src/windows/shared.rs (2)
12-12: LGTM!Also applies to: 43-43, 54-75, 97-101, 143-201
287-290: 🎯 Functional Correctness | ⚡ Quick win"OCR cache hit" log won't appear under
RUST_LOG=quickview::perf=debug.Unlike the sibling "decode"/"downscale prep"/"ocr" events, this one has no explicit
target: "quickview::perf", so it defaults to the module path (quickview_ui::windows::shared). Per the PR objectives, OCR cache hits are supposed to be one of the four events gated byRUST_LOG=quickview::perf=debug; as written this event is filtered out by that target.🔧 Proposed fix
if let Some(cached) = entry.as_deref().and_then(cache::load_ocr) { - tracing::debug!("OCR cache hit for {}", path.display()); + tracing::debug!(target: "quickview::perf", path = %path.display(), "ocr cache hit"); return Ok(cached); }crates/quickview-ui/src/windows/full_viewer.rs (1)
23-23: LGTM!crates/quickview-ui/src/windows/quick_preview.rs (1)
30-30: LGTM!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41977cadd7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let prep = if probably_cached { | ||
| None | ||
| } else { | ||
| plan.and_then(|plan| match crate::ocr_prep::download_rgba(texture) { |
There was a problem hiding this comment.
For an oversized image with no cache entry, start_ocr is called synchronously from finish_decode immediately after set_texture, and this branch performs download_rgba(texture) on the GTK thread before the main loop can repaint. A first open of an 8k/10k image therefore still shows the old canvas/spinner and hitches while copying hundreds of MB, breaking the render-first behavior; schedule the OCR prep after an idle/frame or otherwise yield before the readback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — start_ocr ran synchronously in finish_decode, so the oversized-image download blocked the first paint of the very image it belonged to. Fixed in cb11bb5: start_ocr is scheduled via glib::idle_add_local_once (default-idle priority runs after GTK's redraw priority, so the frame paints first), guarded by the decode job id so navigation during the idle wait supersedes it cleanly.
| let entry = cache::cache_dir() | ||
| .map(|root| cache::ocr_cache_path(&root, &path, &ocr_opts, downscale_target)); |
There was a problem hiding this comment.
For oversized cache misses, this cache path stats the live file, but the OCR input below is built from the already-decoded texture; if the file is overwritten between the async decode and this call, old pixels are OCRed and stored under the new mtime/size key. Future opens of the edited file can then hit stale OCR, whereas the prior path-based OCR kept the key and input observing the same file; capture/verify the metadata associated with the decode before storing the texture-derived result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — the key statted the live file while the pixels came from the earlier decode, so an edit in between could store stale OCR under the live key (the pre-guardrail path never had this because tesseract read the same file the key described). Fixed in cb11bb5: new cache::FileStamp snapshots mtime+size in load_file before the decode reads content, and the key derives from that stamp — an edit after the stamp lands the result under a key the edited file misses, restoring the original invariant.
Two Codex findings on PR #10: - The cache key statted the live file at OCR time while the pixels came from the earlier async decode: a file replaced in between stored stale OCR under the live key, which future opens would wrongly hit. New cache::FileStamp snapshots mtime+size in load_file, before the decode reads content, and the key derives from that stamp — an edit after the stamp lands the result under a key the edited file simply misses (same invariant the pre-guardrail path had). - start_ocr ran synchronously inside finish_decode, so an oversized image's main-thread texture download blocked the first paint of the image it belonged to. start_ocr is now scheduled with glib::idle_add_local_once (default-idle runs after GTK's redraw priority), guarded by the decode job id in case navigation supersedes it while waiting for the idle slot.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb11bb522a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub fn download_rgba(texture: &gdk::Texture) -> Result<RgbaPixels> { | ||
| let mut downloader = gdk::TextureDownloader::new(texture); | ||
| downloader.set_format(gdk::MemoryFormat::R8g8b8a8); | ||
| let (bytes, stride) = downloader.download_bytes(); |
There was a problem hiding this comment.
For an oversized texture whose RGBA copy cannot be allocated, download_bytes() aborts the process instead of returning an error (GTK documents this for gdk_texture_downloader_download_bytes: https://docs.gtk.org/gdk4/method.TextureDownloader.download_bytes.html), so the Result/warn-and-fallback path around download_rgba is never reached. This is exactly the path used before downscaling large images, so a very large image or memory pressure can kill QuickView rather than degrading/skipping OCR; use a checked allocation plus download_into() or an explicit size guard before calling into GTK.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — download_bytes() g_mallocs and aborts on OOM, so the fallback path could never fire for exactly the allocations most likely to fail. Fixed in af58876: the buffer is allocated with Vec::try_reserve_exact (failure → Err → warn + full-resolution OCR) and filled via the gdk_texture_downloader_download_into ffi, which gdk4-rs 0.10 doesn't bind — the single unsafe call documents the size/stride invariants. Smoke-verified the downscale path still recognizes words with correct original-space coordinates through the new download.
gdk_texture_downloader_download_bytes g_mallocs its buffer, which aborts the whole process on allocation failure — the warn-and-fallback path around download_rgba could never see it. The RGBA buffer is now allocated with Vec::try_reserve_exact (a failure surfaces as Err and degrades to full-resolution OCR) and filled via the download_into ffi, which gdk4-rs 0.10 leaves unbound; the one unsafe call documents its size/stride invariants. Found by Codex on PR #10.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af588760cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // OCR cache key must describe the bytes that were actually decoded, | ||
| // not whatever the file becomes while the async decode runs (see | ||
| // cache::FileStamp). | ||
| let stamp = cache::FileStamp::read(path); |
There was a problem hiding this comment.
When the file is replaced in the gap between this stat and decode_texture actually opening path (for example an auto-updated screenshot path while glycin is still probing/loading), the displayed texture can be the new bytes while the OCR cache key still uses the old mtime/size. If an old cache entry exists it will be returned and stale boxes are shown over the new image; on a miss, OCR for the new image can be stored under the old key. Fresh evidence: this stamp is captured in load_file before the async decoder has opened the file, so it is not guaranteed to describe the decoded version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — the pre-decode stamp wasn't guaranteed to describe the bytes the decoder actually read. Fixed in b70a7fa with a double-stat: finish_decode re-reads the stamp after the decode completes, and on any mismatch the cache sits that load out entirely (no probe, no read, no store — the entry is None through the whole pipeline), so neither the stale-hit nor the wrong-store direction can happen. Matching nanosecond stamps on both sides of the decode pin the decoded version to the key; a change after the post-stat is fine because the key and content are then a consistent snapshot the edited file will simply miss.
The pre-decode stamp could describe a different version than the bytes the decoder actually read (file replaced in the gap), letting an old cache entry paint stale boxes over new pixels, or filing the new OCR under the old key. finish_decode now re-stats after the decode: on a stamp mismatch the cache sits the load out entirely (no probe, no read, no store — entry is None end to end); matching nanosecond stamps on both sides of the decode pin the decoded version to the key. Found by Codex on PR #10.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b70a7fa407
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cache::cache_dir() | ||
| .map(|root| cache::ocr_cache_path(&root, &path, &ocr_opts, downscale_target, stamp)) | ||
| }); | ||
| let probably_cached = entry.as_deref().is_some_and(|e| e.exists()); |
There was a problem hiding this comment.
When an oversized image has an unreadable/corrupt cache file, or the entry is removed between this probe and the worker’s load_ocr, probably_cached skips download_rgba, but the worker then sees a cache miss with prep == None and runs tesseract on the original full-size file. That bypasses the new max-dimension guardrail for exactly the miss path load_ocr is supposed to tolerate; verify the cache can actually be loaded before suppressing prep, or arrange for the miss path to still prepare/downscale pixels.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — and it exposed that the probe/read split-brain only existed because the download was pinned to the main thread. Fixed in 5c47688 by removing the split entirely: GdkTexture is immutable and threadsafe (decode.rs already sends textures across threads) and both decode backends produce memory textures, so the texture now travels into the OCR worker and download_rgba runs only after the authoritative load_ocr misses. No existence probe, no main-thread pixel work at all (which also supersedes the earlier idle-defer fix — dropped as moot), and every miss path — including corrupt/raced/schema-changed entries — goes through the downscale prep. Smoke re-verified: fresh miss preps on the worker, reopen hits, coordinates unchanged.
The main-thread existence probe could suppress the pixel download while the worker's authoritative load_ocr still missed (corrupt entry, schema change, raced deletion), silently running tesseract on the full-size original — bypassing the guardrail on exactly the miss path load_ocr is meant to tolerate. Root cause was the split-brain between probe and read, which only existed because the download was pinned to the main thread. GdkTexture is immutable and threadsafe (decode.rs already sends textures across threads) and both decode backends produce memory textures whose download is a plain copy, so the texture now travels into the OCR worker and the download happens only after the authoritative cache read misses: no probe, no main-thread pixel work at all, and a hit never touches pixels. The idle deferral from cb11bb5 is dropped as moot — nothing heavy runs before the first paint anymore. Found by Codex on PR #10.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c47688f8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // the tesseract run; drop deletes it on every path. A prep | ||
| // failure degrades to full resolution, which stores a | ||
| // (strictly better) full-res result under the downscaled key. | ||
| let mut ocr_input = path.clone(); |
There was a problem hiding this comment.
Fresh evidence after the decode-stamp fix: when the image is below max_ocr_dimension (or downscale prep falls back), ocr_input remains the live path even though entry was already derived from the decoded file's stamp above. If an auto-updated screenshot path is overwritten after finish_decode's second stat but before tesseract opens this path, OCR for the new bytes is displayed over the old decoded texture and can be stored under the old cache key; re-stat/disable caching (and ideally ignore the OCR result) before using the live file path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid for the display (the stored entry lands under the old stamp's key, which the replaced file can never hit again — so the cache side was already safe — but the overlay could show new-bytes boxes over the old texture). Fixed in 42db735: after a full-resolution run the worker re-reads the stamp and discards the result on mismatch — no overlay, nothing stored. Downscaled runs OCR the decoded pixels and are immune by construction; a None stamp (file already changed during decode) keeps the existing best-effort behavior with the cache bypassed. Verified the normal path still recognizes and stores.
On the full-resolution path (below threshold, or degraded prep) tesseract reads the live file, which can be replaced between the post-decode stat and tesseract's open: the words would describe different bytes than the displayed texture. The worker now re-verifies the stamp after a full-resolution run and drops the result on mismatch (no overlay, nothing stored). Downscaled runs OCR the decoded pixels and are immune; a None stamp already bypasses the cache and stays best-effort. Found by Codex on PR #10.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42db735e08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if tmp_guard.is_none() { | ||
| if let Some(stamp) = stamp { |
There was a problem hiding this comment.
Fresh evidence after the prior stamp fix: when finish_decode has already detected a pre/post-decode metadata mismatch it passes stamp == None, but on below-threshold images (or oversized images whose downscale prep fails) tmp_guard.is_none() and this if let Some(stamp) skips validation entirely. In the case where the decoder displayed the pre-replacement bytes while tesseract just read the live replacement path, the mismatched OCR is still accepted and painted over the old texture; treat None as unsafe for full-resolution OCR and drop the result or OCR from decoded pixels.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — the None-stamp best-effort path still trusted the live file for full-resolution OCR. Fixed in ecfb73c with your second option: when the stamp is unknown, the decoded pixels are the only safe input, so the worker feeds them through the existing prep pipeline at original size (factor 1.0) and never falls back to the live path; if that pixel prep fails, the result is dropped rather than shown mismatched. The post-OCR stamp re-verification now only guards the stamped full-resolution path, since every pixel-fed run is immune by construction.
With stamp == None (file changed during decode), the below-threshold and degraded paths still ran tesseract on the untrusted live file and showed whatever it found over the old texture. The worker now treats the decoded pixels as the only safe OCR input in that case: it encodes them at original size (factor 1.0) through the existing prep pipeline and never falls back to the live path — if the pixel prep fails, the result is dropped rather than mismatched. Found by Codex on PR #10.
1 participant
Closes out Phase 7 (memory limits explicitly deferred to Phase 8, noted in PHASED_PLAN).
Downscale guardrail
Images over
[ocr] max_dimension(default 4000,0disables,--max-ocr-dimoverrides; resolves CLI > config in the invoking process and travels the single-instance argv) are fed to tesseract as a downscaled temp PNG.Pixels come from the already-decoded
gdk::Texture, never an in-process re-decode of the untrusted file — that would bypass the glycin sandbox (NFR-002, tesseract decodes in a subprocess; its decoder bugs can't take the viewer down) and miss formats gdk-pixbuf has no loader for (webp/avif/heif).quickview-core/src/ocr/downscale.rs(pure, tested):plan_downscale(aspect-preserving, target dims never below 1,0disables) andupscale_bboxeswith per-axis factors computed from the actually-scaled dimensions (per-axis rounding makes one uniform factor drift boxes on the minor axis). Bboxes return to original image space before caching/indexing — everything downstream only sees original-space coordinates.quickview-ui/src/ocr_prep.rs:TextureDownloaderon the main thread (GL/dmabuf downloads aren't reliably thread-safe pre-GTK 4.12; bounded memcpy, paid only for oversized images), pixbuf scale + PNG encode on the OCR worker. Temp file is 0600 in$XDG_RUNTIME_DIR(→ cache dir → system tmp), deleted on every path via theNamedTempFileguard. Any prep failure warns and degrades to full-resolution OCR — the guardrail is performance, not correctness.fullorWxH), derived after prep succeeds: below-threshold images keep their entries acrossmax_dimensionedits (ADR-0009), and a failed prep is keyed as full resolution. One more one-time invalidation for previously-cached entries.Timing hooks
tracingdebug events under targetquickview::perf: decode (elapsed, dims), OCR cache hit, downscale prep (elapsed, factor, target), tesseract+parse (elapsed, word count, lang, downscaled flag). Opt in withRUST_LOG=quickview::perf=debug; not user-facing per ARCHITECTURE.md.Testing
--max-ocr-dimincl. 0 and rejects garbage); clippy-D warningsclean.decode 283ms → downscale prep 226ms (factor 1.5, 4000×2667) → ocr 249ms, 10 words; cached bboxes verified in original space (word at x=410/y=655 matching the 200pt annotation at +400+800; un-upscaled would be x≈273); second open is a cache hit; noquickview-ocr-*temp files left in$XDG_RUNTIME_DIR;--max-ocr-dim 0runs full-res (downscaled=false) under its own key.