image_viewer: Fix PDF zoom lag and silent annotation no-ops

Zoom lag: page rasterization ran synchronously on the UI thread with the
exact zoom in the cache key, so every wheel tick re-rendered every
visible page before the next frame. Rasterization now happens in a
background task at a quantized zoom bucket (x1.25) while the closest
already-rendered bitmap stays on screen scaled to the layout; completed
renders swap in via notify. Frames the cache refuses (larger than the
budget) are never re-rendered, which previously spun an infinite
render loop.

The mock fallback renderer skipped the MAX_PAGE_DIMENSION clamp the
native path applies, so its frames grew past the cache budget at high
zoom and were refused on every attempt; the cap is now shared, and
lowered from 8192 to 4096 (a 1080p viewport is oversampled ~4x at that
point, and the worst-case frame drops from 207MB to 67MB).

Annotations: the bundled libpdfium.so was never loaded because the
loader only tried working-directory-relative paths; it now also
resolves next to the executable, matching the bundled layout. Ink,
highlight, and delete editing returned Ok with unchanged bytes when no
native engine was present, so strokes silently did nothing; they now
return errors that surface through the existing logging.
This commit is contained in:
yenru0 committed 2026-10-08 12:00:01 +09:00
1 parent 67b9fe8b73
commit 0030767614
4 files changed
+270 -51

No files matched your search

