diff --git a/crates/image_viewer/src/image_viewer.rs b/crates/image_viewer/src/image_viewer.rs index 0cfced3e56..beed6e47a6 100644 --- a/crates/image_viewer/src/image_viewer.rs +++ b/crates/image_viewer/src/image_viewer.rs @@ -2,7 +2,8 @@ mod image_info; mod image_viewer_settings; use std::{ - cell::RefCell, collections::HashMap, ops::RangeInclusive, path::Path, rc::Rc, sync::Arc, + cell::RefCell, collections::{HashMap, HashSet}, ops::RangeInclusive, path::Path, rc::Rc, + sync::Arc, }; use anyhow::Context as _; @@ -113,6 +114,17 @@ const MAX_ZOOM: f32 = 20.0; const ZOOM_STEP: f32 = 1.1; const SCROLL_LINE_MULTIPLIER: f32 = 20.0; +/// Rasterizations are quantized to these zoom steps: continuous wheel zoom then costs +/// one render per crossed step instead of one per wheel tick, and the layout scales the +/// bitmap to the exact zoom in between. +const ZOOM_BUCKET_STEP: f32 = 1.25; + +fn zoom_bucket(zoom: f32) -> f32 { + ZOOM_BUCKET_STEP + .powf(zoom.log(ZOOM_BUCKET_STEP).round()) + .clamp(MIN_ZOOM, MAX_ZOOM) +} + /// Scroll offset that keeps the document point under `anchor_rel` (offset from the /// viewport top/left on one axis) stationary while the zoom scales by `zoom_ratio`. /// `max_offset` and `viewport_extent` describe the pre-zoom layout on that axis; the @@ -329,6 +341,11 @@ pub struct ImageView { thumbnail_scroll_handle: gpui::ScrollHandle, pdf_thumbnail_cache: PageLruCache, pdf_page_cache: PageLruCache, + /// Page/zoom keys with a rasterization already running in the background. + pdf_render_inflight: HashSet, + /// Rasterizations the cache refused (frame larger than the budget); never retried, + /// so an oversized frame cannot turn into a render loop. + pdf_render_refused: HashSet, } struct DisplayedImage { @@ -483,6 +500,8 @@ impl ImageView { thumbnail_scroll_handle: gpui::ScrollHandle::new(), pdf_thumbnail_cache: PageLruCache::new(DEFAULT_MEMORY_BUDGET_BYTES), pdf_page_cache: PageLruCache::new(DEFAULT_MEMORY_BUDGET_BYTES), + pdf_render_inflight: HashSet::new(), + pdf_render_refused: HashSet::new(), } } @@ -3005,50 +3024,95 @@ impl ImageView { fn get_or_render_page_image( &mut self, - pdf_bytes: &[u8], + pdf_bytes: Arc>, page_index: usize, + cx: &mut Context, ) -> Option> { - let key = CacheKey::new(page_index, self.zoom_level, false); + // Rasterize at the bucket zoom, not the exact one, so continuous wheel zoom + // costs one render per crossed step; the layout scales the bitmap anyway. + let bucket_zoom = zoom_bucket(self.zoom_level); + let key = CacheKey::new(page_index, bucket_zoom, false); if let Some(cached) = self.pdf_page_cache.get(&key) { return Some(cached.image); } - let engine = PdfiumEngine::new(); - let options = RasterizerOptions { - target_dpi: 144.0, - zoom_factor: self.zoom_level, - dark_mode: false, - saturation_threshold: 0.18, - rotation_degrees: self.page_rotation, - }; - - if let Ok(rendered) = engine.render_page_from_bytes(pdf_bytes, page_index, options) { - let byte_size = rendered.byte_size(); - // `RenderImage` carries BGRA bytes (see `gpui::assets`); like every other - // decode path, swap R/B before handing pdfium's RGBA output to the atlas. - let mut frame_buffer = rendered.rgba_buffer.as_ref().clone(); - for pixel in frame_buffer.chunks_exact_mut(4) { - pixel.swap(0, 2); - } - if let Some(img_buf) = image::ImageBuffer::, _>::from_raw( - rendered.width, - rendered.height, - frame_buffer, - ) { - let render_image = - Arc::new(gpui::RenderImage::new(vec![image::Frame::new(img_buf)])); - self.pdf_page_cache.insert( - key, - CachedPageImage { - image: render_image.clone(), - byte_size, - }, - ); - return Some(render_image); - } + // Show the closest already-rendered zoom scaled to the layout size while the + // fresh bitmap renders in the background. + let fallback = self + .pdf_page_cache + .closest_to_zoom(page_index, bucket_zoom) + .map(|cached| cached.image.clone()); + if self.pdf_render_refused.contains(&key) { + return fallback; } - None + if self.pdf_render_inflight.insert(key) { + let rotation = self.page_rotation; + cx.spawn(async move |this, cx| { + let rendered = cx + .background_spawn(async move { + PdfiumEngine::new().render_page_from_bytes( + &pdf_bytes, + page_index, + RasterizerOptions { + target_dpi: 144.0, + zoom_factor: bucket_zoom, + dark_mode: false, + saturation_threshold: 0.18, + rotation_degrees: rotation, + }, + ) + }) + .await; + + this.update(cx, |this, cx| { + this.pdf_render_inflight.remove(&key); + let rendered = match rendered { + Ok(rendered) => rendered, + Err(error) => { + log::error!("Failed to render PDF page {page_index}: {error:#}"); + return; + } + }; + let byte_size = rendered.byte_size(); + // `RenderImage` carries BGRA bytes (see `gpui::assets`); like every + // other decode path, swap R/B before building the frame. + let mut frame_buffer = rendered.rgba_buffer.as_ref().clone(); + for pixel in frame_buffer.chunks_exact_mut(4) { + pixel.swap(0, 2); + } + let Some(img_buf) = image::ImageBuffer::, _>::from_raw( + rendered.width, + rendered.height, + frame_buffer, + ) else { + return; + }; + this.pdf_page_cache.insert( + key, + CachedPageImage { + image: Arc::new(gpui::RenderImage::new(vec![image::Frame::new( + img_buf, + )])), + byte_size, + }, + ); + if !this.pdf_page_cache.contains(&key) { + // The cache refused the frame (larger than the budget); never + // re-render this key, or completion would respawn forever. + this.pdf_render_refused.insert(key); + log::warn!( + "PDF page {page_index} at zoom {bucket_zoom} exceeded the page cache budget; keeping scaled fallback" + ); + } + cx.notify(); + }) + .ok(); + }) + .detach(); + } + + fallback } } @@ -3389,6 +3453,8 @@ impl Item for ImageView { thumbnail_scroll_handle: gpui::ScrollHandle::new(), pdf_thumbnail_cache: PageLruCache::new(DEFAULT_MEMORY_BUDGET_BYTES), pdf_page_cache: PageLruCache::new(DEFAULT_MEMORY_BUDGET_BYTES), + pdf_render_inflight: HashSet::new(), + pdf_render_refused: HashSet::new(), }))) } @@ -3524,7 +3590,7 @@ impl Render for ImageView { page_images.entry(page_idx) { if let Some(img) = - self.get_or_render_page_image(&info.pdf_bytes, page_idx) + self.get_or_render_page_image(info.pdf_bytes.clone(), page_idx, cx) { e.insert(img); } @@ -5663,6 +5729,78 @@ mod tests { out } + #[gpui::test] + async fn test_pdf_zoom_renders_asynchronously(cx: &mut TestAppContext) { + init_test(cx); + let pdf_bytes = build_annotation_fixture(); + let fs = FakeFs::new(cx.executor()); + fs.create_dir(Path::new("/root")) + .await + .expect("test root should be created"); + fs.insert_file("/root/fixture.pdf", pdf_bytes).await; + + let project = Project::test(fs, [Path::new("/root")], cx).await; + let worktree_id = cx.update(|cx| { + project + .read(cx) + .worktrees(cx) + .next() + .expect("test project should contain a worktree") + .read(cx) + .id() + }); + let image_item = project + .update(cx, |project, cx| { + project.open_image( + ProjectPath { + worktree_id, + path: rel_path("fixture.pdf").into(), + }, + cx, + ) + }) + .await + .expect("test pdf should open"); + + let (image_view, cx) = cx.add_window_view(|window, cx| { + ImageView::new(image_item.clone(), project.clone(), window, cx) + }); + cx.run_until_parked(); + cx.update(|window, _| window.refresh()); + cx.run_until_parked(); + + // The initial page rasterized into the cache at the bucketed zoom. + let initial_zoom = cx.read(|cx| image_view.read(cx).zoom_level); + let cached = cx.read(|cx| { + image_view + .read(cx) + .pdf_page_cache + .closest_to_zoom(0, zoom_bucket(initial_zoom)) + .map(|cached| cached.byte_size()) + }); + assert!( + cached.is_some_and(|size| size > 0), + "initial page should render into the page cache" + ); + + // Jumping exactly two bucket steps queues a background rasterization at the + // new bucket; after parking, both buckets are cached (the old one stays for + // fallback display during future zooms). + cx.update(|window, cx| { + image_view.update(cx, |view, cx| { + view.set_zoom(initial_zoom * ZOOM_BUCKET_STEP * ZOOM_BUCKET_STEP, None, cx); + }); + window.refresh(); + }); + cx.run_until_parked(); + + let cache_len = cx.read(|cx| image_view.read(cx).pdf_page_cache.len()); + assert!( + cache_len >= 2, + "zooming to a new bucket should cache a second rasterization, cache has {cache_len}" + ); + } + #[gpui::test] async fn test_pdf_wheel_zoom_and_middle_drag_pan(cx: &mut TestAppContext) { init_test(cx); diff --git a/crates/pdf_viewer/src/cache.rs b/crates/pdf_viewer/src/cache.rs index 2cea0f5e05..3a2fb82ec9 100644 --- a/crates/pdf_viewer/src/cache.rs +++ b/crates/pdf_viewer/src/cache.rs @@ -191,6 +191,26 @@ impl PageLruCache { false } + /// Returns true if an entry with exactly this key is currently cached. + pub fn contains(&self, key: &CacheKey) -> bool { + self.entries.contains_key(key) + } + + /// Returns the cached page for `page_index` whose zoom is closest to `zoom_factor`, + /// ignoring LRU order, so a zoom change can keep showing the previous bitmap while a + /// fresh one renders. + pub fn closest_to_zoom(&self, page_index: usize, zoom_factor: f32) -> Option<&T> { + self.entries + .iter() + .filter(|(key, _)| key.page_index == page_index) + .min_by(|(a, _), (b, _)| { + let distance_a = (a.zoom_bucket as f32 - zoom_factor * 100.0).abs(); + let distance_b = (b.zoom_bucket as f32 - zoom_factor * 100.0).abs(); + distance_a.total_cmp(&distance_b) + }) + .map(|(_, value)| value) + } + /// Clears all entries from the cache and resets memory tracking. pub fn clear(&mut self) { self.entries.clear(); diff --git a/crates/pdf_viewer/src/pdfium.rs b/crates/pdf_viewer/src/pdfium.rs index f073f36325..dc95883564 100644 --- a/crates/pdf_viewer/src/pdfium.rs +++ b/crates/pdf_viewer/src/pdfium.rs @@ -35,7 +35,11 @@ static GLOBAL_PDFIUM: OnceLock>> = OnceLock::new(); /// Maximum dimension (width or height) allowed when rasterizing a PDF page, /// preventing out-of-memory denial of service attacks from malicious documents. -pub const MAX_PAGE_DIMENSION: f32 = 8192.0; +/// +/// 4096 keeps the worst-case frame at ~67MB RGBA: beyond roughly 4x oversampling of +/// a 1080p viewport, bigger bitmaps only burn render time and memory without visible +/// sharpness gains (tile-based rendering would be the real answer at extreme zoom). +pub const MAX_PAGE_DIMENSION: f32 = 4096.0; /// Computes target pixel dimensions preserving the page aspect ratio, /// scaling down uniformly if either dimension exceeds `MAX_PAGE_DIMENSION`. @@ -174,10 +178,16 @@ impl PdfiumEngine { PathBuf::from("/usr/local/lib/libpdfium.dylib"), ]); - // macOS app bundles ship libpdfium.dylib in Contents/Frameworks. + // Resolve relative to the running executable: bundled layouts place the engine + // in `/lib/` next to `libexec/`, and a desktop launch gives no guarantee + // about the working directory. if let Ok(executable_path) = std::env::current_exe() { if let Some(executable_dir) = executable_path.parent() { common_paths.push(executable_dir.join("../Frameworks/libpdfium.dylib")); + for name in ["libpdfium.so", "libpdfium.dylib", "pdfium.dll"] { + common_paths.push(executable_dir.join("../lib").join(name)); + common_paths.push(executable_dir.join(name)); + } } } @@ -2203,14 +2213,14 @@ impl PdfiumEngine { page_index: usize, rects: &[(f32, f32, f32, f32)], color: PdfAnnotationColor, - ) -> Result> { + ) -> Result> { if rects.is_empty() { return Ok(bytes.to_vec()); } let guard = self.inner.lock(); let Some(pdfium) = guard.0.as_ref() else { - return Ok(bytes.to_vec()); + anyhow::bail!("PDF highlight editing requires the native Pdfium engine"); }; let bindings = pdfium.bindings(); @@ -2309,7 +2319,7 @@ impl PdfiumEngine { let guard = self.inner.lock(); let Some(pdfium) = guard.0.as_ref() else { - return Ok(bytes.to_vec()); + anyhow::bail!("PDF ink annotation editing requires the native Pdfium engine"); }; let bindings = pdfium.bindings(); @@ -2506,7 +2516,7 @@ impl PdfiumEngine { ) -> Result> { let guard = self.inner.lock(); let Some(pdfium) = guard.0.as_ref() else { - return Ok(bytes.to_vec()); + anyhow::bail!("PDF annotation deletion requires the native Pdfium engine"); }; let bindings = pdfium.bindings(); @@ -2631,6 +2641,47 @@ mod tests { assert_eq!(page.height_pt, 792.0); } + #[test] + fn test_annotation_edits_fail_without_native_library() { + let engine = PdfiumEngine { + inner: Arc::new(Mutex::new(SerializedPdfium(None))), + }; + let dummy_pdf = b"%PDF-1.7\nSample"; + let color = PdfAnnotationColor { + red: 255, + green: 0, + blue: 0, + }; + + // The engine must not silently no-op: edits without a native engine return + // errors so callers never commit unchanged bytes as if a change had landed. + let ink = engine.add_ink_annotation( + dummy_pdf, + 0, + &[vec![(10.0, 10.0), (20.0, 20.0)]], + color, + 2.0, + ); + assert!(ink.is_err(), "ink editing must fail without native pdfium"); + + let highlight = engine.add_highlight_annotation( + dummy_pdf, + 0, + &[(10.0, 10.0, 20.0, 20.0)], + color, + ); + assert!( + highlight.is_err(), + "highlight editing must fail without native pdfium" + ); + + let deletion = engine.delete_annotation(dummy_pdf, 0, 0); + assert!( + deletion.is_err(), + "annotation deletion must fail without native pdfium" + ); + } + #[test] fn test_extract_outline_without_native_library() { let engine = PdfiumEngine { @@ -2674,11 +2725,11 @@ mod tests { #[test] fn test_max_page_dimension_bounds() { - assert_eq!(MAX_PAGE_DIMENSION, 8192.0); + assert_eq!(MAX_PAGE_DIMENSION, 4096.0); // Verify clamping logic for extreme/malicious dimensions let huge_dim: f32 = 100_000.0; let clamped = huge_dim.clamp(1.0, MAX_PAGE_DIMENSION); - assert_eq!(clamped, 8192.0); + assert_eq!(clamped, 4096.0); let tiny_dim: f32 = -50.0; let clamped_tiny = tiny_dim.clamp(1.0, MAX_PAGE_DIMENSION); @@ -2686,13 +2737,13 @@ mod tests { // Verify uniform aspect ratio scaling under extreme dimensions let (w, h) = compute_target_dimensions(10_000.0, 2_000.0, 72.0, 1.0); - assert_eq!(w, 8192); - assert_eq!(h, 1638); + assert_eq!(w, 4096); + assert_eq!(h, 819); assert!(((w as f32 / h as f32) - 5.0).abs() < 0.01); let (w2, h2) = compute_target_dimensions(2_000.0, 10_000.0, 72.0, 1.0); - assert_eq!(w2, 1638); - assert_eq!(h2, 8192); + assert_eq!(w2, 819); + assert_eq!(h2, 4096); assert!(((h2 as f32 / w2 as f32) - 5.0).abs() < 0.01); } } diff --git a/crates/pdf_viewer/src/rasterizer.rs b/crates/pdf_viewer/src/rasterizer.rs index 0f96d25165..1a2417e98a 100644 --- a/crates/pdf_viewer/src/rasterizer.rs +++ b/crates/pdf_viewer/src/rasterizer.rs @@ -5,6 +5,7 @@ use crate::cache::RenderedPage; use crate::document::PageDimensions; +use crate::pdfium::compute_target_dimensions; use anyhow::Result; /// Rendering and tone mapping configuration. @@ -160,7 +161,16 @@ impl PageRasterizer { dimensions: PageDimensions, options: RasterizerOptions, ) -> Result { - let (width, height) = dimensions.to_pixel_size(options.zoom_factor, options.target_dpi); + // Mirror the native renderer's clamp: an unclamped frame at high zoom can grow + // past the page cache budget, which would make the cache refuse the entry and + // the caller re-render it forever. + let (width, height) = compute_target_dimensions( + dimensions.width, + dimensions.height, + options.target_dpi, + options.zoom_factor, + ); + let (width, height) = (width.max(1) as u32, height.max(1) as u32); let total_bytes = (width as usize) * (height as usize) * 4; // Create standard white paper background with an inner border