From e869710d8279a70a3912581ae6cf105c5454d1ff Mon Sep 17 00:00:00 2001 From: altair823 Date: Sat, 2 May 2026 05:45:25 +0000 Subject: [PATCH] =?UTF-8?q?review(p6-2):=20=ED=9A=8C=EC=B0=A8=201=20?= =?UTF-8?q?=EC=A7=80=EC=A0=81=20=EB=B0=98=EC=98=81?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - crates/kebab-config/src/lib.rs: • `OcrCfg.endpoint: String` (\"\" sentinel) → `Option` 으로 교체. `#[serde(default)]` 적용. `KEBAB_IMAGE_OCR_ENDPOINT=\"\"` (빈 값) 도 None 으로 매핑하는 분기 추가. • 신규 회귀 테스트 `image_ocr_endpoint_empty_env_value_is_none`. - crates/kebab-parse-image/src/ocr.rs: • `OllamaVisionOcr::new` 의 endpoint fallback 로직을 새 `Option` 스키마에 맞춰 정리 (`as_deref` + match). • `OllamaGenerateResponse` 의 dead `_other: HashMap` 필드 제거. `serde_json::Value` import 도 같이 정리. • `OllamaGenerateRequest.images: Vec<&'a str>` → `[&'a str; 1]` (호출당 vec! 알로케이션 제거, multi-image 는 OcrEngine trait 가 단일 이미지를 받으므로 OOS). • `downscale_to_long_edge` 단일-디코드로 리팩터. PNG passthrough hot path 보존 (header sniff 만으로 분기), 그 외 모든 경로는 decode 1회 + (필요 시) resize + PNG re-encode 1회로 통일. • `pub fn max_pixels(&self) -> u32` accessor 추가 — clamp 결과 검증 용 (단순 inspector). - crates/kebab-parse-image/tests/ocr.rs: • `cfg_for_endpoint` / 통합 테스트가 `Some(endpoint)` 형태로 갱신. • `from_parts_clamps_max_pixels_into_legal_range` 가 새 accessor 로 실제 클램프 결과 (256 / 4096 / 1024) 를 검증하도록 강화. • 통합 테스트가 폰트 부재 시 panic 대신 skip 하도록 분기. - crates/kebab-parse-image/tests/common/mod.rs: • `hello_world_png` 가 `anyhow::Result>` 반환하도록 변경. expect(\"DejaVu Sans Bold required\") 메시지를 \"only the opt-in OCR integration fixture needs this font\" 로 의도 명확화. cargo test -p kebab-parse-image — 28 pass + 1 ignored. cargo test -p kebab-config — 21 pass (+1 회귀). cargo clippy --workspace --all-targets -- -D warnings — pass. Reviewer-suggested workspace.dependencies 통합 (reqwest / base64) 은 P6-3 와 함께 처리할 수 있도록 follow-up 으로 두고 본 PR scope 에서 제외 (회차 1 본문에서 명시). --- crates/kebab-config/src/lib.rs | 39 +++++++-- crates/kebab-parse-image/src/ocr.rs | 87 +++++++++++--------- crates/kebab-parse-image/tests/common/mod.rs | 25 +++--- crates/kebab-parse-image/tests/ocr.rs | 40 ++++++--- 4 files changed, 125 insertions(+), 66 deletions(-) diff --git a/crates/kebab-config/src/lib.rs b/crates/kebab-config/src/lib.rs index 73ab144..f9a737c 100644 --- a/crates/kebab-config/src/lib.rs +++ b/crates/kebab-config/src/lib.rs @@ -136,10 +136,11 @@ pub struct OcrCfg { /// Model id passed to the engine (e.g. `"gemma4:e4b"` for /// Ollama-vision). pub model: String, - /// HTTP endpoint for the OCR engine. Empty string means "fall back - /// to `models.llm.endpoint`" — convenient when the same Ollama - /// host serves both LLM and vision. - pub endpoint: String, + /// HTTP endpoint for the OCR engine. `None` (or a missing key in + /// TOML) means "fall back to `models.llm.endpoint`" — convenient + /// when the same Ollama host serves both LLM and vision. + #[serde(default)] + pub endpoint: Option, /// BCP-47 language hints (e.g. `["eng", "kor"]`). The adapter /// renders them into the prompt; the LLM honours them probabilistically. pub languages: Vec, @@ -154,7 +155,7 @@ impl OcrCfg { enabled: false, engine: "ollama-vision".to_string(), model: "gemma4:e4b".to_string(), - endpoint: String::new(), + endpoint: None, languages: vec!["eng".to_string(), "kor".to_string()], max_pixels: 1600, } @@ -393,7 +394,15 @@ impl Config { } "KEBAB_IMAGE_OCR_ENGINE" => self.image.ocr.engine = v.clone(), "KEBAB_IMAGE_OCR_MODEL" => self.image.ocr.model = v.clone(), - "KEBAB_IMAGE_OCR_ENDPOINT" => self.image.ocr.endpoint = v.clone(), + "KEBAB_IMAGE_OCR_ENDPOINT" => { + // Empty env value is treated the same as "fall back + // to models.llm.endpoint" — i.e. set None. + self.image.ocr.endpoint = if v.is_empty() { + None + } else { + Some(v.clone()) + }; + } "KEBAB_IMAGE_OCR_LANGUAGES" => { // Comma-separated list, e.g. "eng,kor". self.image.ocr.languages = v @@ -578,6 +587,8 @@ mod tests { "KEBAB_IMAGE_OCR_ENDPOINT".to_string(), "http://192.168.0.47:11434".to_string(), ); + // Empty env value should map to None (= fall back to llm.endpoint). + // We exercise that branch in a separate test. env.insert( "KEBAB_IMAGE_OCR_LANGUAGES".to_string(), "eng, kor, jpn".to_string(), @@ -586,7 +597,10 @@ mod tests { let c = Config::defaults().apply_env(&env); assert!(c.image.ocr.enabled); assert_eq!(c.image.ocr.model, "gemma4:31b"); - assert_eq!(c.image.ocr.endpoint, "http://192.168.0.47:11434"); + assert_eq!( + c.image.ocr.endpoint.as_deref(), + Some("http://192.168.0.47:11434") + ); assert_eq!(c.image.ocr.languages, vec!["eng", "kor", "jpn"]); assert_eq!(c.image.ocr.max_pixels, 2048); } @@ -594,6 +608,17 @@ mod tests { /// Pre-P6 config files don't have an `[image]` section. The /// `#[serde(default)]` attribute on `Config::image` must let those /// files load with `ImageCfg::defaults()` instead of erroring. + /// `KEBAB_IMAGE_OCR_ENDPOINT=""` (empty value) should map to `None` + /// rather than to `Some("")` so the fallback to `models.llm.endpoint` + /// kicks in. Covers the env-equivalent of a missing TOML key. + #[test] + fn image_ocr_endpoint_empty_env_value_is_none() { + let mut env = HashMap::new(); + env.insert("KEBAB_IMAGE_OCR_ENDPOINT".to_string(), String::new()); + let c = Config::defaults().apply_env(&env); + assert_eq!(c.image.ocr.endpoint, None); + } + #[test] fn pre_p6_config_without_image_section_loads_with_defaults() { let toml_text = r#" diff --git a/crates/kebab-parse-image/src/ocr.rs b/crates/kebab-parse-image/src/ocr.rs index 07912e9..c20abfe 100644 --- a/crates/kebab-parse-image/src/ocr.rs +++ b/crates/kebab-parse-image/src/ocr.rs @@ -34,7 +34,6 @@ use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use image::{ImageFormat, ImageReader}; use kebab_core::{ImageRefBlock, Lang, OcrRegion, OcrText, ProvenanceEvent, ProvenanceKind}; use serde::{Deserialize, Serialize}; -use serde_json::Value; use time::OffsetDateTime; /// Engine name written into `OcrText.engine` for the Ollama-vision adapter. @@ -135,10 +134,9 @@ impl OllamaVisionOcr { /// happens inside [`OcrEngine::recognize`]. pub fn new(config: &kebab_config::Config) -> Result { let ocr = &config.image.ocr; - let endpoint = if ocr.endpoint.is_empty() { - config.models.llm.endpoint.clone() - } else { - ocr.endpoint.clone() + let endpoint = match ocr.endpoint.as_deref() { + Some(s) if !s.is_empty() => s.to_string(), + _ => config.models.llm.endpoint.clone(), }; if endpoint.is_empty() { anyhow::bail!( @@ -184,6 +182,14 @@ impl OllamaVisionOcr { }) } + /// Effective `max_pixels` after the `[MIN_LONG_EDGE, MAX_LONG_EDGE]` + /// clamp. Exposed so tests can verify the clamp result without + /// reaching into the private field; production callers don't need + /// it. + pub fn max_pixels(&self) -> u32 { + self.max_pixels + } + fn build_prompt(&self, lang_hint: Option<&Lang>) -> String { let langs = if self.languages.is_empty() { "any".to_string() @@ -228,7 +234,7 @@ impl OcrEngine for OllamaVisionOcr { let body = OllamaGenerateRequest { model: &self.model, prompt: &prompt, - images: vec![&b64], + images: [b64.as_str()], stream: false, options: OllamaOptions { temperature: 0.0, @@ -297,9 +303,11 @@ impl OcrEngine for OllamaVisionOcr { /// Decode `bytes`, downscale so the long edge is at most `max_long_edge`, /// and re-encode as PNG. Returns `(png_bytes, final_w, final_h)`. /// -/// Bypasses encode work when the source already fits — we simply pass -/// the bytes through. PNG re-encode is only paid when downscaling is -/// actually needed. +/// PNG sources that already fit the cap are passthrough (zero decodes, +/// just a `Vec` clone). Every other path decodes the image exactly +/// once: the cheap header sniff peeks at the format / dimensions before +/// committing to a decode, so non-PNG passthrough and downscale share +/// the same `decode → optionally resize → re-encode` tail. fn downscale_to_long_edge(bytes: &[u8], max_long_edge: u32) -> Result<(Vec, u32, u32)> { let reader = ImageReader::new(Cursor::new(bytes)) .with_guessed_format() @@ -310,40 +318,39 @@ fn downscale_to_long_edge(bytes: &[u8], max_long_edge: u32) -> Result<(Vec, .context("reading image dimensions for OCR")?; let long = w.max(h); - if long <= max_long_edge { - // Source fits — avoid the round-trip through `image::DynamicImage`. - // Re-encode only when the source isn't already PNG, since the - // wire format we send Ollama is PNG. - return match format { - Some(ImageFormat::Png) => Ok((bytes.to_vec(), w, h)), - _ => { - let img = ImageReader::new(Cursor::new(bytes)) - .with_guessed_format() - .context("re-reading image for PNG re-encode")? - .decode() - .context("decoding image for PNG re-encode")?; - let mut out = Cursor::new(Vec::new()); - img.write_to(&mut out, ImageFormat::Png) - .context("re-encoding image as PNG")?; - Ok((out.into_inner(), w, h)) - } - }; + + // Hot path — PNG within budget already matches the wire format we + // send Ollama, so we ship the bytes verbatim without paying for a + // decode + re-encode round-trip. + if long <= max_long_edge && format == Some(ImageFormat::Png) { + return Ok((bytes.to_vec(), w, h)); } - let scale = max_long_edge as f32 / long as f32; - let new_w = ((w as f32) * scale).round().max(1.0) as u32; - let new_h = ((h as f32) * scale).round().max(1.0) as u32; + // Every remaining branch needs the pixels — either to re-encode as + // PNG (non-PNG within budget) or to resize first (over budget). + // One decode covers both. let img = ImageReader::new(Cursor::new(bytes)) .with_guessed_format() - .context("re-reading image for downscale")? + .context("re-reading image for OCR decode")? .decode() - .context("decoding image for downscale")?; - let resized = img.resize_exact(new_w, new_h, image::imageops::FilterType::Triangle); + .context("decoding image for OCR")?; + + let (final_w, final_h, final_img) = if long <= max_long_edge { + (w, h, img) + } else { + let scale = max_long_edge as f32 / long as f32; + let new_w = ((w as f32) * scale).round().max(1.0) as u32; + let new_h = ((h as f32) * scale).round().max(1.0) as u32; + let resized = + img.resize_exact(new_w, new_h, image::imageops::FilterType::Triangle); + (new_w, new_h, resized) + }; + let mut out = Cursor::new(Vec::new()); - resized + final_img .write_to(&mut out, ImageFormat::Png) - .context("encoding downscaled image as PNG")?; - Ok((out.into_inner(), new_w, new_h)) + .context("encoding image as PNG for OCR")?; + Ok((out.into_inner(), final_w, final_h)) } fn truncate(s: &str, n: usize) -> String { @@ -361,7 +368,11 @@ fn truncate(s: &str, n: usize) -> String { struct OllamaGenerateRequest<'a> { model: &'a str, prompt: &'a str, - images: Vec<&'a str>, + /// Always exactly one image — the `OcrEngine` trait takes a single + /// `&[u8]`, so multi-image batching is out of scope until a future + /// trait extension. Fixed-size array avoids the `vec![]` + /// allocation per call. + images: [&'a str; 1], stream: bool, options: OllamaOptions, } @@ -378,8 +389,6 @@ struct OllamaGenerateResponse { response: Option, #[serde(default)] error: Option, - #[serde(flatten)] - _other: std::collections::HashMap, } #[cfg(test)] diff --git a/crates/kebab-parse-image/tests/common/mod.rs b/crates/kebab-parse-image/tests/common/mod.rs index 9be6f06..7c8e8e0 100644 --- a/crates/kebab-parse-image/tests/common/mod.rs +++ b/crates/kebab-parse-image/tests/common/mod.rs @@ -60,18 +60,23 @@ pub fn large_blue_4000x3000_png() -> Vec { /// `ocr_integration_real_ollama_transcribes_text` integration test — /// regular hermetic tests never call it. /// -/// Renders with a font shipped by `dejavu` (path under -/// `/usr/share/fonts/truetype/dejavu/`) — common across most Linux dev -/// boxes. Falls back to a tiny built-in glyph map if the font is -/// missing so the helper compiles even without DejaVu installed. -pub fn hello_world_png() -> Vec { +/// Returns `Err` (not panic) if the DejaVu Sans Bold font is missing +/// from the standard Linux path, so dev boxes without the font can +/// gracefully skip the integration test rather than crashing the +/// process. +pub fn hello_world_png() -> anyhow::Result> { use ab_glyph::{Font, FontRef, ScaleFont}; + use anyhow::Context; let mut img: ImageBuffer, _> = ImageBuffer::from_fn(400, 100, |_, _| Rgb([255, 255, 255])); - let font_bytes = std::fs::read("/usr/share/fonts/truetype/dejavu/DejaVuSans-Bold.ttf") - .expect("DejaVu Sans Bold required for OCR integration fixture"); - let font = FontRef::try_from_slice(&font_bytes).expect("font parses"); + let font_path = "/usr/share/fonts/truetype/dejavu/DejaVuSans-Bold.ttf"; + let font_bytes = std::fs::read(font_path).with_context(|| { + format!( + "{font_path} not found — only the opt-in OCR integration fixture needs this font" + ) + })?; + let font = FontRef::try_from_slice(&font_bytes).context("DejaVu font parses")?; let scaled = font.as_scaled(40.0); let text = "Hello World 2026"; let mut x = 10.0_f32; @@ -93,8 +98,8 @@ pub fn hello_world_png() -> Vec { } let mut buf = Cursor::new(Vec::new()); img.write_to(&mut buf, image::ImageFormat::Png) - .expect("encoding hello-world PNG must not fail"); - buf.into_inner() + .context("encoding hello-world PNG")?; + Ok(buf.into_inner()) } /// JPEG with embedded EXIF APP1 segment carrying GPS + Make + Model + diff --git a/crates/kebab-parse-image/tests/ocr.rs b/crates/kebab-parse-image/tests/ocr.rs index 7cc0314..149cfd9 100644 --- a/crates/kebab-parse-image/tests/ocr.rs +++ b/crates/kebab-parse-image/tests/ocr.rs @@ -20,7 +20,7 @@ use crate::common::red_100x50_png; fn cfg_for_endpoint(endpoint: &str) -> Config { let mut cfg = Config::defaults(); - cfg.image.ocr.endpoint = endpoint.to_string(); + cfg.image.ocr.endpoint = Some(endpoint.to_string()); cfg.image.ocr.model = "gemma4:e4b".to_string(); cfg.image.ocr.languages = vec!["eng".to_string(), "kor".to_string()]; cfg.image.ocr.max_pixels = 1024; @@ -321,13 +321,26 @@ async fn ocr_downscales_large_image_before_sending() { #[test] fn from_parts_clamps_max_pixels_into_legal_range() { + // Below MIN_LONG_EDGE — bumped up to the floor. let too_small = OllamaVisionOcr::from_parts("http://x", "m", vec![], 10).unwrap(); - let too_big = OllamaVisionOcr::from_parts("http://x", "m", vec![], 99_999).unwrap(); - // We can't read the private field directly, but engine_version is - // observable; assert the engine constructed at all (clamp didn't - // panic) and run a synthetic prompt. - assert_eq!(too_small.engine_name(), "ollama-vision"); - assert_eq!(too_big.engine_name(), "ollama-vision"); + assert_eq!( + too_small.max_pixels(), + 256, + "max_pixels must be raised to MIN_LONG_EDGE" + ); + + // Above MAX_LONG_EDGE — capped at the ceiling. + let too_big = + OllamaVisionOcr::from_parts("http://x", "m", vec![], 99_999).unwrap(); + assert_eq!( + too_big.max_pixels(), + 4096, + "max_pixels must be capped at MAX_LONG_EDGE" + ); + + // Inside the legal range — pass through untouched. + let in_range = OllamaVisionOcr::from_parts("http://x", "m", vec![], 1024).unwrap(); + assert_eq!(in_range.max_pixels(), 1024); } // ── Integration test against real Ollama (opt-in) ──────────────────────── @@ -355,11 +368,18 @@ async fn ocr_integration_real_ollama_transcribes_text() { let model = std::env::var("KEBAB_IMAGE_OCR_MODEL").unwrap_or_else(|_| "gemma4:e4b".to_string()); - // Generate a fixture with known text. - let bytes = common::hello_world_png(); + // Generate a fixture with known text. If the DejaVu font is + // missing from this dev box, skip rather than crash. + let bytes = match common::hello_world_png() { + Ok(b) => b, + Err(e) => { + eprintln!("skipping ocr_integration: {e:#}"); + return; + } + }; let cfg = { let mut c = Config::defaults(); - c.image.ocr.endpoint = endpoint; + c.image.ocr.endpoint = Some(endpoint); c.image.ocr.model = model; c.image.ocr.max_pixels = 1024; c