+176 -38
View File
@@ -2,7 +2,8 @@ mod image_info;
mod image_viewer_settings; mod image_viewer_settings;
use std::{ 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 _; use anyhow::Context as _;
@@ -113,6 +114,17 @@ const MAX_ZOOM: f32 = 20.0;
const ZOOM_STEP: f32 = 1.1; const ZOOM_STEP: f32 = 1.1;
const SCROLL_LINE_MULTIPLIER: f32 = 20.0; 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 /// 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`. /// 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 /// `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, thumbnail_scroll_handle: gpui::ScrollHandle,
pdf_thumbnail_cache: PageLruCache<CachedPageImage>, pdf_thumbnail_cache: PageLruCache<CachedPageImage>,
pdf_page_cache: PageLruCache<CachedPageImage>, pdf_page_cache: PageLruCache<CachedPageImage>,
/// Page/zoom keys with a rasterization already running in the background.
pdf_render_inflight: HashSet<CacheKey>,
/// 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<CacheKey>,
} }
struct DisplayedImage { struct DisplayedImage {
@@ -483,6 +500,8 @@ impl ImageView {
thumbnail_scroll_handle: gpui::ScrollHandle::new(), thumbnail_scroll_handle: gpui::ScrollHandle::new(),
pdf_thumbnail_cache: PageLruCache::new(DEFAULT_MEMORY_BUDGET_BYTES), pdf_thumbnail_cache: PageLruCache::new(DEFAULT_MEMORY_BUDGET_BYTES),
pdf_page_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( fn get_or_render_page_image(
&mut self, &mut self,
pdf_bytes: &[u8], pdf_bytes: Arc<Vec<u8>>,
page_index: usize, page_index: usize,
cx: &mut Context<Self>,
) -> Option<Arc<gpui::RenderImage>> { ) -> Option<Arc<gpui::RenderImage>> {
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) { if let Some(cached) = self.pdf_page_cache.get(&key) {
return Some(cached.image); return Some(cached.image);
} }
let engine = PdfiumEngine::new(); // Show the closest already-rendered zoom scaled to the layout size while the
let options = RasterizerOptions { // fresh bitmap renders in the background.
target_dpi: 144.0, let fallback = self
zoom_factor: self.zoom_level, .pdf_page_cache
dark_mode: false, .closest_to_zoom(page_index, bucket_zoom)
saturation_threshold: 0.18, .map(|cached| cached.image.clone());
rotation_degrees: self.page_rotation, if self.pdf_render_refused.contains(&key) {
}; return fallback;
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::<image::Rgba<u8>, _>::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);
}
} }
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::<image::Rgba<u8>, _>::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(), thumbnail_scroll_handle: gpui::ScrollHandle::new(),
pdf_thumbnail_cache: PageLruCache::new(DEFAULT_MEMORY_BUDGET_BYTES), pdf_thumbnail_cache: PageLruCache::new(DEFAULT_MEMORY_BUDGET_BYTES),
pdf_page_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) page_images.entry(page_idx)
{ {
if let Some(img) = 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); e.insert(img);
} }
@@ -5663,6 +5729,78 @@ mod tests {
out 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] #[gpui::test]
async fn test_pdf_wheel_zoom_and_middle_drag_pan(cx: &mut TestAppContext) { async fn test_pdf_wheel_zoom_and_middle_drag_pan(cx: &mut TestAppContext) {
init_test(cx); init_test(cx);
+20
View File
@@ -191,6 +191,26 @@ impl<T: CacheablePage> PageLruCache<T> {
false 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. /// Clears all entries from the cache and resets memory tracking.
pub fn clear(&mut self) { pub fn clear(&mut self) {
self.entries.clear(); self.entries.clear();
+63 -12
View File
@@ -35,7 +35,11 @@ static GLOBAL_PDFIUM: OnceLock<Arc<Mutex<SerializedPdfium>>> = OnceLock::new();
/// Maximum dimension (width or height) allowed when rasterizing a PDF page, /// Maximum dimension (width or height) allowed when rasterizing a PDF page,
/// preventing out-of-memory denial of service attacks from malicious documents. /// 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, /// Computes target pixel dimensions preserving the page aspect ratio,
/// scaling down uniformly if either dimension exceeds `MAX_PAGE_DIMENSION`. /// scaling down uniformly if either dimension exceeds `MAX_PAGE_DIMENSION`.
@@ -174,10 +178,16 @@ impl PdfiumEngine {
PathBuf::from("/usr/local/lib/libpdfium.dylib"), 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 `<app>/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 Ok(executable_path) = std::env::current_exe() {
if let Some(executable_dir) = executable_path.parent() { if let Some(executable_dir) = executable_path.parent() {
common_paths.push(executable_dir.join("../Frameworks/libpdfium.dylib")); 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, page_index: usize,
rects: &[(f32, f32, f32, f32)], rects: &[(f32, f32, f32, f32)],
color: PdfAnnotationColor, color: PdfAnnotationColor,
) -> Result<Vec<u8>> { ) -> Result<Vec<u8>> {
if rects.is_empty() { if rects.is_empty() {
return Ok(bytes.to_vec()); return Ok(bytes.to_vec());
} }
let guard = self.inner.lock(); let guard = self.inner.lock();
let Some(pdfium) = guard.0.as_ref() else { 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(); let bindings = pdfium.bindings();
@@ -2309,7 +2319,7 @@ impl PdfiumEngine {
let guard = self.inner.lock(); let guard = self.inner.lock();
let Some(pdfium) = guard.0.as_ref() else { 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(); let bindings = pdfium.bindings();
@@ -2506,7 +2516,7 @@ impl PdfiumEngine {
) -> Result<Vec<u8>> { ) -> Result<Vec<u8>> {
let guard = self.inner.lock(); let guard = self.inner.lock();
let Some(pdfium) = guard.0.as_ref() else { 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(); let bindings = pdfium.bindings();
@@ -2631,6 +2641,47 @@ mod tests {
assert_eq!(page.height_pt, 792.0); 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] #[test]
fn test_extract_outline_without_native_library() { fn test_extract_outline_without_native_library() {
let engine = PdfiumEngine { let engine = PdfiumEngine {
@@ -2674,11 +2725,11 @@ mod tests {
#[test] #[test]
fn test_max_page_dimension_bounds() { 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 // Verify clamping logic for extreme/malicious dimensions
let huge_dim: f32 = 100_000.0; let huge_dim: f32 = 100_000.0;
let clamped = huge_dim.clamp(1.0, MAX_PAGE_DIMENSION); 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 tiny_dim: f32 = -50.0;
let clamped_tiny = tiny_dim.clamp(1.0, MAX_PAGE_DIMENSION); 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 // Verify uniform aspect ratio scaling under extreme dimensions
let (w, h) = compute_target_dimensions(10_000.0, 2_000.0, 72.0, 1.0); let (w, h) = compute_target_dimensions(10_000.0, 2_000.0, 72.0, 1.0);
assert_eq!(w, 8192); assert_eq!(w, 4096);
assert_eq!(h, 1638); assert_eq!(h, 819);
assert!(((w as f32 / h as f32) - 5.0).abs() < 0.01); 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); let (w2, h2) = compute_target_dimensions(2_000.0, 10_000.0, 72.0, 1.0);
assert_eq!(w2, 1638); assert_eq!(w2, 819);
assert_eq!(h2, 8192); assert_eq!(h2, 4096);
assert!(((h2 as f32 / w2 as f32) - 5.0).abs() < 0.01); assert!(((h2 as f32 / w2 as f32) - 5.0).abs() < 0.01);
} }
} }
+11 -1
View File
@@ -5,6 +5,7 @@
use crate::cache::RenderedPage; use crate::cache::RenderedPage;
use crate::document::PageDimensions; use crate::document::PageDimensions;
use crate::pdfium::compute_target_dimensions;
use anyhow::Result; use anyhow::Result;
/// Rendering and tone mapping configuration. /// Rendering and tone mapping configuration.
@@ -160,7 +161,16 @@ impl PageRasterizer {
dimensions: PageDimensions, dimensions: PageDimensions,
options: RasterizerOptions, options: RasterizerOptions,
) -> Result<RenderedPage> { ) -> Result<RenderedPage> {
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; let total_bytes = (width as usize) * (height as usize) * 4;
// Create standard white paper background with an inner border // Create standard white paper background with an inner border