From 47cf5af9fd935da2180d6afb383005d2432659b2 Mon Sep 17 00:00:00 2001 From: FinnNk Date: Thu, 15 Oct 2026 05:44:55 +0100 Subject: [PATCH] perf: keep effect-cache pins and History snapshots inside the memory budgets (#5336) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Raw: measure the Nikon D90 image area (4288x2848 at (10, 10)) Aligning the decoded D90 sensor plane with the camera's own JPEG puts the image at (10, 10) in the 4352x2868 sensor plane, giving the camera's 4288x2848 frame (10-pixel margins; the right border reads the camera's flat 255 from column 4310). Add the measured ImageArea so a D90 NEF opens on the camera's frame instead of the uncropped plane, and make a measured profile leave a file that carries its own colour calibration (a DNG) untouched. * Codecs: open Cineon (.cin) film frames read-only Cineon is the 10-bit logarithmic film-scan format DPX grew from. Read it from the same Kodak header family: the generic header (big-endian, orientation at 192, channel count at 193, the channel-1 bits/dimensions), one or three channels, three 10-bit samples packed into each 32-bit word on a word boundary. The 10-bit code values open as U16 (round(c x 65535 / 1023)); the file's log black/white points are a display choice and are not applied. A depth other than 10 bits or a channel count other than 1/3 is Unsupported; truncation errors rather than panicking. Read-only (no Cineon writer), so .cin joins the open filters and Cineon read closes part of #1049's G10 gap. * PSD: write tiff blocks without copies, inflate merged ZIP once - ImageSourceData::to_bytes writes the global mask and global blocks straight from the model with 4-byte alignment instead of cloning each block (data included) to set its padding. Same bytes. - The tiff transcoder's nested regions and blocks write into the parent's buffer (taken and handed back, error or not) instead of building their own output and copying it up per nesting level. Bodies that fail or are not transcoded are truncated before being copied as stored or dropped. Same bytes. - PsdFile::from_bytes_and_merged decodes the merged image with the parse when asked: a ZIP stream is inflated once (the decode's stream check is the parse's); the parse-only check runs only when the decode fails, to keep from_bytes' errors. PSD import uses it with merged_needed. - PsdBuilder::composite documents that Gray/CMYK composites stay straight. Co-Authored-By: Claude Opus 5.5 * Content-Aware Fill: measure a thin hole by its thickness, not its bounds (#1366) Removing long, thin objects (coiled guitar strings, wires) left their outline as darker and lighter blotches and copied pieces of other strings into the fill. The pyramid shrank until the hole's bounding box was four patches across: for a string across the image that is a level where the hole has swallowed its surroundings, so the matches found there copy whatever looks like the blur, and the finer levels (refine-only for wide bounds) keep them. A hole at most a quarter as thick as its bounds are long now counts as thin: the pyramid stops while it is still about two patches thick, and that level starts from a membrane across it (over a few pixels, the surroundings themselves) instead of the onion peel, whose exhaustive search is what made a shallow pyramid unaffordable. Compact holes are unchanged bit for bit. Refs #1366 * perf(compose,gpu): effect caches reserve tile addresses instead of pinning layers Effect map cache entries kept a clone of their layer (a deep clone for groups) so the tile addresses the cache key hashes couldn't be reused. That kept an edited or deleted layer's pixels alive, uncounted, until the entry was evicted. Keep a Weak to each hashed tile instead: it reserves the address without keeping the pixels. The GPU compositor's styled-group pins and shape-source tile snapshots do the same. Part of #4530. Co-Authored-By: Claude Opus 5.5 * fix(history): count snapshot-only tiles against the history budget History panel snapshots outlived every budget: a Merged Layers snapshot is a full flattened copy that neither the history budget, the GPU budget's document bytes nor Purge accounted for. Trim now counts the tiles only snapshots hold as fixed cost, the GPU budget includes them, and Purge > Histories no longer reports tiles a kept snapshot still holds as freed. Part of #4530. Co-Authored-By: Claude Opus 5.5 * Content-Aware Fill: every part of a thin selection keeps its surroundings (#1366) Two more causes of ghosts when removing strings and the small knobs selected with them: * Auto sampling summarises the selection's surroundings by a few clusters. Around a long selection those come from the strings' surroundings, so a small part elsewhere (a tuning knob in a differently coloured corner) lost its own surroundings and was filled from far away in another colour. Around a thin selection the Auto area now always reaches as far as the selection is thick. * The membrane that starts a thin hole now starts a patch radius out, past the anti-aliased fringe or halo that a selection drawn close to the object leaves outside it. Refs #1366 * Content-Aware Fill: peel a thin hole where that stays affordable (#1366) Starting every thin hole from a membrane smoothed Spot Healing on film grain (spot_healing_keeps_film_grain_through_overlapping_strokes: grain 9.0 against 12.1 around). The onion peel copies the grain; it was only given up because its exhaustive search is too slow on the big level a long thin hole stops at. It now starts the thin hole whenever hole cells times sources stay within 2^28 comparisons, and the membrane only beyond. Refs #1366 * Select Subject: keep subject parts coloured like the backdrop, leave enclosed gaps open Users on #2788 reported Select Subject dropping clothes that are close in colour to a plain wall and bridging the gap between an arm and the torso. Both come from the same place: the saliency pass labels low-contrast parts of the subject as background, the background colour model then learns them and explains them away, and the final blind hole fill closes every enclosed gap up to 5 % of the mask. Two changes in crates/algo: - A second GrabCut pass whose background model only knows the backdrop. A colour model fitted to the image frame (minus a margin around the salient blob, in case the subject runs out of the picture) marks every pixel it explains as probable background; the blob and whatever the backdrop cannot account for start as probable foreground. Only components that touch the first pass's result survive. - Hole filling that consults the colour models: specks (<= 0.2 % of the mask) always fill, larger holes up to 5 % fill only when their pixels fit the foreground model better than the background's. Tests: two regressions (a cream-shouldered figure on a near-white wall keeps its shoulders; a ring keeps its backdrop-coloured hole open), plus ignored timing and picture-dump helpers. The 6 MP first run goes from about 100 ms to about 200 ms on one core; the P29 repeat path is untouched. * Keyboard shortcut sets: Set menu, New Set, Save, Delete, .kys Export and Summarize (K2) Edit › Keyboard Shortcuts gets Photoshop's set management. A set is a saved copy of the bindings (Preferences.shortcutSets, the one in use in shortcutSet; "" is PhotoCraft Defaults), and the Set menu shows "(modified)" when the bindings differ from it. New Set…, Save and Delete wait for OK like the rest of the dialog. Export… writes the dialog's bindings as a .kys file (commands then tools, Photoshop's macOS key spelling), each row carrying our id as pc-id so PhotoCraft imports its own sets back exactly, whatever the label; Summarize… writes Photoshop's HTML list of every bound command by menu. edit.keyboardShortcuts takes useSet (before the call's edits), saveSet, deleteSet, sets and activeSet (after them), exportKys and summarize; every reply reports the sets, the set in use and whether it is modified. Bad names, keys or counts are errors that change nothing. * .kys import: tool keys and sets saved on Windows (K3) Photoshop's rows now import: each matches our tool by name (our tool names are Photoshop's) or by pc-id in sets PhotoCraft exported, and a letter other than the tool's own becomes its override, while its own letter keeps the tool cycling with the others (#5036). A set saved on Windows spells the primary modifier Ctrl where a Mac set writes Cmd and keeps Control for the Control key. A set with no Cmd or Opt anywhere but some Ctrl is read as a Windows set, its Ctrl as our Cmd: Ctrl+L is the Cmd+L default, not a Control-key override that would also mean Control on a Mac. The import reports `windows`. * cargo fmt the extension lists Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Vikram Bhaskaran Co-authored-by: vangel76 Co-authored-by: Claude Opus 5.5 Co-authored-by: Gert Pellin Co-authored-by: Brandon Thomas Co-authored-by: Diego Chagas Co-authored-by: Tanish Manku --- crates/compose/src/lib.rs | 31 ++++++----- crates/compose/src/tests.rs | 25 +++++++++ crates/engine/src/analysis_cmds.rs | 2 +- crates/engine/src/edit_menu_cmds.rs | 8 ++- crates/engine/src/history_cmds.rs | 54 ++++++++++++++++++- crates/engine/src/lib.rs | 16 +++++- crates/engine/src/prefs.rs | 2 +- crates/gpu/src/fx.rs | 68 +++++++++++++++++++----- crates/gpu/src/lib.rs | 16 ++++-- crates/ops/src/lib.rs | 82 ++++++++++++++++++++++++++++- crates/ui-egui/src/canvas.rs | 2 +- 11 files changed, 267 insertions(+), 39 deletions(-) diff --git a/crates/compose/src/lib.rs b/crates/compose/src/lib.rs index b742540b95..31153867e9 100644 --- a/crates/compose/src/lib.rs +++ b/crates/compose/src/lib.rs @@ -577,7 +577,7 @@ impl LayerKeys { #[cfg(test)] self.built.fetch_add(1, std::sync::atomic::Ordering::Relaxed); let mut h = std::collections::hash_map::DefaultHasher::new(); - layer_identity(layer, &mut h); + layer_identity(layer, &mut h, &mut |_| {}); h.finish() } } @@ -1531,11 +1531,12 @@ fn split_parts(layer: &Layer, rect: Rect, cx: &Ctx) -> Option<(Buffer, Buffer, O /// Order-independent identity of a layer's pixels, masks, effects, Advanced Blending and Blend If /// and (for groups) children. -/// Pixels are identified by their copy-on-write tile pointers; cache entries pin a clone of the -/// layer so those tiles (and their addresses) stay alive while the entry exists. -fn layer_identity(layer: &Layer, h: &mut std::collections::hash_map::DefaultHasher) { +/// Pixels are identified by their copy-on-write tile pointers. `pin` sees every tile hashed: a +/// cache entry keeps a [`std::sync::Weak`] to each, which reserves the tile's address (so no +/// other tile can reuse it and match the key) without keeping its pixels alive (#4530). +fn layer_identity(layer: &Layer, h: &mut std::collections::hash_map::DefaultHasher, pin: &mut dyn FnMut(&std::sync::Arc)) { use std::hash::{Hash, Hasher}; - fn surface_fp(s: &Surface) -> u64 { + fn surface_fp(s: &Surface, pin: &mut dyn FnMut(&std::sync::Arc)) -> u64 { let mut h = std::collections::hash_map::DefaultHasher::new(); s.format().hash(&mut h); // Missing tiles read the default pixel, so tile identity alone is not pixel identity. @@ -1543,6 +1544,7 @@ fn layer_identity(layer: &Layer, h: &mut std::collections::hash_map::DefaultHash sample.to_bits().hash(&mut h); } let tiles = s.tiles().fold(s.tile_count() as u64, |acc, (c, t)| { + pin(t); let mut x = (std::sync::Arc::as_ptr(t) as usize as u64) ^ ((c.tx as u64) << 40) ^ ((c.ty as u32 as u64) << 8); x = x.wrapping_mul(0x9e37_79b9_7f4a_7c15); acc.wrapping_add(x ^ (x >> 29)) @@ -1563,7 +1565,7 @@ fn layer_identity(layer: &Layer, h: &mut std::collections::hash_map::DefaultHash // composites (unlike the group's own visibility and opacity) shapes them. (c.visible, c.opacity.to_bits(), c.clipped).hash(h); format!("{:?}", c.blend).hash(h); - layer_identity(c, h); + layer_identity(c, h, pin); } format!("{:?}", g.artboard).hash(h); } @@ -1575,14 +1577,14 @@ fn layer_identity(layer: &Layer, h: &mut std::collections::hash_map::DefaultHash if let Some(c) = &layer.fill_cache && c.fill == *f { - surface_fp(&c.surface).hash(h); + surface_fp(&c.surface, pin).hash(h); } } LayerContent::Adjustment(a) => format!("{a:?}").hash(h), - _ => layer.surface().map_or(0, surface_fp).hash(h), + _ => layer.surface().map_or(0, |s| surface_fp(s, pin)).hash(h), } if let Some(m) = &layer.mask { - (surface_fp(&m.surface), m.enabled, m.density.to_bits(), m.feather.to_bits()).hash(h); + (surface_fp(&m.surface, pin), m.enabled, m.density.to_bits(), m.feather.to_bits()).hash(h); } if let Some(vm) = &layer.vector_mask { format!("{vm:?}").hash(h); @@ -1597,7 +1599,8 @@ fn layer_identity(layer: &Layer, h: &mut std::collections::hash_map::DefaultHash struct FxEntry { maps: std::sync::Arc, - _pin: Layer, + /// The tiles the key identifies (see [`layer_identity`]): their addresses, not their pixels. + _pin: Vec>, bytes: usize, } @@ -1716,9 +1719,13 @@ fn cached_effect_maps(layer: &Layer, region: Rect, key: u64, cx: &Ctx) -> std::s None => { let shape: Vec = if region.is_empty() { Vec::new() } else { effect_shape(layer, region, cx) }; let maps = effects::build_maps_prepared(layer, shape, region, &cx.light, &texture_ctx(layer, region, cx), cx.patterns); - let bytes = maps.bytes(); + let mut pin = Vec::new(); + layer_identity(layer, &mut std::collections::hash_map::DefaultHasher::new(), &mut |t| pin.push(std::sync::Arc::downgrade(t))); + // The maps, plus what each reserved tile address keeps allocated. + let bytes = + maps.bytes().saturating_add(pin.len().saturating_mul(std::mem::size_of::() + 2 * std::mem::size_of::())); let maps = std::sync::Arc::new(maps); - if slot.set(FxEntry { maps: maps.clone(), _pin: layer.clone(), bytes }).is_ok() { + if slot.set(FxEntry { maps: maps.clone(), _pin: pin, bytes }).is_ok() { // Counted exactly once, by the build that filled the slot, and only while the // slot is still cached (eviction may have dropped it meanwhile). let mut c = fx_cache().lock().unwrap_or_else(|e| e.into_inner()); diff --git a/crates/compose/src/tests.rs b/crates/compose/src/tests.rs index d8842a24fd..eb4b250728 100644 --- a/crates/compose/src/tests.rs +++ b/crates/compose/src/tests.rs @@ -3165,3 +3165,28 @@ fn a_pass_through_clipping_base_keeps_its_adjustments_inside() { d.layers.push(clip); assert!(close4(px(&d, 1, 1), [1.0, 1.0, 1.0, 1.0]), "{:?}", px(&d, 1, 1)); } + +// A cache entry reserves the addresses of the tiles its key hashes, not their pixels (#4530): once +// the document lets go of a styled layer or group (an edit replaced its tiles, or it was +// deleted), the effect cache keeps none of its pixel memory alive, and the maps are still +// reused while the layer is unchanged. +#[test] +fn effect_cache_keeps_no_pixels_of_a_dropped_layer() { + let mut d = doc_white(300, 300); + let mut l = solid_layer("styled", Rect::new(20, 20, 280, 280), [0.9, 0.2, 0.2, 1.0]); + l.effects.items = vec![stroke(3.0, StrokePosition::Outside)]; + d.layers.push(l); + let mut g = Layer::group("g", vec![solid_layer("child", Rect::new(40, 40, 120, 120), [0.1, 0.2, 0.9, 1.0])]); + g.effects.items = vec![Effect::DropShadow(Shadow { size: 2.0, spread: 0.0, ..shadow(3.0, 90.0) })]; + d.layers.push(g); + let first = render(&d, d.bounds()); + let tile = |l: &Layer| l.surface().and_then(|s| s.tiles().next()).map(|(_, t)| t.clone()).unwrap(); + let own = tile(&d.layers[1]); + let LayerContent::Group(grp) = &d.layers[2].content else { panic!("not a group") }; + let child = tile(&grp.children[0]); + assert_eq!(render(&d, d.bounds()).px, first.px, "warm render"); + drop(d); + // Only this test's handles are left: the cache pins neither the layer's nor the child's pixels. + assert_eq!(std::sync::Arc::strong_count(&own), 1); + assert_eq!(std::sync::Arc::strong_count(&child), 1); +} diff --git a/crates/engine/src/analysis_cmds.rs b/crates/engine/src/analysis_cmds.rs index 6ad25951db..85a6be4513 100644 --- a/crates/engine/src/analysis_cmds.rs +++ b/crates/engine/src/analysis_cmds.rs @@ -198,7 +198,7 @@ pub(crate) fn compound(s: &mut Session, label: &str, f: impl FnOnce(&mut Sess // As in `Session::edit`: undo targets what was selected just before the step (#1356). st.history.set_current_layers(prior); st.history.record(label, before, st.layer_target()); - st.history.trim(&st.doc); + st.trim_history(); st.coalesce = None; Ok(v) } diff --git a/crates/engine/src/edit_menu_cmds.rs b/crates/engine/src/edit_menu_cmds.rs index cda3de84d4..11e2273601 100644 --- a/crates/engine/src/edit_menu_cmds.rs +++ b/crates/engine/src/edit_menu_cmds.rs @@ -224,7 +224,13 @@ fn fade(s: &mut Session, p: &Value) -> Result { fn history_bytes(s: &Session, all: bool) -> usize { let docs: Vec<&crate::DocState> = if all { s.documents().iter().collect() } else { s.active().into_iter().collect() }; - docs.iter().map(|d| d.history.unique_bytes(&d.doc)).sum() + // Purge keeps the snapshots, so the tiles they share with the states aren't freed. + docs.iter() + .map(|d| { + let snaps: Vec<&photocraft_doc::Document> = d.snapshots.iter().map(|s| &*s.doc).collect(); + d.history.unique_bytes_besides(&d.doc, &snaps) + }) + .sum() } fn clip_bytes(s: &Session) -> usize { diff --git a/crates/engine/src/history_cmds.rs b/crates/engine/src/history_cmds.rs index cc18df581e..c9c91b3d14 100644 --- a/crates/engine/src/history_cmds.rs +++ b/crates/engine/src/history_cmds.rs @@ -8,8 +8,10 @@ //! states: `history.newSnapshot` takes one, `history.selectSnapshot` brings it back as a new, //! undoable state. They live on [`crate::DocState::snapshots`] for the session only, as in //! Photoshop, and are addressed by an id that is never reused. Their tiles are shared -//! copy-on-write with the document and its history, and are not counted in the history's pixel -//! budget. +//! copy-on-write with the document and its history; the tiles only snapshots hold count against +//! the history's pixel budget ([`crate::DocState::trim_history`]), so a Merged Layers snapshot +//! (a full flattened copy) makes room by dropping the oldest states rather than taking memory +//! beyond the budget unseen (#4530). Edit › Purge keeps them. use std::sync::Arc; @@ -451,6 +453,54 @@ mod tests { assert_eq!(s.active().unwrap().doc.layers.len(), 2); } + fn tile_bytes(doc: &Document) -> usize { + doc.layers.iter().filter_map(|l| l.surface()).flat_map(|s| s.tiles().map(|(_, t)| t.bytes().len())).sum() + } + + /// A Merged Layers snapshot is a flattened copy only it holds (#4530): it counts in the + /// document's memory and against the history budget, so the same edits keep one state less. + #[test] + fn merged_snapshot_counts_against_the_history_budget() { + let (mut with, mut without) = (session(), session()); + let base = with.active().unwrap().memory_bytes(); + with.execute("history.newSnapshot", json!({"from": "mergedLayers"})).unwrap(); + let st = with.active().unwrap(); + let flat = tile_bytes(&st.snapshots[0].doc); + assert!(flat > 0); + assert_eq!(st.memory_bytes(), base + flat); + // Room for the document's tiles and four fill tiles. + let cur = tile_bytes(&st.doc); + let fill = tile_bytes(&Document { layers: st.doc.layers[1..].to_vec(), ..(*st.doc).clone() }); + assert!(flat <= fill, "the flattened tile is no bigger than a fill's"); + let budget = cur + 4 * fill; + for s in [&mut with, &mut without] { + s.active_mut().unwrap().history.max_bytes = budget; + for i in 0..6 { + s.execute("edit.fill", json!({"color": format!("#0000{:02x}", 0x40 + i * 0x20)})).unwrap(); + } + } + let held = |s: &Session| s.active().unwrap().history.past_len(); + assert_eq!(held(&without), 4); + assert_eq!(held(&with), 3, "the snapshot's tile takes one state's room"); + assert!(with.active().unwrap().memory_bytes() <= budget); + } + + /// Purge › Histories keeps the snapshots, so it doesn't report the tiles they share with + /// the states as freed (#4530). + #[test] + fn purging_histories_frees_only_what_no_snapshot_keeps() { + let mut s = session(); + s.execute("history.newSnapshot", json!({})).unwrap(); + s.execute("edit.fill", json!({"color": "#0000ff"})).unwrap(); + let st = s.active().unwrap(); + // The snapshot's green layer tile, now only in it and the last state. + let shared = tile_bytes(&Document { layers: st.snapshots[0].doc.layers[1..].to_vec(), ..(*st.doc).clone() }); + let all = st.history.unique_bytes(&st.doc); + let r = s.execute("edit.purge.histories", json!({})).unwrap(); + assert_eq!(r["bytes"], json!(all - shared), "the green tile stays with the snapshot"); + assert_eq!(s.active().unwrap().snapshots.len(), 1); + } + #[test] fn rename_and_delete_snapshots_keep_the_other_ids() { let mut s = session(); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 6512a5047f..d2aecf37db 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -235,6 +235,20 @@ pub struct DocState { } impl DocState { + /// Trim the history to its budget, counting the tiles only History panel snapshots hold + /// against it too (#4530). Returns how many states were dropped. + pub fn trim_history(&mut self) -> usize { + let snaps: Vec<&Document> = self.snapshots.iter().map(|s| &*s.doc).collect(); + self.history.trim_with(&self.doc, &snaps) + } + + /// Pixel bytes of the document, the tiles only its history holds and the tiles only its + /// History panel snapshots hold. + pub fn memory_bytes(&self) -> usize { + let snaps: Vec<&Document> = self.snapshots.iter().map(|s| &*s.doc).collect(); + self.history.pixel_bytes(&self.doc).saturating_add(self.history.extra_bytes(&self.doc, &snaps)) + } + pub fn new(doc: Document, path: Option) -> Self { let active_layer = doc.top_layer(); let mut history = History::default(); @@ -743,7 +757,7 @@ impl Session { if key.is_none() || st.coalesce != key || !st.history.can_undo() { st.history.set_current_layers(prior); st.history.record(label, before, layers); - st.history.trim(&st.doc); + st.trim_history(); } else { st.history.set_current_layers(layers); } diff --git a/crates/engine/src/prefs.rs b/crates/engine/src/prefs.rs index 17117153c5..c732daeefd 100644 --- a/crates/engine/src/prefs.rs +++ b/crates/engine/src/prefs.rs @@ -1557,7 +1557,7 @@ impl Session { for st in &mut self.docs { st.history.max_states = n; st.history.max_bytes = bytes; - st.history.trim(&st.doc); + st.trim_history(); } photocraft_compose::set_effect_cache_budget(budget << 20); // Without waiting for the system font scan: this runs on the UI thread at launch. diff --git a/crates/gpu/src/fx.rs b/crates/gpu/src/fx.rs index 2149aaa412..01e147d51a 100644 --- a/crates/gpu/src/fx.rs +++ b/crates/gpu/src/fx.rs @@ -579,10 +579,12 @@ pub(crate) fn paste(shape: &mut [f32], region: Rect, r: Rect, v: &[f32]) { // --------------------------------------------------------------------------------------------- // Shape identity and damage -pub(crate) type Tiles = HashMap>; +/// A shape source's tiles by address: a [`std::sync::Weak`] reserves the address (so a new tile +/// can't reuse it and pass for unchanged) without keeping the pixels alive (#4530). +pub(crate) type Tiles = HashMap>; pub(crate) fn snapshot(s: &Surface) -> Tiles { - s.tiles().map(|(c, t)| (*c, t.clone())).collect() + s.tiles().map(|(c, t)| (*c, Arc::downgrade(t))).collect() } /// Union of the tiles that differ between `old` and `s`. @@ -590,7 +592,7 @@ pub(crate) fn damage(old: &Tiles, s: &Surface) -> Rect { let mut d = Rect::EMPTY; let mut add = |r: Rect| d = if d.is_empty() { r } else { d.union(&r) }; for (c, t) in s.tiles() { - if old.get(c).is_none_or(|o| !Arc::ptr_eq(o, t)) { + if old.get(c).is_none_or(|o| !std::ptr::eq(o.as_ptr(), Arc::as_ptr(t))) { add(c.rect()); } } @@ -642,39 +644,42 @@ pub(crate) fn shape_key(layer: &Layer, canvas: Rect) -> u64 { h.finish() } -/// Deep identity of a group (children, their pixels by tile pointer, settings). -pub(crate) fn group_key(layer: &Layer, light: &GlobalLight) -> u64 { - fn surface_fp(s: &Surface, h: &mut std::collections::hash_map::DefaultHasher) { +/// Deep identity of a group (children, their pixels by tile pointer, settings). `pin` sees every +/// tile hashed, so a cache entry can reserve their addresses (see [`Tiles`]). +pub(crate) fn group_key(layer: &Layer, light: &GlobalLight, pin: &mut dyn FnMut(&Arc)) -> u64 { + type Pin<'a> = &'a mut dyn FnMut(&Arc); + fn surface_fp(s: &Surface, h: &mut std::collections::hash_map::DefaultHasher, pin: Pin) { format!("{:?}{:?}", s.format(), s.default_pixel()).hash(h); for (c, t) in s.tiles() { + pin(t); (c.tx, c.ty, Arc::as_ptr(t) as usize).hash(h); } } - fn walk(l: &Layer, h: &mut std::collections::hash_map::DefaultHasher) { + fn walk(l: &Layer, h: &mut std::collections::hash_map::DefaultHasher, pin: Pin) { l.id.0.hash(h); (l.visible, l.clipped, l.opacity.to_bits(), l.fill_opacity.to_bits(), format!("{:?}", l.blend)).hash(h); match &l.content { LayerContent::Group(g) => { for c in &g.children { - walk(c, h); + walk(c, h, pin); } } LayerContent::Fill(f) => { format!("{f:?}").hash(h); if let Some(c) = &l.fill_cache { - surface_fp(&c.surface, h); + surface_fp(&c.surface, h, pin); } } LayerContent::Adjustment(a) => format!("{a:?}").hash(h), _ => { if let Some(s) = l.surface() { - surface_fp(s, h); + surface_fp(s, h, pin); } } } if let Some(m) = &l.mask { (m.enabled, m.density.to_bits(), m.feather.to_bits()).hash(h); - surface_fp(&m.surface, h); + surface_fp(&m.surface, h, pin); } format!("{:?}{:?}", l.vector_mask, l.effects).hash(h); h.write_u8(0xfe); @@ -684,14 +689,14 @@ pub(crate) fn group_key(layer: &Layer, light: &GlobalLight) -> u64 { match &layer.content { LayerContent::Group(g) => { for c in &g.children { - walk(c, &mut h); + walk(c, &mut h, pin); } if let Some(m) = &layer.mask { (m.enabled, m.density.to_bits(), m.feather.to_bits()).hash(&mut h); - surface_fp(&m.surface, &mut h); + surface_fp(&m.surface, &mut h, pin); } } - _ => walk(layer, &mut h), + _ => walk(layer, &mut h, pin), } h.finish() } @@ -874,4 +879,39 @@ mod tests { let shadow = program(&Effect::DropShadow(s), &light, false); assert!(shadow.fields.iter().any(|(k, _)| *k == FieldKind::StrokeOutside), "the shadow spread keeps the chamfer: {:?}", shadow.fields); } + + /// Cached shapes and group keys reserve tile addresses without keeping pixels (#4530): + /// damage still tells a replaced tile from an unchanged one, and a dropped tile's pixels + /// are freed although the entry remembers it. + #[test] + fn shape_tiles_and_group_pins_hold_addresses_not_pixels() { + use photocraft_color::{ColorMode, PixelFormat, SampleType}; + let fmt = PixelFormat::new(ColorMode::Rgb, SampleType::U8, true); + let mut s = Surface::new(fmt); + // A ramp, so no two tiles share their pixels. + for x in 0..600 { + let v = x as f32 / 600.0; + s.fill_rect(Rect::new(x, 0, x + 1, 300), &[v, 0.2, 0.9, 1.0]); + } + let first = s.tiles().find(|(c, _)| c.tx == 0 && c.ty == 0).map(|(_, t)| t.clone()).unwrap(); + assert_eq!(Arc::strong_count(&first), 2); + let old = snapshot(&s); + assert_eq!(Arc::strong_count(&first), 2, "the snapshot adds no strong reference"); + assert!(damage(&old, &s).is_empty()); + s.fill_rect(Rect::new(0, 0, 4, 4), &[0.0, 0.0, 0.0, 1.0]); + let origin = s.tiles().map(|(c, _)| *c).find(|c| c.tx == 0 && c.ty == 0).unwrap(); + assert_eq!(damage(&old, &s), origin.rect(), "only the edited tile"); + drop(first); + assert!(old.values().any(|w| w.strong_count() == 0), "the replaced tile's pixels are freed"); + + let child = Layer::new("c", LayerContent::Raster(s)); + let n = child.surface().map_or(0, |s| s.tile_count()); + let group = Layer::group("g", vec![child]); + let mut pins = Vec::new(); + let key = group_key(&group, &GlobalLight::default(), &mut |t| pins.push(Arc::downgrade(t))); + assert_eq!(key, group_key(&group, &GlobalLight::default(), &mut |_| {}), "pinning doesn't change the key"); + assert_eq!(pins.len(), n); + drop(group); + assert!(pins.iter().all(|w| w.strong_count() == 0), "a group's pins keep none of its pixels"); + } } diff --git a/crates/gpu/src/lib.rs b/crates/gpu/src/lib.rs index de6ca3e56e..8f8839b9bd 100644 --- a/crates/gpu/src/lib.rs +++ b/crates/gpu/src/lib.rs @@ -49,7 +49,7 @@ use std::sync::Arc; use photocraft_color::{PixelFormat, SampleType}; use photocraft_compose::effects::FieldKind; -use photocraft_doc::{DocId, Document, Layer, LayerContent, LayerId, Pattern}; +use photocraft_doc::{DocId, Document, LayerContent, LayerId, Pattern}; use photocraft_geom::{Rect, TILE_SIZE, TileCoord}; use photocraft_raster::{Surface, Tile}; @@ -303,8 +303,8 @@ struct FxEntry { shape_key: u64, /// Tiles the shape was rendered from (content, mask): damage is their difference. tiles: [Option; 2], - /// Groups: a clone pinning the tile pointers `shape_key` hashes. - _pin: Option, + /// Groups: the tiles `shape_key` hashes, by address only (see [`fx::Tiles`]). + _pin: Vec>, /// The shape on the CPU (distance fields are computed from it) and on the GPU. shape_cpu: Vec, shape: Tex, @@ -1719,7 +1719,7 @@ impl Compositor { let layer = f.layer; let canvas = doc.bounds(); let is_group = matches!(layer.content, LayerContent::Group(_)); - let shape_key = if is_group { fx::group_key(layer, &doc.global_light) } else { fx::shape_key(layer, canvas) }; + let shape_key = if is_group { fx::group_key(layer, &doc.global_light, &mut |_| {}) } else { fx::shape_key(layer, canvas) }; let (content_src, mask_src) = if is_group { (None, None) } else { fx::shape_sources(layer) }; let frame = self.frame; let (rw, rh) = (region.width(), region.height()); @@ -1767,7 +1767,13 @@ impl Compositor { region, shape_key, tiles: [None, None], - _pin: is_group.then(|| layer.clone()), + _pin: if is_group { + let mut pin = Vec::new(); + fx::group_key(layer, &doc.global_light, &mut |t| pin.push(Arc::downgrade(t))); + pin + } else { + Vec::new() + }, shape_cpu: vec![0.0; n], shape, fields: HashMap::new(), diff --git a/crates/ops/src/lib.rs b/crates/ops/src/lib.rs index a63fc559e7..fdf6dcff9f 100644 --- a/crates/ops/src/lib.rs +++ b/crates/ops/src/lib.rs @@ -209,14 +209,55 @@ impl History { self.undo.iter().chain(self.redo.iter()).fold(own, |n, s| n.saturating_add(tile_bytes(&s.doc, &mut seen))) } + /// Bytes of the tiles only `extra` documents hold (History panel snapshots, #4530): not + /// `current`'s and not any state's. 0 without `extra`, without walking the states. + pub fn extra_bytes(&self, current: &Document, extra: &[&Document]) -> usize { + if extra.is_empty() { + return 0; + } + let mut seen = HashSet::new(); + tile_bytes(current, &mut seen); + for s in self.undo.iter().chain(self.redo.iter()) { + tile_bytes(&s.doc, &mut seen); + } + extra.iter().fold(0, |n, d| n.saturating_add(tile_bytes(d, &mut seen))) + } + + /// [`History::unique_bytes`] less the tiles `keep` documents also hold: what clearing the + /// history frees while they stay (Edit › Purge › Histories keeps the snapshots). + pub fn unique_bytes_besides(&self, current: &Document, keep: &[&Document]) -> usize { + let mut seen = HashSet::new(); + tile_bytes(current, &mut seen); + for d in keep { + tile_bytes(d, &mut seen); + } + self.undo.iter().chain(self.redo.iter()).fold(0, |n, s| n.saturating_add(tile_bytes(&s.doc, &mut seen))) + } + /// Keep pixel memory within [`History::max_bytes`]: the current document's tiles plus the /// tiles only history holds (newest states first). The oldest undo states that don't fit are /// dropped; the most recent one is always kept so the last step can be undone. Returns how /// many states were dropped. pub fn trim(&mut self, current: &Document) -> usize { + self.trim_with(current, &[]) + } + + /// [`History::trim`] counting the tiles only `pinned` documents hold (History panel + /// snapshots, which outlive the states) against the budget too, so they can't take memory + /// beyond it unseen (#4530). The most recent state is still always kept. + pub fn trim_with(&mut self, current: &Document, pinned: &[&Document]) -> usize { if self.max_bytes == 0 || self.undo.len() <= 1 { return 0; } + // An upper bound of what only `pinned` holds (some of it may be in states too): if the + // states fit beside it, they fit. + let pinned_bytes = if pinned.is_empty() { + 0 + } else { + let mut seen = HashSet::new(); + tile_bytes(current, &mut seen); + pinned.iter().fold(0usize, |n, d| n.saturating_add(tile_bytes(d, &mut seen))) + }; let tiles = self.tiles.get_or_insert_with(|| { let mut tiles = tiles::Tiles::default(); for state in self.undo.iter().chain(&self.redo) { @@ -224,11 +265,14 @@ impl History { } tiles }); - if tiles.fits(current, self.max_bytes) { + if tiles.fits(current, self.max_bytes.saturating_sub(pinned_bytes)) { return 0; } let mut seen = HashSet::new(); let mut total = tile_bytes(current, &mut seen); + for d in pinned { + total = total.saturating_add(tile_bytes(d, &mut seen)); + } for s in self.redo.iter().rev() { total = total.saturating_add(tile_bytes(&s.doc, &mut seen)); } @@ -392,6 +436,42 @@ mod tests { assert!(h.undo(cur.clone()).is_some()); } + /// Snapshot-only tiles count against the budget (#4530); tiles a snapshot shares with the + /// document or a state count once, and a purge frees only what no snapshot keeps. + #[test] + fn snapshot_tiles_count_against_the_budget() { + let mut h = History::new(50); + let mut cur = Arc::new(base()); + let tile = 256 * 256 * 4; + let paint = |d: &mut Document, v: f32| { + let id = d.layers[0].id; + d.layer_mut(id).unwrap().surface_mut().unwrap().write_pixel(1, 1, &[v, 0.0, 0.0, 1.0]); + }; + // A snapshot holding a tile nothing else does, then one sharing the first state's tile. + let mut own = (*cur).clone(); + paint(&mut own, 0.9); + edit(&mut h, &mut cur, "paint 0", |d| paint(d, 0.1)); + let shared = h.state(0).unwrap(); + for i in 1..6 { + edit(&mut h, &mut cur, &format!("paint {i}"), |d| paint(d, i as f32 / 8.0)); + } + let snaps = [&own, &*shared]; + assert_eq!(h.extra_bytes(&cur, &snaps), tile, "the shared tile is history's"); + assert_eq!(h.extra_bytes(&cur, &[]), 0); + assert_eq!(h.unique_bytes(&cur), 6 * tile); + assert_eq!(h.unique_bytes_besides(&cur, &snaps), 5 * tile, "clearing keeps the shared tile"); + // Both snapshot tiles stay whatever is trimmed (the shared one too), so beside the + // current tile they leave room for two history tiles; without them, four would fit. + h.max_bytes = 5 * tile; + assert_eq!(h.trim_with(&cur, &snaps), 4); + assert_eq!(h.past_len(), 2); + assert_eq!(h.pixel_bytes(&cur) + h.extra_bytes(&cur, &snaps), 5 * tile); + // Still keeps the last step undoable. + h.max_bytes = 1; + h.trim_with(&cur, &snaps); + assert_eq!(h.past_len(), 1); + } + #[test] fn history_shares_untouched_tiles() { let mut h = History::default(); diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 9227c2a0fe..391ad464fd 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -1676,7 +1676,7 @@ fn gpu_budget(app: &mut PhotocraftApp, gpu: &crate::gpu_canvas::GpuCanvas, idx: return; } let Some(st) = app.session.documents().get(idx) else { return }; - let pixels = st.history.pixel_bytes(&st.doc) as u64; + let pixels = st.memory_bytes() as u64; let budget = crate::gpu_canvas::memory_budget(allowance, pixels, crate::gpu_canvas::physical_memory()); gpu.set_memory_budget(budget); app.perf.gpu_budget_mb = budget >> 20;