From ec3ad7364b42be900c28653cb1c9feb59992e2c0 Mon Sep 17 00:00:00 2001 From: Babken Egoian <101829110+green2grey@users.noreply.github.com> Date: Mon, 6 Jul 2026 01:22:11 -0700 Subject: [PATCH 1/8] feat: OCR max-dimension downscale guardrail and perf timing hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .claude/CLAUDE.md | 9 +- Cargo.lock | 1 + adrs/ADR-0009-Caching.md | 10 +- crates/quickview-core/src/cache.rs | 77 +++++++-- crates/quickview-core/src/config.rs | 4 +- crates/quickview-core/src/ocr/downscale.rs | 146 ++++++++++++++++++ crates/quickview-core/src/ocr/mod.rs | 1 + crates/quickview-ui/Cargo.toml | 1 + crates/quickview-ui/src/ipc.rs | 25 ++- crates/quickview-ui/src/lib.rs | 4 + crates/quickview-ui/src/ocr_prep.rs | 123 +++++++++++++++ .../quickview-ui/src/windows/full_viewer.rs | 2 +- .../quickview-ui/src/windows/quick_preview.rs | 2 +- crates/quickview-ui/src/windows/shared.rs | 109 +++++++++++-- crates/quickview/src/main.rs | 20 ++- docs/PHASED_PLAN.md | 15 +- templates/config.example.toml | 5 +- 17 files changed, 509 insertions(+), 45 deletions(-) create mode 100644 crates/quickview-core/src/ocr/downscale.rs create mode 100644 crates/quickview-ui/src/ocr_prep.rs diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 5b9b45d..9c0f22a 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -61,6 +61,13 @@ The scaffold is functional with image display, async OCR pipeline, drag-select o `config.rs`): lang (precedence `--lang` > `QUICKVIEW_LANG` > config > `eng`) and `tessdata_dir` (`--tessdata-dir` > config); both live in `OcrOptions` and join the cache key +- OCR max-dimension guardrail (`max_dimension`/`--max-ocr-dim`, default 4000): + oversized images OCR a downscaled temp PNG made from the decoded texture + (`ocr_prep.rs`; main-thread download, worker-thread scale+encode), bboxes + map back to original space (`ocr/downscale.rs`), effective target joins the + cache key +- `quickview::perf` debug timing events (decode, cache hit, downscale prep, + OCR) — `RUST_LOG=quickview::perf=debug` - On-disk OCR cache (`~/.cache/quickview/ocr/`, keyed by path+lang+mtime+size; no eviction in v1 — see ADR-0009 implementation notes) - Drag-select overlay with word highlighting @@ -69,7 +76,7 @@ The scaffold is functional with image display, async OCR pipeline, drag-select o - Zoom & pan (Ctrl+scroll, pinch, +/- keys, middle-drag pan) via custom `ZoomableCanvas` widget ### What's not implemented yet: -- Performance benchmarks +- Memory usage limits (deferred to Phase 8) ## Development diff --git a/Cargo.lock b/Cargo.lock index a5775bc..5b3874a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1673,6 +1673,7 @@ dependencies = [ "gtk4-layer-shell", "libadwaita", "quickview-core", + "tempfile", "tracing", ] diff --git a/adrs/ADR-0009-Caching.md b/adrs/ADR-0009-Caching.md index 91f8143..4a20df5 100644 --- a/adrs/ADR-0009-Caching.md +++ b/adrs/ADR-0009-Caching.md @@ -53,10 +53,12 @@ The implementation went **straight to on-disk**, revising the decision above: derived from the lowercased app name, so the app-ID rename (done: io.github.Green2Grey2.QuickView) did not move it on Linux. - Tesseract is invoked with no psm/oem flags, so the OCR settings in the key - are `lang` and (since Phase 7's config work) the optional `tessdata_dir`, - hashed with a presence marker so `None` and empty stay distinct. **The rule - stands: any newly configurable OCR setting (psm/oem, the downscale target) - must join the key.** + are `lang`, the optional `tessdata_dir` (presence marker keeps `None` and + empty distinct), and the **effective** downscale target (`full` or + `WxH`) — the target rather than the configured `max_dimension` threshold, + so below-threshold images keep their entries across threshold edits. **The + rule stands: any newly configurable OCR setting (psm/oem) must join the + key.** - Writes are atomic (temp file + rename in the same directory): concurrent QuickView processes are a designed use case. - Entries are created `0600` in `0700` directories — they hold recognized diff --git a/crates/quickview-core/src/cache.rs b/crates/quickview-core/src/cache.rs index 76c7925..fb18d8d 100644 --- a/crates/quickview-core/src/cache.rs +++ b/crates/quickview-core/src/cache.rs @@ -1,10 +1,10 @@ //! On-disk OCR result cache. //! //! Entries are JSON files under `/ocr/`, keyed by a blake3 hash of -//! the image path, the OCR settings (language, tessdata dir), and file -//! mtime+size — so an edited file is simply a cache miss (no invalidation -//! logic needed). There is no eviction in -//! v1: entries are a few KB each, and users can clear the directory manually. +//! the image path, the OCR settings (language, tessdata dir), the effective +//! downscale target, and file mtime+size — so an edited file is simply a +//! cache miss (no invalidation logic needed). There is no eviction in v1: +//! entries are a few KB each, and users can clear the directory manually. //! Phase 8's persistent SQLite cache is the planned successor (ADR-0009). use std::path::{Path, PathBuf}; @@ -20,7 +20,20 @@ pub fn cache_dir() -> Option { Some(crate::config::project_dirs()?.cache_dir().to_path_buf()) } -pub fn ocr_cache_path(cache_root: &Path, file: &Path, opts: &OcrOptions) -> PathBuf { +/// Compute the cache entry path for one OCR run. +/// +/// `downscale_target` is the *effective* image size tesseract will see: +/// `None` for a full-resolution run, `Some((w, h))` when the max-dimension +/// guardrail feeds it a downscaled copy. Hashing the effective target rather +/// than the configured threshold keeps below-threshold images' entries valid +/// across `max_dimension` edits — per ADR-0009, only inputs that change the +/// recognition output join the key. +pub fn ocr_cache_path( + cache_root: &Path, + file: &Path, + opts: &OcrOptions, + downscale_target: Option<(u32, u32)>, +) -> PathBuf { // Include file metadata to avoid stale caches. Full nanosecond mtime: // whole seconds would alias a same-second rewrite of the same path with // an unchanged byte length (rapid screenshot/editor saves). @@ -51,6 +64,11 @@ pub fn ocr_cache_path(cache_root: &Path, file: &Path, opts: &OcrOptions) -> Path } } hasher.update(b"\0"); + match downscale_target { + Some((w, h)) => hasher.update(format!("{w}x{h}").as_bytes()), + None => hasher.update(b"full"), + }; + hasher.update(b"\0"); hasher.update(&mtime.to_le_bytes()); hasher.update(&size.to_le_bytes()); let key = hasher.finalize().to_hex().to_string(); @@ -157,41 +175,41 @@ mod tests { let img = dir.path().join("a.png"); std::fs::write(&img, b"xx").unwrap(); - let base = ocr_cache_path(root, &img, &eng()); + let base = ocr_cache_path(root, &img, &eng(), None); // Same inputs -> same key. - assert_eq!(base, ocr_cache_path(root, &img, &eng())); + assert_eq!(base, ocr_cache_path(root, &img, &eng(), None)); // Different language -> different key. let deu = OcrOptions { lang: "deu".into(), ..eng() }; - assert_ne!(base, ocr_cache_path(root, &img, &deu)); + assert_ne!(base, ocr_cache_path(root, &img, &deu, None)); // Different path -> different key. let img2 = dir.path().join("b.png"); std::fs::write(&img2, b"xx").unwrap(); - assert_ne!(base, ocr_cache_path(root, &img2, &eng())); + assert_ne!(base, ocr_cache_path(root, &img2, &eng(), None)); // Different size -> different key. std::fs::write(&img, b"xxxx").unwrap(); - assert_ne!(base, ocr_cache_path(root, &img, &eng())); + assert_ne!(base, ocr_cache_path(root, &img, &eng(), None)); // Different mtime (same size) -> different key. std::fs::write(&img, b"xx").unwrap(); - let before = ocr_cache_path(root, &img, &eng()); + let before = ocr_cache_path(root, &img, &eng(), None); let old = std::time::SystemTime::UNIX_EPOCH + std::time::Duration::from_secs(1_000_000); std::fs::File::open(&img) .unwrap() .set_modified(old) .unwrap(); - assert_ne!(before, ocr_cache_path(root, &img, &eng())); + assert_ne!(before, ocr_cache_path(root, &img, &eng(), None)); // Subsecond mtime change (same second, same size) -> different key. let with_key = |t| { std::fs::File::open(&img).unwrap().set_modified(t).unwrap(); - ocr_cache_path(root, &img, &eng()) + ocr_cache_path(root, &img, &eng(), None) }; assert_ne!( with_key(old + std::time::Duration::from_nanos(1)), @@ -199,6 +217,31 @@ mod tests { ); } + #[test] + fn key_changes_with_downscale_target_only_when_downscaled() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + let img = dir.path().join("a.png"); + std::fs::write(&img, b"xx").unwrap(); + + let with_target = |t| ocr_cache_path(root, &img, &eng(), t); + + // A downscaled run is keyed apart from full resolution, and targets + // are keyed apart from each other. + assert_ne!(with_target(None), with_target(Some((4000, 2000)))); + assert_ne!( + with_target(Some((4000, 2000))), + with_target(Some((2000, 1000))) + ); + // Same effective target -> same key (threshold edits don't invalidate + // below-threshold images, which always hash as full resolution). + assert_eq!(with_target(None), with_target(None)); + assert_eq!( + with_target(Some((4000, 2000))), + with_target(Some((4000, 2000))) + ); + } + #[test] fn key_changes_with_tessdata_dir() { let dir = tempfile::tempdir().unwrap(); @@ -211,7 +254,7 @@ mod tests { lang: "eng".into(), tessdata_dir: d.map(PathBuf::from), }; - ocr_cache_path(root, &img, &opts) + ocr_cache_path(root, &img, &opts, None) }; // Some(dir) differs from None, and dirs differ from each other. @@ -234,7 +277,7 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let img = dir.path().join("a.png"); std::fs::write(&img, b"xx").unwrap(); - let entry = ocr_cache_path(dir.path(), &img, &eng()); + let entry = ocr_cache_path(dir.path(), &img, &eng(), None); let result = sample_result(); store_ocr(&entry, &result).unwrap(); @@ -267,7 +310,7 @@ mod tests { let img = dir.path().join("a.png"); std::fs::write(&img, b"xx").unwrap(); - let entry = ocr_cache_path(dir.path(), &img, &eng()); + let entry = ocr_cache_path(dir.path(), &img, &eng(), None); assert!(load_ocr(&entry).is_none()); } @@ -277,7 +320,7 @@ mod tests { let img = dir.path().join("a.png"); std::fs::write(&img, b"xx").unwrap(); - let entry = ocr_cache_path(dir.path(), &img, &eng()); + let entry = ocr_cache_path(dir.path(), &img, &eng(), None); std::fs::create_dir_all(entry.parent().unwrap()).unwrap(); std::fs::write(&entry, b"{not json").unwrap(); diff --git a/crates/quickview-core/src/config.rs b/crates/quickview-core/src/config.rs index 9314c6c..08f9dbf 100644 --- a/crates/quickview-core/src/config.rs +++ b/crates/quickview-core/src/config.rs @@ -43,8 +43,8 @@ pub struct OcrSection { /// Directory with `.traineddata` files, e.g. a `tessdata_fast` or /// `tessdata_best` checkout (overridden by `--tessdata-dir`). pub tessdata_dir: Option, - /// Maximum image dimension before OCR downscales (not consumed yet; - /// accepted so configs written for the guardrail don't error). + /// Maximum image dimension before OCR runs on a downscaled copy + /// (overridden by `--max-ocr-dim`; `0` disables the guardrail). pub max_dimension: Option, } diff --git a/crates/quickview-core/src/ocr/downscale.rs b/crates/quickview-core/src/ocr/downscale.rs new file mode 100644 index 0000000..63ca17c --- /dev/null +++ b/crates/quickview-core/src/ocr/downscale.rs @@ -0,0 +1,146 @@ +//! Downscale planning for the OCR max-dimension guardrail. +//! +//! Tesseract re-decodes the image file itself, so oversized images make OCR +//! slow and memory-hungry out of proportion to recognition quality. When an +//! image exceeds the configured maximum dimension, the UI feeds tesseract a +//! downscaled temporary copy instead and maps the resulting word boxes back +//! into original image space with [`upscale_bboxes`] — so everything +//! downstream (cache entries, the spatial index, selection) only ever sees +//! original-space coordinates. +//! +//! This module is pure math; producing the actual downscaled pixels is the +//! UI layer's job (it owns the decoded texture). + +use crate::ocr::models::OcrResult; + +/// Default for the `[ocr] max_dimension` config setting. `0` disables the +/// guardrail entirely. +pub const DEFAULT_MAX_OCR_DIMENSION: u32 = 4000; + +/// A decision to OCR a downscaled copy instead of the original. +#[derive(Debug, Clone, Copy, PartialEq)] +pub struct DownscalePlan { + pub target_w: u32, + pub target_h: u32, + /// `original / target` (> 1.0). Multiply target-space lengths by this to + /// get back to original image space. + pub factor: f64, +} + +/// Decide whether an image of `width` x `height` needs downscaling to fit +/// `max_dimension`. Returns `None` when it already fits (or the guardrail is +/// disabled with `max_dimension == 0`); aspect ratio is preserved and no +/// target dimension falls below 1. +pub fn plan_downscale(width: u32, height: u32, max_dimension: u32) -> Option { + if max_dimension == 0 || width == 0 || height == 0 { + return None; + } + let largest = width.max(height); + if largest <= max_dimension { + return None; + } + + let factor = f64::from(largest) / f64::from(max_dimension); + let scale = |dim: u32| ((f64::from(dim) / factor).round() as u32).max(1); + Some(DownscalePlan { + target_w: scale(width), + target_h: scale(height), + factor, + }) +} + +/// Map word bounding boxes recognized in downscaled space back to original +/// image space. Run this *before* the result is cached or indexed, so a +/// cache hit is indistinguishable from a full-resolution parse. +/// +/// Factors are per-axis (`original / actual target`, computed from the +/// dimensions of the image actually fed to tesseract): target sizes are +/// rounded independently per axis, so one uniform factor would drift boxes +/// on the minor axis. +pub fn upscale_bboxes(result: &mut OcrResult, factor_x: f64, factor_y: f64) { + for word in &mut result.words { + word.bbox.x *= factor_x; + word.bbox.y *= factor_y; + word.bbox.w *= factor_x; + word.bbox.h *= factor_y; + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::geometry::Rect; + use crate::ocr::models::OcrWord; + + #[test] + fn below_and_at_threshold_are_untouched() { + assert_eq!(plan_downscale(1000, 500, 4000), None); + assert_eq!(plan_downscale(4000, 3000, 4000), None); + assert_eq!(plan_downscale(3000, 4000, 4000), None); + } + + #[test] + fn zero_disables_and_degenerate_dims_are_ignored() { + assert_eq!(plan_downscale(100_000, 50, 0), None); + assert_eq!(plan_downscale(0, 5000, 4000), None); + assert_eq!(plan_downscale(5000, 0, 4000), None); + } + + #[test] + fn above_threshold_scales_and_preserves_aspect() { + let plan = plan_downscale(8000, 4000, 4000).unwrap(); + assert_eq!((plan.target_w, plan.target_h), (4000, 2000)); + assert_eq!(plan.factor, 2.0); + + // Portrait: the larger dimension drives the factor. + let plan = plan_downscale(3000, 6000, 4000).unwrap(); + assert_eq!((plan.target_w, plan.target_h), (2000, 4000)); + assert_eq!(plan.factor, 1.5); + } + + #[test] + fn extreme_aspect_never_reaches_zero() { + let plan = plan_downscale(100_000, 50, 4000).unwrap(); + assert_eq!(plan.target_w, 4000); + assert!(plan.target_h >= 1); + assert_eq!(plan.target_h, 2); // 50 / 25 = 2 + + let plan = plan_downscale(1_000_000, 1, 4000).unwrap(); + assert_eq!(plan.target_h, 1); + } + + #[test] + fn upscale_round_trips_within_epsilon() { + let factor = 8000.0 / 4000.0; + let original = Rect { + x: 123.0, + y: 456.0, + w: 78.0, + h: 90.0, + }; + // What tesseract would report in downscaled space. + let mut result = OcrResult { + words: vec![OcrWord { + text: "hi".into(), + confidence: 90.0, + bbox: Rect { + x: original.x / factor, + y: original.y / factor, + w: original.w / factor, + h: original.h / factor, + }, + order: 0, + }], + }; + upscale_bboxes(&mut result, factor, factor); + let got = &result.words[0].bbox; + for (a, b) in [ + (got.x, original.x), + (got.y, original.y), + (got.w, original.w), + (got.h, original.h), + ] { + assert!((a - b).abs() < 1e-9, "{a} vs {b}"); + } + } +} diff --git a/crates/quickview-core/src/ocr/mod.rs b/crates/quickview-core/src/ocr/mod.rs index 2552ffd..85c1608 100644 --- a/crates/quickview-core/src/ocr/mod.rs +++ b/crates/quickview-core/src/ocr/mod.rs @@ -1,5 +1,6 @@ //! OCR-related types and helpers. +pub mod downscale; pub mod index; pub mod models; pub mod select; diff --git a/crates/quickview-ui/Cargo.toml b/crates/quickview-ui/Cargo.toml index 091c78b..d104180 100644 --- a/crates/quickview-ui/Cargo.toml +++ b/crates/quickview-ui/Cargo.toml @@ -22,3 +22,4 @@ async-channel = "2" # runtime by probing installed loaders. Links libseccomp, lcms2, and # fontconfig (see docs/DEPENDENCIES.md). glycin = { version = "3", features = ["gdk4"] } +tempfile = "3" diff --git a/crates/quickview-ui/src/ipc.rs b/crates/quickview-ui/src/ipc.rs index 803d984..e908fb1 100644 --- a/crates/quickview-ui/src/ipc.rs +++ b/crates/quickview-ui/src/ipc.rs @@ -7,7 +7,7 @@ //! //! ```text //! quickview --mode= --lang= \ -//! [--tessdata-dir=] --file= +//! [--tessdata-dir=] --max-ocr-dim= --file= //! ``` //! //! Every value is glued to its key in a single `--key=value` token: GLib's @@ -46,6 +46,7 @@ pub(crate) fn to_argv(opts: &LaunchOptions) -> Vec { if let Some(dir) = &opts.ocr.tessdata_dir { argv.push(format!("--tessdata-dir={}", dir.to_string_lossy())); } + argv.push(format!("--max-ocr-dim={}", opts.max_ocr_dimension)); argv.push(format!("--file={}", opts.file.to_string_lossy())); argv } @@ -54,6 +55,7 @@ pub(crate) fn from_argv(argv: &[OsString]) -> Result { let mut mode = None; let mut lang = None; let mut tessdata_dir: Option = None; + let mut max_ocr_dimension = None; let mut file: Option = None; for arg in argv.iter().skip(1) { @@ -71,6 +73,12 @@ pub(crate) fn from_argv(argv: &[OsString]) -> Result { lang = Some(value.to_owned()); } else if let Some(value) = arg.strip_prefix("--tessdata-dir=") { tessdata_dir = Some(PathBuf::from(value)); + } else if let Some(value) = arg.strip_prefix("--max-ocr-dim=") { + max_ocr_dimension = Some( + value + .parse::() + .map_err(|err| anyhow!("bad --max-ocr-dim {value:?}: {err}"))?, + ); } else if let Some(value) = arg.strip_prefix("--file=") { file = Some(PathBuf::from(value)); } else { @@ -84,6 +92,7 @@ pub(crate) fn from_argv(argv: &[OsString]) -> Result { lang: lang.ok_or_else(|| anyhow!("missing --lang"))?, tessdata_dir, }, + max_ocr_dimension: max_ocr_dimension.ok_or_else(|| anyhow!("missing --max-ocr-dim"))?, file: file.ok_or_else(|| anyhow!("missing --file"))?, }) } @@ -100,6 +109,7 @@ mod tests { lang: lang.to_owned(), tessdata_dir: None, }, + max_ocr_dimension: 4000, } } @@ -138,6 +148,11 @@ mod tests { original.ocr.tessdata_dir = Some(PathBuf::from("/opt/tess data/fast")); assert_eq!(round_trip(&original), original); + // Guardrail disabled round-trips too. + let mut disabled = opts(Mode::QuickPreview, "eng", "/tmp/a.png"); + disabled.max_ocr_dimension = 0; + assert_eq!(round_trip(&disabled), disabled); + // Absent stays absent (no --tessdata-dir token emitted at all). let without = opts(Mode::QuickPreview, "eng", "/tmp/a.png"); assert!(!to_argv(&without) @@ -168,6 +183,14 @@ mod tests { #[test] fn rejects_garbage() { assert!(from_argv(&os_argv(&["quickview", "--bogus=x"])).is_err()); + assert!(from_argv(&os_argv(&[ + "quickview", + "--mode=full-viewer", + "--lang=eng", + "--max-ocr-dim=lots", + "--file=/a" + ])) + .is_err()); assert!(from_argv(&os_argv(&[ "quickview", "--mode=sideways", diff --git a/crates/quickview-ui/src/lib.rs b/crates/quickview-ui/src/lib.rs index 9d08d69..66b6781 100644 --- a/crates/quickview-ui/src/lib.rs +++ b/crates/quickview-ui/src/lib.rs @@ -11,6 +11,7 @@ use gtk::gio; mod decode; mod ipc; +mod ocr_prep; pub mod widgets; pub mod windows; @@ -30,6 +31,8 @@ pub struct LaunchOptions { /// Fully resolved OCR settings: CLI/env/config precedence is applied in /// the invoking process before these cross the instance boundary. pub ocr: OcrOptions, + /// Max image dimension before OCR downscales (0 disables the guardrail). + pub max_ocr_dimension: u32, } /// Windows the primary instance manages across invocations. @@ -104,6 +107,7 @@ fn dispatch(app: &adw::Application, state: &Rc, opts: &LaunchOptions) // Explicit request for a different file: show it, with // the OCR settings this invocation asked for. controller.set_ocr_options(opts.ocr.clone()); + controller.set_max_ocr_dimension(opts.max_ocr_dimension); controller.load_file(&opts.file); window.present(); } diff --git a/crates/quickview-ui/src/ocr_prep.rs b/crates/quickview-ui/src/ocr_prep.rs new file mode 100644 index 0000000..98041cb --- /dev/null +++ b/crates/quickview-ui/src/ocr_prep.rs @@ -0,0 +1,123 @@ +//! Producing downscaled pixels for the OCR max-dimension guardrail. +//! +//! Tesseract reads a file path, so downscaling means materializing a smaller +//! temporary image. The pixels come from the already-decoded `gdk::Texture` +//! — never from re-decoding the untrusted file in-process, which would +//! bypass the glycin sandbox (NFR-002) and miss formats gdk-pixbuf has no +//! loader for. Split across threads: +//! +//! - [`download_rgba`] runs on the **main thread**: GL/dmabuf-backed texture +//! downloads are not reliably thread-safe before GTK 4.12. It is a bounded +//! memcpy, paid only for oversized images. +//! - [`write_downscaled_png`] runs on the OCR worker thread: scales the +//! (trusted, self-produced) RGBA buffer with gdk-pixbuf and encodes it to +//! a private temp PNG for tesseract. + +use std::path::PathBuf; + +use anyhow::{Context, Result}; +use gtk4 as gtk; + +use gtk::prelude::*; +use gtk::{gdk, gdk_pixbuf}; + +use quickview_core::ocr::downscale::DownscalePlan; + +/// RGBA pixels downloaded from a texture, ready to cross to a worker thread +/// (`glib::Bytes` is `Send + Sync`; no GObject travels with it). +pub struct RgbaPixels { + bytes: glib::Bytes, + width: i32, + height: i32, + stride: i32, +} + +/// Download `texture` as unpremultiplied RGBA. Main thread only. +pub fn download_rgba(texture: &gdk::Texture) -> Result { + let mut downloader = gdk::TextureDownloader::new(texture); + downloader.set_format(gdk::MemoryFormat::R8g8b8a8); + let (bytes, stride) = downloader.download_bytes(); + let stride = i32::try_from(stride).context("texture stride exceeds i32")?; + Ok(RgbaPixels { + bytes, + width: texture.width(), + height: texture.height(), + stride, + }) +} + +/// The downscaled temp image tesseract will read. +/// +/// The temp file is deleted when this guard drops (including on every error +/// path), so keep it alive across the tesseract run. +pub struct DownscaledImage { + file: tempfile::NamedTempFile, + /// Per-axis `original / actual` factors for mapping OCR bboxes back to + /// original image space, computed from the scaled image's real + /// dimensions (per-axis rounding makes one uniform factor drift). + pub factor_x: f64, + pub factor_y: f64, + /// Actual dimensions fed to tesseract (equals the plan's target). + pub target: (u32, u32), +} + +impl DownscaledImage { + pub fn path(&self) -> &std::path::Path { + self.file.path() + } +} + +/// Scale `pixels` to `plan`'s target and write a private temp PNG. +/// Worker-thread safe: the pixbuf is created and dropped here. +pub fn write_downscaled_png(pixels: &RgbaPixels, plan: &DownscalePlan) -> Result { + let src = gdk_pixbuf::Pixbuf::from_bytes( + &pixels.bytes, + gdk_pixbuf::Colorspace::Rgb, + true, // has_alpha (R8g8b8a8, unpremultiplied) + 8, + pixels.width, + pixels.height, + pixels.stride, + ); + let target_w = i32::try_from(plan.target_w).context("target width exceeds i32")?; + let target_h = i32::try_from(plan.target_h).context("target height exceeds i32")?; + let scaled = src + .scale_simple(target_w, target_h, gdk_pixbuf::InterpType::Bilinear) + .context("pixbuf scaling failed (out of memory?)")?; + drop(src); + + // NamedTempFile is created 0600, matching the OCR cache's posture: the + // downscaled copy holds the same sensitive content as the screenshot. + let file = tempfile::Builder::new() + .prefix("quickview-ocr-") + .suffix(".png") + .tempfile_in(temp_dir()) + .context("failed to create OCR temp file")?; + scaled + .savev(file.path(), "png", &[]) + .context("failed to encode downscaled PNG")?; + + Ok(DownscaledImage { + file, + factor_x: f64::from(pixels.width) / f64::from(scaled.width()), + factor_y: f64::from(pixels.height) / f64::from(scaled.height()), + target: (scaled.width() as u32, scaled.height() as u32), + }) +} + +/// Where the temp image lives: `$XDG_RUNTIME_DIR` first (0700 tmpfs — same +/// privacy expectations as the OCR cache), then the cache dir, then the +/// system temp dir. +fn temp_dir() -> PathBuf { + if let Some(dir) = std::env::var_os("XDG_RUNTIME_DIR").map(PathBuf::from) { + if dir.is_dir() { + return dir; + } + } + if let Some(dir) = quickview_core::cache::cache_dir() { + if std::fs::create_dir_all(&dir).is_ok() { + return dir; + } + } + std::env::temp_dir() +} diff --git a/crates/quickview-ui/src/windows/full_viewer.rs b/crates/quickview-ui/src/windows/full_viewer.rs index 853f5ad..6101c08 100644 --- a/crates/quickview-ui/src/windows/full_viewer.rs +++ b/crates/quickview-ui/src/windows/full_viewer.rs @@ -20,7 +20,7 @@ pub fn present(app: &adw::Application, opts: &LaunchOptions) { let header = adw::HeaderBar::new(); header.set_title_widget(Some(&title)); - let viewer = ViewerController::new(opts.file.clone(), opts.ocr.clone()); + let viewer = ViewerController::new(opts.file.clone(), opts.ocr.clone(), opts.max_ocr_dimension); { let title = title.clone(); diff --git a/crates/quickview-ui/src/windows/quick_preview.rs b/crates/quickview-ui/src/windows/quick_preview.rs index 6a87ed1..76aa02a 100644 --- a/crates/quickview-ui/src/windows/quick_preview.rs +++ b/crates/quickview-ui/src/windows/quick_preview.rs @@ -27,7 +27,7 @@ pub fn present( .default_height(PANEL_HEIGHT) .build(); - let viewer = ViewerController::new(opts.file.clone(), opts.ocr.clone()); + let viewer = ViewerController::new(opts.file.clone(), opts.ocr.clone(), opts.max_ocr_dimension); // Layer shell if supported. if gtk4_layer_shell::is_supported() { diff --git a/crates/quickview-ui/src/windows/shared.rs b/crates/quickview-ui/src/windows/shared.rs index 3ce1f37..fa55fd5 100644 --- a/crates/quickview-ui/src/windows/shared.rs +++ b/crates/quickview-ui/src/windows/shared.rs @@ -9,7 +9,7 @@ use gtk4 as gtk; use quickview_core::{ cache, fs, - ocr::{tesseract, tesseract::OcrOptions, tsv}, + ocr::{downscale, tesseract, tesseract::OcrOptions, tsv}, }; use crate::widgets::image_overlay::ImageOverlayWidget; @@ -40,6 +40,7 @@ pub struct ViewerController { dir_index: Rc>, ocr: Rc>, + max_ocr_dimension: Rc>, // Monotonic ids to ignore late results from superseded jobs. decode_job_id: Rc>, @@ -50,7 +51,7 @@ pub struct ViewerController { } impl ViewerController { - pub fn new(initial_file: PathBuf, ocr: OcrOptions) -> Self { + pub fn new(initial_file: PathBuf, ocr: OcrOptions, max_ocr_dimension: u32) -> Self { let overlay = ImageOverlayWidget::new(); let current_file = Rc::new(RefCell::new(initial_file.clone())); @@ -62,6 +63,7 @@ impl ViewerController { dir_images: Rc::new(RefCell::new(dir_images)), dir_index: Rc::new(Cell::new(dir_index)), ocr: Rc::new(RefCell::new(ocr)), + max_ocr_dimension: Rc::new(Cell::new(max_ocr_dimension)), decode_job_id: Rc::new(Cell::new(0)), ocr_job_id: Rc::new(Cell::new(0)), on_file_loaded: Rc::new(RefCell::new(None)), @@ -92,6 +94,11 @@ impl ViewerController { *self.ocr.borrow_mut() = ocr; } + /// Change the OCR max-dimension guardrail for subsequent loads. + pub fn set_max_ocr_dimension(&self, max: u32) { + self.max_ocr_dimension.set(max); + } + /// Register a callback fired whenever a file finishes loading (or fails). /// /// If a file has already been loaded, the callback is invoked immediately @@ -133,9 +140,10 @@ impl ViewerController { let this = self.clone(); let path = path.to_path_buf(); + let started = std::time::Instant::now(); glib::MainContext::default().spawn_local(async move { let result = crate::decode::decode_texture(&path).await; - this.finish_decode(decode_id, path, name, size_bytes, result); + this.finish_decode(decode_id, path, name, size_bytes, result, started); }); } @@ -147,6 +155,7 @@ impl ViewerController { name: String, size_bytes: Option, result: anyhow::Result, + started: std::time::Instant, ) { if job_id != self.decode_job_id.get() { // Late result for a file the user already navigated away from. @@ -155,6 +164,14 @@ impl ViewerController { match result { Ok(texture) => { + tracing::debug!( + target: "quickview::perf", + elapsed_ms = started.elapsed().as_millis() as u64, + width = texture.width(), + height = texture.height(), + path = %path.display(), + "decode" + ); let info = FileInfo { name, width: texture.width(), @@ -162,9 +179,9 @@ impl ViewerController { size_bytes, load_failed: false, }; - self.overlay.set_texture(texture); + self.overlay.set_texture(texture.clone()); self.emit_file_loaded(info); - self.start_ocr(path); + self.start_ocr(path, &texture); } Err(err) => { tracing::error!("Failed to load image: {err:#}"); @@ -216,12 +233,37 @@ impl ViewerController { self.overlay.copy_selection_to_clipboard(); } - fn start_ocr(&self, path: PathBuf) { + fn start_ocr(&self, path: PathBuf, texture: >k::gdk::Texture) { self.overlay.set_ocr_busy(true); self.overlay.set_ocr_result(None); let ocr_opts = self.ocr.borrow().clone(); + // Max-dimension guardrail: oversized images are fed to tesseract as + // a downscaled temp copy. The plan is pure math; the pixel download + // must happen here on the main thread (see ocr_prep), and a download + // failure degrades to full-resolution OCR — the guardrail is a + // performance measure, not a correctness one. + let plan = downscale::plan_downscale( + texture.width().max(0) as u32, + texture.height().max(0) as u32, + self.max_ocr_dimension.get(), + ); + let prep_started = std::time::Instant::now(); + let prep = plan.and_then(|plan| match crate::ocr_prep::download_rgba(texture) { + Ok(pixels) => Some((pixels, plan)), + Err(err) => { + tracing::warn!("texture download failed; OCR at full resolution: {err:#}"); + None + } + }); + // The effective size tesseract will see, for the cache key. Derived + // from the successful prep, not the plan: a failed prep runs at full + // resolution and must be keyed as such. + let downscale_target = prep + .as_ref() + .map(|(_, plan)| (plan.target_w, plan.target_h)); + let (sender, receiver) = async_channel::bounded::<( u64, anyhow::Result, @@ -241,13 +283,62 @@ impl ViewerController { // which the edited file then correctly misses. let entry = cache_root .as_deref() - .map(|root| cache::ocr_cache_path(root, &path, &ocr_opts)); + .map(|root| cache::ocr_cache_path(root, &path, &ocr_opts, downscale_target)); if let Some(cached) = entry.as_deref().and_then(cache::load_ocr) { tracing::debug!("OCR cache hit for {}", path.display()); return Ok(cached); } - let tsv_out = tesseract::run_tesseract_tsv(&path, &ocr_opts)?; - let parsed = tsv::parse_tesseract_tsv(&tsv_out)?; + + // Materialize the downscaled copy (cache misses only — a hit + // never needs the pixels). The temp file guard must outlive + // the tesseract run; drop deletes it on every path. A write + // failure degrades to full resolution, which stores a + // (strictly better) full-res result under the downscaled key. + let mut ocr_input = path.clone(); + let mut tmp_guard = None; + let mut factors = None; + if let Some((pixels, plan)) = &prep { + match crate::ocr_prep::write_downscaled_png(pixels, plan) { + Ok(downscaled) => { + tracing::debug!( + target: "quickview::perf", + elapsed_ms = prep_started.elapsed().as_millis() as u64, + factor = plan.factor, + target_w = downscaled.target.0, + target_h = downscaled.target.1, + "downscale prep" + ); + ocr_input = downscaled.path().to_path_buf(); + factors = Some((downscaled.factor_x, downscaled.factor_y)); + tmp_guard = Some(downscaled); + } + Err(err) => { + tracing::warn!("downscale failed; OCR at full resolution: {err:#}"); + } + } + } + + let ocr_started = std::time::Instant::now(); + let tsv_out = tesseract::run_tesseract_tsv(&ocr_input, &ocr_opts)?; + let mut parsed = tsv::parse_tesseract_tsv(&tsv_out)?; + tracing::debug!( + target: "quickview::perf", + elapsed_ms = ocr_started.elapsed().as_millis() as u64, + words = parsed.words.len(), + lang = %ocr_opts.lang, + downscaled = tmp_guard.is_some(), + "ocr" + ); + drop(tmp_guard); + + // Bboxes go back to original image space before caching and + // indexing: everything downstream only sees original-space + // coordinates, so cache hits are indistinguishable from + // full-resolution parses. + if let Some((fx, fy)) = factors { + downscale::upscale_bboxes(&mut parsed, fx, fy); + } + // Empty results are cached too (text-free images shouldn't // re-run tesseract); failures are not, so transient errors // retry on the next open. diff --git a/crates/quickview/src/main.rs b/crates/quickview/src/main.rs index beb1c5d..6064b49 100644 --- a/crates/quickview/src/main.rs +++ b/crates/quickview/src/main.rs @@ -27,6 +27,11 @@ struct Cli { #[arg(long)] tessdata_dir: Option, + /// Max image dimension (px) before OCR works on a downscaled copy; + /// 0 disables. Overrides the config file; defaults to 4000. + #[arg(long)] + max_ocr_dim: Option, + /// Image file path. Use '-' (or omit) to read a path from stdin. file: Option, } @@ -53,12 +58,13 @@ fn main() -> Result<()> { // single-instance app, the primary must never re-resolve a remote // invocation's environment or config — only fully resolved values cross // the process boundary (see quickview-ui's ipc module). - let ocr = resolve_ocr_options(cli.lang, cli.tessdata_dir); + let (ocr, max_ocr_dimension) = resolve_ocr_options(cli.lang, cli.tessdata_dir, cli.max_ocr_dim); let code = quickview_ui::run(quickview_ui::LaunchOptions { mode, file: file_path, ocr, + max_ocr_dimension, })?; std::process::exit(code); @@ -72,8 +78,10 @@ fn main() -> Result<()> { fn resolve_ocr_options( cli_lang: Option, cli_tessdata_dir: Option, -) -> quickview_core::ocr::tesseract::OcrOptions { + cli_max_ocr_dim: Option, +) -> (quickview_core::ocr::tesseract::OcrOptions, u32) { use quickview_core::config; + use quickview_core::ocr::downscale::DEFAULT_MAX_OCR_DIMENSION; let cfg = config::config_path() .map(|path| { @@ -85,12 +93,16 @@ fn resolve_ocr_options( .unwrap_or_default(); let env_lang = std::env::var("QUICKVIEW_LANG").ok(); - quickview_core::ocr::tesseract::OcrOptions { + let opts = quickview_core::ocr::tesseract::OcrOptions { lang: config::resolve_lang(cli_lang.as_deref(), env_lang.as_deref(), &cfg), tessdata_dir: non_blank(cli_tessdata_dir) .or_else(|| non_blank(cfg.ocr.tessdata_dir.clone())) .map(absolutize), - } + }; + let max_ocr_dimension = cli_max_ocr_dim + .or(cfg.ocr.max_dimension) + .unwrap_or(DEFAULT_MAX_OCR_DIMENSION); + (opts, max_ocr_dimension) } /// Blank is unset, at each precedence level — same rule as `resolve_lang`. diff --git a/docs/PHASED_PLAN.md b/docs/PHASED_PLAN.md index 32404db..e56c155 100644 --- a/docs/PHASED_PLAN.md +++ b/docs/PHASED_PLAN.md @@ -170,7 +170,9 @@ If priorities change, you can reshuffle phases, but try to keep the “render fi `~/.cache/quickview/ocr/`, checked/written on the OCR worker thread, atomic writes, no eviction in v1. Survives restarts, so Quick Preview's process-per-invocation benefits too. -- Add basic benchmarking hooks (decode + OCR timing) +- Add basic benchmarking hooks ✅ — `tracing` debug events under target + `quickview::perf` (decode, OCR cache hit, downscale prep, tesseract+parse); + opt in with `RUST_LOG=quickview::perf=debug` - Improve OCR accuracy options: - language selection via config file / env var ✅ — precedence `--lang` > `QUICKVIEW_LANG` > `~/.config/quickview/config.toml` > `eng` @@ -180,8 +182,15 @@ If priorities change, you can reshuffle phases, but try to keep the “render fi the fast/best sets have no conventional install location, users clone them anywhere. Joins the OCR cache key per ADR-0009. - Add guardrails: - - maximum image dimensions for OCR (downscale) - - memory usage limits (where feasible) + - maximum image dimensions for OCR ✅ — `[ocr] max_dimension` (default + 4000, 0 disables, `--max-ocr-dim` overrides): oversized images are fed to + tesseract as a downscaled temp PNG produced from the already-decoded + texture (never an in-process re-decode of the untrusted file); word boxes + are mapped back to original image space before caching/indexing, and the + effective target joins the cache key + - memory usage limits — deferred to Phase 8 (the transient RGBA copy during + downscale prep is the only known spike, bounded and measured by the perf + hooks) **Definition of done** - Opening a large image does not freeze or hitch the UI diff --git a/templates/config.example.toml b/templates/config.example.toml index 8aa2ad3..29c4b11 100644 --- a/templates/config.example.toml +++ b/templates/config.example.toml @@ -18,6 +18,7 @@ # Changing this re-runs OCR (it is part of the cache key). #tessdata_dir = "/home/you/src/tessdata_fast" -# Maximum image dimension (px) before OCR works on a downscaled copy. -# Accepted but not consumed yet; the guardrail lands in a follow-up. +# Maximum image dimension (px) before OCR works on a downscaled temporary +# copy (word positions are mapped back to the full-size image). 0 disables +# the guardrail. Overridden by --max-ocr-dim. Default: 4000. #max_dimension = 4000 From 41977cadd7f029628862b4ef1f1900e22601180f Mon Sep 17 00:00:00 2001 From: Babken Egoian <101829110+green2grey@users.noreply.github.com> Date: Mon, 6 Jul 2026 01:30:31 -0700 Subject: [PATCH 2/8] fix: probe the OCR cache before downloading oversized textures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- adrs/ADR-0009-Caching.md | 6 ++- crates/quickview-core/src/cache.rs | 14 +++--- crates/quickview-ui/src/windows/shared.rs | 54 ++++++++++++++--------- 3 files changed, 45 insertions(+), 29 deletions(-) diff --git a/adrs/ADR-0009-Caching.md b/adrs/ADR-0009-Caching.md index 4a20df5..d293c1d 100644 --- a/adrs/ADR-0009-Caching.md +++ b/adrs/ADR-0009-Caching.md @@ -54,9 +54,11 @@ The implementation went **straight to on-disk**, revising the decision above: io.github.Green2Grey2.QuickView) did not move it on Linux. - Tesseract is invoked with no psm/oem flags, so the OCR settings in the key are `lang`, the optional `tessdata_dir` (presence marker keeps `None` and - empty distinct), and the **effective** downscale target (`full` or + empty distinct), and the **planned** downscale target (`full` or `WxH`) — the target rather than the configured `max_dimension` threshold, - so below-threshold images keep their entries across threshold edits. **The + so below-threshold images keep their entries across threshold edits, and + the entry path is derivable before any pixels are prepared (a cache probe + skips the texture download on reopens of oversized images). **The rule stands: any newly configurable OCR setting (psm/oem) must join the key.** - Writes are atomic (temp file + rename in the same directory): concurrent diff --git a/crates/quickview-core/src/cache.rs b/crates/quickview-core/src/cache.rs index fb18d8d..c6fe171 100644 --- a/crates/quickview-core/src/cache.rs +++ b/crates/quickview-core/src/cache.rs @@ -22,12 +22,14 @@ pub fn cache_dir() -> Option { /// Compute the cache entry path for one OCR run. /// -/// `downscale_target` is the *effective* image size tesseract will see: -/// `None` for a full-resolution run, `Some((w, h))` when the max-dimension -/// guardrail feeds it a downscaled copy. Hashing the effective target rather -/// than the configured threshold keeps below-threshold images' entries valid -/// across `max_dimension` edits — per ADR-0009, only inputs that change the -/// recognition output join the key. +/// `downscale_target` is the *planned* image size for tesseract: `None` for +/// a full-resolution run, `Some((w, h))` when the max-dimension guardrail +/// intends to feed it a downscaled copy. Hashing the target rather than the +/// configured threshold keeps below-threshold images' entries valid across +/// `max_dimension` edits — per ADR-0009, only inputs that change the +/// recognition output join the key. (A degraded run that falls back to full +/// resolution still stores under the planned key: strictly better content, +/// and it lets future opens hit the cache without re-preparing pixels.) pub fn ocr_cache_path( cache_root: &Path, file: &Path, diff --git a/crates/quickview-ui/src/windows/shared.rs b/crates/quickview-ui/src/windows/shared.rs index fa55fd5..c040986 100644 --- a/crates/quickview-ui/src/windows/shared.rs +++ b/crates/quickview-ui/src/windows/shared.rs @@ -241,7 +241,7 @@ impl ViewerController { // Max-dimension guardrail: oversized images are fed to tesseract as // a downscaled temp copy. The plan is pure math; the pixel download - // must happen here on the main thread (see ocr_prep), and a download + // must happen here on the main thread (see ocr_prep), and any prep // failure degrades to full-resolution OCR — the guardrail is a // performance measure, not a correctness one. let plan = downscale::plan_downscale( @@ -249,20 +249,33 @@ impl ViewerController { texture.height().max(0) as u32, self.max_ocr_dimension.get(), ); + + // The cache key uses the *planned* downscale target — the size the + // guardrail intends tesseract to see. Deriving the entry up front + // (still before OCR runs, so the mid-edit snapshot semantics below + // are unchanged) lets an existing entry skip the expensive + // main-thread texture download entirely; the entry-path stat is the + // same cheap metadata I/O load_file already does on this thread. + // Degraded runs (failed download or scale) store their + // full-resolution result under the same planned key: strictly better + // content, and future opens then hit it without downloading either. + let downscale_target = plan.map(|p| (p.target_w, p.target_h)); + let entry = cache::cache_dir() + .map(|root| cache::ocr_cache_path(&root, &path, &ocr_opts, downscale_target)); + let probably_cached = entry.as_deref().is_some_and(|e| e.exists()); + let prep_started = std::time::Instant::now(); - let prep = plan.and_then(|plan| match crate::ocr_prep::download_rgba(texture) { - Ok(pixels) => Some((pixels, plan)), - Err(err) => { - tracing::warn!("texture download failed; OCR at full resolution: {err:#}"); - None - } - }); - // The effective size tesseract will see, for the cache key. Derived - // from the successful prep, not the plan: a failed prep runs at full - // resolution and must be keyed as such. - let downscale_target = prep - .as_ref() - .map(|(_, plan)| (plan.target_w, plan.target_h)); + let prep = if probably_cached { + None + } else { + plan.and_then(|plan| match crate::ocr_prep::download_rgba(texture) { + Ok(pixels) => Some((pixels, plan)), + Err(err) => { + tracing::warn!("texture download failed; OCR at full resolution: {err:#}"); + None + } + }) + }; let (sender, receiver) = async_channel::bounded::<( u64, @@ -273,17 +286,16 @@ impl ViewerController { let new_id = self.ocr_job_id.get().wrapping_add(1); self.ocr_job_id.set(new_id); - // All cache I/O stays on the worker thread; hits flow through the same - // channel as fresh results, so the job-id guard applies unchanged. - let cache_root = cache::cache_dir(); + // Cache reads and writes stay on the worker thread; hits flow through + // the same channel as fresh results, so the job-id guard applies + // unchanged. (The main thread only stat()ed the entry path above; the + // authoritative read is here, and its miss path copes without pixels + // by falling back to full resolution.) std::thread::spawn(move || { let r = (|| { - // Snapshot the cache key before OCR runs: if the file is + // The entry was snapshotted before OCR runs: if the file is // edited mid-OCR, the stale result lands under the old key, // which the edited file then correctly misses. - let entry = cache_root - .as_deref() - .map(|root| cache::ocr_cache_path(root, &path, &ocr_opts, downscale_target)); if let Some(cached) = entry.as_deref().and_then(cache::load_ocr) { tracing::debug!("OCR cache hit for {}", path.display()); return Ok(cached); From cb11bb522add09b3dcd21b04848bbcc713517972 Mon Sep 17 00:00:00 2001 From: Babken Egoian <101829110+green2grey@users.noreply.github.com> Date: Mon, 6 Jul 2026 01:40:51 -0700 Subject: [PATCH 3/8] fix: stamp file identity pre-decode and defer OCR prep past first paint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/quickview-core/src/cache.rs | 90 +++++++++++++++++------ crates/quickview-ui/src/windows/shared.rs | 26 ++++++- 2 files changed, 88 insertions(+), 28 deletions(-) diff --git a/crates/quickview-core/src/cache.rs b/crates/quickview-core/src/cache.rs index c6fe171..c053f65 100644 --- a/crates/quickview-core/src/cache.rs +++ b/crates/quickview-core/src/cache.rs @@ -20,8 +20,41 @@ pub fn cache_dir() -> Option { Some(crate::config::project_dirs()?.cache_dir().to_path_buf()) } +/// Snapshot of the file-identity fields that join the OCR cache key. +/// +/// Read the stamp **before** the image content is read (i.e. before decode +/// starts): if the file is replaced afterwards, the derived key belongs to +/// the old content and the edited file simply misses it. Stamping *after* +/// the content was read would let old pixels be stored under the live +/// file's key, which future opens would then wrongly hit. +/// +/// Full nanosecond mtime: whole seconds would alias a same-second rewrite +/// of the same path with an unchanged byte length (rapid screenshot/editor +/// saves). Unreadable metadata stamps as zeros. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct FileStamp { + mtime_nanos: u128, + size: u64, +} + +impl FileStamp { + pub fn read(file: &Path) -> Self { + let meta = std::fs::metadata(file).ok(); + let mtime_nanos = meta + .as_ref() + .and_then(|m| m.modified().ok()) + .and_then(|t| t.duration_since(std::time::UNIX_EPOCH).ok()) + .map(|d| d.as_nanos()) + .unwrap_or(0); + let size = meta.as_ref().map(|m| m.len()).unwrap_or(0); + Self { mtime_nanos, size } + } +} + /// Compute the cache entry path for one OCR run. /// +/// `stamp` must have been read before the image content (see [`FileStamp`]). +/// /// `downscale_target` is the *planned* image size for tesseract: `None` for /// a full-resolution run, `Some((w, h))` when the max-dimension guardrail /// intends to feed it a downscaled copy. Hashing the target rather than the @@ -35,18 +68,12 @@ pub fn ocr_cache_path( file: &Path, opts: &OcrOptions, downscale_target: Option<(u32, u32)>, + stamp: FileStamp, ) -> PathBuf { - // Include file metadata to avoid stale caches. Full nanosecond mtime: - // whole seconds would alias a same-second rewrite of the same path with - // an unchanged byte length (rapid screenshot/editor saves). - let meta = std::fs::metadata(file).ok(); - let mtime = meta - .as_ref() - .and_then(|m| m.modified().ok()) - .and_then(|t| t.duration_since(std::time::UNIX_EPOCH).ok()) - .map(|d| d.as_nanos()) - .unwrap_or(0); - let size = meta.as_ref().map(|m| m.len()).unwrap_or(0); + let FileStamp { + mtime_nanos: mtime, + size, + } = stamp; let mut hasher = blake3::Hasher::new(); hasher.update(file.as_os_str().as_encoded_bytes()); @@ -177,41 +204,56 @@ mod tests { let img = dir.path().join("a.png"); std::fs::write(&img, b"xx").unwrap(); - let base = ocr_cache_path(root, &img, &eng(), None); + let base = ocr_cache_path(root, &img, &eng(), None, FileStamp::read(&img)); // Same inputs -> same key. - assert_eq!(base, ocr_cache_path(root, &img, &eng(), None)); + assert_eq!( + base, + ocr_cache_path(root, &img, &eng(), None, FileStamp::read(&img)) + ); // Different language -> different key. let deu = OcrOptions { lang: "deu".into(), ..eng() }; - assert_ne!(base, ocr_cache_path(root, &img, &deu, None)); + assert_ne!( + base, + ocr_cache_path(root, &img, &deu, None, FileStamp::read(&img)) + ); // Different path -> different key. let img2 = dir.path().join("b.png"); std::fs::write(&img2, b"xx").unwrap(); - assert_ne!(base, ocr_cache_path(root, &img2, &eng(), None)); + assert_ne!( + base, + ocr_cache_path(root, &img2, &eng(), None, FileStamp::read(&img2)) + ); // Different size -> different key. std::fs::write(&img, b"xxxx").unwrap(); - assert_ne!(base, ocr_cache_path(root, &img, &eng(), None)); + assert_ne!( + base, + ocr_cache_path(root, &img, &eng(), None, FileStamp::read(&img)) + ); // Different mtime (same size) -> different key. std::fs::write(&img, b"xx").unwrap(); - let before = ocr_cache_path(root, &img, &eng(), None); + let before = ocr_cache_path(root, &img, &eng(), None, FileStamp::read(&img)); let old = std::time::SystemTime::UNIX_EPOCH + std::time::Duration::from_secs(1_000_000); std::fs::File::open(&img) .unwrap() .set_modified(old) .unwrap(); - assert_ne!(before, ocr_cache_path(root, &img, &eng(), None)); + assert_ne!( + before, + ocr_cache_path(root, &img, &eng(), None, FileStamp::read(&img)) + ); // Subsecond mtime change (same second, same size) -> different key. let with_key = |t| { std::fs::File::open(&img).unwrap().set_modified(t).unwrap(); - ocr_cache_path(root, &img, &eng(), None) + ocr_cache_path(root, &img, &eng(), None, FileStamp::read(&img)) }; assert_ne!( with_key(old + std::time::Duration::from_nanos(1)), @@ -226,7 +268,7 @@ mod tests { let img = dir.path().join("a.png"); std::fs::write(&img, b"xx").unwrap(); - let with_target = |t| ocr_cache_path(root, &img, &eng(), t); + let with_target = |t| ocr_cache_path(root, &img, &eng(), t, FileStamp::read(&img)); // A downscaled run is keyed apart from full resolution, and targets // are keyed apart from each other. @@ -256,7 +298,7 @@ mod tests { lang: "eng".into(), tessdata_dir: d.map(PathBuf::from), }; - ocr_cache_path(root, &img, &opts, None) + ocr_cache_path(root, &img, &opts, None, FileStamp::read(&img)) }; // Some(dir) differs from None, and dirs differ from each other. @@ -279,7 +321,7 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let img = dir.path().join("a.png"); std::fs::write(&img, b"xx").unwrap(); - let entry = ocr_cache_path(dir.path(), &img, &eng(), None); + let entry = ocr_cache_path(dir.path(), &img, &eng(), None, FileStamp::read(&img)); let result = sample_result(); store_ocr(&entry, &result).unwrap(); @@ -312,7 +354,7 @@ mod tests { let img = dir.path().join("a.png"); std::fs::write(&img, b"xx").unwrap(); - let entry = ocr_cache_path(dir.path(), &img, &eng(), None); + let entry = ocr_cache_path(dir.path(), &img, &eng(), None, FileStamp::read(&img)); assert!(load_ocr(&entry).is_none()); } @@ -322,7 +364,7 @@ mod tests { let img = dir.path().join("a.png"); std::fs::write(&img, b"xx").unwrap(); - let entry = ocr_cache_path(dir.path(), &img, &eng(), None); + let entry = ocr_cache_path(dir.path(), &img, &eng(), None, FileStamp::read(&img)); std::fs::create_dir_all(entry.parent().unwrap()).unwrap(); std::fs::write(&entry, b"{not json").unwrap(); diff --git a/crates/quickview-ui/src/windows/shared.rs b/crates/quickview-ui/src/windows/shared.rs index c040986..54e6154 100644 --- a/crates/quickview-ui/src/windows/shared.rs +++ b/crates/quickview-ui/src/windows/shared.rs @@ -125,6 +125,11 @@ impl ViewerController { .map(|s| s.to_string_lossy().into_owned()) .unwrap_or_else(|| path.display().to_string()); let size_bytes = std::fs::metadata(path).ok().map(|m| m.len()); + // Stamp the file identity before the decode reads its content: the + // 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); // Supersede any in-flight decode. let decode_id = self.decode_job_id.get().wrapping_add(1); @@ -143,11 +148,12 @@ impl ViewerController { let started = std::time::Instant::now(); glib::MainContext::default().spawn_local(async move { let result = crate::decode::decode_texture(&path).await; - this.finish_decode(decode_id, path, name, size_bytes, result, started); + this.finish_decode(decode_id, path, name, size_bytes, result, started, stamp); }); } /// Apply a finished decode, unless a newer `load_file` superseded it. + #[allow(clippy::too_many_arguments)] fn finish_decode( &self, job_id: u64, @@ -156,6 +162,7 @@ impl ViewerController { size_bytes: Option, result: anyhow::Result, started: std::time::Instant, + stamp: cache::FileStamp, ) { if job_id != self.decode_job_id.get() { // Late result for a file the user already navigated away from. @@ -181,7 +188,18 @@ impl ViewerController { }; self.overlay.set_texture(texture.clone()); self.emit_file_loaded(info); - self.start_ocr(path, &texture); + // Let the freshly set texture reach the screen before OCR + // prep: an oversized image's pixel download blocks the main + // thread, and glib's default-idle priority runs after GTK's + // redraw, so the first paint isn't hitched by its own OCR. + let this = self.clone(); + glib::idle_add_local_once(move || { + if job_id != this.decode_job_id.get() { + // Superseded while waiting for the idle slot. + return; + } + this.start_ocr(path, &texture, stamp); + }); } Err(err) => { tracing::error!("Failed to load image: {err:#}"); @@ -233,7 +251,7 @@ impl ViewerController { self.overlay.copy_selection_to_clipboard(); } - fn start_ocr(&self, path: PathBuf, texture: >k::gdk::Texture) { + fn start_ocr(&self, path: PathBuf, texture: >k::gdk::Texture, stamp: cache::FileStamp) { self.overlay.set_ocr_busy(true); self.overlay.set_ocr_result(None); @@ -261,7 +279,7 @@ impl ViewerController { // content, and future opens then hit it without downloading either. let downscale_target = plan.map(|p| (p.target_w, p.target_h)); let entry = cache::cache_dir() - .map(|root| cache::ocr_cache_path(&root, &path, &ocr_opts, downscale_target)); + .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()); let prep_started = std::time::Instant::now(); From af588760cc1102c472fb4b248f70e59e8c15f1ca Mon Sep 17 00:00:00 2001 From: Babken Egoian <101829110+green2grey@users.noreply.github.com> Date: Mon, 6 Jul 2026 01:50:25 -0700 Subject: [PATCH 4/8] fix: make the texture download OOM-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/quickview-ui/src/ocr_prep.rs | 42 ++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/crates/quickview-ui/src/ocr_prep.rs b/crates/quickview-ui/src/ocr_prep.rs index 98041cb..6a6c1bc 100644 --- a/crates/quickview-ui/src/ocr_prep.rs +++ b/crates/quickview-ui/src/ocr_prep.rs @@ -34,15 +34,45 @@ pub struct RgbaPixels { /// Download `texture` as unpremultiplied RGBA. Main thread only. pub fn download_rgba(texture: &gdk::Texture) -> Result { + let width = texture.width(); + let height = texture.height(); + let stride = usize::try_from(width) + .ok() + .and_then(|w| w.checked_mul(4)) + .context("image width overflows RGBA stride")?; + let len = usize::try_from(height) + .ok() + .and_then(|h| h.checked_mul(stride)) + .context("image size overflows RGBA buffer")?; + + // Checked allocation first: `download_bytes()` would g_malloc the same + // buffer and abort the whole process on OOM. A failed allocation must + // instead surface as an Err so the caller degrades to full-resolution + // OCR. + let mut buf: Vec = Vec::new(); + buf.try_reserve_exact(len) + .with_context(|| format!("cannot allocate {len} bytes for the RGBA copy"))?; + buf.resize(len, 0); + let mut downloader = gdk::TextureDownloader::new(texture); downloader.set_format(gdk::MemoryFormat::R8g8b8a8); - let (bytes, stride) = downloader.download_bytes(); - let stride = i32::try_from(stride).context("texture stride exceeds i32")?; + // SAFETY: gdk4-rs 0.10 does not bind download_into (only the aborting + // download_bytes). The buffer is exactly `stride * height` bytes with + // `stride == width * 4`, which is what an R8g8b8a8 download writes, and + // the downloader stays alive for the duration of the call. + unsafe { + let ptr: *const gdk::ffi::GdkTextureDownloader = glib::translate::ToGlibPtr::< + *const gdk::ffi::GdkTextureDownloader, + >::to_glib_none(&downloader) + .0; + gdk::ffi::gdk_texture_downloader_download_into(ptr, buf.as_mut_ptr(), stride); + } + Ok(RgbaPixels { - bytes, - width: texture.width(), - height: texture.height(), - stride, + bytes: glib::Bytes::from_owned(buf), + width, + height, + stride: i32::try_from(stride).context("texture stride exceeds i32")?, }) } From b70a7fa4078a6c30b1ff370589d93e9a1b801b93 Mon Sep 17 00:00:00 2001 From: Babken Egoian <101829110+green2grey@users.noreply.github.com> Date: Mon, 6 Jul 2026 01:57:51 -0700 Subject: [PATCH 5/8] fix: bypass the OCR cache when the file changes during decode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/quickview-ui/src/windows/shared.rs | 31 ++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/crates/quickview-ui/src/windows/shared.rs b/crates/quickview-ui/src/windows/shared.rs index 54e6154..0fb50df 100644 --- a/crates/quickview-ui/src/windows/shared.rs +++ b/crates/quickview-ui/src/windows/shared.rs @@ -188,6 +188,22 @@ impl ViewerController { }; self.overlay.set_texture(texture.clone()); self.emit_file_loaded(info); + // Re-stat now that the decode is done: if the file changed + // while the decoder had it open, the texture's provenance is + // ambiguous (old or new bytes), so the cache must sit this + // load out — a hit could paint stale boxes over new pixels + // and a store could file this OCR under a mismatched key. + // Matching nanosecond stamps on both sides of the decode pin + // the decoded version to the key. + let stamp = { + let after = cache::FileStamp::read(&path); + if after != stamp { + tracing::debug!( + "file changed during decode; skipping OCR cache for this load" + ); + } + (after == stamp).then_some(stamp) + }; // Let the freshly set texture reach the screen before OCR // prep: an oversized image's pixel download blocks the main // thread, and glib's default-idle priority runs after GTK's @@ -251,7 +267,12 @@ impl ViewerController { self.overlay.copy_selection_to_clipboard(); } - fn start_ocr(&self, path: PathBuf, texture: >k::gdk::Texture, stamp: cache::FileStamp) { + fn start_ocr( + &self, + path: PathBuf, + texture: >k::gdk::Texture, + stamp: Option, + ) { self.overlay.set_ocr_busy(true); self.overlay.set_ocr_result(None); @@ -278,8 +299,12 @@ impl ViewerController { // full-resolution result under the same planned key: strictly better // content, and future opens then hit it without downloading either. let downscale_target = plan.map(|p| (p.target_w, p.target_h)); - let entry = cache::cache_dir() - .map(|root| cache::ocr_cache_path(&root, &path, &ocr_opts, downscale_target, stamp)); + // No stamp (file changed during decode) means no entry: neither the + // probe below nor the worker's load/store touch the cache this load. + let entry = stamp.and_then(|stamp| { + 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()); let prep_started = std::time::Instant::now(); From 5c47688f8c2e81ea211364ca3d55c4eb77ea74e4 Mon Sep 17 00:00:00 2001 From: Babken Egoian <101829110+green2grey@users.noreply.github.com> Date: Mon, 6 Jul 2026 02:05:41 -0700 Subject: [PATCH 6/8] refactor: download OCR pixels on the worker, after the cache read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/quickview-ui/src/ocr_prep.rs | 14 ++--- crates/quickview-ui/src/windows/shared.rs | 71 ++++++++--------------- 2 files changed, 30 insertions(+), 55 deletions(-) diff --git a/crates/quickview-ui/src/ocr_prep.rs b/crates/quickview-ui/src/ocr_prep.rs index 6a6c1bc..9bb6b7c 100644 --- a/crates/quickview-ui/src/ocr_prep.rs +++ b/crates/quickview-ui/src/ocr_prep.rs @@ -4,14 +4,12 @@ //! temporary image. The pixels come from the already-decoded `gdk::Texture` //! — never from re-decoding the untrusted file in-process, which would //! bypass the glycin sandbox (NFR-002) and miss formats gdk-pixbuf has no -//! loader for. Split across threads: +//! loader for. //! -//! - [`download_rgba`] runs on the **main thread**: GL/dmabuf-backed texture -//! downloads are not reliably thread-safe before GTK 4.12. It is a bounded -//! memcpy, paid only for oversized images. -//! - [`write_downscaled_png`] runs on the OCR worker thread: scales the -//! (trusted, self-produced) RGBA buffer with gdk-pixbuf and encodes it to -//! a private temp PNG for tesseract. +//! Everything here runs on the OCR worker thread, only after the cache +//! misses: GdkTexture is immutable and threadsafe, and both decode backends +//! (glycin, GDK fallback) produce memory textures whose download is a plain +//! copy with no GL involvement. use std::path::PathBuf; @@ -32,7 +30,7 @@ pub struct RgbaPixels { stride: i32, } -/// Download `texture` as unpremultiplied RGBA. Main thread only. +/// Download `texture` as unpremultiplied RGBA. pub fn download_rgba(texture: &gdk::Texture) -> Result { let width = texture.width(); let height = texture.height(); diff --git a/crates/quickview-ui/src/windows/shared.rs b/crates/quickview-ui/src/windows/shared.rs index 0fb50df..6f9476a 100644 --- a/crates/quickview-ui/src/windows/shared.rs +++ b/crates/quickview-ui/src/windows/shared.rs @@ -204,18 +204,7 @@ impl ViewerController { } (after == stamp).then_some(stamp) }; - // Let the freshly set texture reach the screen before OCR - // prep: an oversized image's pixel download blocks the main - // thread, and glib's default-idle priority runs after GTK's - // redraw, so the first paint isn't hitched by its own OCR. - let this = self.clone(); - glib::idle_add_local_once(move || { - if job_id != this.decode_job_id.get() { - // Superseded while waiting for the idle slot. - return; - } - this.start_ocr(path, &texture, stamp); - }); + self.start_ocr(path, &texture, stamp); } Err(err) => { tracing::error!("Failed to load image: {err:#}"); @@ -279,10 +268,9 @@ impl ViewerController { let ocr_opts = self.ocr.borrow().clone(); // Max-dimension guardrail: oversized images are fed to tesseract as - // a downscaled temp copy. The plan is pure math; the pixel download - // must happen here on the main thread (see ocr_prep), and any prep - // failure degrades to full-resolution OCR — the guardrail is a - // performance measure, not a correctness one. + // a downscaled temp copy made from the decoded texture (see + // ocr_prep). Any prep failure degrades to full-resolution OCR — the + // guardrail is a performance measure, not a correctness one. let plan = downscale::plan_downscale( texture.width().max(0) as u32, texture.height().max(0) as u32, @@ -290,35 +278,18 @@ impl ViewerController { ); // The cache key uses the *planned* downscale target — the size the - // guardrail intends tesseract to see. Deriving the entry up front - // (still before OCR runs, so the mid-edit snapshot semantics below - // are unchanged) lets an existing entry skip the expensive - // main-thread texture download entirely; the entry-path stat is the - // same cheap metadata I/O load_file already does on this thread. - // Degraded runs (failed download or scale) store their + // guardrail intends tesseract to see — so it is derivable before any + // pixels are prepared, and a cache hit never touches the pixels at + // all. Degraded runs (failed download or scale) store their // full-resolution result under the same planned key: strictly better - // content, and future opens then hit it without downloading either. + // content, and future opens then hit it like any other entry. let downscale_target = plan.map(|p| (p.target_w, p.target_h)); - // No stamp (file changed during decode) means no entry: neither the - // probe below nor the worker's load/store touch the cache this load. + // No stamp (file changed during decode) means no entry: the worker + // neither reads nor writes the cache for this load. let entry = stamp.and_then(|stamp| { 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()); - - let prep_started = std::time::Instant::now(); - let prep = if probably_cached { - None - } else { - plan.and_then(|plan| match crate::ocr_prep::download_rgba(texture) { - Ok(pixels) => Some((pixels, plan)), - Err(err) => { - tracing::warn!("texture download failed; OCR at full resolution: {err:#}"); - None - } - }) - }; let (sender, receiver) = async_channel::bounded::<( u64, @@ -329,11 +300,14 @@ impl ViewerController { let new_id = self.ocr_job_id.get().wrapping_add(1); self.ocr_job_id.set(new_id); - // Cache reads and writes stay on the worker thread; hits flow through - // the same channel as fresh results, so the job-id guard applies - // unchanged. (The main thread only stat()ed the entry path above; the - // authoritative read is here, and its miss path copes without pixels - // by falling back to full resolution.) + // All cache I/O and all pixel work stays on the worker thread; hits + // flow through the same channel as fresh results, so the job-id + // guard applies unchanged. The texture travels with the closure: + // GdkTexture is immutable and threadsafe (decode.rs already sends + // textures across threads), so the download happens only after the + // authoritative cache read misses — never for a hit, and never on + // the main thread. + let texture = texture.clone(); std::thread::spawn(move || { let r = (|| { // The entry was snapshotted before OCR runs: if the file is @@ -346,14 +320,17 @@ impl ViewerController { // Materialize the downscaled copy (cache misses only — a hit // never needs the pixels). The temp file guard must outlive - // the tesseract run; drop deletes it on every path. A write + // 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(); let mut tmp_guard = None; let mut factors = None; - if let Some((pixels, plan)) = &prep { - match crate::ocr_prep::write_downscaled_png(pixels, plan) { + if let Some(plan) = &plan { + let prep_started = std::time::Instant::now(); + let downscaled = crate::ocr_prep::download_rgba(&texture) + .and_then(|pixels| crate::ocr_prep::write_downscaled_png(&pixels, plan)); + match downscaled { Ok(downscaled) => { tracing::debug!( target: "quickview::perf", From 42db735e08b9ad4f00eda17475fe714700f87723 Mon Sep 17 00:00:00 2001 From: Babken Egoian <101829110+green2grey@users.noreply.github.com> Date: Mon, 6 Jul 2026 02:12:14 -0700 Subject: [PATCH 7/8] fix: discard full-resolution OCR when the file changed under it 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. --- crates/quickview-ui/src/windows/shared.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/crates/quickview-ui/src/windows/shared.rs b/crates/quickview-ui/src/windows/shared.rs index 6f9476a..195ba85 100644 --- a/crates/quickview-ui/src/windows/shared.rs +++ b/crates/quickview-ui/src/windows/shared.rs @@ -361,6 +361,21 @@ impl ViewerController { downscaled = tmp_guard.is_some(), "ocr" ); + + // A full-resolution run had tesseract read the *live* file, + // which may have been replaced since the decode; a stamp + // mismatch means these words describe different bytes than + // the displayed texture (and than the entry's key), so the + // result is dropped — no overlay, nothing stored. Downscaled + // runs OCR the decoded pixels and are immune; a `None` stamp + // already bypasses the cache and stays best-effort. + if tmp_guard.is_none() { + if let Some(stamp) = stamp { + if cache::FileStamp::read(&path) != stamp { + anyhow::bail!("file changed during OCR; discarding mismatched result"); + } + } + } drop(tmp_guard); // Bboxes go back to original image space before caching and From ecfb73c2b5d19f09c0ee0f60f9c4fb0f7027c66e Mon Sep 17 00:00:00 2001 From: Babken Egoian <101829110+green2grey@users.noreply.github.com> Date: Mon, 6 Jul 2026 02:20:53 -0700 Subject: [PATCH 8/8] fix: OCR from decoded pixels when the decode stamp is unknown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/quickview-ui/src/windows/shared.rs | 39 +++++++++++++++++------ 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/crates/quickview-ui/src/windows/shared.rs b/crates/quickview-ui/src/windows/shared.rs index 195ba85..261d07f 100644 --- a/crates/quickview-ui/src/windows/shared.rs +++ b/crates/quickview-ui/src/windows/shared.rs @@ -318,15 +318,30 @@ impl ViewerController { return Ok(cached); } - // Materialize the downscaled copy (cache misses only — a hit - // never needs the pixels). The temp file guard must outlive - // 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. + // Materialize the temp copy tesseract will read (cache + // misses only — a hit never needs the pixels). The temp file + // guard must outlive the tesseract run; drop deletes it on + // every path. + // + // With an unknown stamp (the file changed during decode) the + // live path cannot be trusted to match the displayed texture, + // so the decoded pixels are the only safe OCR input: OCR them + // at original size (factor 1.0) and never fall back to the + // live file. With a known stamp, a prep failure degrades to + // full resolution, which stores a (strictly better) full-res + // result under the downscaled key. + let pixels_only = stamp.is_none(); + let prep_plan = plan.or_else(|| { + pixels_only.then(|| downscale::DownscalePlan { + target_w: texture.width().max(1) as u32, + target_h: texture.height().max(1) as u32, + factor: 1.0, + }) + }); let mut ocr_input = path.clone(); let mut tmp_guard = None; let mut factors = None; - if let Some(plan) = &plan { + if let Some(plan) = &prep_plan { let prep_started = std::time::Instant::now(); let downscaled = crate::ocr_prep::download_rgba(&texture) .and_then(|pixels| crate::ocr_prep::write_downscaled_png(&pixels, plan)); @@ -344,6 +359,11 @@ impl ViewerController { factors = Some((downscaled.factor_x, downscaled.factor_y)); tmp_guard = Some(downscaled); } + Err(err) if pixels_only => { + return Err(err.context( + "cannot OCR from decoded pixels and the live file is untrusted", + )); + } Err(err) => { tracing::warn!("downscale failed; OCR at full resolution: {err:#}"); } @@ -366,9 +386,10 @@ impl ViewerController { // which may have been replaced since the decode; a stamp // mismatch means these words describe different bytes than // the displayed texture (and than the entry's key), so the - // result is dropped — no overlay, nothing stored. Downscaled - // runs OCR the decoded pixels and are immune; a `None` stamp - // already bypasses the cache and stays best-effort. + // result is dropped — no overlay, nothing stored. Pixel-fed + // runs (downscaled, or pixels_only above) OCR the decoded + // texture and are immune, so only the stamped full-resolution + // path needs re-verification. if tmp_guard.is_none() { if let Some(stamp) = stamp { if cache::FileStamp::read(&path) != stamp {