From ca474e225294db7ea7510e280f2fe57cdeabe780 Mon Sep 17 00:00:00 2001 From: Ron Harel <55725807+ronharel02@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:35:59 +0300 Subject: [PATCH] image_viewer: Fix texture leak when reloading images (#58803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the image viewer reloads an image after the underlying file changes, it now tracks the `RenderImage` currently displayed by the view and drops that image’s atlas entry before replacing it with a newly decoded render image. This prevents rapidly updated images from accumulating atlas textures per reload while keeping cleanup scoped to the window that displayed the image. Closes #35894 Release Notes: - Fixed a memory leak when previewing images that are repeatedly updated on disk. --------- Co-authored-by: Smit Barmase --- Cargo.lock | 2 + crates/gpui/src/app.rs | 8 + crates/gpui/src/elements/img.rs | 11 + crates/gpui/src/platform.rs | 12 + crates/gpui/src/platform/test/window.rs | 4 + crates/gpui/src/window.rs | 15 + crates/image_viewer/Cargo.toml | 2 + crates/image_viewer/src/image_viewer.rs | 349 +++++++++++++++++++++++- 8 files changed, 394 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 201fa6304b..83ae5ecc82 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8998,6 +8998,7 @@ dependencies = [ "db", "editor", "file_icons", + "fs", "gpui", "language", "log", @@ -9005,6 +9006,7 @@ dependencies = [ "project", "serde", "settings", + "theme", "theme_settings", "ui", "util", diff --git a/crates/gpui/src/app.rs b/crates/gpui/src/app.rs index 1a7877d377..37cca13acb 100644 --- a/crates/gpui/src/app.rs +++ b/crates/gpui/src/app.rs @@ -2491,6 +2491,14 @@ impl App { self.loading_assets.remove(&asset_id); } + /// Check whether an asset is present in GPUI's cache (loading or loaded), + /// without fetching it. + #[cfg(any(test, feature = "test-support"))] + pub fn has_asset(&self, source: &A::Source) -> bool { + let asset_id = (TypeId::of::(), hash(source)); + self.loading_assets.contains_key(&asset_id) + } + /// Asynchronously load an asset, if the asset hasn't finished loading this will return None. /// /// Note that the multiple calls to this method will only result in one `Asset::load` call at a diff --git a/crates/gpui/src/elements/img.rs b/crates/gpui/src/elements/img.rs index 0b571a6f43..13c8df1f8c 100644 --- a/crates/gpui/src/elements/img.rs +++ b/crates/gpui/src/elements/img.rs @@ -580,6 +580,17 @@ impl ImageSource { ImageSource::Image(data) => cx.remove_asset::>(data), } } + + /// Check whether this image source is present in the asset system (loading + /// or loaded), without fetching it. + #[cfg(any(test, feature = "test-support"))] + pub fn is_asset_cached(&self, cx: &App) -> bool { + match self { + ImageSource::Resource(resource) => cx.has_asset::(resource), + ImageSource::Custom(_) | ImageSource::Render(_) => false, + ImageSource::Image(data) => cx.has_asset::>(data), + } + } } #[derive(Clone)] diff --git a/crates/gpui/src/platform.rs b/crates/gpui/src/platform.rs index fe7a2edd9c..5f8c63f932 100644 --- a/crates/gpui/src/platform.rs +++ b/crates/gpui/src/platform.rs @@ -1309,6 +1309,11 @@ pub trait PlatformAtlas { build: &mut dyn FnMut() -> Result, Cow<'a, [u8]>)>>, ) -> Result>; fn remove(&self, key: &AtlasKey); + + #[cfg(any(test, feature = "test-support"))] + fn contains(&self, _key: &AtlasKey) -> bool { + false + } } #[doc(hidden)] @@ -2562,6 +2567,13 @@ impl Image { ImageSource::Image(self).remove_asset(cx); } + /// Check whether this image is present in GPUI's asset cache (loading or + /// loaded), without fetching it. + #[cfg(any(test, feature = "test-support"))] + pub fn is_asset_cached(self: &Arc, cx: &App) -> bool { + ImageSource::Image(self.clone()).is_asset_cached(cx) + } + /// Convert the clipboard image to an `ImageData` object. pub fn to_image_data(&self, svg_renderer: SvgRenderer) -> Result> { let frames = match self.format { diff --git a/crates/gpui/src/platform/test/window.rs b/crates/gpui/src/platform/test/window.rs index 84de0d28d7..8ab9147b36 100644 --- a/crates/gpui/src/platform/test/window.rs +++ b/crates/gpui/src/platform/test/window.rs @@ -449,4 +449,8 @@ impl PlatformAtlas for TestAtlas { let mut state = self.0.lock(); state.tiles.remove(key); } + + fn contains(&self, key: &AtlasKey) -> bool { + self.0.lock().tiles.contains_key(key) + } } diff --git a/crates/gpui/src/window.rs b/crates/gpui/src/window.rs index 4c2ecbccfc..de525d4545 100644 --- a/crates/gpui/src/window.rs +++ b/crates/gpui/src/window.rs @@ -4503,6 +4503,21 @@ impl Window { Ok(()) } + /// Returns whether every frame of an image is present in the sprite atlas. + #[cfg(any(test, feature = "test-support"))] + pub fn has_image_atlas_entry(&self, data: &RenderImage) -> bool { + data.frame_count() > 0 + && (0..data.frame_count()).all(|frame_index| { + self.sprite_atlas.contains( + &RenderImageParams { + image_id: data.id, + frame_index, + } + .into(), + ) + }) + } + /// Add a node to the layout tree for the current frame. Takes the `Style` of the element for which /// layout is being requested, along with the layout ids of any children. This method is called during /// calls to the [`Element::request_layout`] trait method and enables any element to participate in layout. diff --git a/crates/image_viewer/Cargo.toml b/crates/image_viewer/Cargo.toml index 17d338a2ca..f200641e57 100644 --- a/crates/image_viewer/Cargo.toml +++ b/crates/image_viewer/Cargo.toml @@ -34,3 +34,5 @@ workspace.workspace = true [dev-dependencies] editor = { workspace = true, features = ["test-support"] } +fs.workspace = true +theme.workspace = true diff --git a/crates/image_viewer/src/image_viewer.rs b/crates/image_viewer/src/image_viewer.rs index a2cf1d7140..0a19816d57 100644 --- a/crates/image_viewer/src/image_viewer.rs +++ b/crates/image_viewer/src/image_viewer.rs @@ -1,7 +1,7 @@ mod image_info; mod image_viewer_settings; -use std::path::Path; +use std::{path::Path, sync::Arc}; use anyhow::Context as _; use editor::{ @@ -13,8 +13,9 @@ use gpui::{ AnyElement, App, Bounds, Context, DispatchPhase, Element, ElementId, Entity, EventEmitter, FocusHandle, Focusable, Font, GlobalElementId, InspectorElementId, InteractiveElement, IntoElement, LayoutId, MouseButton, MouseDownEvent, MouseMoveEvent, MouseUpEvent, - ParentElement, PinchEvent, Pixels, Point, Render, ScrollDelta, ScrollWheelEvent, Style, Styled, - Subscription, Task, WeakEntity, Window, actions, checkerboard, div, img, point, px, size, + ParentElement, PinchEvent, Pixels, Point, Render, RenderImage, ScrollDelta, ScrollWheelEvent, + Style, Styled, Subscription, Task, WeakEntity, Window, actions, checkerboard, div, img, point, + px, size, }; use language::File as _; use persistence::ImageViewerDb; @@ -22,7 +23,7 @@ use project::{ImageItem, Project, ProjectPath, image_store::ImageItemEvent}; use settings::Settings; use theme_settings::ThemeSettings; use ui::{Tooltip, prelude::*}; -use util::paths::PathExt; +use util::{ResultExt as _, paths::PathExt}; use workspace::{ ItemId, ItemSettings, Pane, ToolbarItemEvent, ToolbarItemLocation, ToolbarItemView, Workspace, WorkspaceId, delete_unloaded_items, @@ -68,6 +69,24 @@ pub struct ImageView { last_mouse_position: Option>, container_bounds: Option>, image_size: Option<(u32, u32)>, + pending_image: Option>, + displayed_image: Option, +} + +struct DisplayedImage { + source_image: Arc, + render_image: Arc, +} + +impl DisplayedImage { + fn drop_atlas_entry(&self, window: &mut Window) { + window.drop_image(self.render_image.clone()).log_err(); + } + + fn release(self, window: &mut Window, cx: &mut App) { + self.drop_atlas_entry(window); + self.source_image.remove_asset(cx); + } } impl ImageView { @@ -75,6 +94,48 @@ impl ImageView { self.last_mouse_position.is_some() } + fn update_displayed_image( + &mut self, + image: &Arc, + render_image: Option>, + window: &mut Window, + cx: &mut App, + ) { + if let Some(pending_image) = self.pending_image.take() { + if pending_image.id() != image.id() { + pending_image.remove_asset(cx); + } else if render_image.is_none() { + self.pending_image = Some(pending_image); + } + } + + let Some(render_image) = render_image else { + self.pending_image.get_or_insert_with(|| image.clone()); + return; + }; + + if self + .displayed_image + .as_ref() + .is_some_and(|displayed_image| displayed_image.render_image.id == render_image.id) + { + return; + } + + if let Some(previous) = self.displayed_image.take() { + if previous.source_image.id() == image.id() { + previous.drop_atlas_entry(window); + } else { + previous.release(window, cx); + } + } + + self.displayed_image = Some(DisplayedImage { + source_image: image.clone(), + render_image, + }); + } + pub fn new( image_item: Entity, project: Entity, @@ -83,9 +144,8 @@ impl ImageView { ) -> Self { // Start loading the image to render in the background to prevent the view // from flickering in most cases. - let _ = image_item.update(cx, |image, cx| { - image.image.clone().get_render_image(window, cx) - }); + let pending_image = image_item.read(cx).image.clone(); + let _render_image = pending_image.clone().get_render_image(window, cx); cx.subscribe(&image_item, Self::on_image_event).detach(); cx.on_release_in(window, |this, window, cx| { @@ -111,6 +171,8 @@ impl ImageView { last_mouse_position: None, container_bounds: None, image_size, + pending_image: Some(pending_image), + displayed_image: None, } } @@ -384,7 +446,9 @@ impl Element for ImageContentElement { top = center_y - (scaled_height / 2.0) + pan_offset.y; } - self.image_view.update(cx, |this, _| { + self.image_view.update(cx, |this, cx| { + let render_image = image.clone().use_render_image(window, cx); + this.update_displayed_image(&image, render_image, window, cx); this.container_bounds = Some(bounds); if let Some(initial_zoom_level) = initial_zoom_level { this.zoom_level = initial_zoom_level; @@ -566,7 +630,7 @@ impl Item for ImageView { fn clone_on_split( &self, _workspace_id: Option, - _: &mut Window, + _window: &mut Window, cx: &mut Context, ) -> Task>> where @@ -581,6 +645,8 @@ impl Item for ImageView { last_mouse_position: None, container_bounds: None, image_size: self.image_size, + pending_image: None, + displayed_image: None, }))) } @@ -1008,6 +1074,271 @@ pub fn init(cx: &mut App) { workspace::register_serializable_item::(cx); } +#[cfg(test)] +mod tests { + use super::*; + use fs::{FakeFs, Fs as _}; + use gpui::{TestAppContext, VisualTestContext}; + use settings::SettingsStore; + use util::rel_path::rel_path; + + fn init_test(cx: &mut TestAppContext) { + cx.update(|cx| { + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + theme_settings::init(theme::LoadThemes::JustBase, cx); + }); + } + + fn test_image(red: u8) -> Arc { + let bytes = format!("P3\n1 1\n255\n{red} 0 0\n").into_bytes(); + Arc::new(gpui::Image::from_bytes(gpui::ImageFormat::Pnm, bytes)) + } + + async fn open_test_image(cx: &mut TestAppContext) -> (Entity, Entity) { + let fs = FakeFs::new(cx.executor()); + fs.create_dir(Path::new("/root")) + .await + .expect("test root should be created"); + fs.insert_file("/root/image.ppm", test_image(0).bytes.clone()) + .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("image.ppm").into(), + }, + cx, + ) + }) + .await + .expect("test image should open"); + + (project, image_item) + } + + fn draw_window(cx: &mut VisualTestContext) { + cx.update(|window, cx| { + window.refresh(); + window.draw(cx).clear(cx); + }); + } + + fn displayed_source_id(image_view: &Entity, cx: &VisualTestContext) -> Option { + cx.read(|cx| { + image_view + .read(cx) + .displayed_image + .as_ref() + .map(|displayed_image| displayed_image.source_image.id()) + }) + } + + fn displayed_render_image( + image_view: &Entity, + cx: &VisualTestContext, + ) -> Option> { + cx.read(|cx| { + image_view + .read(cx) + .displayed_image + .as_ref() + .map(|displayed_image| displayed_image.render_image.clone()) + }) + } + + fn replace_image( + image_item: &Entity, + image: Arc, + cx: &mut VisualTestContext, + ) { + cx.update(|_window, cx| { + image_item.update(cx, |image_item, cx| { + image_item.image = image; + cx.emit(ImageItemEvent::Reloaded); + }); + }); + } + + fn replace_image_and_draw( + image_item: &Entity, + image: Arc, + cx: &mut VisualTestContext, + ) { + replace_image(image_item, image, cx); + draw_window(cx); + } + + fn image_is_cached(image: &Arc, cx: &VisualTestContext) -> bool { + cx.read(|cx| image.is_asset_cached(cx)) + } + + #[gpui::test] + async fn test_reloading_removes_replaced_image_from_asset_cache(cx: &mut TestAppContext) { + init_test(cx); + let (project, image_item) = open_test_image(cx).await; + let original_image = cx.read(|cx| image_item.read(cx).image.clone()); + + let (image_view, cx) = cx + .add_window_view(|window, cx| ImageView::new(image_item.clone(), project, window, cx)); + + cx.run_until_parked(); + draw_window(cx); + assert_eq!( + displayed_source_id(&image_view, cx), + Some(original_image.id()), + "the original image should finish decoding and be displayed" + ); + + let reloaded_image = test_image(1); + replace_image_and_draw(&image_item, reloaded_image.clone(), cx); + cx.run_until_parked(); + draw_window(cx); + + assert_eq!( + displayed_source_id(&image_view, cx), + Some(reloaded_image.id()), + "the reloaded image should replace the original" + ); + assert!( + !image_is_cached(&original_image, cx), + "the replaced image remained in GPUI's asset cache" + ); + } + + #[gpui::test] + async fn test_superseded_in_flight_image_is_removed_from_asset_cache(cx: &mut TestAppContext) { + init_test(cx); + let (project, image_item) = open_test_image(cx).await; + + let (image_view, cx) = cx + .add_window_view(|window, cx| ImageView::new(image_item.clone(), project, window, cx)); + + let superseded_image = test_image(1); + replace_image_and_draw(&image_item, superseded_image.clone(), cx); + assert_ne!( + displayed_source_id(&image_view, cx), + Some(superseded_image.id()), + "the superseded image should still be decoding" + ); + + let current_image = test_image(2); + replace_image_and_draw(&image_item, current_image.clone(), cx); + assert_ne!( + displayed_source_id(&image_view, cx), + Some(current_image.id()), + "the current image should start decoding before the superseded decode completes" + ); + + cx.run_until_parked(); + draw_window(cx); + + assert_eq!( + displayed_source_id(&image_view, cx), + Some(current_image.id()), + "the current image should finish decoding" + ); + assert!( + !image_is_cached(&superseded_image, cx), + "the superseded image remained in GPUI's asset cache" + ); + } + + #[gpui::test] + async fn test_superseded_constructor_prefetch_is_removed_from_asset_cache( + cx: &mut TestAppContext, + ) { + init_test(cx); + let (project, image_item) = open_test_image(cx).await; + let prefetched_image = cx.read(|cx| image_item.read(cx).image.clone()); + + let cx = cx.add_empty_window(); + let image_view = cx.update(|window, cx| { + cx.new(|cx| ImageView::new(image_item.clone(), project, window, cx)) + }); + + let current_image = test_image(1); + replace_image(&image_item, current_image, cx); + cx.draw(point(px(0.), px(0.)), size(px(1.), px(1.)), |_, _| { + image_view.clone().into_any_element() + }); + cx.run_until_parked(); + + assert!( + !image_is_cached(&prefetched_image, cx), + "the superseded constructor prefetch remained in GPUI's asset cache" + ); + } + + #[gpui::test] + async fn test_releasing_one_split_keeps_shared_atlas_entry(cx: &mut TestAppContext) { + init_test(cx); + let (project, image_item) = open_test_image(cx).await; + + let cx = cx.add_empty_window(); + let original_image_view = cx.update(|window, cx| { + cx.new(|cx| ImageView::new(image_item.clone(), project, window, cx)) + }); + let split_image_view = original_image_view + .update_in(cx, |image_view, window, cx| { + image_view.clone_on_split(None, window, cx) + }) + .await + .expect("image view should support splitting"); + + let draw_split_views = |cx: &mut VisualTestContext| { + cx.draw(point(px(0.), px(0.)), size(px(2.), px(1.)), |_, _| { + div() + .size_full() + .child(original_image_view.clone()) + .child(split_image_view.clone()) + }); + }; + + draw_split_views(cx); + cx.run_until_parked(); + draw_split_views(cx); + + let original_render_image = displayed_render_image(&original_image_view, cx) + .expect("the original image view should finish decoding"); + let split_render_image = displayed_render_image(&split_image_view, cx) + .expect("the split image view should finish decoding"); + assert_eq!( + original_render_image.id, split_render_image.id, + "both image views should share the decoded render image" + ); + assert!( + cx.update(|window, _| window.has_image_atlas_entry(&original_render_image)), + "the shared image should be present in the window atlas" + ); + + drop(original_image_view); + cx.update(|_, _| {}); + cx.run_until_parked(); + + assert!( + cx.update(|window, _| window.has_image_atlas_entry(&split_render_image)), + "releasing one image view removed an atlas entry still used by its split" + ); + + cx.draw(point(px(0.), px(0.)), size(px(1.), px(1.)), |_, _| { + split_image_view.clone().into_any_element() + }); + } +} + mod persistence { use std::path::PathBuf;