From 6c12e0bc20ed03776a21ed97f36f0c98cbbdef07 Mon Sep 17 00:00:00 2001 From: vistaero Date: Sat, 10 Oct 2026 07:49:47 +0200 Subject: [PATCH 01/20] Integrate streamed PSB output with the shared save service --- apps/photocraft/src/services.rs | 139 +++++++++++++++++++++- crates/automation/src/files.rs | 2 +- crates/automation/src/headless.rs | 9 +- crates/automation/src/workspace.rs | 28 ++++- crates/engine/src/file_cmds.rs | 22 ++-- crates/format/src/atomic/tests.rs | 33 +++++ crates/io/src/lib.rs | 88 ++++++++++---- crates/io/src/psd_export.rs | 6 +- crates/io/src/psd_stream.rs | 52 ++++++-- crates/io/tests/streamed_save.rs | 102 ++++++++++++++++ crates/io/tests/streaming_psd.rs | 3 +- crates/ui-egui/src/lib.rs | 33 +++-- crates/ui-egui/src/save_identity_tests.rs | 37 ++++++ docs/roadmap.md | 2 + docs/streaming-io.md | 3 +- docs/streaming-save.md | 27 +++++ 16 files changed, 523 insertions(+), 63 deletions(-) create mode 100644 crates/io/tests/streamed_save.rs create mode 100644 docs/streaming-save.md diff --git a/apps/photocraft/src/services.rs b/apps/photocraft/src/services.rs index 3cae110ea3..e28dc6df80 100644 --- a/apps/photocraft/src/services.rs +++ b/apps/photocraft/src/services.rs @@ -273,13 +273,60 @@ fn save_through( Ok(r.warnings) } +/// A save on a worker thread (`Services::save_file`, #2017): encode, then write atomically. +/// Encoding writes an owned temporary, leaving the destination untouched. After the final +/// flush, `commit` makes atomic publication uncancellable. fn save_file( doc: &Document, path: &str, settings: &photocraft_ui_egui::ExportSettings, ctl: &photocraft_ui_egui::jobs_ui::SaveCtl, ) -> Result, String> { - save_through(doc, path, settings, ctl, |p, b, progress| write_atomic_progress(Path::new(p), b, progress)) + if !photocraft_io::needs_psb_stream(doc, path, &export_options(settings)) { + return save_through(doc, path, settings, ctl, |p, b, progress| write_atomic_progress(Path::new(p), b, progress)); + } + ctl.progress(0.0, "Encoding"); + let opts = export_options(settings); + let cancelled = || ctl.check().is_err(); + let progress = |fraction| ctl.progress(fraction * 0.95, "Encoding"); + let interrupt = photocraft_raster::Interrupt::new(&cancelled, &progress); + crate::crash_guard::guard("Save", || { + photocraft_io::export_path_with_commit(doc, Path::new(path), &opts, &interrupt, || { + ctl.commit().map_err(photocraft_io::IoError::Unsupported)?; + ctl.progress(0.97, "Writing"); + Ok(()) + }) + .map_err(|e| e.to_string()) + }) + .inspect(|_| { + ctl.progress(1.0, ""); + }) +} + +fn export_authorized( + workspace: &photocraft_automation::AuthorizedWorkspace, + doc: &Document, + path: &str, + settings: &photocraft_ui_egui::ExportSettings, +) -> Result, String> { + let opts = export_options(settings); + crate::crash_guard::guard("Save", || { + if photocraft_io::needs_psb_stream(doc, path, &opts) { + let mut warnings = Vec::new(); + workspace + .write_stream(path, |out| { + warnings = photocraft_io::export_psb_into_with_options(doc, out, &opts, &photocraft_raster::Interrupt::NONE) + .map_err(|e| std::io::Error::other(e.to_string()))?; + Ok(()) + }) + .map_err(|e| e.to_string())?; + Ok(warnings) + } else { + let result = photocraft_io::export(doc, path, &opts).map_err(|e| e.to_string())?; + workspace.write(path, &result.bytes).map_err(|e| e.to_string())?; + Ok(result.warnings) + } + }) } type SharedRecovery = Rc>>; @@ -411,6 +458,10 @@ pub fn native(automation: Option) -> crate::crash_guard::guard("Open", || workspace.read_document(path, ctl).map_err(|e| e.to_string())) }) as photocraft_ui_egui::AutomationImportPathFn }); + let automation_export_path = automation.clone().map(|workspace| { + Box::new(move |doc: &Document, path: &str, settings: &photocraft_ui_egui::ExportSettings| export_authorized(&workspace, doc, path, settings)) + as photocraft_ui_egui::ExportPathFn + }); let automation_read = automation.clone().map(|workspace| { Box::new(move |path: &str| { let bytes = workspace.read(path).map_err(|error| error.to_string())?; @@ -430,7 +481,32 @@ pub fn native(automation: Option) -> }); let automation_save = automation.clone().map(|workspace| { std::sync::Arc::new(move |doc: &Document, path: &str, settings: &photocraft_ui_egui::ExportSettings, ctl: &photocraft_ui_egui::jobs_ui::SaveCtl| { - save_through(doc, path, settings, ctl, |p, b, _| workspace.write(p, b).map_err(|e| e.to_string())) + if !photocraft_io::needs_psb_stream(doc, path, &export_options(settings)) { + return save_through(doc, path, settings, ctl, |p, b, _| workspace.write(p, b).map_err(|e| e.to_string())); + } + let cancelled = || ctl.check().is_err(); + let progress = |p| ctl.progress(p * 0.95, "Encoding"); + let interrupt = photocraft_raster::Interrupt::new(&cancelled, &progress); + crate::crash_guard::guard("Save", || { + let mut warnings = Vec::new(); + workspace + .write_stream_with_commit( + path, + |out| { + warnings = photocraft_io::export_psb_into_with_options(doc, out, &export_options(settings), &interrupt) + .map_err(|e| std::io::Error::other(e.to_string()))?; + Ok(()) + }, + || { + ctl.commit().map_err(std::io::Error::other)?; + ctl.progress(0.97, "Writing"); + Ok(()) + }, + ) + .map_err(|e| e.to_string())?; + ctl.progress(1.0, ""); + Ok(warnings) + }) }) as photocraft_ui_egui::SaveFileFn }); let step: fn(&str, &serde_json::Value) -> photocraft_engine::Result<()> = photocraft_automation::workspace::authorize_desktop_engine_step; @@ -450,6 +526,11 @@ pub fn native(automation: Option) -> let opts = export_options(settings); crate::crash_guard::guard("Export", || photocraft_io::export(doc, path, &opts).map(|r| (r.bytes, r.warnings)).map_err(|e| e.to_string())) })), + export_path: Some(Box::new(|doc, path, settings| { + crate::crash_guard::guard("Save", || { + photocraft_io::export_path_with(doc, Path::new(path), &export_options(settings), &photocraft_raster::Interrupt::NONE).map_err(|e| e.to_string()) + }) + })), save_file: Some(Arc::new(save_file)), file_dialog: Some(Box::new(show_file_dialog)), write: Some(Box::new(|path: &str, bytes: &[u8]| write_atomic(Path::new(path), bytes))), @@ -458,6 +539,7 @@ pub fn native(automation: Option) -> automation_write, automation_open_pcraft, automation_save, + automation_export_path, automation_command, automation_authorize, encode_png: Some(Box::new(|w, h, rgba| { @@ -603,6 +685,59 @@ mod tests { use photocraft_ui_egui::{PhotocraftApp, prefs_ui}; use serde_json::{Value, json}; + #[test] + fn native_streamed_save_finishes_the_original_document_after_a_tab_switch() { + let dir = temp("streamed-save-job"); + let path = dir.join("original.psb").to_string_lossy().into_owned(); + let mut session = Session::new(); + session.execute("file.new", json!({"width":32,"height":16,"depth":16})).unwrap(); + session.execute("layer.new.layer", json!({})).unwrap(); + session.execute("edit.fill", json!({"color":"#336699"})).unwrap(); + let original = session.active().unwrap().doc.id; + let revision = session.active().unwrap().revision; + let services = Services { save_file: Some(Arc::new(save_file)), ..Default::default() }; + let mut app = PhotocraftApp::new(session, services); + app.background_jobs = true; + let reply = app.save_as(Some(path.clone())).unwrap(); + assert_eq!(reply["pending"], true); + assert!(app.session.execute("edit.fill", json!({"color":"#ffffff"})).is_err(), "upstream save lock is retained"); + app.session.execute("file.new", json!({"width":4,"height":4})).unwrap(); + let current = app.session.active().unwrap().doc.id; + let context = egui::Context::default(); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(15); + while app.session.has_jobs() && std::time::Instant::now() < deadline { + photocraft_ui_egui::jobs_ui::tick(&mut app, &context); + std::thread::sleep(std::time::Duration::from_millis(2)); + } + assert!(!app.session.has_jobs()); + assert_eq!(app.session.active().unwrap().doc.id, current); + let saved = app.session.documents().iter().find(|s| s.doc.id == original).unwrap(); + assert_eq!(saved.path.as_deref(), Some(path.as_str())); + assert_eq!(saved.saved_revision, revision); + let reopened = photocraft_io::import_path_with("saved", Path::new(&path), &photocraft_raster::Interrupt::NONE).unwrap(); + assert_eq!(reopened.document.size, saved.doc.size); + assert_eq!(reopened.document.depth, saved.doc.depth); + assert_eq!(photocraft_compose::flatten(&reopened.document).px, photocraft_compose::flatten(&saved.doc).px); + } + + #[test] + fn native_authorized_stream_save_rejects_ambient_and_parent_paths() { + let dir = temp("streamed-save-authority"); + let workspace = photocraft_automation::AuthorizedWorkspace::new(Some(&dir), Some(&dir)).unwrap(); + let mut session = Session::new(); + session.execute("file.new", json!({"width":4,"height":4})).unwrap(); + let doc = &session.active().unwrap().doc; + let settings = photocraft_ui_egui::ExportSettings::default(); + assert!(export_authorized(&workspace, doc, "../outside.psb", &settings).is_err()); + let absolute = dir.join("absolute.psb"); + export_authorized(&workspace, doc, &absolute.to_string_lossy(), &settings).unwrap(); + assert!(export_authorized(&workspace, doc, &dir.parent().unwrap().join("outside.psb").to_string_lossy(), &settings).is_err()); + export_authorized(&workspace, doc, "allowed.psb", &settings).unwrap(); + assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 2); + let reopened = workspace.read_document("allowed.psb", &photocraft_raster::Interrupt::NONE).unwrap(); + assert_eq!(reopened.document.size, doc.size); + } + #[cfg(all(unix, not(target_os = "macos")))] #[test] fn open_filters_cover_uppercase_and_mixed_case_extensions() { diff --git a/crates/automation/src/files.rs b/crates/automation/src/files.rs index 61c6ba246a..d119d8da7b 100644 --- a/crates/automation/src/files.rs +++ b/crates/automation/src/files.rs @@ -82,7 +82,7 @@ pub fn save( return Ok(Vec::new()); } #[cfg(not(target_arch = "wasm32"))] - if ext == "psb" || ext == "psd" && (opts.force_psb || doc.size.width > 30_000 || doc.size.height > 30_000) { + if photocraft_io::needs_psb_stream(doc, &format!("output.{ext}"), opts) { let mut warnings = Vec::new(); photocraft_format::atomic::atomic_write_stream(path, |file| { warnings = photocraft_io::export_psb_into(doc, file, &photocraft_raster::Interrupt::NONE).map_err(|e| std::io::Error::other(e.to_string()))?; diff --git a/crates/automation/src/headless.rs b/crates/automation/src/headless.rs index 3a61ffa4dc..f036e4b5f5 100644 --- a/crates/automation/src/headless.rs +++ b/crates/automation/src/headless.rs @@ -260,10 +260,11 @@ impl Headless { } Filesystem::Workspace(workspace) => { #[cfg(not(target_arch = "wasm32"))] - if format - .map(|f| f.trim_start_matches('.').eq_ignore_ascii_case("psb")) - .unwrap_or_else(|| target.extension().is_some_and(|ext| ext.eq_ignore_ascii_case("psb"))) - { + if photocraft_io::needs_psb_stream( + &doc, + &format.map(|f| format!("output.{}", f.trim_start_matches('.'))).unwrap_or_else(|| target_text.to_string()), + opts, + ) { let mut warnings = Vec::new(); workspace.write_stream(target_text, |file| { warnings = photocraft_io::export_psb_into(&doc, file, &photocraft_raster::Interrupt::NONE) diff --git a/crates/automation/src/workspace.rs b/crates/automation/src/workspace.rs index 6321bc241f..a379b15c80 100644 --- a/crates/automation/src/workspace.rs +++ b/crates/automation/src/workspace.rs @@ -102,6 +102,16 @@ impl AuthorizedWorkspace { /// Atomically publish a seekable encoder through the held write capability. No ambient /// paths are exposed to the encoder, including for temporary-file creation and rename. pub fn write_stream(&self, path: &str, encode: impl FnOnce(&mut cap_std::fs::File) -> std::io::Result<()>) -> Result<(), AutomationError> { + self.write_stream_with_commit(path, encode, || Ok(())) + } + + /// Approve publication after encoding and flushing; rejection preserves the old destination. + pub fn write_stream_with_commit( + &self, + path: &str, + encode: impl FnOnce(&mut cap_std::fs::File) -> std::io::Result<()>, + commit: impl FnOnce() -> std::io::Result<()>, + ) -> Result<(), AutomationError> { struct Cleanup(Arc, PathBuf); impl Drop for Cleanup { fn drop(&mut self) { @@ -120,7 +130,9 @@ impl AuthorizedWorkspace { // This binding drops before the cleanup guard if an encoder unwinds. let mut file = file; let written = encode(&mut file).and_then(|()| photocraft_format::atomic::sync_file(&file.into_std())); - let renamed = written.and_then(|()| photocraft_format::atomic::retry_rename(RenameRetry::platform(), || root.dir.rename(&tmp, &root.dir, &relative))); + let renamed = written + .and_then(|()| commit()) + .and_then(|()| photocraft_format::atomic::retry_rename(RenameRetry::platform(), || root.dir.rename(&tmp, &root.dir, &relative))); if let Err(e) = renamed { let _ = root.dir.remove_file(&tmp); return Err(file_error("write", path, e)); @@ -524,6 +536,20 @@ mod tests { assert_eq!(std::fs::read(inside.join("kept.psb")).unwrap(), b"0007payload"); } + #[test] + fn seekable_capability_rejected_commit_preserves_destination_and_cleans_temporary() { + let (inside, _, workspace) = roots("stream-commit-rejected"); + std::fs::write(inside.join("kept.psb"), b"original").unwrap(); + let result = workspace.write_stream_with_commit( + "kept.psb", + |file| file.write_all(b"replacement"), + || Err(std::io::Error::new(std::io::ErrorKind::Interrupted, "cancelled before publication")), + ); + assert!(result.is_err()); + assert_eq!(std::fs::read(inside.join("kept.psb")).unwrap(), b"original"); + assert!(temp_files(&inside).is_empty()); + } + #[test] fn seekable_capability_unwind_removes_temporary_after_closing_handle() { let (inside, _, workspace) = roots("stream-unwind"); diff --git a/crates/engine/src/file_cmds.rs b/crates/engine/src/file_cmds.rs index 58c4e6ca7f..d616a24350 100644 --- a/crates/engine/src/file_cmds.rs +++ b/crates/engine/src/file_cmds.rs @@ -331,10 +331,7 @@ pub fn dpx_bits_param(p: &Value, cmd: &str) -> Result) -> Result<(Vec, Vec)> { - crate::allocation::checkpoint("saving document")?; - let save = save.into(); +fn save_options(doc: &Document, save: SaveOpts) -> photocraft_io::ExportOptions { let mut opts = photocraft_io::ExportOptions { tiff_layers: save.tiff_layers, tga_bits: save.tga_bits, @@ -353,20 +350,21 @@ pub(crate) fn encode(doc: &Document, path: &str, save: impl Into) -> R opts.encode.webp_quality = q; opts.encode.webp_lossless = false; } + opts +} + +/// Encodes `doc` for `path`'s extension. +pub(crate) fn encode(doc: &Document, path: &str, save: impl Into) -> Result<(Vec, Vec)> { + crate::allocation::checkpoint("saving document")?; + let opts = save_options(doc, save.into()); photocraft_io::export(doc, path, &opts).map(|r| (r.bytes, r.warnings)).map_err(|e| EngineError::Other(format!("{path}: {e}"))) } pub(crate) fn save_doc(doc: &Document, path: &str, save: impl Into) -> Result> { #[cfg(not(target_arch = "wasm32"))] { - let save = save.into(); - let mut opts = photocraft_io::ExportOptions { tiff_layers: save.tiff_layers, ..Default::default() }; - if let Some(q) = save.quality { - let q = (q.clamp(0.0, 12.0) / 12.0 * 99.0 + 1.0).round() as u8; - opts.encode.jpeg_quality = q; - opts.encode.webp_quality = q; - opts.encode.webp_lossless = false; - } + crate::allocation::checkpoint("saving document")?; + let opts = save_options(doc, save.into()); photocraft_io::export_path_with(doc, std::path::Path::new(path), &opts, &photocraft_raster::Interrupt::NONE) .map_err(|e| EngineError::Other(format!("{path}: {e}"))) } diff --git a/crates/format/src/atomic/tests.rs b/crates/format/src/atomic/tests.rs index eef149d476..9b7b176246 100644 --- a/crates/format/src/atomic/tests.rs +++ b/crates/format/src/atomic/tests.rs @@ -349,3 +349,36 @@ fn streaming_encoder_unwind_closes_and_removes_temporary_file() { assert_eq!(std::fs::read(&p).unwrap(), ORIGINAL); assert_no_temp(&d.0); } + +#[cfg(windows)] +#[test] +fn streaming_read_only_temporary_is_removed_on_rejected_or_unwinding_commit() { + for unwind in [false, true] { + let dir = TempDir::new("stream-read-only"); + let path = dir.0.join("document.psb"); + std::fs::write(&path, ORIGINAL).unwrap(); + let mut permissions = std::fs::metadata(&path).unwrap().permissions(); + permissions.set_readonly(true); + std::fs::set_permissions(&path, permissions).unwrap(); + let result = std::panic::catch_unwind(|| { + atomic_write_stream_with_commit( + &path, + |file| file.write_all(b"complete"), + || { + if unwind { + panic!("injected read-only commit panic"); + } + Err(io::Error::new(io::ErrorKind::Interrupted, "cancelled before commit")) + }, + ) + }); + if unwind { + assert!(result.is_err()); + } else { + assert_eq!(result.unwrap().unwrap_err().kind(), io::ErrorKind::Interrupted); + } + assert_eq!(std::fs::read(&path).unwrap(), ORIGINAL); + assert!(std::fs::metadata(&path).unwrap().permissions().readonly()); + assert_no_temp(&dir.0); + } +} diff --git a/crates/io/src/lib.rs b/crates/io/src/lib.rs index 0cb2b481f9..15f72cf26f 100644 --- a/crates/io/src/lib.rs +++ b/crates/io/src/lib.rs @@ -502,36 +502,84 @@ pub fn export_with(doc: &Document, name_or_ext: &str, opts: &ExportOptions, ctl: /// formats with only a bytes encoder use the checked working-set admission in [`export`]. #[cfg(not(target_arch = "wasm32"))] pub fn export_path_with(doc: &Document, path: &std::path::Path, opts: &ExportOptions, ctl: &photocraft_raster::Interrupt<'_>) -> Result, IoError> { + export_path_with_commit(doc, path, opts, ctl, || Ok(())) +} + +/// Whether native output needs the bounded PSB writer, including automatic PSD size promotion. +#[cfg(not(target_arch = "wasm32"))] +pub fn needs_psb_stream(doc: &Document, name: &str, opts: &ExportOptions) -> bool { + let ext = extension(name); + ext == "psb" || ext == "psd" && (opts.force_psb || doc.size.width > 30_000 || doc.size.height > 30_000 || psd_export::psd_size_exceeds_limit(doc)) +} + +/// Encode and flush an owned temporary, then call `commit` immediately before publication. +/// After commit succeeds, no cancellation check may turn a successful publication into failure. +#[cfg(not(target_arch = "wasm32"))] +pub fn export_path_with_commit( + doc: &Document, + path: &std::path::Path, + opts: &ExportOptions, + ctl: &photocraft_raster::Interrupt<'_>, + commit: impl FnOnce() -> Result<(), IoError>, +) -> Result, IoError> { + use std::io::Write; ctl.check().map_err(|_| IoError::Cancelled)?; - let ext = path.extension().and_then(|s| s.to_str()).unwrap_or("").to_ascii_lowercase(); - if ext == "psb" || ext == "psd" && (opts.force_psb || doc.size.width > 30_000 || doc.size.height > 30_000) { + let mut warnings = Vec::new(); + let mut commit_error = None; + let streaming = needs_psb_stream(doc, &path.to_string_lossy(), opts); + let encoded = if streaming { photocraft_raster::spill::check_integrity().map_err(IoError::Unsupported)?; - let mut warnings = Vec::new(); - let result = photocraft_format::atomic::atomic_write_stream_with_commit( - path, - |out| { - warnings = psd_stream::export_into(doc, out, *ctl).map_err(|e| std::io::Error::other(e.to_string()))?; + None + } else { + let result = export_with(doc, &path.to_string_lossy(), opts, ctl)?; + warnings = result.warnings; + Some(result.bytes) + }; + let result = photocraft_format::atomic::atomic_write_stream_with_commit( + path, + |out| { + if let Some(bytes) = encoded { + out.write_all(&bytes) + } else { + warnings = psd_stream::export_into(doc, out, opts, *ctl).map_err(|e| std::io::Error::other(e.to_string()))?; Ok(()) - }, - || ctl.check().map_err(|e| std::io::Error::new(std::io::ErrorKind::Interrupted, e)), - ); - if result.is_err() && ctl.cancelled() { - return Err(IoError::Cancelled); - } - result.map_err(|e| IoError::Unsupported(e.to_string()))?; - return Ok(warnings); + } + }, + || { + ctl.check().map_err(|e| std::io::Error::new(std::io::ErrorKind::Interrupted, e))?; + commit().map_err(|e| { + let message = e.to_string(); + commit_error = Some(e); + std::io::Error::other(message) + }) + }, + ); + if let Some(error) = commit_error { + return Err(error); } - let result = export(doc, &path.to_string_lossy(), opts)?; - ctl.check().map_err(|_| IoError::Cancelled)?; - photocraft_format::atomic_write(path, &result.bytes).map_err(|e| IoError::Unsupported(e.to_string()))?; - Ok(result.warnings) + if result.is_err() && ctl.cancelled() { + return Err(IoError::Cancelled); + } + result.map_err(|e| IoError::Unsupported(e.to_string()))?; + Ok(warnings) } /// Write a native PSB to an already authorized seekable output. The caller owns atomic /// publication and cleanup; this never opens an ambient path. #[cfg(not(target_arch = "wasm32"))] pub fn export_psb_into(doc: &Document, out: &mut W, ctl: &photocraft_raster::Interrupt<'_>) -> Result, IoError> { - psd_stream::export_into(doc, out, *ctl) + export_psb_into_with_options(doc, out, &ExportOptions { image_previews: false, ..Default::default() }, ctl) +} + +/// Seekable PSB output honoring the same compatibility and preview settings as byte exports. +#[cfg(not(target_arch = "wasm32"))] +pub fn export_psb_into_with_options( + doc: &Document, + out: &mut W, + opts: &ExportOptions, + ctl: &photocraft_raster::Interrupt<'_>, +) -> Result, IoError> { + psd_stream::export_into(doc, out, opts, *ctl) } /// Run `f` with an interrupt whose progress `0..=1` fills `lo..=hi` of `ctl`'s (same cancellation). diff --git a/crates/io/src/psd_export.rs b/crates/io/src/psd_export.rs index e2117b237e..f5c5e9e943 100644 --- a/crates/io/src/psd_export.rs +++ b/crates/io/src/psd_export.rs @@ -1369,9 +1369,9 @@ pub fn document_to_psd_with(doc: &Document, opts: &PsdExportOptions) -> (PsdFile } #[cfg(not(target_arch = "wasm32"))] -pub(crate) fn stream_metadata(doc: &Document, opts: &PsdExportOptions) -> (PsdFile, Vec, StreamSources) { +pub(crate) fn stream_metadata(doc: &Document, opts: &PsdExportOptions, ctl: &Interrupt) -> (PsdFile, Vec, StreamSources) { let mut sources = StreamSources::new(); - let (file, warnings) = document_to_psd_nested(doc, opts, 0, &Interrupt::NONE, Some(&mut sources)); + let (file, warnings) = document_to_psd_nested(doc, opts, 0, ctl, Some(&mut sources)); (file, warnings, sources) } @@ -1622,7 +1622,7 @@ fn document_to_psd_nested( // live nowhere else. let real_merged = opts.maximize_compatibility || ex.records.is_empty(); let (mut planes, has_alpha) = if ex.deferred { - (Vec::new(), true) + (Vec::new(), real_merged) } else if real_merged { // Merged composite, rendered and encoded in bands (no full-size float composite). Matting // against white only changes pixels with alpha < 1; if some are slightly translucent but diff --git a/crates/io/src/psd_stream.rs b/crates/io/src/psd_stream.rs index c11a9e593b..3b303f614f 100644 --- a/crates/io/src/psd_stream.rs +++ b/crates/io/src/psd_stream.rs @@ -185,7 +185,12 @@ fn sink_error(e: std::io::Error) -> photocraft_psd::PsdError { photocraft_psd::PsdError::Invalid(format!("stream output: {e}")) } -pub(crate) fn export_into(doc: &photocraft_doc::Document, out: &mut W, ctl: Interrupt<'_>) -> Result, IoError> { +pub(crate) fn export_into( + doc: &photocraft_doc::Document, + out: &mut W, + options: &crate::ExportOptions, + ctl: Interrupt<'_>, +) -> Result, IoError> { if !matches!(doc.mode, ColorMode::Rgb | ColorMode::Grayscale | ColorMode::Cmyk | ColorMode::Lab) { return Err(IoError::Unsupported("seekable PSB export requires RGB, Gray, CMYK or Lab".into())); } @@ -218,8 +223,25 @@ pub(crate) fn export_into(doc: &photocraft_doc::Document, out: .saturating_add(metadata.saturating_mul(4)) .saturating_add(embedded.saturating_mul(4)); let _working = photocraft_raster::memory::try_reserve(working).map_err(IoError::Unsupported)?; - let opts = crate::psd_export::PsdExportOptions { force_psb: true, ..Default::default() }; - let (file, warnings, sources) = crate::psd_export::stream_metadata(doc, &opts); + let opts = crate::psd_export::PsdExportOptions { force_psb: true, maximize_compatibility: options.psd_maximize_compatibility, ..Default::default() }; + let (mut file, mut warnings, sources) = crate::with_span(&ctl, 0.0, 0.1, |ctl| crate::psd_export::stream_metadata(doc, &opts, ctl)); + ctl.check().map_err(|_| IoError::Cancelled)?; + if !doc.metadata.text.is_empty() { + warnings.push("free-form text metadata is not supported by PSD/PSB; it will be dropped".into()); + } + warnings.extend(crate::tiff_layers::strip_foreign_order_blocks(&mut file)); + if options.image_previews { + match crate::psd_thumbnail::resource(doc) { + Ok(resource) => { + let at = file.resources.iter().position(|r| r.id == photocraft_psd::resources::ids::VERSION_INFO).unwrap_or(file.resources.len()); + file.resources.insert(at, resource); + } + Err(e) => warnings.push(format!("image preview not saved: {e}")), + } + } + file.header.validate()?; + let merged_channels = fmt.mode.color_channels() + usize::from(file.merged_has_alpha()); + let real_merged = options.psd_maximize_compatibility || file.layers().is_empty(); let check = || -> photocraft_psd::Result<()> { if ctl.cancelled() { return Err(photocraft_psd::PsdError::Invalid("stream export cancelled".into())); @@ -231,6 +253,7 @@ pub(crate) fn export_into(doc: &photocraft_doc::Document, out: out, |i, id, out| { check()?; + ctl.progress(0.15 + 0.35 * i as f32 / file.layers().len().max(1) as f32); let rec = file.layers().get(i).ok_or_else(|| photocraft_psd::PsdError::Invalid("missing output record".into()))?; let source = rec.layer_id().and_then(|id| sources.get(&id)).and_then(|(pixels, mask)| if id == -2 { mask.as_ref() } else { pixels.as_ref() }); let r = rec.channel_rect(id); @@ -270,9 +293,9 @@ pub(crate) fn export_into(doc: &photocraft_doc::Document, out: let cc = fmt.mode.color_channels(); let white = photocraft_raster::from_rgba(&fmt, [1.0; 4]); let space = photocraft_compose::cmyk_space(doc); - photocraft_compose::render_bands(doc, doc.bounds(), 64, |band| -> photocraft_psd::Result<()> { + let mut write_band = |band: photocraft_compose::Buffer| -> photocraft_psd::Result<()> { check()?; - let mut planes: Vec> = (0..=cc).map(|_| Vec::with_capacity(band.px.len() * fmt.sample.bytes())).collect(); + let mut planes: Vec> = (0..merged_channels).map(|_| Vec::with_capacity(band.px.len() * fmt.sample.bytes())).collect(); photocraft_color::convert::with_cmyk_space(space.as_ref(), || { let mut v = [0.0; 5]; for p in &band.px { @@ -304,9 +327,22 @@ pub(crate) fn export_into(doc: &photocraft_doc::Document, out: } ctl.progress(0.5 + 0.5 * band.rect.y1 as f32 / doc.size.height.max(1) as f32); Ok(()) - })?; - for (i, channel) in doc.channels.iter().chain(doc.quick_mask.iter()).take(usize::from(file.header.channels).saturating_sub(cc + 1)).enumerate() { - let index = cc + 1 + i; + }; + if real_merged { + photocraft_compose::render_bands(doc, doc.bounds(), 64, &mut write_band)?; + } else { + let mut y = 0; + while y < doc.size.height { + let end = y.saturating_add(64).min(doc.size.height); + let rect = Rect::from_xywh(0, y as i32, doc.size.width, end - y); + write_band(photocraft_compose::Buffer { rect, px: vec![[1.0; 4]; doc.size.width as usize * (end - y) as usize] })?; + y = end; + } + } + for (i, channel) in + doc.channels.iter().chain(doc.quick_mask.iter()).take(usize::from(file.header.channels).saturating_sub(merged_channels)).enumerate() + { + let index = merged_channels + i; let mask_fmt = photocraft_color::PixelFormat::new(ColorMode::Grayscale, fmt.sample, false); let converted = (channel.surface.format() != mask_fmt).then(|| channel.surface.convert(mask_fmt)); let surface = converted.as_ref().unwrap_or(&channel.surface); diff --git a/crates/io/tests/streamed_save.rs b/crates/io/tests/streamed_save.rs new file mode 100644 index 0000000000..252ba12b69 --- /dev/null +++ b/crates/io/tests/streamed_save.rs @@ -0,0 +1,102 @@ +#![cfg(not(target_arch = "wasm32"))] + +use photocraft_color::{ColorMode, SampleType}; +use photocraft_doc::{Document, Layer, Size}; +use photocraft_io::{ExportOptions, IoError, export_path_with_commit, needs_psb_stream}; +use photocraft_raster::Interrupt; +use std::sync::atomic::{AtomicBool, Ordering}; + +struct TempRoot(std::path::PathBuf); +impl TempRoot { + fn new() -> Self { + static NEXT: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + let nonce = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos(); + let path = std::env::temp_dir().join(format!("photocraft-save-{}-{nonce}-{}", std::process::id(), NEXT.fetch_add(1, Ordering::Relaxed))); + std::fs::create_dir(&path).unwrap(); + Self(path) + } + fn path(&self) -> &std::path::Path { + &self.0 + } +} +impl Drop for TempRoot { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +fn document() -> Document { + let mut doc = Document::new("save", Size::new(32, 16), ColorMode::Rgb, SampleType::U16); + let mut layer = Layer::raster("pixels", doc.pixel_format()); + layer.surface_mut().unwrap().fill_rect(doc.bounds(), &[0.25, 0.5, 0.75, 1.0]); + doc.layers.push(layer); + doc +} + +#[test] +fn all_native_formats_share_the_final_commit_boundary() { + let root = TempRoot::new(); + let doc = document(); + for ext in ["psb", "png"] { + let target = root.path().join(format!("document.{ext}")); + std::fs::write(&target, b"precious original").unwrap(); + let result = export_path_with_commit(&doc, &target, &ExportOptions::default(), &Interrupt::NONE, || { + assert_eq!(std::fs::read(&target).unwrap(), b"precious original"); + Err(IoError::Cancelled) + }); + assert!(matches!(result, Err(IoError::Cancelled))); + assert_eq!(std::fs::read(&target).unwrap(), b"precious original"); + assert_eq!(std::fs::read_dir(root.path()).unwrap().count(), if ext == "psb" { 1 } else { 2 }); + } +} + +#[test] +fn cancellation_after_commit_does_not_report_a_published_save_as_failed() { + let root = TempRoot::new(); + let doc = document(); + for ext in ["psb", "png"] { + let target = root.path().join(format!("document.{ext}")); + let cancelled = AtomicBool::new(false); + let check = || cancelled.load(Ordering::Relaxed); + let interrupt = Interrupt::cancel_only(&check); + export_path_with_commit(&doc, &target, &ExportOptions::default(), &interrupt, || { + cancelled.store(true, Ordering::Relaxed); + Ok(()) + }) + .unwrap(); + let reopened = photocraft_io::import_path_with("saved", &target, &Interrupt::NONE).unwrap(); + assert_eq!(reopened.document.size, doc.size); + } +} + +#[test] +fn automatic_psd_size_promotion_uses_the_streaming_writer() { + let opts = ExportOptions::default(); + let small = document(); + assert!(!needs_psb_stream(&small, "small.PSD", &opts)); + assert!(needs_psb_stream(&small, "small.PSB", &opts)); + let sparse = Document::new("large", Size::new(30_000, 30_000), ColorMode::Rgb, SampleType::U8); + assert!(needs_psb_stream(&sparse, "large.psd", &opts)); + assert!(!needs_psb_stream(&sparse, "large.png", &opts)); +} + +#[test] +fn streaming_save_respects_compatibility_and_thumbnail_options() { + let doc = document(); + for compatibility in [false, true] { + for previews in [false, true] { + let options = ExportOptions { psd_maximize_compatibility: compatibility, image_previews: previews, ..Default::default() }; + let reference = photocraft_io::export(&doc, "reference.psb", &options).unwrap(); + let reference = photocraft_psd::PsdFile::from_bytes(&reference.bytes).unwrap(); + let mut output = std::io::Cursor::new(Vec::new()); + photocraft_io::export_psb_into_with_options(&doc, &mut output, &options, &Interrupt::NONE).unwrap(); + let output = photocraft_psd::PsdFile::from_bytes(output.get_ref()).unwrap(); + assert_eq!(output.resources, reference.resources, "compatibility={compatibility}, previews={previews}"); + let expected = reference.image_data.decode(&reference.header).unwrap(); + let actual = output.image_data.decode(&output.header).unwrap(); + // Streaming keeps a merged alpha plane for compatibility; the colour planes agree. + assert_eq!(&actual[..3], &expected[..3], "compatibility={compatibility}"); + assert_eq!(output.layers().len(), reference.layers().len()); + } + } +} diff --git a/crates/io/tests/streaming_psd.rs b/crates/io/tests/streaming_psd.rs index 0a8038b3ac..1a9eca6b44 100644 --- a/crates/io/tests/streaming_psd.rs +++ b/crates/io/tests/streaming_psd.rs @@ -64,7 +64,8 @@ fn seekable_export_preserves_merged_layers_masks_and_unknown_metadata() { let input = photocraft_psd::testgen::layered(Version::Psb, mode, depth, Compression::ZipPrediction); let doc = photocraft_io::import("input", &input.to_bytes().unwrap()).unwrap().document; let (reference, _) = photocraft_io::document_to_psd_with(&doc, &photocraft_io::PsdExportOptions { force_psb: true, ..Default::default() }); - photocraft_io::export_path_with(&doc, &path, &Default::default(), &Interrupt::NONE).unwrap(); + photocraft_io::export_path_with(&doc, &path, &photocraft_io::ExportOptions { image_previews: false, ..Default::default() }, &Interrupt::NONE) + .unwrap(); let output = photocraft_psd::PsdFile::from_bytes(&std::fs::read(&path).unwrap()).unwrap(); assert_eq!(output.header, reference.header, "{mode:?}/{depth}"); assert_eq!(output.resources, reference.resources); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 10d050d5e4..7506e836a5 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -312,10 +312,13 @@ pub(crate) fn plain_raster(doc: &Document) -> Option<&photocraft_doc::Layer> { /// Encode a document: (file bytes, warnings about anything approximated or dropped). pub type ExportFn = Box Result<(Vec, Vec), String>>; +/// Synchronous native path encoder, with bounded PSB output and owned atomic publication. +pub type ExportPathFn = Box Result, String>>; pub type WriteFn = Box Result<(), String>>; /// Encode a document and write it to a path, reporting the stage to the job; returns the export /// warnings. Runs on a worker thread (see [`Services::save_file`]). It must call -/// [`jobs_ui::SaveCtl::commit`] right before replacing the file, and write nothing if that fails. +/// [`jobs_ui::SaveCtl::commit`] right before replacing the file. A rejected commit must retain +/// the destination and remove any temporary created during encoding. pub type SaveFileFn = std::sync::Arc Result, String> + Send + Sync>; /// Who a finished save serves. @@ -408,6 +411,7 @@ pub struct Services { /// Background save over the automation write authority (`app.save`, #2017): same worker /// contract as `save_file`, but the bytes go through the capability-scoped workspace. pub automation_save: Option, + pub export_path: Option, /// Show an Open or Save dialog without waiting for it (see `file_dialog`). pub file_dialog: Option, /// Write bytes to a path (native) or trigger a download (web). @@ -421,6 +425,7 @@ pub struct Services { pub automation_import_path: Option, pub automation_write: Option, pub automation_open_pcraft: Option, + pub automation_export_path: Option, pub automation_command: Option, /// Same policy as [`Self::automation_command`], as a function pointer the engine calls for /// each step of `actions.play`. Installed on the session only while a control request or @@ -1488,10 +1493,15 @@ impl PhotocraftApp { } let st = self.session.active().ok_or("no document")?; let (doc, revision) = (st.doc.id, st.revision); - let export = self.services.export.as_ref().ok_or("no exporter configured")?; - let (bytes, warnings) = export(&st.doc, &path, settings)?; - let write = self.services.write.as_mut().ok_or("no writer configured")?; - write(&path, &bytes)?; + let warnings = if let Some(export) = self.services.export_path.as_ref() { + export(&st.doc, &path, settings)? + } else { + let export = self.services.export.as_ref().ok_or("no exporter configured")?; + let (bytes, warnings) = export(&st.doc, &path, settings)?; + let write = self.services.write.as_mut().ok_or("no writer configured")?; + write(&path, &bytes)?; + warnings + }; self.saved(doc, revision, &path, &warnings, copy); Ok(Some((path, warnings))) } @@ -1602,10 +1612,15 @@ impl PhotocraftApp { let state = self.session.active().ok_or("no document")?; let (doc, revision) = (state.doc.id, state.revision); let settings = ExportSettings { tga_bits, ..self.save_settings() }; - let export = self.services.export.as_ref().ok_or("no exporter configured")?; - let (bytes, warnings) = export(&state.doc, &target, &settings)?; - let write = self.services.automation_write.as_mut().ok_or("automation write authority is not configured")?; - write(&target, &bytes)?; + let warnings = if let Some(export) = self.services.automation_export_path.as_ref() { + export(&state.doc, &target, &settings)? + } else { + let export = self.services.export.as_ref().ok_or("no exporter configured")?; + let (bytes, warnings) = export(&state.doc, &target, &settings)?; + let write = self.services.automation_write.as_mut().ok_or("automation write authority is not configured")?; + write(&target, &bytes)?; + warnings + }; self.saved_automation(doc, revision, &target, &warnings, false); Ok(reply(&target, &warnings)) } diff --git a/crates/ui-egui/src/save_identity_tests.rs b/crates/ui-egui/src/save_identity_tests.rs index c947afc9b6..ea6ca8a63d 100644 --- a/crates/ui-egui/src/save_identity_tests.rs +++ b/crates/ui-egui/src/save_identity_tests.rs @@ -30,6 +30,43 @@ fn identity(app: &PhotocraftApp) -> (String, Option, u64, u64) { (st.doc.name.clone(), st.path.clone(), st.revision, st.saved_revision) } +#[test] +fn synchronous_path_save_records_identity_only_after_success() { + let mut app = app(); + let before = identity(&app); + app.background_jobs = false; + app.services.export = Some(Box::new(|_, _, _| panic!("path save must not use the byte exporter"))); + app.services.write = Some(Box::new(|_, _| panic!("path save owns its atomic writer"))); + app.services.export_path = Some(Box::new(|_, _, _| Err("cancelled before publication".into()))); + assert!(app.save_as(Some("saved.psb".into())).is_err()); + assert_eq!(identity(&app), before); + app.services.export_path = Some(Box::new(|_, path, _| { + assert_eq!(path, "saved.psb"); + Ok(Vec::new()) + })); + app.save_as(Some("saved.psb".into())).unwrap(); + let st = app.session.active().unwrap(); + assert_eq!(st.path.as_deref(), Some("saved.psb")); + assert!(!st.is_dirty()); +} + +#[test] +fn automation_path_save_is_synchronous_and_uses_only_its_authorized_service() { + let mut app = app(); + app.background_jobs = true; + app.services.export = Some(Box::new(|_, _, _| panic!("must not materialize output bytes"))); + app.services.export_path = Some(Box::new(|_, _, _| panic!("automation must not use ambient output authority"))); + app.services.automation_write = Some(Box::new(|_, _| panic!("authorized stream owns publication"))); + app.services.automation_export_path = Some(Box::new(|_, path, _| { + assert_eq!(path, "agent.psb"); + Ok(Vec::new()) + })); + let result = app.save_automation(Some("agent.psb".into())).unwrap(); + assert_eq!(result["path"], "agent.psb"); + assert!(!app.session.has_jobs()); + assert!(!app.session.active().unwrap().is_dirty()); +} + #[test] fn new_document_psd_and_subsequent_save_as_adopt_the_written_file_name() { for depth in [8, 16, 32] { diff --git a/docs/roadmap.md b/docs/roadmap.md index dca334cfe0..52d9fec5d7 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -128,3 +128,5 @@ Each milestone has a **definition of done (DoD)** and must leave `main` green on 2026-10-13: seekable native PSD/PSB and supported scanline EXR imports decode bounded bands at the original resolution. Authorized opens retain read-only source protection. See [native streaming I/O](streaming-io.md). 2026-10-13: large canvases render visible original-resolution pages with bounded RAM/VRAM, cancellable workers and stale-result rejection; the new upstream background redraw remains available for full-canvas paths. See [GPU streaming](streaming-gpu.md). + +2026-10-13: native interactive and authorized saves share the save service, bounded PSB encoding and atomic publication. Upstream progress, copy and OpenRaster save semantics are retained. See [streaming saves](streaming-save.md). diff --git a/docs/streaming-io.md b/docs/streaming-io.md index 3cc4d3a674..481f728b52 100644 --- a/docs/streaming-io.md +++ b/docs/streaming-io.md @@ -27,8 +27,7 @@ document without a source save path. The seekable output API accepts a caller-owned writer; atomic path output encodes into a temporary file beside the destination, syncs it using the shared platform helper, then renames it. Encoder failure, cancellation or unwinding removes the temporary and preserves the old destination. Its -integration with the desktop background-save service is a separate change, using upstream's -`SaveCtl` and document-lock/saved-revision contract. +integration with the desktop background-save service is documented in [streamed saves](streaming-save.md). Synthetic tests compare streamed import with the byte decoder across PSD/PSB versions, depths, colour modes and compression, and verify masks, metadata, cancellation, EXR chunk handling, diff --git a/docs/streaming-save.md b/docs/streaming-save.md new file mode 100644 index 0000000000..1fd95d16c0 --- /dev/null +++ b/docs/streaming-save.md @@ -0,0 +1,27 @@ +# Streamed native saves + +Native PSB output uses the background-save service shared by the desktop UI. The service holds +upstream's Arc snapshot and document lock, reports band progress, and records the saved revision +on the original document even when the active tab changes. Same-destination exclusion, worker +completion, and close/quit handling remain those of the upstream save jobs. + +The encoder creates and owns a temporary file beside the destination, writes original-depth +channel bands, and flushes the file before calling `SaveCtl.commit()`. This final boundary runs +immediately before atomic replacement. Failure or cancellation before the boundary preserves the +existing destination and removes the temporary. After commit, a late cancellation cannot turn a +successful publication into a reported failure. PSD output that exceeds the PSD size limit also +uses this PSB writer. + +Synchronous interactive saves use the same native path encoder. Automation `app.save` uses +upstream's background-save service when available, and retains a synchronous fallback. Both use +only the session's held write-directory capability, including the temporary output and atomic +rename. The background stream checks cancellation before publication through `SaveCtl.commit()`. +Other formats retain their checked byte encoders while sharing the atomic publication boundary. + +Streaming PSB saves respect the Maximize Compatibility and Image Previews settings, including +the placeholder merged image when compatibility is disabled and optional PSD thumbnails. + +Synthetic regressions exercise precommit rejection, late cancellation, automatic PSD promotion, +tab switching during a native background save, saved identity, and authorized-path rejection. +The integration builds on the save implementation merged in +[PR #2072](https://github.com/storytold/photocraft/pull/2072). From 2bad5596289973bb2b568ae677ce8bb242cf7330 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 10:33:52 +0100 Subject: [PATCH 02/20] psd: copy source-backed channel and merged data through in short reads A channel or merged image still in its source (an imported file's, or one spilled to scratch) is copied to the output in 8 MiB pieces instead of being read whole into memory first. --- crates/psd/src/file.rs | 34 +++++++++++++++++++++++++++++++--- crates/psd/src/io.rs | 17 +++++++++++++++++ crates/psd/src/layer.rs | 5 ++--- 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/crates/psd/src/file.rs b/crates/psd/src/file.rs index 1e2c60f827..babcef4f58 100644 --- a/crates/psd/src/file.rs +++ b/crates/psd/src/file.rs @@ -382,10 +382,9 @@ impl PsdFile { o.put_u16(self.image_data.compression.as_u16()); Ok(()) })?; - let merged = self.image_data.encoded()?; match self.merged_widened_rows(widen)? { - 0 => out.put(&merged), - rows => out.put_widened_rle(&merged, rows), + 0 => out.put_encoded(&self.image_data.data, self.image_data.stored.as_ref()), + rows => out.put_widened_rle(&self.image_data.encoded()?, rows), } } @@ -719,6 +718,35 @@ mod write_tests { } } + /// Channel data and a merged image still in their source (an imported file's, or what an + /// exporter spilled to scratch) are copied through in short reads, never read whole. + #[test] + fn source_backed_data_is_copied_in_short_reads() { + struct Reads(Vec, std::sync::Mutex); + impl crate::source::ReadAt for Reads { + fn len(&self) -> u64 { + self.0.len() as u64 + } + fn read_at(&self, offset: u64, buf: &mut [u8]) -> Result<()> { + let mut most = self.1.lock().unwrap(); + *most = (*most).max(buf.len()); + self.0.as_slice().read_at(offset, buf) + } + } + let mut file = testgen::small(Version::Psd, Compression::Raw); + file.image_data = ImageData::new(Compression::Raw, vec![5; 300]); + let info = file.layer_info.as_mut().unwrap(); + info.layers[0].channels[0] = crate::layer::ChannelData::new(0, Some(Compression::Raw), vec![6; 301]); + let bytes = file.to_bytes().unwrap(); + let source = std::sync::Arc::new(Reads(bytes.clone(), std::sync::Mutex::new(0))); + let lazy = PsdFile::from_source(crate::Source::from_arc(source.clone())).unwrap(); + *source.1.lock().unwrap() = 0; + let mut w = std::io::Cursor::new(Vec::new()); + lazy.write_to(&mut w).unwrap(); + assert!(w.into_inner() == bytes); + assert!(*source.1.lock().unwrap() <= 7, "read {} bytes at once", source.1.lock().unwrap()); + } + /// #6607: pixel data goes to the stream as it is, never through a staging copy, so a save /// holds the model and a short buffer, not a second copy of the file. #[test] diff --git a/crates/psd/src/io.rs b/crates/psd/src/io.rs index eb7185bde2..bf4468711a 100644 --- a/crates/psd/src/io.rs +++ b/crates/psd/src/io.rs @@ -213,6 +213,8 @@ impl WriteExt for Vec { /// Staged bytes [`Sink`] collects before writing them out in one call. const STAGE: usize = 64 << 10; +/// Bytes of stored channel data read per piece by [`Sink::put_encoded`]. +const COPY_CHUNK: u64 = if cfg!(test) { 7 } else { 8 << 20 }; #[cfg(test)] pub(crate) const STAGE_FOR_TESTS: usize = STAGE; @@ -283,6 +285,21 @@ impl<'w, W: Write + Seek + ?Sized> Sink<'w, W> { self.put(packed) } + /// Appends encoded bytes held in memory (`data`) or, when `stored`, still in their source: + /// those are copied through in [`COPY_CHUNK`] pieces, so a channel or merged image kept on + /// disk (an imported file's, or one an exporter spilled to scratch) is never read whole. + pub(crate) fn put_encoded(&mut self, data: &[u8], stored: Option<&crate::source::Stored>) -> std::io::Result<()> { + let Some(s) = stored else { return self.put(data) }; + let mut at = 0u64; + while at < s.len { + let n = (s.len - at).min(COPY_CHUNK); + let part = s.read_part(at, n)?; + self.put(&part)?; + at += n; + } + Ok(()) + } + /// `n` zero bytes (padding, at most a few). pub(crate) fn zeros(&mut self, n: usize) -> std::io::Result<()> { self.small(|b| { diff --git a/crates/psd/src/layer.rs b/crates/psd/src/layer.rs index fc1a3ff571..3b492e6a04 100644 --- a/crates/psd/src/layer.rs +++ b/crates/psd/src/layer.rs @@ -858,10 +858,9 @@ impl LayerInfo { o.put_u16(comp.as_u16()); Ok(()) })?; - let data = c.encoded()?; match l.widened_rows(c, widen)? { - 0 => out.put(&data)?, - rows => out.put_widened_rle(&data, rows)?, + 0 => out.put_encoded(&c.data, c.stored.as_ref())?, + rows => out.put_widened_rle(&c.encoded()?, rows)?, } } } From 4b27da939f7d70f614e72fa03517c3bc8efaecf3 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 10:49:38 +0100 Subject: [PATCH 03/20] raster: anonymous append-only stage file on the scratch disk Shares ScratchFile's private open (0600, unlinked at once, delete-on-close on Windows) so encoded save data can be parked off the heap and read back by offset. --- crates/raster/src/lib.rs | 1 + crates/raster/src/scratch_file.rs | 81 ++++++++++++--------- crates/raster/src/stage_file.rs | 112 ++++++++++++++++++++++++++++++ 3 files changed, 160 insertions(+), 34 deletions(-) create mode 100644 crates/raster/src/stage_file.rs diff --git a/crates/raster/src/lib.rs b/crates/raster/src/lib.rs index 994c02c57c..77121d0e7d 100644 --- a/crates/raster/src/lib.rs +++ b/crates/raster/src/lib.rs @@ -49,6 +49,7 @@ impl std::error::Error for AllocationError {} pub mod memory; mod scratch_file; pub mod spill; +pub mod stage_file; /// One shared tile per pixel value (and tile size), for [`Surface::share_uniform_tiles`]. #[derive(Debug, Default)] diff --git a/crates/raster/src/scratch_file.rs b/crates/raster/src/scratch_file.rs index bcb46c3d0f..fc131543b4 100644 --- a/crates/raster/src/scratch_file.rs +++ b/crates/raster/src/scratch_file.rs @@ -118,35 +118,9 @@ impl std::fmt::Debug for ScratchFile { impl ScratchFile { pub fn create(dir: &Path) -> std::io::Result { - std::fs::create_dir_all(dir)?; - static COUNTER: AtomicUsize = AtomicUsize::new(0); - // A reused process id must not collide with an abandoned file from an older launch. - let n = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap_or_default() - .as_nanos() - .saturating_add(COUNTER.fetch_add(1, Ordering::Relaxed) as u128); - let path = dir.join(format!("photocraft-{}-{n}.scratch", std::process::id())); - let mut options = std::fs::OpenOptions::new(); - options.read(true).write(true).create_new(true); - #[cfg(unix)] - { - use std::os::unix::fs::OpenOptionsExt; - options.mode(0o600); - } + let (main, path, linked) = open_private(dir)?; #[cfg(windows)] - { - use std::os::windows::fs::OpenOptionsExt; - const FILE_FLAG_DELETE_ON_CLOSE: u32 = 0x0400_0000; - options.custom_flags(FILE_FLAG_DELETE_ON_CLOSE); - } - let main = options.open(&path)?; - #[cfg(unix)] - reap_abandoned(dir, &main); - // Unix: the open handle keeps the inode; nothing is left behind at any exit. A - // filesystem that refuses the unlink keeps the name until drop or the next reap. - #[cfg(not(windows))] - let linked = std::fs::remove_file(&path).is_err(); + let _ = linked; Ok(Self { main, #[cfg(windows)] @@ -231,6 +205,45 @@ impl ScratchFile { } } +/// Creates a file only this process can reach in `dir`, gone once its last handle closes: Unix +/// opens it private (0600) and unlinks it right away, Windows opens it delete-on-close. Files +/// of dead processes are reaped. Returns the file, its name, and whether the name is still +/// there (a filesystem refused the unlink: remove it when done). +pub(crate) fn open_private(dir: &Path) -> std::io::Result<(File, PathBuf, bool)> { + std::fs::create_dir_all(dir)?; + static COUNTER: AtomicUsize = AtomicUsize::new(0); + // A reused process id must not collide with an abandoned file from an older launch. + let n = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_nanos() + .saturating_add(COUNTER.fetch_add(1, Ordering::Relaxed) as u128); + let path = dir.join(format!("photocraft-{}-{n}.scratch", std::process::id())); + let mut options = std::fs::OpenOptions::new(); + options.read(true).write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.mode(0o600); + } + #[cfg(windows)] + { + use std::os::windows::fs::OpenOptionsExt; + const FILE_FLAG_DELETE_ON_CLOSE: u32 = 0x0400_0000; + options.custom_flags(FILE_FLAG_DELETE_ON_CLOSE); + } + let file = options.open(&path)?; + #[cfg(unix)] + reap_abandoned(dir, &file); + // Unix: the open handle keeps the inode; nothing is left behind at any exit. A + // filesystem that refuses the unlink keeps the name until drop or the next reap. + #[cfg(not(windows))] + let linked = std::fs::remove_file(&path).is_err(); + #[cfg(windows)] + let linked = false; + Ok((file, path, linked)) +} + /// Accept only the names we generate, never other temporary files or directories. #[cfg(any(unix, test))] fn scratch_pid(name: &str) -> Option { @@ -297,17 +310,17 @@ fn thread_slot() -> usize { } #[cfg(unix)] -fn write_all_at(file: &File, data: &[u8], offset: u64) -> std::io::Result<()> { +pub(crate) fn write_all_at(file: &File, data: &[u8], offset: u64) -> std::io::Result<()> { std::os::unix::fs::FileExt::write_all_at(file, data, offset) } #[cfg(unix)] -fn read_exact_at(file: &File, buf: &mut [u8], offset: u64) -> std::io::Result<()> { +pub(crate) fn read_exact_at(file: &File, buf: &mut [u8], offset: u64) -> std::io::Result<()> { std::os::unix::fs::FileExt::read_exact_at(file, buf, offset) } #[cfg(windows)] -fn write_all_at(file: &File, mut data: &[u8], mut offset: u64) -> std::io::Result<()> { +pub(crate) fn write_all_at(file: &File, mut data: &[u8], mut offset: u64) -> std::io::Result<()> { use std::os::windows::fs::FileExt; while !data.is_empty() { let n = file.seek_write(data, offset)?; @@ -321,7 +334,7 @@ fn write_all_at(file: &File, mut data: &[u8], mut offset: u64) -> std::io::Resul } #[cfg(windows)] -fn read_exact_at(file: &File, mut buf: &mut [u8], mut offset: u64) -> std::io::Result<()> { +pub(crate) fn read_exact_at(file: &File, mut buf: &mut [u8], mut offset: u64) -> std::io::Result<()> { use std::os::windows::fs::FileExt; while !buf.is_empty() { let n = file.seek_read(buf, offset)?; @@ -335,12 +348,12 @@ fn read_exact_at(file: &File, mut buf: &mut [u8], mut offset: u64) -> std::io::R } #[cfg(not(any(unix, windows)))] -fn write_all_at(_file: &File, _data: &[u8], _offset: u64) -> std::io::Result<()> { +pub(crate) fn write_all_at(_file: &File, _data: &[u8], _offset: u64) -> std::io::Result<()> { Err(std::io::ErrorKind::Unsupported.into()) } #[cfg(not(any(unix, windows)))] -fn read_exact_at(_file: &File, _buf: &mut [u8], _offset: u64) -> std::io::Result<()> { +pub(crate) fn read_exact_at(_file: &File, _buf: &mut [u8], _offset: u64) -> std::io::Result<()> { Err(std::io::ErrorKind::Unsupported.into()) } diff --git a/crates/raster/src/stage_file.rs b/crates/raster/src/stage_file.rs new file mode 100644 index 0000000000..e371fbe6be --- /dev/null +++ b/crates/raster/src/stage_file.rs @@ -0,0 +1,112 @@ +//! Encoded data an exporter parks on the scratch disk instead of in memory (#5333). +//! +//! A large PSD/PSB save encodes every channel before it writes a byte (so it can still be +//! cancelled, and its scratch reads are checked, before the file is replaced). Kept in memory, +//! those channels are a second copy of the document. A [`StageFile`] holds them on the scratch +//! disk instead: an append-only file made like the tile scratch file (private, unlinked on Unix, +//! delete-on-close on Windows), so nothing is left behind at any exit, and the space returns as +//! soon as the save drops it. + +use std::fs::File; +use std::path::{Path, PathBuf}; +use std::sync::{Mutex, PoisonError}; + +use crate::scratch_file::{open_private, read_exact_at, write_all_at}; + +pub struct StageFile { + file: File, + path: PathBuf, + /// The directory entry still exists (the unlink was refused): remove it on drop. + linked: bool, + /// Bytes reserved so far; appends take their offset from it. + end: Mutex, +} + +impl std::fmt::Debug for StageFile { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("StageFile").field("path", &self.path).field("len", &self.len()).finish() + } +} + +impl StageFile { + /// A new, empty stage file in `dir`. + pub fn create(dir: &Path) -> std::io::Result { + let (file, path, linked) = open_private(dir)?; + Ok(Self { file, path, linked, end: Mutex::new(0) }) + } + + /// A stage file in the configured scratch directory, when spilling is on (`None` otherwise, + /// or when the file cannot be made: the caller then keeps its data in memory). + pub fn in_scratch() -> Option { + if !crate::spill::enabled() { + return None; + } + Self::create(&crate::spill::stats().dir?).ok() + } + + /// Appends `data` and returns its offset. Parallel appends each get their own range. + pub fn append(&self, data: &[u8]) -> std::io::Result { + let offset = { + let mut end = self.end.lock().unwrap_or_else(PoisonError::into_inner); + let offset = *end; + *end = offset.checked_add(data.len() as u64).ok_or_else(|| std::io::Error::other("stage file too large"))?; + offset + }; + write_all_at(&self.file, data, offset)?; + Ok(offset) + } + + /// Fills `buf` from `offset`, which must lie within what was appended. + pub fn read_exact_at(&self, buf: &mut [u8], offset: u64) -> std::io::Result<()> { + if offset.checked_add(buf.len() as u64).is_none_or(|end| end > self.len()) { + return Err(std::io::ErrorKind::UnexpectedEof.into()); + } + read_exact_at(&self.file, buf, offset) + } + + /// Bytes appended (reserved) so far. + pub fn len(&self) -> u64 { + *self.end.lock().unwrap_or_else(PoisonError::into_inner) + } + + pub fn is_empty(&self) -> bool { + self.len() == 0 + } +} + +impl Drop for StageFile { + fn drop(&mut self) { + if self.linked { + let _ = std::fs::remove_file(&self.path); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn appends_read_back_and_leave_nothing_behind() { + let dir = std::env::temp_dir().join(format!("pc-stage-{}", std::process::id())); + let stage = StageFile::create(&dir).unwrap(); + #[cfg(unix)] + assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 0, "the stage file is unlinked while open"); + let parts: Vec> = (0..8u8).map(|i| vec![i; 1000 + usize::from(i)]).collect(); + let offsets: Vec = std::thread::scope(|s| { + let handles: Vec<_> = parts.iter().map(|p| s.spawn(|| stage.append(p).unwrap())).collect(); + handles.into_iter().map(|h| h.join().unwrap()).collect() + }); + assert_eq!(stage.len(), parts.iter().map(|p| p.len() as u64).sum::()); + for (p, &at) in parts.iter().zip(&offsets) { + let mut buf = vec![0; p.len()]; + stage.read_exact_at(&mut buf, at).unwrap(); + assert_eq!(&buf, p); + } + let mut past = [0u8; 2]; + assert!(stage.read_exact_at(&mut past, stage.len() - 1).is_err(), "reads stop at what was appended"); + drop(stage); + assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 0); + let _ = std::fs::remove_dir(&dir); + } +} From dbcee2ba476d694b5e59ecdd5324f4dd709dfbac Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 10:49:38 +0100 Subject: [PATCH 04/20] io: stage encoded PSD/PSB channels on the scratch disk for large saves Each colour channel and the merged image is parked in a scratch stage file as soon as it is encoded (documents of 1 GiB+ raw pixels, chunks of 64 KiB+), so a save no longer holds every encoded plane in memory before #8115's checked write. The written file is byte-identical; when staging is unavailable the data stays in memory. --- crates/io/src/lib.rs | 1 + crates/io/src/psd_export.rs | 17 ++++- crates/io/src/psd_export/document.rs | 8 +++ crates/io/src/psd_export/stage.rs | 103 +++++++++++++++++++++++++++ crates/io/src/psd_export/tests.rs | 53 ++++++++++++++ crates/io/src/tiff_layers.rs | 2 + 6 files changed, 182 insertions(+), 2 deletions(-) create mode 100644 crates/io/src/psd_export/stage.rs diff --git a/crates/io/src/lib.rs b/crates/io/src/lib.rs index 2d4371d4c1..f976867778 100644 --- a/crates/io/src/lib.rs +++ b/crates/io/src/lib.rs @@ -793,6 +793,7 @@ fn export_stages(doc: &Document, name_or_ext: &str, opts: &ExportOptions, ctl: & maximize_compatibility: opts.psd_maximize_compatibility, document_path: opts.document_path.clone(), read_linked_files: opts.read_linked_files, + stage_on_scratch_from: Some(psd_export::STAGE_DOCUMENTS_FROM), ..Default::default() }; // The preview is rendered first, while only the document is in memory: its full-size diff --git a/crates/io/src/psd_export.rs b/crates/io/src/psd_export.rs index 6086aaf644..6f87aa7588 100644 --- a/crates/io/src/psd_export.rs +++ b/crates/io/src/psd_export.rs @@ -20,6 +20,8 @@ mod estimate; mod merged; mod resources; mod smart; +mod stage; +pub(crate) use stage::STAGE_DOCUMENTS_FROM; pub(crate) use document::document_to_psd_interruptible; #[cfg(test)] @@ -74,6 +76,10 @@ pub struct PsdExportOptions { /// off, since a stored path is document data rather than a granted file: such a layer is /// then written as pixels, with a warning. pub read_linked_files: bool, + /// Park each encoded channel on the scratch disk, not in memory, when the document's raw + /// pixel bytes (layers and composite) reach this and spilling is on (`None`: never). The + /// save then holds the file model's structure, not a second copy of its pixels (#5333). + pub stage_on_scratch_from: Option, } impl Default for PsdExportOptions { @@ -86,6 +92,7 @@ impl Default for PsdExportOptions { maximize_compatibility: true, document_path: None, read_linked_files: true, + stage_on_scratch_from: None, } } } @@ -126,6 +133,8 @@ struct Ex { patterns: Vec, /// The layer written as Photoshop's Background (see [`psd_background`]). background: Option, + /// Where encoded channels go instead of memory ([`PsdExportOptions::stage_on_scratch_from`]). + stage: Option>, } /// The layer Photoshop should open as its Background: the bottom top-level layer when it is a @@ -186,9 +195,13 @@ impl Ex { // A ZIP stream for a zero-sized channel makes readers try to decompress an // invalid image extent (notably psd-tools for 32-bit adjustment layers). let compression = if w == 0 || h == 0 { Compression::Raw } else { self.compression() }; - ChannelData::encode(id, compression, plane, w, h, depth, self.version) + let c = ChannelData::encode(id, compression, plane, w, h, depth, self.version) .or_else(|_| ChannelData::encode(id, Compression::Raw, plane, w, h, depth, self.version)) - .unwrap_or(ChannelData::new(id, Some(Compression::Raw), plane.to_vec())) + .unwrap_or(ChannelData::new(id, Some(Compression::Raw), plane.to_vec())); + match &self.stage { + Some(stage) => stage.channel(c), + None => c, + } } fn empty_channels(&self) -> Vec { diff --git a/crates/io/src/psd_export/document.rs b/crates/io/src/psd_export/document.rs index f6c36c2d42..a87ed93010 100644 --- a/crates/io/src/psd_export/document.rs +++ b/crates/io/src/psd_export/document.rs @@ -88,6 +88,10 @@ fn export_supported_document( smart: SmartOut::new(doc, depth), patterns: doc.patterns.clone(), background: psd_background(doc), + stage: { + let raw = doc.size.area().saturating_mul(fmt.bytes_per_pixel() as u64).saturating_mul(doc.walk().len() as u64 + 1); + super::stage::Stage::for_document(raw, opts.stage_on_scratch_from) + }, }; add_format_warnings(doc, opts, fmt, big, duotone_inks.is_some(), &mut ex.warnings); index_layer_ids_and_text(doc, &mut ex); @@ -331,6 +335,10 @@ fn prepare_merged_planes<'a>( data: planes, stored: None, }); + let image_data = match &ex.stage { + Some(stage) => stage.image(image_data), + None => image_data, + }; ctl.progress(1.0); MergedExport { header, image_data, extra, has_alpha, quick_mask_id, real_merged } } diff --git a/crates/io/src/psd_export/stage.rs b/crates/io/src/psd_export/stage.rs new file mode 100644 index 0000000000..3e04d53247 --- /dev/null +++ b/crates/io/src/psd_export/stage.rs @@ -0,0 +1,103 @@ +//! Encoded channels parked on the scratch disk while a large PSD/PSB save is assembled (#5333). +//! +//! A save encodes the whole file model before it writes a byte (#8115: it can still be +//! cancelled, and the scratch reads behind it are checked, before the file is replaced). In +//! memory that model is a second, compressed copy of every layer. With +//! [`PsdExportOptions::stage_on_scratch_from`](super::PsdExportOptions::stage_on_scratch_from) +//! set and spilling on, each channel is appended to a [`StageFile`] in the scratch directory as +//! soon as it is encoded, and the model keeps only a [`Stored`] range: the writer then copies it +//! through in short reads (`photocraft_psd`'s `put_encoded`). A channel that cannot be staged +//! (the disk is full, say) stays in memory: staging only ever saves memory. + +use std::sync::Arc; + +use photocraft_psd::source::{ReadAt, Source, Stored}; +use photocraft_psd::{ChannelData, ImageData, PsdError}; +use photocraft_raster::stage_file::StageFile; + +/// Encoded data shorter than this stays in memory: a range costs more than it saves. +const MIN_STAGED_BYTES: usize = 64 << 10; +/// Raw pixel bytes (layers and composite, as sampled) from which a save made by +/// [`crate::export_with`] stages its channels: about a 24 MP RGBA/16 document with five layers. +pub(crate) const STAGE_DOCUMENTS_FROM: u64 = 1 << 30; + +#[cfg(test)] +thread_local! { + /// Where this thread's exports stage, instead of the process-wide scratch directory. + pub(crate) static DIR_FOR_TESTS: std::cell::RefCell> = const { std::cell::RefCell::new(None) }; +} + +struct Backing(StageFile); + +impl ReadAt for Backing { + fn len(&self) -> u64 { + self.0.len() + } + + fn read_at(&self, offset: u64, buf: &mut [u8]) -> photocraft_psd::Result<()> { + self.0.read_exact_at(buf, offset).map_err(|e| PsdError::Read(format!("staged export data, {} bytes at offset {offset}: {e}", buf.len()))) + } +} + +/// The stage of one export: a file on the scratch disk shared by every channel. +pub(crate) struct Stage { + file: Arc, + source: Source, +} + +impl Stage { + /// A stage for a document of `raw_bytes` pixel bytes, when `from` asks for one at that size + /// and a stage file can be made in the scratch directory. + pub(crate) fn for_document(raw_bytes: u64, from: Option) -> Option> { + if raw_bytes < from? { + return None; + } + #[cfg(test)] + if let Some(dir) = DIR_FOR_TESTS.with(|d| d.borrow().clone()) { + return StageFile::create(&dir).ok().map(Self::new); + } + StageFile::in_scratch().map(Self::new) + } + + fn new(file: StageFile) -> Arc { + let file = Arc::new(Backing(file)); + let source = Source::from_arc(file.clone()); + Arc::new(Self { file, source }) + } + + /// Moves `data` to the stage; `None` (the data kept by the caller) when it is short or the + /// write fails. + fn park(&self, data: &[u8]) -> Option { + if data.len() < MIN_STAGED_BYTES { + return None; + } + let offset = self.file.0.append(data).ok()?; + Some(Stored { source: self.source.clone(), offset, len: data.len() as u64 }) + } + + pub(crate) fn channel(&self, mut c: ChannelData) -> ChannelData { + if c.stored.is_none() + && let Some(stored) = self.park(&c.data) + { + c.data = Vec::new(); + c.stored = Some(stored); + } + c + } + + pub(crate) fn image(&self, mut image: ImageData) -> ImageData { + if image.stored.is_none() + && let Some(stored) = self.park(&image.data) + { + image.data = Vec::new(); + image.stored = Some(stored); + } + image + } + + /// Bytes on the scratch disk. + #[cfg(test)] + pub(crate) fn len(&self) -> u64 { + self.file.0.len() + } +} diff --git a/crates/io/src/psd_export/tests.rs b/crates/io/src/psd_export/tests.rs index c53f2c06f3..4fd78e8c73 100644 --- a/crates/io/src/psd_export/tests.rs +++ b/crates/io/src/psd_export/tests.rs @@ -619,3 +619,56 @@ fn a_linked_fifo_is_not_read() { let _ = std::fs::remove_dir_all(&dir); assert!(warnings.iter().any(|w| w.contains("smart object written as pixels") && w.contains("not a regular file")), "{warnings:?}"); } + +/// #5333: with staging on, every large encoded channel and the merged image are parked on the +/// scratch disk as they are encoded, and the file written from that model is the same, byte for +/// byte, as one assembled in memory: as PSD, as PSB, and as a PSD promoted to PSB on the way out. +#[test] +fn staged_channels_write_the_same_file_as_in_memory_ones() { + let dir = std::env::temp_dir().join(format!("pc-psd-stage-{}", std::process::id())); + stage::DIR_FOR_TESTS.with(|d| *d.borrow_mut() = Some(dir.clone())); + let doc = sized_document(true, SampleType::U8); + let ctl = Interrupt::NONE; + for (force_psb, limit) in [(false, PSD_MAX_FILE_SIZE), (true, PSD_MAX_FILE_SIZE), (false, SIZED_LIMIT)] { + let plain = PsdExportOptions { force_psb, ..Default::default() }; + let staged = PsdExportOptions { stage_on_scratch_from: Some(0), ..plain.clone() }; + let (want, _) = document_to_psd_sized(&doc, &plain, 0, &ctl, limit); + let (file, _) = document_to_psd_sized(&doc, &staged, 0, &ctl, limit); + let layer = &file.layers()[0]; + // The opaque alpha packs to a few KB and stays in memory; the noise colour channels go. + assert!(layer.channels.iter().filter(|c| c.id >= 0).all(|c| c.stored.is_some() && c.data.is_empty()), "every noise channel is staged"); + assert!(layer.channels.iter().filter(|c| c.id == -1).all(|c| c.stored.is_none()), "the short alpha stays in memory"); + assert!(file.image_data.stored.is_some() && file.image_data.data.is_empty(), "the merged image is staged"); + assert_eq!(file.write_as_psb, want.write_as_psb); + let bytes = want.to_bytes().unwrap(); + assert!(file.to_bytes().unwrap() == bytes, "psb {force_psb}, limit {limit}: staged bytes differ"); + let mut w = std::io::Cursor::new(Vec::new()); + file.write_to(&mut w).unwrap(); + assert!(w.into_inner() == bytes, "psb {force_psb}, limit {limit}: streamed staged bytes differ"); + } + // Under the threshold, or with no stage asked for, nothing is staged. + let (file, _) = document_to_psd_sized(&doc, &PsdExportOptions { stage_on_scratch_from: Some(u64::MAX), ..Default::default() }, 0, &ctl, PSD_MAX_FILE_SIZE); + assert!(file.layers()[0].channels.iter().all(|c| c.stored.is_none()) && file.image_data.stored.is_none()); + stage::DIR_FOR_TESTS.with(|d| *d.borrow_mut() = None); + #[cfg(unix)] + assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 0, "the stage file has no name on disk"); + let _ = std::fs::remove_dir(&dir); +} + +/// Short channels stay in memory, and a stage that cannot be written keeps the data in memory. +#[test] +fn staging_keeps_short_or_unwritable_data_in_memory() { + let dir = std::env::temp_dir().join(format!("pc-psd-stage-short-{}", std::process::id())); + stage::DIR_FOR_TESTS.with(|d| *d.borrow_mut() = Some(dir.clone())); + let stage = stage::Stage::for_document(1, Some(0)).unwrap(); + stage::DIR_FOR_TESTS.with(|d| *d.borrow_mut() = None); + let short = stage.channel(ChannelData::new(0, Some(Compression::Raw), vec![1; 100])); + assert!(short.stored.is_none() && short.data.len() == 100); + let long = stage.channel(ChannelData::new(0, Some(Compression::Raw), vec![2; 1 << 17])); + assert!(long.stored.is_some() && long.data.is_empty()); + assert_eq!(long.encoded().unwrap().as_ref(), &vec![2u8; 1 << 17][..]); + assert_eq!(stage.len(), 1 << 17); + assert!(stage::Stage::for_document(10, Some(11)).is_none(), "under the threshold"); + assert!(stage::Stage::for_document(10, None).is_none(), "not asked for"); + let _ = std::fs::remove_dir(&dir); +} diff --git a/crates/io/src/tiff_layers.rs b/crates/io/src/tiff_layers.rs index f001b6a265..f0bb148179 100644 --- a/crates/io/src/tiff_layers.rs +++ b/crates/io/src/tiff_layers.rs @@ -265,6 +265,8 @@ pub(crate) fn export_layered(doc: &Document, opts: &ExportOptions, ctl: &photocr maximize_compatibility: true, document_path: opts.document_path.clone(), read_linked_files: opts.read_linked_files, + // The layered TIFF reads the merged image's bytes directly: they stay in memory. + stage_on_scratch_from: None, }; // TIFF has no palette, bitmap or duotone layout: the PSD model of an Indexed Color, Bitmap // or Duotone document is made in its pixel format (RGB or Grayscale), which the PSD exporter From d1f112c38ee6f8fe291a98e0e1e2a4d31bc7d2a0 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 10:49:38 +0100 Subject: [PATCH 05/20] automation: stream workspace saves instead of buffering the encoded file AuthorizedWorkspace::write_stream keeps write()'s refusals and atomic replace but lets the encoder write (and seek back) straight into the temp file, so an automation PSB save no longer builds the whole file in a Vec. --- apps/photocraft/src/services.rs | 2 +- crates/automation/src/workspace.rs | 9 ++++++- crates/automation/src/workspace/tests.rs | 30 ++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/apps/photocraft/src/services.rs b/apps/photocraft/src/services.rs index d9ddcf64d8..7741a8d8a3 100644 --- a/apps/photocraft/src/services.rs +++ b/apps/photocraft/src/services.rs @@ -685,7 +685,7 @@ pub fn native(automation: Option) -> }); let automation_save = automation.clone().map(|workspace| { std::sync::Arc::new(move |doc: &Document, path: &str, settings: &photocraft_ui_egui::ExportSettings, ctl: &photocraft_ui_egui::jobs_ui::SaveCtl| { - save_through(doc, path, settings, ctl, |p, encoded, _| workspace.write(p, &encoded.bytes().map_err(|e| e.to_string())?).map_err(|e| e.to_string())) + save_through(doc, path, settings, ctl, |p, encoded, _| workspace.write_stream(p, &mut |w| encoded.write_to(w)).map_err(|e| e.to_string())) }) as photocraft_ui_egui::SaveFileFn }); let step: fn(&str, &serde_json::Value) -> photocraft_engine::Result<()> = photocraft_automation::workspace::authorize_desktop_engine_step; diff --git a/crates/automation/src/workspace.rs b/crates/automation/src/workspace.rs index a60cc66a16..d288f42f61 100644 --- a/crates/automation/src/workspace.rs +++ b/crates/automation/src/workspace.rs @@ -87,6 +87,13 @@ impl AuthorizedWorkspace { /// resolution and file creation relative to the held directory handle, so /// a non-existent final target is supported without ambient path access. pub fn write(&self, path: &str, bytes: &[u8]) -> Result<(), AutomationError> { + self.write_stream(path, &mut |w| w.write_all(bytes)) + } + + /// [`Self::write`] with the file's bytes produced by `write` straight into the temporary + /// file (it may seek back to patch lengths), so a large PSD/PSB is never held in memory + /// whole (#5333). The same refusals, the same atomic replace. + pub fn write_stream(&self, path: &str, write: &mut photocraft_format::atomic::StreamWriter<'_>) -> Result<(), AutomationError> { let relative = request_path(path, self.write.as_ref())?; let root = self.write.as_ref().ok_or_else(|| AutomationError::BadRequest(format!("{DENIED}: write authority is absent")))?; let leaf = relative.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_default(); @@ -112,7 +119,7 @@ impl AuthorizedWorkspace { options.write(true).create_new(true); let written = root.dir.open_with(&tmp, &options).and_then(|file| { let mut file = file.into_std(); - file.write_all(bytes)?; + write(&mut file)?; #[cfg(unix)] { use std::os::fd::AsFd; diff --git a/crates/automation/src/workspace/tests.rs b/crates/automation/src/workspace/tests.rs index e96ccb369d..d44f984ece 100644 --- a/crates/automation/src/workspace/tests.rs +++ b/crates/automation/src/workspace/tests.rs @@ -122,6 +122,36 @@ fn write_replaces_atomically_and_leaves_no_temp_file() { assert!(temp_files(&inside).is_empty()); } +/// #5333: a streamed write (a PSD/PSB serialized as it goes, lengths patched by seeking back) +/// replaces the file the same way; a writer that fails leaves the original and no temp file. +#[test] +fn write_stream_seeks_back_and_keeps_the_original_on_failure() { + use std::io::SeekFrom; + let (inside, _, workspace) = roots("stream"); + workspace.write("big.psb", b"original").unwrap(); + workspace + .write_stream("big.psb", &mut |w| { + w.write_all(b"....body")?; + w.seek(SeekFrom::Start(0))?; + w.write_all(b"head")?; + w.seek(SeekFrom::End(0))?; + Ok(()) + }) + .unwrap(); + assert_eq!(std::fs::read(inside.join("big.psb")).unwrap(), b"headbody"); + let failed = workspace.write_stream("big.psb", &mut |w| { + w.write_all(b"partial")?; + Err(std::io::Error::other("encoder failed")) + }); + assert!(failed.is_err()); + assert_eq!(std::fs::read(inside.join("big.psb")).unwrap(), b"headbody"); + assert!(temp_files(&inside).is_empty()); + // The same refusals as a plain write. + std::fs::write(inside.join("notes.txt"), b"keep").unwrap(); + assert!(workspace.write_stream("notes.txt", &mut |w| w.write_all(b"x")).is_err()); + assert_eq!(std::fs::read(inside.join("notes.txt")).unwrap(), b"keep"); +} + /// A directory at the target is refused before anything is opened, even under an image name. #[test] fn write_refuses_a_directory_target_before_opening_it() { From be3b2918c34fc83363b5143dbbe1e16441479824 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 12:16:23 +0100 Subject: [PATCH 06/20] raster: checksum stage-file appends and fail a read pass over changed bytes Like the tile scratch file, each append records a CRC-32; a save copying it (whole or in consecutive parts) fails on the last part if the scratch disk returned different bytes, so the document is never replaced with them. --- crates/raster/src/stage_file.rs | 74 +++++++++++++++++++++++++++++++-- 1 file changed, 70 insertions(+), 4 deletions(-) diff --git a/crates/raster/src/stage_file.rs b/crates/raster/src/stage_file.rs index e371fbe6be..aff858c279 100644 --- a/crates/raster/src/stage_file.rs +++ b/crates/raster/src/stage_file.rs @@ -6,7 +6,13 @@ //! disk instead: an append-only file made like the tile scratch file (private, unlinked on Unix, //! delete-on-close on Windows), so nothing is left behind at any exit, and the space returns as //! soon as the save drops it. +//! +//! Like the tile scratch file, every append carries a CRC-32: a read pass that covers an append +//! from its start to its end (whole, or in consecutive parts, as a save copies it) fails on its +//! last part if the disk handed back different bytes, so the save errors before it replaces +//! anything instead of writing them into the document. +use std::collections::BTreeMap; use std::fs::File; use std::path::{Path, PathBuf}; use std::sync::{Mutex, PoisonError}; @@ -20,6 +26,16 @@ pub struct StageFile { linked: bool, /// Bytes reserved so far; appends take their offset from it. end: Mutex, + /// Each append by its offset, with its checksum and the read pass under way. + extents: Mutex>, +} + +struct Extent { + len: u64, + crc32: u32, + /// Where the current read pass continues (`None` once it skipped or jumped: not checked). + next: Option, + hasher: crc32fast::Hasher, } impl std::fmt::Debug for StageFile { @@ -32,7 +48,7 @@ impl StageFile { /// A new, empty stage file in `dir`. pub fn create(dir: &Path) -> std::io::Result { let (file, path, linked) = open_private(dir)?; - Ok(Self { file, path, linked, end: Mutex::new(0) }) + Ok(Self { file, path, linked, end: Mutex::new(0), extents: Mutex::default() }) } /// A stage file in the configured scratch directory, when spilling is on (`None` otherwise, @@ -53,15 +69,42 @@ impl StageFile { offset }; write_all_at(&self.file, data, offset)?; + let extent = Extent { len: data.len() as u64, crc32: crc32fast::hash(data), next: Some(offset), hasher: crc32fast::Hasher::new() }; + self.extents.lock().unwrap_or_else(PoisonError::into_inner).insert(offset, extent); Ok(offset) } - /// Fills `buf` from `offset`, which must lie within what was appended. + /// Fills `buf` from `offset`, which must lie within what was appended. A read that ends a + /// pass over one append from its start fails with `InvalidData` if the append's bytes changed. pub fn read_exact_at(&self, buf: &mut [u8], offset: u64) -> std::io::Result<()> { - if offset.checked_add(buf.len() as u64).is_none_or(|end| end > self.len()) { + let Some(end) = offset.checked_add(buf.len() as u64).filter(|&end| end <= self.len()) else { return Err(std::io::ErrorKind::UnexpectedEof.into()); + }; + read_exact_at(&self.file, buf, offset)?; + let mut extents = self.extents.lock().unwrap_or_else(PoisonError::into_inner); + let Some((&start, extent)) = extents.range_mut(..=offset).next_back() else { return Ok(()) }; + let extent_end = start + extent.len; + if end > extent_end { + return Ok(()); // spans appends (no save reads this way): not checked } - read_exact_at(&self.file, buf, offset) + if offset == start { + extent.hasher = crc32fast::Hasher::new(); + extent.next = Some(start); + } + if extent.next != Some(offset) { + extent.next = None; + return Ok(()); + } + extent.hasher.update(buf); + extent.next = Some(end); + if end == extent_end { + let crc32 = std::mem::take(&mut extent.hasher).finalize(); + extent.next = None; + if crc32 != extent.crc32 { + return Err(std::io::Error::new(std::io::ErrorKind::InvalidData, "staged save data changed on the scratch disk (checksum mismatch)")); + } + } + Ok(()) } /// Bytes appended (reserved) so far. @@ -109,4 +152,27 @@ mod tests { assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 0); let _ = std::fs::remove_dir(&dir); } + + /// Bytes that change on the disk fail the pass that copies them, whole or in parts. + #[test] + fn a_changed_append_fails_its_read_pass() { + let dir = std::env::temp_dir().join(format!("pc-stage-crc-{}", std::process::id())); + let stage = StageFile::create(&dir).unwrap(); + let first = stage.append(&[1; 100]).unwrap(); + let second = stage.append(&[2; 100]).unwrap(); + let pass = |at: u64, part: usize| -> std::io::Result<()> { + let mut buf = vec![0; part]; + (0..100 / part as u64).try_for_each(|i| stage.read_exact_at(&mut buf, at + i * part as u64)) + }; + pass(first, 10).unwrap(); + pass(second, 100).unwrap(); + write_all_at(&stage.file, &[9], second + 57).unwrap(); + pass(first, 25).unwrap(); + assert_eq!(pass(second, 10).unwrap_err().kind(), std::io::ErrorKind::InvalidData, "in parts"); + assert_eq!(pass(second, 100).unwrap_err().kind(), std::io::ErrorKind::InvalidData, "whole"); + let mut part = [0; 10]; + stage.read_exact_at(&mut part, second + 50).unwrap(); // a read that starts mid-append is not a pass + drop(stage); + let _ = std::fs::remove_dir(&dir); + } } From 0a528eea1934cebb5dc7539d86286687ba591042 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 12:16:23 +0100 Subject: [PATCH 07/20] automation: remove the temp file on every exit but a rename, a panicking writer included A drop guard armed only once this call created the temp file replaces the error-path remove_file, so a writer panic no longer leaves a temp file and a create_new collision never removes a file this call did not make. --- crates/automation/src/workspace.rs | 23 +++++++++++++++++++++-- crates/automation/src/workspace/tests.rs | 16 ++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/crates/automation/src/workspace.rs b/crates/automation/src/workspace.rs index d288f42f61..3e068258fe 100644 --- a/crates/automation/src/workspace.rs +++ b/crates/automation/src/workspace.rs @@ -117,7 +117,11 @@ impl AuthorizedWorkspace { let tmp = parent.join(photocraft_format::atomic::temp_name(&leaf)); let mut options = OpenOptions::new(); options.write(true).create_new(true); - let written = root.dir.open_with(&tmp, &options).and_then(|file| { + let opened = root.dir.open_with(&tmp, &options); + // Removes the temporary file on every way out but a rename, a panicking writer included + // (unwinding closes the handle inside the closure first), and only one this call made. + let mut cleanup = TempRemoval { dir: &root.dir, path: &tmp, armed: opened.is_ok() }; + let written = opened.and_then(|file| { let mut file = file.into_std(); write(&mut file)?; #[cfg(unix)] @@ -137,9 +141,9 @@ impl AuthorizedWorkspace { }); let renamed = written.and_then(|()| photocraft_format::atomic::retry_rename(RenameRetry::platform(), || root.dir.rename(&tmp, &root.dir, &relative))); if let Err(e) = renamed { - let _ = root.dir.remove_file(&tmp); return Err(file_error("write", path, e)); } + cleanup.armed = false; // Flush the directory entry (Unix); best effort, the new bytes are already in place. #[cfg(unix)] { @@ -152,6 +156,21 @@ impl AuthorizedWorkspace { } } +/// Removes a temporary file on drop unless `armed` was cleared after its rename. +struct TempRemoval<'a> { + dir: &'a Dir, + path: &'a Path, + armed: bool, +} + +impl Drop for TempRemoval<'_> { + fn drop(&mut self) { + if self.armed { + let _ = self.dir.remove_file(self.path); + } + } +} + /// Whether `name`'s extension is one of the formats PhotoCraft saves or exports. fn written_kind(name: &str) -> bool { let Some((_, ext)) = name.rsplit_once('.') else { return false }; diff --git a/crates/automation/src/workspace/tests.rs b/crates/automation/src/workspace/tests.rs index d44f984ece..5ea4a5b3fe 100644 --- a/crates/automation/src/workspace/tests.rs +++ b/crates/automation/src/workspace/tests.rs @@ -152,6 +152,22 @@ fn write_stream_seeks_back_and_keeps_the_original_on_failure() { assert_eq!(std::fs::read(inside.join("notes.txt")).unwrap(), b"keep"); } +/// A writer that panics leaves the original and no temporary file behind. +#[test] +fn write_stream_cleans_up_after_a_panicking_writer() { + let (inside, _, workspace) = roots("stream-panic"); + workspace.write("big.psb", b"original").unwrap(); + let unwound = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let _ = workspace.write_stream("big.psb", &mut |w| { + w.write_all(b"partial")?; + panic!("encoder bug") + }); + })); + assert!(unwound.is_err()); + assert_eq!(std::fs::read(inside.join("big.psb")).unwrap(), b"original"); + assert!(temp_files(&inside).is_empty()); +} + /// A directory at the target is refused before anything is opened, even under an image name. #[test] fn write_refuses_a_directory_target_before_opening_it() { From afa0a86341d76c13aede6a87df57d37665e6b4dc Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 13:06:09 +0100 Subject: [PATCH 08/20] psd: widen staged RLE data to PSB in short reads, never reading it back whole --- crates/psd/src/compression.rs | 15 +++++++--- crates/psd/src/file.rs | 52 +++++++++++++++++++++++++++++++---- crates/psd/src/io.rs | 39 ++++++++++++++++++++++---- crates/psd/src/layer.rs | 8 +++--- 4 files changed, 95 insertions(+), 19 deletions(-) diff --git a/crates/psd/src/compression.rs b/crates/psd/src/compression.rs index 5a49284c44..73c71bab1d 100644 --- a/crates/psd/src/compression.rs +++ b/crates/psd/src/compression.rs @@ -228,11 +228,18 @@ pub fn decode_planes(compression: Compression, data: &[u8], layout: &PlaneLayout /// Bytes a PSD RLE stream (two-byte row counts) of `rows` rows grows by when written as PSB /// (four-byte row counts); an error when `data` is too short to hold the count table. pub(crate) fn rle_widening(data: &[u8], rows: usize) -> Result { - let table = rows.checked_mul(2).ok_or(PsdError::LimitExceeded("row count overflow"))?; - if data.len() < table { - return Err(PsdError::UnexpectedEof { offset: data.len(), needed: table - data.len() }); + rle_widening_len(data.len() as u64, rows) +} + +/// [`rle_widening`] from the stream's length alone, so data still in its source (or staged on +/// the scratch disk) is not read to measure it. +pub(crate) fn rle_widening_len(len: u64, rows: usize) -> Result { + let table = rows.checked_mul(2).ok_or(PsdError::LimitExceeded("row count overflow"))? as u64; + if len < table { + let offset = usize::try_from(len).unwrap_or(usize::MAX); + return Err(PsdError::UnexpectedEof { offset, needed: usize::try_from(table - len).unwrap_or(usize::MAX) }); } - Ok(table as u64) + Ok(table) } /// Encodes planar data (excluding the 2-byte compression field). diff --git a/crates/psd/src/file.rs b/crates/psd/src/file.rs index babcef4f58..910d19563a 100644 --- a/crates/psd/src/file.rs +++ b/crates/psd/src/file.rs @@ -2,7 +2,7 @@ use std::io::{Seek, Write}; -use crate::compression::{Compression, MAX_DECODED_BYTES, decode_planes, rle_widening, validate_planes}; +use crate::compression::{Compression, MAX_DECODED_BYTES, decode_planes, rle_widening_len, validate_planes}; use crate::error::{PsdError, Result}; use crate::header::{ColorMode, Header, Version}; use crate::image_data::ImageData; @@ -384,7 +384,7 @@ impl PsdFile { })?; match self.merged_widened_rows(widen)? { 0 => out.put_encoded(&self.image_data.data, self.image_data.stored.as_ref()), - rows => out.put_widened_rle(&self.image_data.encoded()?, rows), + rows => out.put_widened_encoded(&self.image_data.data, self.image_data.stored.as_ref(), rows), } } @@ -414,13 +414,13 @@ impl PsdFile { for c in &layer.channels { match layer.widened_rows(c, true)? { 0 => {} - rows => n = n.saturating_add(rle_widening(&c.encoded()?, rows)?), + rows => n = n.saturating_add(rle_widening_len(c.encoded_len(), rows)?), } } } match self.merged_widened_rows(true)? { 0 => Ok(n), - rows => Ok(n.saturating_add(rle_widening(&self.image_data.encoded()?, rows)?)), + rows => Ok(n.saturating_add(rle_widening_len(self.image_data.encoded_len(), rows)?)), } } @@ -463,7 +463,7 @@ impl PsdFile { } let merged = match self.merged_widened_rows(widen)? { 0 => 0, - rows => rle_widening(&self.image_data.encoded()?, rows)?, + rows => rle_widening_len(self.image_data.encoded_len(), rows)?, }; Ok(n.saturating_add(2).saturating_add(self.image_data.encoded_len()).saturating_add(merged)) } @@ -747,6 +747,48 @@ mod write_tests { assert!(*source.1.lock().unwrap() <= 7, "read {} bytes at once", source.1.lock().unwrap()); } + /// #5333: RLE data still in its source widens to PSB in short reads too, the count table and + /// then the packed rows, one consecutive pass, never the whole merged image or channel at once. + #[test] + fn source_backed_rle_widens_in_short_reads() { + struct Reads(Vec, std::sync::Mutex<(usize, Vec<(u64, usize)>)>); + impl crate::source::ReadAt for Reads { + fn len(&self) -> u64 { + self.0.len() as u64 + } + fn read_at(&self, offset: u64, buf: &mut [u8]) -> Result<()> { + let mut seen = self.1.lock().unwrap(); + seen.0 = seen.0.max(buf.len()); + seen.1.push((offset, buf.len())); + self.0.as_slice().read_at(offset, buf) + } + } + let file = testgen::small(Version::Psd, Compression::Rle); + assert_eq!(file.image_data.compression, Compression::Rle); + let bytes = file.to_bytes().unwrap(); + let mut promoted = file.clone(); + promoted.promote_to_psb().unwrap(); + let want = promoted.to_bytes().unwrap(); + let source = std::sync::Arc::new(Reads(bytes, std::sync::Mutex::new((0, Vec::new())))); + let mut lazy = PsdFile::from_source(crate::Source::from_arc(source.clone())).unwrap(); + assert!(lazy.image_data.stored.is_some(), "the merged image stays in the source"); + lazy.promote_to_psb().unwrap(); + *source.1.lock().unwrap() = (0, Vec::new()); + let mut w = std::io::Cursor::new(Vec::new()); + lazy.write_to(&mut w).unwrap(); + assert!(w.into_inner() == want); + let seen = source.1.lock().unwrap(); + assert!(seen.0 <= 7, "read {} bytes at once", seen.0); + let merged = lazy.image_data.stored.as_ref().unwrap(); + let reads: Vec<_> = seen.1.iter().filter(|(at, _)| (merged.offset..merged.offset + merged.len).contains(at)).collect(); + let mut next = merged.offset; + for &&(at, n) in &reads { + assert_eq!(at, next, "the merged image is read in one consecutive pass"); + next += n as u64; + } + assert_eq!(next, merged.offset + merged.len); + } + /// #6607: pixel data goes to the stream as it is, never through a staging copy, so a save /// holds the model and a short buffer, not a second copy of the file. #[test] diff --git a/crates/psd/src/io.rs b/crates/psd/src/io.rs index bf4468711a..8537b74649 100644 --- a/crates/psd/src/io.rs +++ b/crates/psd/src/io.rs @@ -275,16 +275,43 @@ impl<'w, W: Write + Seek + ?Sized> Sink<'w, W> { let table_len = usize::try_from(crate::compression::rle_widening(data, rows)?).map_err(|_| PsdError::LimitExceeded("row count overflow"))?; let (table, packed) = data.split_at_checked(table_len).ok_or_else(|| PsdError::invalid("RLE row-count table out of range"))?; for part in table.chunks(STAGE / 2) { - self.small(|o| { - for count in part.as_chunks::<2>().0 { - o.extend_from_slice(&[0, 0, count[0], count[1]]); - } - Ok(()) - })?; + self.widen_counts(part)?; } self.put(packed) } + /// [`Self::put_widened_rle`] for data held in memory (`data`) or, when `stored`, still in its + /// source: the count table and then the packed rows are read in [`COPY_CHUNK`]-sized parts + /// (#5333), so a staged merged image or channel is never read back whole to widen it. + pub(crate) fn put_widened_encoded(&mut self, data: &[u8], stored: Option<&crate::source::Stored>, rows: usize) -> std::io::Result<()> { + let Some(s) = stored else { return self.put_widened_rle(data, rows) }; + let table_len = crate::compression::rle_widening_len(s.len, rows)?; + // Whole counts per part; the parts follow each other, so a staged range is read in one pass. + let table_part = (COPY_CHUNK / 2).max(1) * 2; + let mut at = 0u64; + while at < s.len { + let n = if at < table_len { (table_len - at).min(table_part) } else { (s.len - at).min(COPY_CHUNK) }; + let part = s.read_part(at, n)?; + if at < table_len { + self.widen_counts(&part)? + } else { + self.put(&part)? + } + at += n; + } + Ok(()) + } + + /// Two-byte RLE row counts as four-byte ones (`00 00` and the count), staged. + fn widen_counts(&mut self, counts: &[u8]) -> std::io::Result<()> { + self.small(|o| { + for count in counts.as_chunks::<2>().0 { + o.extend_from_slice(&[0, 0, count[0], count[1]]); + } + Ok(()) + }) + } + /// Appends encoded bytes held in memory (`data`) or, when `stored`, still in their source: /// those are copied through in [`COPY_CHUNK`] pieces, so a channel or merged image kept on /// disk (an imported file's, or one an exporter spilled to scratch) is never read whole. diff --git a/crates/psd/src/layer.rs b/crates/psd/src/layer.rs index 3b492e6a04..90f6550863 100644 --- a/crates/psd/src/layer.rs +++ b/crates/psd/src/layer.rs @@ -3,7 +3,7 @@ use std::borrow::Cow; use crate::blend::BlendMode; -use crate::compression::{Compression, PlaneLayout, decode_planes, encode_planes, rle_widening}; +use crate::compression::{Compression, PlaneLayout, decode_planes, encode_planes, rle_widening_len}; use crate::error::{PsdError, Result}; use crate::header::{MAX_DIMENSION, Version}; use crate::io::{Reader, Sink, WriteExt, append_with, decode_legacy_name, read_pascal, write_pascal}; @@ -692,7 +692,7 @@ impl LayerRecord { for c in &self.channels { out.put_i16(c.id); let rows = self.widened_rows(c, widen)?; - let grown = if rows == 0 { 0 } else { rle_widening(&c.encoded()?, rows)? }; + let grown = if rows == 0 { 0 } else { rle_widening_len(c.encoded_len(), rows)? }; out.put_len(c.stored_len().saturating_add(grown), version.is_psb())?; } out.put(b"8BIM"); @@ -783,7 +783,7 @@ impl LayerInfo { for l in &self.layers { for c in l.channels.iter().filter(|c| c.compression.is_some()) { let rows = l.widened_rows(c, widen)?; - let grown = if rows == 0 { 0 } else { rle_widening(&c.encoded()?, rows)? }; + let grown = if rows == 0 { 0 } else { rle_widening_len(c.encoded_len(), rows)? }; n = n.saturating_add(2).saturating_add(c.encoded_len()).saturating_add(grown); } } @@ -860,7 +860,7 @@ impl LayerInfo { })?; match l.widened_rows(c, widen)? { 0 => out.put_encoded(&c.data, c.stored.as_ref())?, - rows => out.put_widened_rle(&c.encoded()?, rows)?, + rows => out.put_widened_encoded(&c.data, c.stored.as_ref(), rows)?, } } } From 52eed152817771aa0392fa8e0ef2c6d74dcdf727 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 13:21:06 +0100 Subject: [PATCH 09/20] io: save layers past the gather size band by band, with their pixels, staged as they are encoded --- crates/io/src/psd_export.rs | 13 ++- crates/io/src/psd_export/banded.rs | 134 +++++++++++++++++++++++++++++ crates/io/src/psd_export/stage.rs | 120 ++++++++++++++++++++++++++ crates/io/src/psd_export/tests.rs | 127 +++++++++++++++++++++++++++ 4 files changed, 386 insertions(+), 8 deletions(-) create mode 100644 crates/io/src/psd_export/banded.rs diff --git a/crates/io/src/psd_export.rs b/crates/io/src/psd_export.rs index 6f87aa7588..a98cb347b1 100644 --- a/crates/io/src/psd_export.rs +++ b/crates/io/src/psd_export.rs @@ -15,6 +15,7 @@ use crate::blocks; use crate::pixels::{deinterleave, encode_be, psd_depth}; use crate::psd_import::REGENERATED; +mod banded; mod document; mod estimate; mod merged; @@ -43,10 +44,6 @@ const PSD_MAX_FILE_SIZE: u64 = 2 * 1024 * 1024 * 1024; /// Room kept under [`PSD_MAX_FILE_SIZE`] for what the save adds after the conversion (the image /// preview resource, a few KB). const PSD_SAVE_HEADROOM: u64 = 1024 * 1024; -/// Most bytes of one layer's interleaved samples the exporter gathers. The interleaved bytes and -/// the channel planes made from them (or the band parts and the joined buffer) are alive together, -/// so the peak is about twice this; it is also what fits in a file of at most [`PSD_MAX_FILE_SIZE`]. -const MAX_LAYER_GATHER_BYTES: u64 = if cfg!(target_pointer_width = "64") { 1 << 30 } else { 256 << 20 }; /// Options for [`document_to_psd_with`]. #[derive(Debug, Clone)] @@ -221,11 +218,11 @@ impl Ex { return (PsdRect::default(), self.empty_channels(), warnings); } // A layer record is one dense rectangle, so a few pixels far apart make a box of mostly - // nothing, gathered and then copied again. Past the cap the layer is saved empty, said so. + // nothing, gathered and then copied again. Past `banded_from` the layer is gathered and + // encoded a band of rows at a time instead (#5333). let dense = u64::from(r.width()).saturating_mul(u64::from(r.height())).saturating_mul(s.format().bytes_per_pixel() as u64); - if dense > MAX_LAYER_GATHER_BYTES { - warnings.push(format!("layer \"{name}\" spans {}×{} px, too large to write: saved without its pixels", r.width(), r.height())); - return (PsdRect::default(), self.empty_channels(), warnings); + if dense > banded::banded_from() { + return (to_psd_rect(r), self.banded_channels(&s, r), warnings); } // The interleaved gather is the sequential pass for full-canvas layers, so it runs // in row bands on all cores and the band results concatenate in order. diff --git a/crates/io/src/psd_export/banded.rs b/crates/io/src/psd_export/banded.rs new file mode 100644 index 0000000000..f2fc0a7751 --- /dev/null +++ b/crates/io/src/psd_export/banded.rs @@ -0,0 +1,134 @@ +//! Layers too large to gather at once, saved band by band (#5333). +//! +//! A layer record is one dense rectangle, gathered interleaved and split into channel planes, +//! so a layer's save held about twice its dense size; past [`BANDED_FROM`] that was refused and the +//! layer saved without its pixels. Now such a layer is gathered, split and encoded a band of +//! rows at a time. RLE rows pack independently (the row-count table is kept and written ahead of +//! the packed rows) and raw rows join, so those channels are the bytes the whole-layer encoder +//! writes. ZIP with prediction (32-bit) predicts per row and feeds one zlib stream: the same +//! pixels, but not always the same bytes, as zlib packs input it is given in parts differently. +//! With a stage each band's parts go to the scratch disk as they are made (see [`Parts`]). + +use std::io::Write as _; + +use photocraft_psd::compression::{PlaneLayout, encode_planes, predict}; +use photocraft_psd::{ChannelData, Compression, PsdError}; +use photocraft_raster::Surface; + +use super::Ex; +use super::stage::Parts; +use crate::pixels::{deinterleave, psd_depth}; + +/// Dense bytes of one layer (as gathered) from which it is saved band by band. Below it a layer +/// is gathered whole, which is faster: the peak is about twice this. +pub(super) const BANDED_FROM: u64 = if cfg!(target_pointer_width = "64") { 1 << 30 } else { 256 << 20 }; +/// Interleaved bytes gathered per band. A band's planes and encoded parts are alive with it, so a +/// banded layer's save holds about three times this. +const BAND_BYTES: usize = 64 << 20; + +#[cfg(test)] +thread_local! { + /// `(BANDED_FROM, BAND_BYTES)` for this thread's exports, so tests band small layers. + pub(crate) static BANDS_FOR_TESTS: std::cell::Cell> = const { std::cell::Cell::new(None) }; +} + +pub(super) fn banded_from() -> u64 { + #[cfg(test)] + if let Some((from, _)) = BANDS_FOR_TESTS.get() { + return from; + } + BANDED_FROM +} + +fn band_bytes() -> usize { + #[cfg(test)] + if let Some((_, bytes)) = BANDS_FOR_TESTS.get() { + return bytes; + } + BAND_BYTES +} + +/// One channel's encoding state across bands. +struct Channel { + id: i16, + table: Vec, + zip: Option>>, + parts: Parts, +} + +impl Ex { + /// The channels of `s` over `r` (non-empty), alpha first, encoded band by band. + pub(super) fn banded_channels(&self, s: &Surface, r: photocraft_geom::Rect) -> Vec { + // RLE fails only on a row too long for its count (not within PSD/PSB limits); then the + // layer is written raw, as a channel encoded whole falls back to. + self.banded_pass(s, r, self.compression()).or_else(|_| self.banded_pass(s, r, Compression::Raw)).unwrap_or_else(|_| self.empty_channels()) + } + + fn banded_pass(&self, s: &Surface, r: photocraft_geom::Rect, compression: Compression) -> photocraft_psd::Result> { + let (w, h) = (r.width() as usize, r.height() as usize); + let bpp = s.format().bytes_per_pixel(); + let band_rows = (band_bytes() / (w * bpp).max(1)).max(1); + let depth = psd_depth(self.fmt.sample); + let count = if self.version.is_psb() { 4 } else { 2 }; + let head = if compression == Compression::Rle { h.checked_mul(count).ok_or(PsdError::LimitExceeded("row count overflow"))? } else { 0 }; + let mut channels: Vec = (0..=self.cc) + .map(|k| Channel { + id: k as i16 - 1, + table: Vec::with_capacity(head), + zip: (compression == Compression::ZipPrediction).then(|| flate2::write::ZlibEncoder::new(Vec::new(), flate2::Compression::default())), + parts: Parts::new(self.stage.clone(), head), + }) + .collect(); + let mut invert = vec![self.cmyk; self.cc]; + invert.push(false); + for y in (0..h).step_by(band_rows) { + let rows = band_rows.min(h - y); + let top = r.y0 + y as i32; + let pieces = crate::pixels::par_map(crate::pixels::bands(rows), |b| { + s.to_interleaved(photocraft_geom::Rect::new(r.x0, top + b.start as i32, r.x1, top + b.end as i32)) + }); + let mut bytes = Vec::with_capacity(w * rows * bpp); + for piece in pieces { + bytes.extend_from_slice(&piece); + } + if self.fmt.mode == photocraft_color::ColorMode::Lab && self.fmt.sample == photocraft_color::SampleType::U16 { + crate::pixels::lab16_chroma(&mut bytes, self.cc + 1, false); + } + let mut planes: Vec>> = deinterleave(&bytes, self.cc + 1, self.fmt.sample, &invert).into_iter().map(Some).collect(); + drop(bytes); + let layout = PlaneLayout { planes: 1, width: w, height: rows, depth, version: self.version }; + // Alpha (planes[cc]) is channel 0, then the colour channels; each on its own thread. + let work: Vec<(&mut Channel, Vec)> = + channels.iter_mut().enumerate().map(|(k, c)| (c, planes[if k == 0 { self.cc } else { k - 1 }].take().unwrap_or_default())).collect(); + let done = crate::pixels::par_map(work, |(c, mut plane)| -> photocraft_psd::Result<()> { + match compression { + Compression::Rle => { + let mut packed = encode_planes(Compression::Rle, &plane, &layout)?; + let rest = packed.split_off(rows * count); + c.table.extend_from_slice(&packed); + c.parts.push(rest); + } + Compression::ZipPrediction => { + predict(&mut plane, &layout)?; + let zip = c.zip.as_mut().ok_or_else(|| PsdError::Invalid("banded ZIP channel without its stream".into()))?; + zip.write_all(&plane).map_err(|e| PsdError::Invalid(format!("zlib: {e}")))?; + let out = std::mem::take(zip.get_mut()); + c.parts.push(out); + } + _ => c.parts.push(plane), + } + Ok(()) + }); + done.into_iter().collect::>()?; + } + channels + .into_iter() + .map(|mut c| { + if let Some(zip) = c.zip.take() { + c.parts.push(zip.finish().map_err(|e| PsdError::Invalid(format!("zlib: {e}")))?); + } + c.parts.finish(c.id, compression, &c.table) + }) + .collect() + } +} diff --git a/crates/io/src/psd_export/stage.rs b/crates/io/src/psd_export/stage.rs index 3e04d53247..869208e319 100644 --- a/crates/io/src/psd_export/stage.rs +++ b/crates/io/src/psd_export/stage.rs @@ -95,9 +95,129 @@ impl Stage { image } + /// Appends one part of a channel written band by band; `None` when the write fails. + fn append(&self, data: &[u8]) -> Option { + self.file.0.append(data).ok() + } + /// Bytes on the scratch disk. #[cfg(test)] pub(crate) fn len(&self) -> u64 { self.file.0.len() } } + +/// One channel of a layer encoded band by band (#5333): its head (an RLE row-count table, filled +/// in when the last band is encoded) and the bands' encoded parts in order. With a stage every +/// part goes to the scratch disk as it is made, so a layer of any size is saved holding about one +/// band; without one the parts are joined in memory, as a channel encoded whole would be. +pub(crate) struct Parts { + stage: Option>, + head_len: usize, + /// Without a stage: the head's room and the parts so far. + joined: Vec, + /// With a stage: the parts, staged or (when a write failed) kept. + parts: Vec, + len: u64, +} + +enum Part { + Kept(Vec), + Staged { offset: u64, len: u64 }, +} + +impl Part { + fn len(&self) -> u64 { + match self { + Part::Kept(v) => v.len() as u64, + Part::Staged { len, .. } => *len, + } + } +} + +impl Parts { + pub(crate) fn new(stage: Option>, head_len: usize) -> Self { + let joined = if stage.is_none() { vec![0; head_len] } else { Vec::new() }; + Parts { stage, head_len, joined, parts: Vec::new(), len: head_len as u64 } + } + + /// Adds the next encoded part. + pub(crate) fn push(&mut self, data: Vec) { + if data.is_empty() { + return; + } + self.len += data.len() as u64; + match &self.stage { + None => self.joined.extend_from_slice(&data), + Some(stage) => self.parts.push(match stage.append(&data) { + Some(offset) => Part::Staged { offset, len: data.len() as u64 }, + None => Part::Kept(data), + }), + } + } + + /// The channel: `head` (of the length given to [`Parts::new`]) and then the parts. + pub(crate) fn finish(mut self, id: i16, compression: photocraft_psd::Compression, head: &[u8]) -> photocraft_psd::Result { + if head.len() != self.head_len { + return Err(PsdError::Invalid("banded channel head has the wrong length".into())); + } + let Some(stage) = self.stage else { + self.joined[..head.len()].copy_from_slice(head); + self.joined.shrink_to_fit(); + return Ok(ChannelData::new(id, Some(compression), self.joined)); + }; + let mut parts = Vec::with_capacity(self.parts.len() + 1); + if !head.is_empty() { + parts.push(Part::Kept(head.to_vec())); + } + parts.append(&mut self.parts); + let mut starts = Vec::with_capacity(parts.len()); + let mut at = 0; + for p in &parts { + starts.push(at); + at += p.len(); + } + let joined = Joined { file: stage.file.clone(), parts, starts, len: self.len }; + let stored = Stored { source: Source::from_arc(Arc::new(joined)), offset: 0, len: self.len }; + Ok(ChannelData { id, compression: Some(compression), data: Vec::new(), stored: Some(stored) }) + } +} + +/// A banded channel's bytes, in memory and in its staged parts. A read is split at the parts, so +/// the writer's consecutive pass reads each staged append from its start to its end, and the stage +/// file checks it. +struct Joined { + file: Arc, + parts: Vec, + starts: Vec, + len: u64, +} + +impl ReadAt for Joined { + fn len(&self) -> u64 { + self.len + } + + fn read_at(&self, offset: u64, buf: &mut [u8]) -> photocraft_psd::Result<()> { + if offset.checked_add(buf.len() as u64).is_none_or(|end| end > self.len) { + return Err(PsdError::Read(format!("staged export data, {} bytes at offset {offset} past its {} bytes", buf.len(), self.len))); + } + let mut pos = offset; + let mut buf = buf; + let mut i = self.starts.partition_point(|&s| s <= pos).saturating_sub(1); + while !buf.is_empty() { + let (part, start) = (&self.parts[i], self.starts[i]); + let within = pos - start; + let n = usize::try_from((part.len() - within).min(buf.len() as u64)).unwrap_or(buf.len()); + let (now, rest) = std::mem::take(&mut buf).split_at_mut(n); + match part { + Part::Kept(v) => now.copy_from_slice(&v[within as usize..within as usize + n]), + Part::Staged { offset, .. } => self.file.read_at(offset + within, now)?, + } + buf = rest; + pos += n as u64; + i += 1; + } + Ok(()) + } +} diff --git a/crates/io/src/psd_export/tests.rs b/crates/io/src/psd_export/tests.rs index 4fd78e8c73..c1d47c4777 100644 --- a/crates/io/src/psd_export/tests.rs +++ b/crates/io/src/psd_export/tests.rs @@ -672,3 +672,130 @@ fn staging_keeps_short_or_unwritable_data_in_memory() { assert!(stage::Stage::for_document(10, None).is_none(), "not asked for"); let _ = std::fs::remove_dir(&dir); } + +/// The first layer's channels, decoded. +fn decoded_channels(file: &PsdFile) -> Vec> { + let layer = &file.layers()[0]; + let (w, h) = (layer.rect.width() as usize, layer.rect.height() as usize); + layer.channels.iter().map(|c| c.decode(w, h, file.header.depth, file.header.version).unwrap()).collect() +} + +/// A noisy, translucent layer inset from the canvas: `w`×`h` px in `mode`/`depth`. +fn banding_document(mode: ColorMode, depth: SampleType, w: u32, h: u32) -> Document { + let mut doc = Document::new("banded", photocraft_geom::Size::new(w, h), mode, depth); + let mut layer = Layer::raster("Layer 1", doc.pixel_format()); + let rect = photocraft_geom::Rect::new(5, 3, w as i32 - 7, h as i32 - 2); + let surface = layer.surface_mut().unwrap(); + let ch = surface.channels(); + let mut state = 0x2545_f491_u32; + let n = rect.width() as usize * rect.height() as usize * ch; + let data: Vec = (0..n) + .map(|i| { + state ^= state << 13; + state ^= state >> 17; + state ^= state << 5; + // Runs as well as noise, so RLE packs both kinds of row. + let v = if (i / (ch * 40)).is_multiple_of(3) { 0.5 } else { (state >> 20) as f32 / 4095.0 }; + if i % ch == ch - 1 { v.max(0.25) } else { v } + }) + .collect(); + surface.write_region(rect, &data); + doc.layers.push(layer); + doc +} + +/// Exports `doc` with layers past `banded` bytes saved in bands of `rows` rows. +fn export_banded(doc: &Document, options: &PsdExportOptions, banded: Option) -> (PsdFile, Vec) { + let row_bytes = doc.size.width as usize * doc.pixel_format().bytes_per_pixel(); + banded::BANDS_FOR_TESTS.set(banded.map(|rows| (0, rows * row_bytes + 3))); + let out = document_to_psd_sized(doc, options, 0, &Interrupt::NONE, PSD_MAX_FILE_SIZE); + banded::BANDS_FOR_TESTS.set(None); + out +} + +/// #5333: a layer saved band by band (here every layer, in bands of a few rows) writes the bytes +/// of one gathered whole (ZIP with prediction, 32-bit, the same pixels); RGB, CMYK and Lab; PSD, PSB and a PSD +/// promoted to PSB; in memory and staged on the scratch disk (then also streamed). +#[test] +fn banded_layers_write_the_bytes_of_whole_ones() { + let dir = std::env::temp_dir().join(format!("pc-psd-banded-{}", std::process::id())); + let cases = [ + (ColorMode::Rgb, SampleType::U8), + (ColorMode::Rgb, SampleType::U16), + (ColorMode::Rgb, SampleType::F32), + (ColorMode::Cmyk, SampleType::U8), + (ColorMode::Lab, SampleType::U16), + ]; + for (mode, depth) in cases { + let doc = banding_document(mode, depth, 203, 157); + for force_psb in [false, true] { + for staged in [false, true] { + stage::DIR_FOR_TESTS.with(|d| *d.borrow_mut() = staged.then(|| dir.clone())); + let options = PsdExportOptions { force_psb, stage_on_scratch_from: staged.then_some(0), ..Default::default() }; + let (want, want_warnings) = export_banded(&doc, &options, None); + for rows in [1, 7, 1000] { + let what = format!("{mode:?} {depth:?}, psb {force_psb}, staged {staged}, {rows} rows a band"); + let (file, warnings) = export_banded(&doc, &options, Some(rows)); + assert_eq!(warnings, want_warnings, "{what}"); + let layer = &file.layers()[0]; + assert_eq!(layer.rect, want.layers()[0].rect, "{what}"); + if staged { + assert!(layer.channels.iter().all(|c| c.stored.is_some() && c.data.is_empty()), "{what}: banded channels are staged"); + } + if depth == SampleType::F32 { + // One zlib stream fed band by band packs differently from one fed whole + // (the encoder sees a different amount of input ahead), so the ZIP + // channels are compared decoded, here and as the streamed file reads back. + let mut w = std::io::Cursor::new(Vec::new()); + file.write_to(&mut w).unwrap(); + let back = PsdFile::from_bytes(&w.into_inner()).unwrap(); + assert_eq!(decoded_channels(&back), decoded_channels(&want), "{what}: banded ZIP channels differ"); + assert!(back.image_data.encoded().unwrap() == want.image_data.encoded().unwrap(), "{what}"); + continue; + } + let bytes = want.to_bytes().unwrap(); + assert!(file.to_bytes().unwrap() == bytes, "{what}: banded bytes differ"); + let mut w = std::io::Cursor::new(Vec::new()); + file.write_to(&mut w).unwrap(); + assert!(w.into_inner() == bytes, "{what}: streamed banded bytes differ"); + if !force_psb { + let (mut promoted, mut whole) = (file.clone(), want.clone()); + promoted.promote_to_psb().unwrap(); + whole.promote_to_psb().unwrap(); + let mut w = std::io::Cursor::new(Vec::new()); + promoted.write_to(&mut w).unwrap(); + assert!(w.into_inner() == whole.to_bytes().unwrap(), "{what}: promoted banded bytes differ"); + } + } + } + } + } + stage::DIR_FOR_TESTS.with(|d| *d.borrow_mut() = None); + let _ = std::fs::remove_dir_all(&dir); +} + +/// #5333: a layer past the gather size is saved with its pixels (it used to be saved empty, with a +/// warning), and they come back on import. +#[test] +fn banded_layers_round_trip() { + let dir = std::env::temp_dir().join(format!("pc-psd-banded-rt-{}", std::process::id())); + for depth in [SampleType::U8, SampleType::U16, SampleType::F32] { + let doc = banding_document(ColorMode::Rgb, depth, 181, 133); + for staged in [false, true] { + stage::DIR_FOR_TESTS.with(|d| *d.borrow_mut() = staged.then(|| dir.clone())); + let options = PsdExportOptions { stage_on_scratch_from: staged.then_some(0), ..Default::default() }; + let (file, warnings) = export_banded(&doc, &options, Some(5)); + assert!(warnings.is_empty(), "{warnings:?}"); + let mut w = std::io::Cursor::new(Vec::new()); + file.write_to(&mut w).unwrap(); + let back = PsdFile::from_bytes(&w.into_inner()).unwrap(); + let (read, _) = crate::psd_import::psd_to_document(&back); + let full = photocraft_geom::Rect::new(0, 0, doc.size.width as i32, doc.size.height as i32); + let want = doc.layers[0].surface().unwrap().read_region(full); + let got = read.layers[0].surface().unwrap().read_region(full); + assert!(got == want, "{depth:?}, staged {staged}: pixels differ after the round trip"); + } + } + stage::DIR_FOR_TESTS.with(|d| *d.borrow_mut() = None); + let _ = std::fs::remove_dir_all(&dir); +} From 381d42ad416b12443462e23fc8d5ad9007c79185 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 13:58:35 +0100 Subject: [PATCH 10/20] Viewport preview: sample the last document pixel for partial edge cells A partial reduced cell at the document's right or bottom edge sampled c*k + k/2, past the document, so the provisional preview showed the layer's default there instead of the edge pixel. Clamp the sample to the last document pixel (document bounds, so neighbouring pages agree). Addresses CodeRabbit review r4454088350 on #5332. --- crates/compose/src/viewport.rs | 53 ++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/crates/compose/src/viewport.rs b/crates/compose/src/viewport.rs index c7a5cc3c10..fe4db1551b 100644 --- a/crates/compose/src/viewport.rs +++ b/crates/compose/src/viewport.rs @@ -25,56 +25,63 @@ pub fn preview_page(doc: &Document, source: Rect, factor: u32, interrupt: Interr if rect.width() > 1024 || rect.height() > 1024 { return Err("viewport page is too large".into()); } - fn shrink(s: &Surface, r: Rect, k: i32, i: Interrupt<'_>) -> Result { + // Cell `c` samples source pixel `c·k + k/2`, clamped to the last document pixel: a partial + // cell at the document's edge samples inside it, not the layer's default past it. (Document + // bounds, not this page's, so neighbouring pages agree.) + let bounds = doc.bounds(); + let last = (bounds.x1 - 1, bounds.y1 - 1); + fn shrink(s: &Surface, r: Rect, k: i32, last: (i32, i32), i: Interrupt<'_>) -> Result { let mut out = Surface::with_default(s.format(), &s.default_pixel()); let bpp = s.format().bytes_per_pixel(); + let x0 = r.x0.saturating_mul(k); for y in r.y0..r.y1 { i.check().map_err(|e| e.to_string())?; - let sy = y.saturating_mul(k).saturating_add(k / 2); - let row = s.to_interleaved(Rect::new(r.x0.saturating_mul(k), sy, r.x1.saturating_mul(k), sy.saturating_add(1))); + let sy = y.saturating_mul(k).saturating_add(k / 2).min(last.1); + let row = s.to_interleaved(Rect::new(x0, sy, r.x1.saturating_mul(k).min(last.0 + 1), sy.saturating_add(1))); let mut reduced = Vec::with_capacity(r.width() as usize * bpp); - for x in 0..r.width() as usize { - let offset = (x * k as usize + k as usize / 2) * bpp; + for x in r.x0..r.x1 { + let sx = x.saturating_mul(k).saturating_add(k / 2).min(last.0); + let offset = (sx - x0) as usize * bpp; reduced.extend_from_slice(row.get(offset..offset + bpp).ok_or("invalid viewport sample")?); } out.write_interleaved(Rect::new(r.x0, y, r.x1, y + 1), &reduced); } Ok(out) } - fn layer(l: &mut photocraft_doc::Layer, r: Rect, k: i32, i: Interrupt<'_>, depth: usize) -> Result<(), String> { + fn layer(l: &mut photocraft_doc::Layer, r: Rect, k: i32, i: Interrupt<'_>, depth: usize, last: (i32, i32)) -> Result<(), String> { if depth > 256 { return Err("viewport layer nesting limit exceeded".into()); } if let Some(m) = &mut l.mask { - m.surface = shrink(&m.surface, r, k, i)?; + m.surface = shrink(&m.surface, r, k, last, i)?; } if let Some(c) = &mut l.fill_cache { - c.surface = shrink(&c.surface, r, k, i)?; + c.surface = shrink(&c.surface, r, k, last, i)?; } use photocraft_doc::LayerContent; match &mut l.content { - LayerContent::Raster(s) => *s = shrink(s, r, k, i)?, + LayerContent::Raster(s) => *s = shrink(s, r, k, last, i)?, LayerContent::Group(g) => { if let Some(a) = &mut g.artboard { a.rect = Rect::new(a.rect.x0.div_euclid(k), a.rect.y0.div_euclid(k), a.rect.x1.div_euclid(k), a.rect.y1.div_euclid(k)); } for c in &mut g.children { - layer(c, r, k, i, depth + 1)?; + layer(c, r, k, i, depth + 1, last)?; } } LayerContent::Text(t) => { if let Some(s) = &mut t.cache { - *s = shrink(s, r, k, i)?; + *s = shrink(s, r, k, last, i)?; } } LayerContent::Shape(t) => { if let Some(s) = &mut t.cache { - *s = shrink(s, r, k, i)?; + *s = shrink(s, r, k, last, i)?; } } LayerContent::Smart(t) => { if let Some(s) = &mut t.cache { - *s = shrink(s, r, k, i)?; + *s = shrink(s, r, k, last, i)?; } } _ => {} @@ -84,10 +91,10 @@ pub fn preview_page(doc: &Document, source: Rect, factor: u32, interrupt: Interr let mut p = doc.clone(); p.size = photocraft_doc::Size::new(doc.size.width.div_ceil(factor), doc.size.height.div_ceil(factor)); for l in &mut p.layers { - layer(l, rect, k, interrupt, 0)?; + layer(l, rect, k, interrupt, 0, last)?; } for c in &mut p.channels { - c.surface = shrink(&c.surface, rect, k, interrupt)?; + c.surface = shrink(&c.surface, rect, k, last, interrupt)?; } interrupt.check().map_err(|e| e.to_string())?; Ok(render(&p, rect)) @@ -200,6 +207,22 @@ mod tests { } } } + /// A partial cell at the document's right and bottom edges samples the last document pixel, + /// not the layer's (transparent) default past it (#5332 review). + #[test] + fn preview_edge_cells_sample_inside_the_document() { + let mut d = Document::new("edge", Size::new(10, 10), ColorMode::Rgb, SampleType::U8); + let mut s = photocraft_raster::Surface::new(d.pixel_format()); + s.fill_rect(d.bounds(), &[1.0, 0.0, 0.0, 1.0]); + d.layers.push(photocraft_doc::Layer::new("red", photocraft_doc::LayerContent::Raster(s))); + for source in [d.bounds(), Rect::new(4, 4, 10, 10)] { + let p = preview_page(&d, source, 4, Interrupt::NONE).unwrap(); + assert_eq!(p.rect, Rect::new(source.x0 / 4, source.y0 / 4, 3, 3)); + for (i, px) in p.px.iter().enumerate() { + assert!((px[0] - 1.0).abs() < 1e-6 && (px[3] - 1.0).abs() < 1e-6, "{source:?} cell {i}: {px:?}"); + } + } + } /// A white document with a noisy, partly transparent layer and a multiplied solid over one-colour /// tiles, so the reduced path takes both its one-colour and per-pixel branches. fn busy(depth: SampleType) -> Document { From ef79ba04ebe33e76f1c3a6deb97165dc61fc51e0 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 13:59:12 +0100 Subject: [PATCH 11/20] Viewport preview: keep artboard cells whose sample is on the board The preview mapped artboard edges with div_euclid, so a board edge in the second half of a cell dropped (upper edge) or kept (lower edge) a cell against its sample at c*k + k/2, and a board narrower than a cell could vanish. Map each edge to the first cell sampling at or after it. Addresses CodeRabbit review r4454088367 on #5332. --- crates/compose/src/viewport.rs | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/crates/compose/src/viewport.rs b/crates/compose/src/viewport.rs index fe4db1551b..40a6accee9 100644 --- a/crates/compose/src/viewport.rs +++ b/crates/compose/src/viewport.rs @@ -63,7 +63,13 @@ pub fn preview_page(doc: &Document, source: Rect, factor: u32, interrupt: Interr LayerContent::Raster(s) => *s = shrink(s, r, k, last, i)?, LayerContent::Group(g) => { if let Some(a) = &mut g.artboard { - a.rect = Rect::new(a.rect.x0.div_euclid(k), a.rect.y0.div_euclid(k), a.rect.x1.div_euclid(k), a.rect.y1.div_euclid(k)); + // A cell is on the board when its sample `c·k + k/2` is: each edge maps to the + // first cell sampling at or after it. + let edge = |e: i32| { + let (s, k) = (i64::from(e) - i64::from(k / 2), i64::from(k)); + (s.div_euclid(k) + i64::from(s.rem_euclid(k) != 0)) as i32 + }; + a.rect = Rect::new(edge(a.rect.x0), edge(a.rect.y0), edge(a.rect.x1), edge(a.rect.y1)); } for c in &mut g.children { layer(c, r, k, i, depth + 1, last)?; @@ -223,6 +229,24 @@ mod tests { } } } + /// An artboard covers the preview cells whose sample (`c·k + k/2`) lies on the board, so a + /// narrow board, or one whose edges fall mid-cell, keeps exactly those cells (#5332 review). + #[test] + fn preview_artboard_covers_the_cells_it_samples() { + let k = 4; + // (board x0..x1, cells expected on it); cell c samples x = 4c + 2. + for (x0, x1, on) in [(2, 3, 0..1), (0, 3, 0..1), (3, 16, 1..4), (1, 7, 0..2), (4, 14, 1..3), (-6, 6, 0..1), (-6, 7, 0..2)] { + let mut d = Document::new("boards", Size::new(16, 4), ColorMode::Rgb, SampleType::U8); + let board = photocraft_doc::Artboard::new(Rect::new(x0, 0, x1, 4)); + let group = photocraft_doc::Group { children: vec![], expanded: true, artboard: Some(board), frame_stroke: None }; + d.layers.push(photocraft_doc::Layer::new("board", photocraft_doc::LayerContent::Group(group))); + let p = preview_page(&d, d.bounds(), k, Interrupt::NONE).unwrap(); + assert_eq!(p.rect, Rect::new(0, 0, 4, 1)); + for (c, px) in p.px.iter().enumerate() { + assert_eq!(px[3] > 0.5, on.contains(&c), "board {x0}..{x1}, cell {c}: {px:?}"); + } + } + } /// A white document with a noisy, partly transparent layer and a multiplied solid over one-colour /// tiles, so the reduced path takes both its one-colour and per-pixel branches. fn busy(depth: SampleType) -> Document { From dadc6a8d5f29a5445fc39dbf7b175d42e667d46c Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 14:02:36 +0100 Subject: [PATCH 12/20] Viewport pages: fall back when an edited page fails to render stuck() only counted a failed page as a hole when no page was held, so a dirty (edited) page whose new render failed kept drawing its pre-edit content until a retry. Treat a held dirty page like a missing one. Addresses CodeRabbit review r4453118133 on #5332. --- crates/ui-egui/src/canvas/viewport_pages.rs | 23 ++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/crates/ui-egui/src/canvas/viewport_pages.rs b/crates/ui-egui/src/canvas/viewport_pages.rs index 72483f9c0e..01c7d791cf 100644 --- a/crates/ui-egui/src/canvas/viewport_pages.rs +++ b/crates/ui-egui/src/canvas/viewport_pages.rs @@ -396,10 +396,11 @@ impl Stream { } } - /// Whether a wanted page can't be shown: its render or upload failed and nothing older of it - /// is held, so the view would have a hole (the CPU canvas draws instead). + /// Whether a wanted page can't be shown: its render or upload failed and nothing current of + /// it is held, so the view would have a hole, or keep showing the page from before an edit + /// (the CPU canvas draws instead). pub fn stuck(&self, wanted: &[PageId]) -> bool { - wanted.iter().any(|p| !self.pages.contains_key(p) && self.failed.contains(&(*p, self.epoch(*p)))) + wanted.iter().any(|p| self.pages.get(p).is_none_or(|q| q.dirty) && self.failed.contains(&(*p, self.epoch(*p)))) } /// Whether work is outstanding (the view repaints until it is done). @@ -926,6 +927,22 @@ mod tests { assert!(!s.stuck(&view)); } + /// An edited page whose new render fails isn't left showing its old content (#5332 review). + #[test] + fn a_dirty_page_whose_render_failed_falls_back() { + let view = [page(1, 0, 0)]; + let mut s = holding(&view); + s.invalidate(id(2), None); + assert!(s.pages[&view[0]].dirty && !s.stuck(&view), "the old page shows while it renders"); + let mut log = Vec::new(); + s.update(&view, true, recorder(&mut log)); + s.tx.send(Done { page: view[0], epoch: log[0].1, exact: true, region: None, result: Err("out of memory".into()) }).unwrap(); + s.update(&view, true, |_, _, _, _, _| panic!("no retry while failed")); + assert!(s.stuck(&view), "a stale page is not drawn in place of the failed one"); + s.retry_after((7, 7)); + assert!(!s.stuck(&view)); + } + #[test] fn prune_drops_finer_and_dirty_pages_once_the_view_is_current() { let (fine, coarse, view) = (page(1, 0, 0), page(4, 0, 0), page(2, 0, 0)); From e291eef6d6e7753a52fdf0afb1a56a91c2e5acd7 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 14:03:59 +0100 Subject: [PATCH 13/20] Viewport pages: free a document's pages when no page fits the budget With a canvas budget too small for one page, show() returned to the CPU canvas after creating or invalidating the stream but before pruning, so the stream's GPU pages and its jobs stayed held. Check the page budget first and drop the stream and its pages, as a document that fits does. Addresses CodeRabbit review r4453118176 on #5332. --- .../src/canvas/background_redraw/tests.rs | 3 +++ crates/ui-egui/src/canvas/viewport_pages.rs | 24 ++++++++++++------- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/crates/ui-egui/src/canvas/background_redraw/tests.rs b/crates/ui-egui/src/canvas/background_redraw/tests.rs index a5397c77dd..a669b18a38 100644 --- a/crates/ui-egui/src/canvas/background_redraw/tests.rs +++ b/crates/ui-egui/src/canvas/background_redraw/tests.rs @@ -687,6 +687,9 @@ fn a_canvas_over_the_texture_budget_draws_its_viewport_pages_on_the_gpu() { gpu.set_canvas_budget(1); g.harness.run_steps(3); assert_eq!(render_path_of(&g), (Some("canvasTooLarge"), Some("CPU Rendering"))); + // Its pages and their work are freed meanwhile (#5332 review). + assert!(gpu.pages(key).0.is_empty(), "pages freed"); + assert!(!g.harness.state().streams.contains_key(&doc.id), "stream dropped"); gpu.set_canvas_budget(set - 1); settle_pages(&mut g); // Closing the document cancels its work and frees its pages. diff --git a/crates/ui-egui/src/canvas/viewport_pages.rs b/crates/ui-egui/src/canvas/viewport_pages.rs index 01c7d791cf..410a7d4dfc 100644 --- a/crates/ui-egui/src/canvas/viewport_pages.rs +++ b/crates/ui-egui/src/canvas/viewport_pages.rs @@ -597,6 +597,20 @@ pub(super) fn show(app: &mut PhotocraftApp, ctx: &egui::Context, idx: usize, xf: } return false; } + let key = stream_key(id.0); + // Half the per-document canvas budget: the rest is headroom for what the compositor needs. + let limit = gpu.canvas_budget() / 2; + let page_bytes = gpu.page_texture_bytes(doc.depth, size, PAGE_SIDE + 2, PAGE_SIDE + 2).max(1); + let max_pages = usize::try_from(limit / page_bytes).unwrap_or(usize::MAX); + // No room for a single page: the CPU canvas draws the document, as main does for every + // document over the budget, and the pages and work held for it are freed. + if max_pages == 0 { + if let Some(s) = app.streams.remove(&id) { + drop(s); + gpu.retain_pages(key, &[]); + } + return false; + } let (display, _) = canvas_display(app, &doc, None); let display_key = texture_key(display.as_deref()); let now = Identity { revision, preview: raw_key, display: display_key, size }; @@ -607,11 +621,6 @@ pub(super) fn show(app: &mut PhotocraftApp, ctx: &egui::Context, idx: usize, xf: .and_then(|s| damage_since(app, idx, (s.revision, s.preview ^ display_key), (revision, raw_key), display_key, last_damage)); app.streams.entry(id).or_default().invalidate(now, damage); } - let key = stream_key(id.0); - // Half the per-document canvas budget: the rest is headroom for what the compositor needs. - let limit = gpu.canvas_budget() / 2; - let page_bytes = gpu.page_texture_bytes(doc.depth, size, PAGE_SIDE + 2, PAGE_SIDE + 2).max(1); - let max_pages = usize::try_from(limit / page_bytes).unwrap_or(usize::MAX); let wanted = pages(size, visible_doc_rect(xf), zoom, max_pages); if gpu.texture_info(id.0).is_some() { // Switching from the texture set: free it, the pages are budgeted against that memory. @@ -619,9 +628,8 @@ pub(super) fn show(app: &mut PhotocraftApp, ctx: &egui::Context, idx: usize, xf: } let stream = app.streams.entry(id).or_default(); stream.retry_after((photocraft_raster::spill::read_error_generation(), photocraft_raster::memory::configuration_generation())); - // No room for a single page, or a page that failed: the CPU canvas draws the document, as - // main does for every document over the budget. - if max_pages == 0 || stream.stuck(&wanted) { + // A page that failed: the CPU canvas draws the document until it is retried. + if stream.stuck(&wanted) { return false; } let faithful = photocraft_compose::proxy::proxy_faithful(&doc); From 0a97fa7f75a749877ca26b7e0bbbd8ca7f4130e1 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 14:06:05 +0100 Subject: [PATCH 14/20] GPU canvas: catch errors creating or writing a viewport page upload_page created the page texture, uniform buffer and bind group and wrote the texels with no error scope, so an out-of-memory error there reached the uncaptured-error handler and disabled the GPU canvas for the session. Share DocTextures::new's validation/out-of-memory scopes as catch_gpu_errors and run the page work inside them: a refused page fails alone (the view falls back to the CPU canvas) and the device stays usable. Addresses CodeRabbit review r4453118114 on #5332. --- .../src/canvas/gpu_canvas/doc_textures.rs | 30 ++++++--- .../src/canvas/gpu_canvas/mip_tests.rs | 31 +++++++++ crates/ui-egui/src/canvas/gpu_canvas/pages.rs | 67 +++++++++++-------- 3 files changed, 90 insertions(+), 38 deletions(-) diff --git a/crates/ui-egui/src/canvas/gpu_canvas/doc_textures.rs b/crates/ui-egui/src/canvas/gpu_canvas/doc_textures.rs index 155bb21c91..e21efd41ca 100644 --- a/crates/ui-egui/src/canvas/gpu_canvas/doc_textures.rs +++ b/crates/ui-egui/src/canvas/gpu_canvas/doc_textures.rs @@ -44,6 +44,24 @@ pub(super) fn tile_uniform([x, y, w, h]: [u32; 4], core: Option<[i32; 4]>) -> [f [x as f32, y as f32, w as f32, h as f32, cx0, cy0, cx1, cy1] } +/// Run `f` with its validation and out-of-memory errors caught, rather than reaching the +/// device's uncaptured-error handler (which takes the GPU canvas down for the session): what `f` +/// returned, and the first error. Native wgpu reports these synchronously; where they arrive +/// later (WebGPU) none is seen. +pub(super) fn catch_gpu_errors(device: &wgpu::Device, f: impl FnOnce() -> T) -> (T, Option) { + let validation = device.push_error_scope(wgpu::ErrorFilter::Validation); + let memory = device.push_error_scope(wgpu::ErrorFilter::OutOfMemory); + let out = f(); + let mut cx = std::task::Context::from_waker(std::task::Waker::noop()); + let mut error = None; + for pop in [memory.pop(), validation.pop()] { + if let std::task::Poll::Ready(Some(e)) = std::pin::pin!(pop).poll(&mut cx) { + error.get_or_insert(e); + } + } + (out, error) +} + impl DocTextures { /// The texture set for a `size` canvas, or `None` (nothing created) when it doesn't fit /// the canvas budget or the GPU's memory. A set the GPU can't hold (a 3 GP canvas is 16 GB @@ -54,17 +72,7 @@ impl DocTextures { if bytes > res.canvas_budget.load(Ordering::Relaxed) { return None; } - let validation = device.push_error_scope(wgpu::ErrorFilter::Validation); - let memory = device.push_error_scope(wgpu::ErrorFilter::OutOfMemory); - let set = Self::create(device, res, size, tile, format); - // Native wgpu reports these synchronously; where they arrive later (WebGPU) none is seen. - let mut cx = std::task::Context::from_waker(std::task::Waker::noop()); - let mut error = None; - for pop in [memory.pop(), validation.pop()] { - if let std::task::Poll::Ready(Some(e)) = std::pin::pin!(pop).poll(&mut cx) { - error.get_or_insert(e); - } - } + let (set, error) = catch_gpu_errors(device, || Self::create(device, res, size, tile, format)); if let Some(e) = error { log::warn!("{} MB of canvas textures failed ({e}); larger canvases are drawn on the CPU", bytes >> 20); res.canvas_budget.fetch_min(bytes.saturating_sub(1), Ordering::Relaxed); diff --git a/crates/ui-egui/src/canvas/gpu_canvas/mip_tests.rs b/crates/ui-egui/src/canvas/gpu_canvas/mip_tests.rs index feb0df375e..8de45748b6 100644 --- a/crates/ui-egui/src/canvas/gpu_canvas/mip_tests.rs +++ b/crates/ui-egui/src/canvas/gpu_canvas/mip_tests.rs @@ -66,6 +66,37 @@ fn canvas_mips_preserve_opaque_pixels_and_partial_updates() { } // On the same device (see the top of the file). canvas_textures_the_gpu_refuses_leave_it_working(&gpu); + refused_viewport_page_work_is_caught(&gpu); +} + +/// #5332 review: viewport page uploads run their texture, buffer and bind group creation and +/// their write inside the same error scopes as a texture set, so an error the GPU raises there +/// (out of memory, in use) is returned instead of disabling the GPU for the session. A texture +/// wider than the device allows is refused the same way, synchronously, on any adapter. +fn refused_viewport_page_work_is_caught(gpu: &GpuCanvas) { + let device = &gpu.rs.device; + let max = device.limits().max_texture_dimension_2d; + let (texture, error) = super::doc_textures::catch_gpu_errors(device, || { + device.create_texture(&wgpu::TextureDescriptor { + label: Some("too_wide_page"), + size: wgpu::Extent3d { width: max + 1, height: 1, depth_or_array_layers: 1 }, + mip_level_count: 1, + sample_count: 1, + dimension: wgpu::TextureDimension::D2, + format: FORMAT, + usage: wgpu::TextureUsages::TEXTURE_BINDING | wgpu::TextureUsages::COPY_DST, + view_formats: &[], + }) + }); + drop(texture); + assert!(error.is_some(), "the refusal is caught"); + assert!(gpu.health.is_ok(), "{:?}", gpu.fault()); + // A page the GPU accepts is stored as before. + let buf = Buffer::filled(Rect::from_xywh(0, 0, 64, 64), [0.5, 0.5, 0.5, 1.0]); + let core = Rect::new(1, 1, 63, 63); + assert!(gpu.upload_page(PageUpload { key: 9, page: 1, size: [64, 64], depth: SampleType::U8, factor: 1, core, buf: &buf })); + assert_eq!(gpu.pages(9).0, vec![1]); + assert!(gpu.health.is_ok(), "{:?}", gpu.fault()); } /// #4017: a 3 GP canvas's 16 GB of textures failed with Out of Memory, and the uncaptured error diff --git a/crates/ui-egui/src/canvas/gpu_canvas/pages.rs b/crates/ui-egui/src/canvas/gpu_canvas/pages.rs index b5f9fe7e7d..05ff59ee40 100644 --- a/crates/ui-egui/src/canvas/gpu_canvas/pages.rs +++ b/crates/ui-egui/src/canvas/gpu_canvas/pages.rs @@ -2,7 +2,7 @@ //! `canvas::viewport_pages`): independent single-level textures at their own reduction, drawn //! by the same tile pipeline. They live under their own key, never next to a texture set. -use super::doc_textures::{init_buffer, tile_uniform}; +use super::doc_textures::{catch_gpu_errors, init_buffer, tile_uniform}; use super::resources::{DocTextures, Resources, Tile, f32_bytes}; use super::texels::{texel_bytes, texels}; use super::{GpuCanvas, Ordering, wgpu}; @@ -108,34 +108,47 @@ impl GpuCanvas { if held.saturating_add(bytes) > budget { return false; } - let texture = device.create_texture(&wgpu::TextureDescriptor { - label: Some("pc_viewport_page"), - size: wgpu::Extent3d { width: w, height: h, depth_or_array_layers: 1 }, - mip_level_count: 1, - sample_count: 1, - dimension: wgpu::TextureDimension::D2, - format, - usage: wgpu::TextureUsages::TEXTURE_BINDING | wgpu::TextureUsages::COPY_DST, - view_formats: &[], - }); - let view = texture.create_view(&Default::default()); let rect = [x, y, dw, dh]; - let uniform = - init_buffer(device, "pc_viewport_page", &f32_bytes(&tile_uniform(rect, Some([core.x0, core.y0, core.x1, core.y1]))), wgpu::BufferUsages::UNIFORM); - let bind_group = device.create_bind_group(&wgpu::BindGroupDescriptor { - label: Some("pc_viewport_page"), - layout: &res.tile_bgl, - entries: &[ - wgpu::BindGroupEntry { binding: 0, resource: wgpu::BindingResource::TextureView(&view) }, - wgpu::BindGroupEntry { binding: 1, resource: uniform.as_entire_binding() }, - ], + // A page the GPU refuses (out of memory) fails alone: it falls back to the CPU canvas + // instead of the uncaptured error disabling the GPU canvas for the session. + let ((texture, view, uniform, bind_group), error) = catch_gpu_errors(device, || { + let texture = device.create_texture(&wgpu::TextureDescriptor { + label: Some("pc_viewport_page"), + size: wgpu::Extent3d { width: w, height: h, depth_or_array_layers: 1 }, + mip_level_count: 1, + sample_count: 1, + dimension: wgpu::TextureDimension::D2, + format, + usage: wgpu::TextureUsages::TEXTURE_BINDING | wgpu::TextureUsages::COPY_DST, + view_formats: &[], + }); + let view = texture.create_view(&Default::default()); + let uniform = init_buffer( + device, + "pc_viewport_page", + &f32_bytes(&tile_uniform(rect, Some([core.x0, core.y0, core.x1, core.y1]))), + wgpu::BufferUsages::UNIFORM, + ); + let bind_group = device.create_bind_group(&wgpu::BindGroupDescriptor { + label: Some("pc_viewport_page"), + layout: &res.tile_bgl, + entries: &[ + wgpu::BindGroupEntry { binding: 0, resource: wgpu::BindingResource::TextureView(&view) }, + wgpu::BindGroupEntry { binding: 1, resource: uniform.as_entire_binding() }, + ], + }); + queue.write_texture( + texture.as_image_copy(), + &texels(format, &buf.px), + wgpu::TexelCopyBufferLayout { offset: 0, bytes_per_row: Some(w * texel_bytes(format) as u32), rows_per_image: Some(h) }, + wgpu::Extent3d { width: w, height: h, depth_or_array_layers: 1 }, + ); + (texture, view, uniform, bind_group) }); - queue.write_texture( - texture.as_image_copy(), - &texels(format, &buf.px), - wgpu::TexelCopyBufferLayout { offset: 0, bytes_per_row: Some(w * texel_bytes(format) as u32), rows_per_image: Some(h) }, - wgpu::Extent3d { width: w, height: h, depth_or_array_layers: 1 }, - ); + if let Some(e) = error { + log::warn!("a {w}×{h} viewport page couldn't be stored on the GPU: {e}"); + return false; + } let Some(d) = res.docs.get_mut(&key) else { return false }; if let Some(i) = replaced { d.tiles.remove(i); From 64c638568490df7b3d1e2189a38ef007f9f204e1 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 14:08:50 +0100 Subject: [PATCH 15/20] Viewport pages: keep the pages another window on the document shows Two windows can show one document (Window > Arrange > New Window for Document) and share its stream, but each frame treated only its own wanted pages as live: it cancelled the other window's jobs, dropped its finished results, evicted its pages first and pruned them as finer than its own view, so the windows undid each other's work every frame. Record each window's wanted pages by viewport id. Jobs and results for any window's pages are kept, pruning skips them, and eviction takes pages no window shows before another window's. A window not seen for VIEW_TTL updates (closed) releases them. With one window nothing changes. Addresses CodeRabbit review r4453118205 on #5332. --- crates/ui-egui/src/canvas/viewport_pages.rs | 88 ++++++++++++++++++++- 1 file changed, 84 insertions(+), 4 deletions(-) diff --git a/crates/ui-egui/src/canvas/viewport_pages.rs b/crates/ui-egui/src/canvas/viewport_pages.rs index 410a7d4dfc..e05455d51b 100644 --- a/crates/ui-egui/src/canvas/viewport_pages.rs +++ b/crates/ui-egui/src/canvas/viewport_pages.rs @@ -209,6 +209,10 @@ pub(crate) struct Stream { /// The page ids last given to the GPU canvas, to tell it only of changes. synced: Vec, health: (u64, u64), + /// Each window showing the document (New Window for Document): the clock of its last frame + /// and the pages it wanted, so one window's frame doesn't cancel, drop or evict the pages + /// another window still shows. + views: HashMap)>, pub stats: Stats, } @@ -227,6 +231,7 @@ impl Default for Stream { clock: 0, synced: Vec::new(), health: (0, 0), + views: HashMap::new(), stats: Stats::default(), } } @@ -317,13 +322,28 @@ impl Stream { } } + /// Window `view` is about to show `wanted` (call before [`Self::update`] in its frame). + pub fn view(&mut self, view: u64, wanted: &[PageId]) { + self.views.insert(view, (self.clock + 1, wanted.to_vec())); + } + + /// Whether a window other than the one updating shows `page`: one whose frame was at most + /// [`VIEW_TTL`] updates ago (a closed window's pages are released after that). + fn shown_elsewhere(&self, page: &PageId) -> bool { + self.views.values().any(|(at, pages)| *at != self.clock && *at + VIEW_TTL >= self.clock && pages.contains(page)) + } + /// Take finished work, cancel work for pages not `wanted`, and start work for wanted pages /// that are missing, dirty or still a quick preview (`faithful`: the document has a quick /// preview at reductions), as `spawn(page, epoch, exact, cancel, region)` allows: `region`, the /// part of a held page to render again ([`Page::partial`]), or the whole page. pub fn update(&mut self, wanted: &[PageId], faithful: bool, mut spawn: impl FnMut(PageId, u64, bool, Arc, Option) -> bool) { self.clock += 1; - let want: HashSet = wanted.iter().copied().collect(); + let clock = self.clock; + self.views.retain(|_, (at, _)| *at + VIEW_TTL >= clock); + // What any window shows: work and results for another window's pages are kept. + let want: HashSet = + wanted.iter().copied().chain(self.views.values().filter(|(at, _)| *at != clock).flat_map(|(_, pages)| pages.iter().copied())).collect(); while let Ok(d) = self.rx.try_recv() { let Some(i) = self.jobs.iter().position(|j| j.page == d.page && j.epoch == d.epoch) else { continue }; // Cancelled work ends in an interruption, not a failure: the page is asked for again. @@ -427,12 +447,18 @@ impl Stream { } /// Make room for `bytes` more under `limit` by evicting the least recently wanted pages not - /// in `wanted`: whether any was evicted, or `None` when the wanted pages leave no room. + /// in `wanted` (another window's last: they are only shown there): whether any was evicted, + /// or `None` when the wanted pages leave no room. pub fn make_room(&mut self, page: PageId, bytes: u64, limit: u64, wanted: &[PageId]) -> Option { let held = |s: &Self| s.pages.iter().filter(|(p, _)| **p != page).map(|(_, q)| q.bytes).sum::(); let mut evicted = false; while held(self) + bytes > limit { - let cold = self.pages.iter().filter(|(p, _)| **p != page && !wanted.contains(p)).min_by_key(|(_, q)| q.touched).map(|(p, _)| *p)?; + let cold = self + .pages + .iter() + .filter(|(p, _)| **p != page && !wanted.contains(p)) + .min_by_key(|(p, q)| (self.shown_elsewhere(p), q.touched)) + .map(|(p, _)| *p)?; self.drop_page(cold); self.stats.evicted += 1; evicted = true; @@ -483,7 +509,8 @@ impl Stream { pub fn prune(&mut self, wanted: &[PageId]) -> Option> { let all_current = wanted.iter().all(|p| self.pages.get(p).is_some_and(|q| !q.dirty)); if all_current && let Some(f) = wanted.first().map(|p| p.factor) { - let drop: Vec = self.pages.iter().filter(|(p, q)| !wanted.contains(p) && (q.dirty || p.factor < f)).map(|(p, _)| *p).collect(); + let drop: Vec = + self.pages.iter().filter(|(p, q)| !wanted.contains(p) && !self.shown_elsewhere(p) && (q.dirty || p.factor < f)).map(|(p, _)| *p).collect(); for p in drop { self.drop_page(p); } @@ -507,6 +534,9 @@ impl Stream { } } +/// How many stream updates a window's pages stay protected after its last frame. +const VIEW_TTL: u64 = 64; + /// Render `page` of `doc` on a worker, if one of the [`MAX_JOBS`] is free. fn spawn( ctx: &egui::Context, @@ -646,6 +676,7 @@ pub(super) fn show(app: &mut PhotocraftApp, ctx: &egui::Context, idx: usize, xf: app.navigator_gpu.remove(&id); crate::render_path::note(app, id.0, crate::render_path::Path::Gpu { full: true }); let stream = app.streams.entry(id).or_default(); + stream.view(ctx.viewport_id().0.value(), &wanted); let tx = stream.tx.clone(); stream.update(&wanted, faithful, |page, epoch, exact, cancel, region| { spawn(ctx, tx.clone(), doc.clone(), display.clone(), (page, epoch, exact, region), cancel) @@ -951,6 +982,55 @@ mod tests { assert!(!s.stuck(&view)); } + /// #5332 review: two windows showing one document (New Window for Document) share its + /// stream. One window's frame doesn't cancel the other's work, drop its results, evict its + /// pages or prune them as finer than its own view; a window gone for a while releases them. + #[test] + fn windows_on_one_document_keep_each_others_pages() { + let (a, b) = ([page(1, 0, 0)], [page(4, 0, 0)]); + let mut s = holding(&[]); + let (mut log_a, mut log_b) = (Vec::new(), Vec::new()); + s.view(1, &a); + s.update(&a, true, recorder(&mut log_a)); + s.view(2, &b); + s.update(&b, false, recorder(&mut log_b)); + assert!(!log_a[0].3.load(Ordering::Relaxed), "window 2's frame keeps window 1's work"); + // Window 1's page finishes during window 2's frame: it is kept for upload. + s.tx.send(done(a[0], log_a[0].1, true, 4)).unwrap(); + s.view(2, &b); + s.update(&b, false, |_, _, _, _, _| true); + assert!(s.ready.iter().any(|d| d.page == a[0]), "window 1's result is kept"); + for d in s.take_uploads(&b, 16) { + s.uploaded(&d, 100, true); + } + s.tx.send(done(b[0], log_b[0].1, true, 4)).unwrap(); + s.view(2, &b); + s.update(&b, false, |_, _, _, _, _| true); + for d in s.take_uploads(&b, 16) { + s.uploaded(&d, 100, true); + } + assert_eq!(s.resident(), 2); + // Window 2's view is current: window 1's finer page isn't pruned, nor evicted first. + s.prune(&b); + assert!(s.pages.contains_key(&a[0]), "not pruned while window 1 shows it"); + s.pages.insert(page(4, 1, 0), Page { dirty: false, exact: true, partial: None, epoch: 0, touched: s.clock, bytes: 100 }); + assert_eq!(s.make_room(page(4, 2, 0), 100, 300, &b), Some(true)); + assert!(s.pages.contains_key(&a[0]) && !s.pages.contains_key(&page(4, 1, 0)), "a page no window shows goes first"); + // Window 1 closed: once its pages are past the grace period, window 2 prunes them. + for _ in 0..=VIEW_TTL { + s.view(2, &b); + s.update(&b, false, |_, _, _, _, _| true); + } + s.prune(&b); + assert!(!s.pages.contains_key(&a[0]), "released after the closed window's grace period"); + // Without a recorded window (one view), nothing changes: other pages are cancelled. + let mut s = holding(&[]); + let mut log = Vec::new(); + s.update(&a, true, recorder(&mut log)); + s.update(&b, true, |_, _, _, _, _| true); + assert!(log[0].3.load(Ordering::Relaxed)); + } + #[test] fn prune_drops_finer_and_dirty_pages_once_the_view_is_current() { let (fine, coarse, view) = (page(1, 0, 0), page(4, 0, 0), page(2, 0, 0)); From 1b7b56d9f5f6a0893cf20306a6ebb2bd8cf061b5 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 14:51:01 +0100 Subject: [PATCH 16/20] GPU canvas: the open documents share one texture budget The canvas budget was each document's: N open documents could hold N times it (#5332 review). It is now all of them together. The app publishes the documents on screen (the arranged tiles or the main canvas's, plus open document windows) with each one's texture set size; those split the budget max-min fairly, and fits_canvas, canvas_fit and the viewport page limit use a document's share, so a document over it is drawn from pages. A document alone on screen still has the whole budget. Documents no window shows keep their textures while there is room, so going back to a tab is free, and are freed first, least recently shown first, when a shown document's texture set or pages need the room. Previews and adjustment proxies are never freed for another document. The split changes only when the shown set or a size changes, not with focus, so switching between shown documents moves no texture. --- .../src/canvas/background_redraw/tests.rs | 57 +++++ crates/ui-egui/src/canvas/gpu_canvas.rs | 43 +++- .../src/canvas/gpu_canvas/doc_textures.rs | 10 + crates/ui-egui/src/canvas/gpu_canvas/pages.rs | 5 +- .../src/canvas/gpu_canvas/residency.rs | 209 ++++++++++++++++++ .../src/canvas/gpu_canvas/resources.rs | 5 +- crates/ui-egui/src/canvas/tabs.rs | 21 ++ crates/ui-egui/src/canvas/viewport_pages.rs | 5 +- 8 files changed, 340 insertions(+), 15 deletions(-) create mode 100644 crates/ui-egui/src/canvas/gpu_canvas/residency.rs diff --git a/crates/ui-egui/src/canvas/background_redraw/tests.rs b/crates/ui-egui/src/canvas/background_redraw/tests.rs index a669b18a38..ce47c0c003 100644 --- a/crates/ui-egui/src/canvas/background_redraw/tests.rs +++ b/crates/ui-egui/src/canvas/background_redraw/tests.rs @@ -699,6 +699,63 @@ fn a_canvas_over_the_texture_budget_draws_its_viewport_pages_on_the_gpu() { assert!(gpu.pages(key).0.is_empty()); } +/// #5332 review: the open documents share one texture budget rather than each having all of +/// it. A document alone on screen keeps the whole budget; one in a background tab gives its +/// textures up when the shown one needs the room; and documents shown side by side split it, +/// a document over its share drawing from viewport pages. +#[test] +fn open_documents_share_one_texture_budget_with_the_shown_ones_first() { + let _gpu = photocraft_testkit::gpu_lock(); + let Some(mut g) = gpu_app() else { return }; + let gpu = g.harness.state().gpu.clone().expect("gpu canvas"); + let new = |g: &mut Gpu| { + g.harness.state_mut().run("file.new", json!({"width": 3000, "height": 2000, "background": "#336699"})).expect("new"); + g.harness.run_steps(3); + assert!(on_gpu_canvas(g), "{:?}", render_path_of(g)); + g.harness.state().session.active().expect("document").doc.clone() + }; + let a = new(&mut g); + let (set, _) = gpu.canvas_fit(&a).expect("fit"); + // Room for one texture set and a half: a document alone on screen has all of it. + let budget = set + set / 2; + gpu.set_canvas_budget(budget); + assert_eq!(gpu.canvas_fit(&a), Some((set, budget)), "alone, the whole budget"); + assert!(gpu.texture_info(a.id.0).is_some()); + // A second tab: the shown document's texture set takes the room of the background one's. + let b = new(&mut g); + assert_eq!(gpu.canvas_fit(&b), Some((set, budget))); + assert!(gpu.texture_info(b.id.0).is_some()); + assert!(gpu.texture_info(a.id.0).is_none(), "the background document's textures are freed"); + // Back to the first tab: it gets its texture set back and the other gives its up. + g.harness.state_mut().session.set_active(0); + g.harness.run_steps(3); + assert!(on_gpu_canvas(&g), "{:?}", render_path_of(&g)); + assert!(gpu.texture_info(a.id.0).is_some()); + assert!(gpu.texture_info(b.id.0).is_none()); + // Side by side, they split the budget: neither set fits its half, so both draw from pages. + g.harness.state_mut().ui.view.arrange = "twoUpVertical".into(); + g.harness.run_steps(3); + assert_eq!(gpu.canvas_fit(&a), Some((set, budget / 2))); + assert_eq!(gpu.canvas_fit(&b), Some((set, budget - budget / 2))); + settle_pages(&mut g); + assert!(on_gpu_canvas(&g), "{:?}", render_path_of(&g)); + let streams = &g.harness.state().streams; + assert!(streams.contains_key(&a.id) && streams.contains_key(&b.id), "both on pages"); + assert!(gpu.texture_info(a.id.0).is_none() && gpu.texture_info(b.id.0).is_none()); + // Switching between them re-splits nothing: no page set is dropped. + let held = gpu.pages(crate::canvas::viewport_pages::stream_key(a.id.0)).0; + g.harness.state_mut().session.set_active(1); + g.harness.run_steps(3); + assert_eq!(gpu.canvas_fit(&a), Some((set, budget / 2))); + assert_eq!(gpu.pages(crate::canvas::viewport_pages::stream_key(a.id.0)).0, held, "no churn on a switch"); + // Back to tabs: the shown document has the whole budget, and its texture set, again. + g.harness.state_mut().ui.view.arrange = "tabs".into(); + g.harness.run_steps(3); + assert_eq!(gpu.canvas_fit(&b), Some((set, budget))); + assert!(on_gpu_canvas(&g), "{:?}", render_path_of(&g)); + assert!(gpu.texture_info(b.id.0).is_some()); +} + #[test] fn a_flipped_view_records_why_it_draws_on_the_cpu() { let _gpu = photocraft_testkit::gpu_lock(); diff --git a/crates/ui-egui/src/canvas/gpu_canvas.rs b/crates/ui-egui/src/canvas/gpu_canvas.rs index 19a3b55846..ef9435f346 100644 --- a/crates/ui-egui/src/canvas/gpu_canvas.rs +++ b/crates/ui-egui/src/canvas/gpu_canvas.rs @@ -42,6 +42,7 @@ mod callback; mod doc_textures; mod pages; mod perf; +mod residency; mod resources; mod setup; mod shaders; @@ -349,24 +350,43 @@ impl GpuCanvas { r.callback_resources.get::()?.compositor_budget } - /// Set the GPU bytes one document's display texture set may take (see [`canvas_budget`]). + /// Set the GPU bytes the open documents' display textures may take together (see + /// [`canvas_budget`] and [`GpuCanvas::show_documents`]). pub fn set_canvas_budget(&self, bytes: u64) { self.canvas_budget.store(bytes, Ordering::Relaxed); } - /// Whether `doc`'s display textures fit [`canvas_budget`]; when they don't, nothing is - /// created for it and it is drawn on the CPU canvas. + /// Whether `doc`'s display textures fit its share of [`canvas_budget`]; when they don't, + /// nothing is created for it and it is drawn from viewport pages or on the CPU canvas. pub fn fits_canvas(&self, doc: &photocraft_doc::Document) -> bool { self.canvas_fit(doc).is_some_and(|(bytes, budget)| bytes <= budget) } - /// The bytes `doc`'s display textures would take and [`canvas_budget`] (`None` without a GPU - /// canvas): the numbers of a [`CpuReason::CanvasTooLarge`]. - pub fn canvas_fit(&self, doc: &photocraft_doc::Document) -> Option<(u64, u64)> { + /// The bytes `doc`'s whole-document texture set would take. + pub fn canvas_bytes(&self, doc: &photocraft_doc::Document) -> u64 { let size = [doc.size.width, doc.size.height]; - let bytes = canvas_texture_bytes(size, self.tile, texel_bytes(self.format_for(doc.depth, size))); + canvas_texture_bytes(size, self.tile, texel_bytes(self.format_for(doc.depth, size))) + } + + /// The bytes `doc`'s display textures would take and its share of [`canvas_budget`] (`None` + /// without a GPU canvas): the numbers of a [`CpuReason::CanvasTooLarge`]. Alone on screen a + /// document has the whole budget; see [`GpuCanvas::show_documents`]. + pub fn canvas_fit(&self, doc: &photocraft_doc::Document) -> Option<(u64, u64)> { + let bytes = self.canvas_bytes(doc); let r = self.rs.renderer.read(); - r.callback_resources.get::().map(|res| (bytes, res.canvas_budget.load(Ordering::Relaxed))) + r.callback_resources.get::().map(|res| (bytes, res.residency.share(res.canvas_budget.load(Ordering::Relaxed), doc.id.0, bytes))) + } + + /// The documents the app's windows show now, with [`GpuCanvas::canvas_bytes`] of each: they + /// split [`canvas_budget`] (see [`residency`]), and the textures of documents not among them + /// are freed first when a shown one needs the room. Cheap when nothing changed. + pub fn show_documents(&self, shown: &[(u64, u64)]) { + if self.rs.renderer.read().callback_resources.get::().is_none_or(|res| res.residency.shows(shown)) { + return; + } + if let Some(res) = self.rs.renderer.write().callback_resources.get_mut::() { + res.residency.publish(shown); + } } /// Act as if the driver couldn't build the compositor's pipelines (the DX12 FXC X3507 case, @@ -513,7 +533,7 @@ impl GpuCanvas { let mut renderer = self.rs.renderer.write(); let Some(res) = renderer.callback_resources.get_mut::() else { return }; if res.docs.get(&doc).is_none_or(|d| d.size != size || d.format != format) { - let Some(tex) = DocTextures::new(device, res, size, self.tile, format) else { + let Some(tex) = DocTextures::new_for(device, res, doc, size, self.tile, format) else { res.docs.remove(&doc); return; }; @@ -646,7 +666,7 @@ impl GpuCanvas { } } if fresh { - let Some(tex) = DocTextures::new(device, res, size, self.tile, format) else { + let Some(tex) = DocTextures::new_for(device, res, key, size, self.tile, format) else { res.docs.remove(&key); res.compositor = Some(comp); return Err(photocraft_gpu::Unsupported(OVER_CANVAS_BUDGET.into())); @@ -861,7 +881,7 @@ impl GpuCanvas { let mut renderer = self.rs.renderer.write(); let res = renderer.callback_resources.get_mut::()?; if res.docs.get(&key).is_none_or(|d| d.size != size || d.format != format) { - let Some(tex) = DocTextures::new(&self.rs.device, res, size, self.tile, format) else { + let Some(tex) = DocTextures::new_for(&self.rs.device, res, key, size, self.tile, format) else { res.docs.remove(&key); return None; }; @@ -886,6 +906,7 @@ impl GpuCanvas { // LUTs and identity-transform signatures can outlive document textures. Prune both, // or a preserved-ID reopen can skip rebuilding a missing LUT. res.luts.retain(|k, _| live.contains(&k.0)); + res.residency.retain(|d| live.contains(&d)); res.display_lut_signatures.retain(|k, _| live.contains(&k.0)); } diff --git a/crates/ui-egui/src/canvas/gpu_canvas/doc_textures.rs b/crates/ui-egui/src/canvas/gpu_canvas/doc_textures.rs index e21efd41ca..e1c2389bfb 100644 --- a/crates/ui-egui/src/canvas/gpu_canvas/doc_textures.rs +++ b/crates/ui-egui/src/canvas/gpu_canvas/doc_textures.rs @@ -81,6 +81,16 @@ impl DocTextures { Some(set) } + /// [`DocTextures::new`] for key `key`, first freeing the textures of documents no window + /// shows when the open documents' together wouldn't fit the canvas budget. + pub(super) fn new_for(device: &wgpu::Device, res: &mut Resources, key: u64, size: [u32; 2], tile: u32, format: wgpu::TextureFormat) -> Option { + let bytes = canvas_texture_bytes(size, tile, texel_bytes(format)); + if bytes <= res.canvas_budget.load(Ordering::Relaxed) { + res.make_room(key, bytes); + } + Self::new(device, res, size, tile, format) + } + fn create(device: &wgpu::Device, res: &Resources, size: [u32; 2], tile: u32, format: wgpu::TextureFormat) -> Self { let mut tiles = Vec::new(); for ty in (0..size[1]).step_by(tile as usize) { diff --git a/crates/ui-egui/src/canvas/gpu_canvas/pages.rs b/crates/ui-egui/src/canvas/gpu_canvas/pages.rs index 05ff59ee40..bc62505d7e 100644 --- a/crates/ui-egui/src/canvas/gpu_canvas/pages.rs +++ b/crates/ui-egui/src/canvas/gpu_canvas/pages.rs @@ -108,6 +108,8 @@ impl GpuCanvas { if held.saturating_add(bytes) > budget { return false; } + // Room among the other documents: those no window shows give theirs up first. + res.make_room(key, held + bytes); let rect = [x, y, dw, dh]; // A page the GPU refuses (out of memory) fails alone: it falls back to the CPU canvas // instead of the uncaptured error disabling the GPU canvas for the session. @@ -189,7 +191,8 @@ impl GpuCanvas { (d.tiles.iter().filter_map(|t| t.page.map(|(id, _)| id)).collect(), d.page_bytes()) } - /// The per-document display texture budget ([`super::canvas_budget`]) pages are held to. + /// The display texture budget all documents share ([`super::canvas_budget`]); a document's + /// pages are held to its share ([`GpuCanvas::canvas_fit`]). pub fn canvas_budget(&self) -> u64 { self.canvas_budget.load(Ordering::Relaxed) } diff --git a/crates/ui-egui/src/canvas/gpu_canvas/residency.rs b/crates/ui-egui/src/canvas/gpu_canvas/residency.rs new file mode 100644 index 0000000000..df681a000b --- /dev/null +++ b/crates/ui-egui/src/canvas/gpu_canvas/residency.rs @@ -0,0 +1,209 @@ +//! One display-texture budget ([`super::canvas_budget`]) for every open document, rather than +//! the whole budget for each (N documents could hold N times it, #5332 review). +//! +//! The app publishes the documents its windows show and what each one's whole-document texture +//! set needs ([`super::GpuCanvas::show_documents`]). Those split the budget max-min fairly: a +//! document that fits its share keeps its texture set, a larger one is drawn from viewport pages +//! within it, and a document alone on screen still has the whole budget. The split changes only +//! when the shown documents (or their sizes) do, never with focus, so switching between windows +//! moves nothing. Documents no window shows have no share: they keep their textures while there +//! is room (switching back to a tab is free) and are the first freed, least recently shown +//! first, when a shown document needs the room. + +use std::collections::HashMap; +use std::sync::atomic::Ordering; + +use super::resources::{DocTextures, Resources}; +use super::texels::texel_bytes; + +/// Keys tagged above this bit are previews (filter preview, adjustment proxies, gradients, +/// thumbnails): never evicted for another document. +const TAG_BITS: u64 = !((1u64 << 58) - 1); +/// The tag of a document's viewport pages (`viewport_pages::stream_key`). +const PAGES_BIT: u64 = 1u64 << 57; + +impl DocTextures { + /// GPU bytes of every texture (all mip levels) in this set or page set. + pub(super) fn bytes(&self) -> u64 { + let bpp = texel_bytes(self.format); + let level = |w: u32, h: u32, l: u32| u64::from((w >> l).max(1)) * u64::from((h >> l).max(1)) * bpp; + self.tiles.iter().map(|t| (0..t.texture.mip_level_count()).map(|l| level(t.texture.width(), t.texture.height(), l)).sum::()).sum() + } +} + +impl Resources { + /// Free the textures of documents no window shows ([`Residency::victims`]) so that `key` + /// can hold `bytes` within the canvas budget. + pub(super) fn make_room(&mut self, key: u64, bytes: u64) { + let budget = self.canvas_budget.load(Ordering::Relaxed); + let held: Vec<(u64, u64)> = self.docs.iter().map(|(k, d)| (*k, d.bytes())).collect(); + for k in self.residency.victims(&held, key, bytes, budget) { + log::debug!("freeing the display textures of background key {k:#x} for {key:#x}"); + self.docs.remove(&k); + } + } +} + +/// Document `doc`'s allocation when documents needing `needs` bytes split `budget` max-min +/// fairly (`doc` indexes `needs`): those needing less than an equal share take what they need, +/// the rest split what is left; the leftover, if any, is `doc`'s too. +pub(super) fn fair_share(budget: u64, needs: &[u64], doc: usize) -> u64 { + let mut order: Vec = (0..needs.len()).collect(); + order.sort_by_key(|&i| needs[i]); + let mut left = budget; + let mut others = 0u64; + for (n, &i) in order.iter().enumerate() { + let take = needs[i].min(left / (order.len() - n) as u64); + left -= take; + if i != doc { + others += take; + } + } + budget - others +} + +/// The documents on screen and when each was last shown. +#[derive(Default)] +pub(super) struct Residency { + /// Shown documents: (document id, bytes of its whole texture set). + shown: Vec<(u64, u64)>, + /// When each document was last among the shown (a counter of changes). + seen: HashMap, + tick: u64, +} + +impl Residency { + /// Record the shown documents. False when nothing changed. + pub(super) fn publish(&mut self, shown: &[(u64, u64)]) -> bool { + if self.shown == shown { + return false; + } + self.tick += 1; + for (id, _) in shown { + self.seen.insert(*id, self.tick); + } + self.shown = shown.to_vec(); + true + } + + /// The share of `budget` document `id` (its texture set `need` bytes) may hold: the whole + /// budget when nothing else is shown. + pub(super) fn share(&self, budget: u64, id: u64, need: u64) -> u64 { + let mut needs: Vec = self.shown.iter().filter(|(d, _)| *d != id).map(|(_, n)| *n).collect(); + needs.push(need); + fair_share(budget, &needs, needs.len() - 1) + } + + /// Which held keys (`held`: key and bytes) to free, in order, so that `key` can hold + /// `bytes` within `budget`: the texture sets and pages of documents no window shows, least + /// recently shown first. Never a shown document's, nor `key`'s own document's, nor a preview + /// or a key of a document never shown (an adjustment proxy). It may not free enough: what + /// a shown document takes is bounded by its share instead. + pub(super) fn victims(&self, held: &[(u64, u64)], key: u64, bytes: u64, budget: u64) -> Vec { + let owner = |k: u64| (k & TAG_BITS == 0).then_some(k & !PAGES_BIT); + let mut total = held.iter().filter(|(k, _)| *k != key).map(|(_, b)| *b).fold(bytes, u64::saturating_add); + if total <= budget { + return Vec::new(); + } + let mine = owner(key); + let mut cold: Vec<(u64, u64, u64)> = held + .iter() + .filter_map(|&(k, b)| { + let d = owner(k)?; + let at = *self.seen.get(&d)?; + (Some(d) != mine && !self.shown.iter().any(|(s, _)| *s == d)).then_some((at, k, b)) + }) + .collect(); + cold.sort_unstable(); + let mut out = Vec::new(); + for (_, k, b) in cold { + if total <= budget { + break; + } + total -= b; + out.push(k); + } + out + } + + /// Whether `shown` is what was published last. + pub(super) fn shows(&self, shown: &[(u64, u64)]) -> bool { + self.shown == shown + } + + /// Forget closed documents. + pub(super) fn retain(&mut self, live: impl Fn(u64) -> bool) { + self.seen.retain(|d, _| live(*d)); + self.shown.retain(|(d, _)| live(*d)); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + const GB: u64 = 1 << 30; + + #[test] + fn one_document_has_the_whole_budget() { + let r = Residency::default(); + assert_eq!(r.share(4 * GB, 1, 9 * GB), 4 * GB); + let mut r = Residency::default(); + r.publish(&[(1, 9 * GB)]); + assert_eq!(r.share(4 * GB, 1, 9 * GB), 4 * GB, "shown alone"); + assert_eq!(r.share(4 * GB, 1, GB), 4 * GB); + } + + #[test] + fn shown_documents_split_the_budget_max_min_fairly() { + let mut r = Residency::default(); + // A small document takes what it needs; the large one the rest. + r.publish(&[(1, GB), (2, 9 * GB)]); + assert_eq!(r.share(4 * GB, 1, GB), GB); + assert_eq!(r.share(4 * GB, 2, 9 * GB), 3 * GB); + // What the others leave is the asking document's too. + r.publish(&[(1, GB), (2, 2 * GB)]); + assert_eq!(r.share(4 * GB, 2, 2 * GB), 3 * GB, "the leftover is its own too"); + // Two large ones split it evenly, whichever has focus: no churn switching windows. + r.publish(&[(1, 3 * GB), (2, 3 * GB)]); + assert_eq!(r.share(4 * GB, 1, 3 * GB), 2 * GB); + assert_eq!(r.share(4 * GB, 2, 3 * GB), 2 * GB); + // Three, one of them small. + r.publish(&[(1, 3 * GB), (2, 3 * GB), (3, GB / 2)]); + assert_eq!(r.share(4 * GB, 1, 3 * GB), 7 * GB / 4); + assert_eq!(r.share(4 * GB, 3, GB / 2), GB / 2, "needs only half a GB"); + // Everything fits: each share covers its need, together within the budget. + r.publish(&[(1, GB), (2, GB)]); + assert!(r.share(4 * GB, 1, GB) >= GB && r.share(4 * GB, 2, GB) >= GB); + // A document no window shows (a refresh of a background tab) is priced as one more. + assert_eq!(r.share(4 * GB, 7, 4 * GB), 2 * GB); + } + + #[test] + fn republishing_the_same_documents_changes_nothing() { + let mut r = Residency::default(); + assert!(r.publish(&[(1, GB)])); + assert!(!r.publish(&[(1, GB)])); + assert!(r.publish(&[(1, 2 * GB)]), "the document was resized"); + } + + #[test] + fn background_documents_are_freed_least_recently_shown_first() { + let mut r = Residency::default(); + // Tabs 1, 2, 3 shown in turn, then 4. + for d in 1..=4 { + r.publish(&[(d, GB)]); + } + let pages = |d: u64| d ^ PAGES_BIT; + let held = [(1, GB), (pages(2), GB / 2), (2, GB / 2), (3, GB), (1 << 61 | 3, GB), (99, GB)]; + // Room enough: nothing goes. + assert!(r.victims(&held, 4, GB, 16 * GB).is_empty()); + // 4 needs 2 GB within 6 GB (5 held): tab 1 goes first, then tab 2's pages and set. + assert_eq!(r.victims(&held, 4, 2 * GB, 6 * GB), vec![1]); + assert_eq!(r.victims(&held, 4, 3 * GB, 6 * GB), vec![1, 2, pages(2)]); + // Never a shown document's, a preview's or an unknown key's, even when that isn't enough. + assert_eq!(r.victims(&held, 4, 10 * GB, 6 * GB), vec![1, 2, pages(2), 3]); + // Nor the document's own: its pages make room by evicting the other tabs. + assert_eq!(r.victims(&held, pages(3), GB, 4 * GB), vec![1, 2, pages(2)]); + } +} diff --git a/crates/ui-egui/src/canvas/gpu_canvas/resources.rs b/crates/ui-egui/src/canvas/gpu_canvas/resources.rs index ce8f8b0881..d9990f0920 100644 --- a/crates/ui-egui/src/canvas/gpu_canvas/resources.rs +++ b/crates/ui-egui/src/canvas/gpu_canvas/resources.rs @@ -36,8 +36,10 @@ pub(super) struct Resources { pub(super) layer_fit: Option, pub(super) compositor_cache_budget: Option, pub(super) compositor_focus: Option, - /// GPU bytes one document's display texture set may take (see [`canvas_budget`]). + /// GPU bytes the documents' display textures may take together (see [`canvas_budget`]). pub(super) canvas_budget: std::sync::Arc, + /// How that budget is split between the open documents (see [`super::residency`]). + pub(super) residency: super::residency::Residency, pub(super) encode_bgl: wgpu::BindGroupLayout, encode_pipeline: wgpu::RenderPipeline, pub(super) lut_bgl: wgpu::BindGroupLayout, @@ -369,6 +371,7 @@ impl Resources { compositor_cache_budget: None, compositor_focus: None, canvas_budget: std::sync::Arc::new(std::sync::atomic::AtomicU64::new(canvas_budget(physical_memory(), None))), + residency: Default::default(), encode_bgl, encode_pipeline, } diff --git a/crates/ui-egui/src/canvas/tabs.rs b/crates/ui-egui/src/canvas/tabs.rs index 2ab777b532..3753ad3d2b 100644 --- a/crates/ui-egui/src/canvas/tabs.rs +++ b/crates/ui-egui/src/canvas/tabs.rs @@ -29,10 +29,29 @@ pub(crate) fn arranged_cells(app: &PhotocraftApp, rect: Rect) -> Option = match arranged_cells(app, app.last_canvas_rect) { + Some(cells) => cells.into_iter().map(|(d, _)| d).collect(), + None => main_canvas_document(app).into_iter().collect(), + }; + shown.extend(app.ui.windows.iter().filter(|w| w.open).map(|w| w.document)); + let mut shown: Vec<(u64, u64)> = shown.into_iter().filter_map(|d| docs.get(d)).map(|st| (st.doc.id.0, gpu.canvas_bytes(&st.doc))).collect(); + shown.sort_unstable(); + shown.dedup(); + gpu.show_documents(&shown); +} + fn documents(app: &mut PhotocraftApp, ui: &mut egui::Ui) { app.drop_canvas_rect = None; app.tab_strip = None; retain_gpu_documents(app); + publish_shown(app); crate::transform_tool::track_steps(app, ui.ctx()); crate::cas_transform::tick(app, ui.ctx()); crate::symmetry_ui::track(app, ui.ctx()); @@ -66,6 +85,8 @@ fn documents(app: &mut PhotocraftApp, ui: &mut egui::Ui) { app.tab_strip = Some(tabs(app, ui)); ui.spacing_mut().item_spacing.y = spacing; drop_slot_line(app, ui); + // A tab click switches documents before any canvas is drawn this frame. + publish_shown(app); } if let Some(job) = app.jobs.focus.or_else(|| (n == 0).then(|| app.jobs.opens.last().map(|o| o.job)).flatten()) { crate::jobs_ui::open_card(app, ui, job); diff --git a/crates/ui-egui/src/canvas/viewport_pages.rs b/crates/ui-egui/src/canvas/viewport_pages.rs index e05455d51b..07933ede30 100644 --- a/crates/ui-egui/src/canvas/viewport_pages.rs +++ b/crates/ui-egui/src/canvas/viewport_pages.rs @@ -628,8 +628,9 @@ pub(super) fn show(app: &mut PhotocraftApp, ctx: &egui::Context, idx: usize, xf: return false; } let key = stream_key(id.0); - // Half the per-document canvas budget: the rest is headroom for what the compositor needs. - let limit = gpu.canvas_budget() / 2; + // Half the document's share of the canvas budget: the rest is headroom for what the + // compositor needs. + let limit = gpu.canvas_fit(&doc).map_or(0, |(_, share)| share) / 2; let page_bytes = gpu.page_texture_bytes(doc.depth, size, PAGE_SIDE + 2, PAGE_SIDE + 2).max(1); let max_pages = usize::try_from(limit / page_bytes).unwrap_or(usize::MAX); // No room for a single page: the CPU canvas draws the document, as main does for every From 43f415bcf051af07a7b9b71acd50dbaae5429aa8 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 15:07:24 +0100 Subject: [PATCH 17/20] Type tool tests: let the options bar harness settle in up to 16 passes options_font_family_preserves_peer_faces_and_batches_layer_flags failed under a loaded parallel run at its first `h.run()` after opening the family field: Harness::run exceeded max_steps (8). It would have settled after 10 steps. The test has no timeout of its own. The limit it hits is the egui_kittest `max_steps` cap of the shared `controls()` harness. The font popup fills in over several passes, and egui 0.37's `widget_style` requests one more repaint for each widget it has not seen yet, so `run` settles only after the last batch of the font list has arrived. Alone that takes six passes, but under load the list arrives in more batches and it took ten. Raise the cap from 8 to 16, well above the ten seen. It stays bounded: a UI that keeps requesting repaints still fails, because `run` errors on any pass past the cap. --- crates/ui-egui/src/tools/type_tool/canvas_tests.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/ui-egui/src/tools/type_tool/canvas_tests.rs b/crates/ui-egui/src/tools/type_tool/canvas_tests.rs index fbe0d88c7c..38baecc450 100644 --- a/crates/ui-egui/src/tools/type_tool/canvas_tests.rs +++ b/crates/ui-egui/src/tools/type_tool/canvas_tests.rs @@ -1943,9 +1943,12 @@ mod multi_layer_formatting { fn controls(app: PhotocraftApp, properties: bool) -> Harness<'static, PhotocraftApp> { // The lazily populated font popup introduces widgets in successive passes. egui 0.37 - // styles each new widget on its following pass; keep a bounded settle assertion while - // allowing those additional passes (the loaded menu requires six under parallel tests). - let mut h = Harness::builder().with_size(vec2(1100.0, 800.0)).with_max_steps(8).build_ui_state( + // styles each new widget on its following pass (`widget_style` requests a repaint for a + // widget it has not seen yet), so `run` settles only after the last batch of the font + // list has arrived. The loaded menu takes six passes alone, but under a loaded parallel + // run the list arrives in more batches and it took ten. Allow 16 passes; a UI that never + // settles still fails, because `run` errors on any pass past the cap. + let mut h = Harness::builder().with_size(vec2(1100.0, 800.0)).with_max_steps(16).build_ui_state( move |ui, app| { if ui.ctx().fonts(|f| f.families().contains(&egui::FontFamily::Name("medium".into()))) { if properties { From 26d151d061ded4150d2f7200091228a375742c42 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 16:09:07 +0100 Subject: [PATCH 18/20] Jobs: a save past its commit point can't be reported as cancelled jobs.cancel, cancel_all_jobs and closing a document (cancel_jobs_on) cancelled any running job, so they could report a save as cancelled while its worker went on to replace the file. The point of no return was tracked only by the desktop shell's own Cancel/Esc path. The commit point now lives in the engine's JobCtx: commit() and a cancel race on one compare-and-swap, so exactly one wins. - Cancelled before commit: commit() fails, the save writes nothing and the job ends Cancelled. - Committed: every cancel path leaves the job running and says so (Session::try_cancel_job -> CancelRefused::Committing; cancel_all_jobs and cancel_jobs_on return a CancelReport). The job then reports its real outcome, applied even if its document was closed meanwhile. - jobs.cancel (engine and desktop control) replies {cancelled: n} as before, plus notCancelled: [{job, reason: "committing"}] for a save left running. This is not an error: the job is fine and will report how it ended. Single-job print refusals are unchanged. - The shell's SaveCtl::commit and Cancel/Esc use the engine gate, so the status bar, control and scripting agree. Tests: cancel before commit (target untouched, Cancelled), cancel after commit (refused, Done, file written), cancel_all and document close with mixed committed/open jobs, a commit/cancel race, and desktop jobs.cancel before and during the write with real files. --- crates/engine/src/jobs.rs | 161 ++++++++++++++++---- crates/engine/src/jobs/tests.rs | 201 +++++++++++++++++++++++++ crates/ui-egui/src/control/dispatch.rs | 22 ++- crates/ui-egui/src/jobs_ui.rs | 48 +++--- crates/ui-egui/src/jobs_ui/tests.rs | 89 +++++++++++ docs/control-protocol.md | 4 +- 6 files changed, 462 insertions(+), 63 deletions(-) diff --git a/crates/engine/src/jobs.rs b/crates/engine/src/jobs.rs index ae1b880a64..fa5f98ce6e 100644 --- a/crates/engine/src/jobs.rs +++ b/crates/engine/src/jobs.rs @@ -14,12 +14,17 @@ //! On wasm `start` runs inline (same API, written once). //! - [`Session::cancel_job`] sets the flag; the algorithms check it per tile, row band or //! iteration, and the result is discarded. The document is never touched by a worker. +//! - A job with an effect outside the session (a save replacing its file) calls +//! [`JobCtx::commit`] right before it. From then on it can't be cancelled: every cancel +//! path ([`Session::try_cancel_job`], [`Session::cancel_all_jobs`], closing its document, +//! `jobs.cancel`) leaves it running and says so ([`CancelRefused::Committing`]), so nothing +//! reports a job as cancelled after its effect has happened. //! - A panic in the worker becomes an error; the document is unchanged. //! - While a job runs on a document, commands that would edit that document are disabled with a //! message naming the job (queries and document-independent commands still run). use std::any::Any; -use std::sync::atomic::{AtomicBool, AtomicU32, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU8, AtomicU32, Ordering}; use std::sync::{Arc, Mutex, PoisonError, Weak}; use photocraft_doc::{DocId, Document, LayerId}; @@ -85,12 +90,22 @@ pub struct JobCtx { #[derive(Default)] struct Shared { cancel: AtomicBool, + /// [`OPEN`], [`COMMITTED`] or [`CANCELLED`]: one compare-and-swap decides between a cancel + /// and [`JobCtx::commit`], so exactly one of them wins. + phase: AtomicU8, /// `f32` bits of the progress fraction. Non-negative floats order like their bits, so /// `fetch_max` keeps progress monotonic even with several reporting threads. progress: AtomicU32, message: Mutex, } +/// Neither cancelled nor committed: a cancel stops the job. +const OPEN: u8 = 0; +/// Past [`JobCtx::commit`]: the job runs to the end and can no longer be cancelled. +const COMMITTED: u8 = 1; +/// Cancelled before it committed. +const CANCELLED: u8 = 2; + impl std::fmt::Debug for JobCtx { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("JobCtx").field("progress", &self.fraction()).field("cancelled", &self.cancelled()).finish() @@ -121,9 +136,38 @@ impl JobCtx { self.shared.cancel.load(Ordering::Relaxed) } - /// Ask the job to stop (idempotent). + /// Ask the job to stop (idempotent). Does nothing once it has [committed](Self::commit). pub fn cancel(&self) { - self.shared.cancel.store(true, Ordering::Relaxed); + let _ = self.try_cancel(); + } + + /// Cancel unless the job has [committed](Self::commit): true if it is cancelled (now or + /// before), false if it is past its point of no return and will run to the end. + pub fn try_cancel(&self) -> bool { + match self.shared.phase.compare_exchange(OPEN, CANCELLED, Ordering::AcqRel, Ordering::Acquire) { + Ok(_) | Err(CANCELLED) => { + self.shared.cancel.store(true, Ordering::Release); + true + } + Err(_) => false, + } + } + + /// The job's point of no return, for work with an effect outside the session (a save calls + /// it right before replacing the file). `Ok`: no cancel can stop the job any more; it must + /// run to the end, and its result is applied even if its document was closed meanwhile. + /// `Err(Cancelled)`: it was cancelled first, and nothing may be done. + pub fn commit(&self) -> Result<()> { + self.check()?; + match self.shared.phase.compare_exchange(OPEN, COMMITTED, Ordering::AcqRel, Ordering::Acquire) { + Ok(_) | Err(COMMITTED) => Ok(()), + Err(_) => Err(EngineError::Cancelled), + } + } + + /// Has the job passed [`Self::commit`]? + pub fn committed(&self) -> bool { + self.shared.phase.load(Ordering::Acquire) == COMMITTED } /// `Err(Cancelled)` once cancelled, for `?` between stages. @@ -151,6 +195,49 @@ impl JobCtx { } } +/// Why [`Session::try_cancel_job`] left a job running. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum CancelRefused { + /// No such job is running. + NotRunning, + /// A print belongs to the system print queue once it has started. + Printing, + /// Past its [commit](JobCtx::commit): it is writing its result (a save replacing its file) + /// and will report its real outcome when it ends. + Committing, +} + +impl CancelRefused { + /// The scripting name (`jobs.cancel`'s `notCancelled[].reason`). + pub fn as_str(self) -> &'static str { + match self { + Self::NotRunning => "notRunning", + Self::Printing => "printing", + Self::Committing => "committing", + } + } +} + +/// What a cancel of several jobs did ([`Session::cancel_all_jobs`]). +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct CancelReport { + pub cancelled: Vec, + /// The jobs left running, and why. + pub refused: Vec<(JobId, CancelRefused)>, +} + +impl CancelReport { + /// The `jobs.cancel` reply: `{cancelled: n}`, with `notCancelled: [{job, reason}]` when a + /// job was left running. + pub fn to_json(&self) -> Value { + let mut v = json!({"cancelled": self.cancelled.len()}); + if !self.refused.is_empty() { + v["notCancelled"] = self.refused.iter().map(|(id, why)| json!({"job": id.0, "reason": why.as_str()})).collect(); + } + v + } +} + /// Final state of a job. #[derive(Clone, Debug, PartialEq)] pub enum JobOutcome { @@ -1071,30 +1158,50 @@ impl Session { } /// Cancel job `id`: it stops at its next check, its result is discarded and the document - /// stays unchanged. The document is unlocked at once. False if no such job is running. + /// stays unchanged. The document is unlocked at once. False if no such job is running or it + /// can't be cancelled (see [`Self::try_cancel_job`]). pub fn cancel_job(&mut self, id: JobId) -> bool { - let Some(i) = self.jobs.running.iter().position(|r| r.id == id) else { return false }; + self.try_cancel_job(id).is_ok() + } + + /// [`Self::cancel_job`], saying why a job was not cancelled. + pub fn try_cancel_job(&mut self, id: JobId) -> std::result::Result<(), CancelRefused> { + let Some(i) = self.jobs.running.iter().position(|r| r.id == id) else { return Err(CancelRefused::NotRunning) }; // A print worker can already have handed the job to the OS. Discarding its result // would falsely report cancellation; removal belongs to the system print queue. if matches!(self.jobs.running[i].command.as_str(), "file.print" | "file.printOneCopy") { - return false; + return Err(CancelRefused::Printing); + } + // Past its commit (a save replacing its file) the job runs to the end and reports its + // real outcome; calling it cancelled would be false. + if !self.jobs.running[i].ctx.try_cancel() { + return Err(CancelRefused::Committing); } let job = self.jobs.running.remove(i); - job.ctx.cancel(); self.end_job(&job, JobOutcome::Cancelled); #[cfg(not(target_arch = "wasm32"))] if let Some(handle) = job.handle { self.jobs.draining.push(Draining { handle, doc: job.doc, release: false }); } - true + Ok(()) } - /// Cancel every running job (e.g. before quitting). - pub fn cancel_all_jobs(&mut self) { + /// Cancel every running job (e.g. before quitting). Prints and jobs past their commit keep + /// running; the report lists them. + pub fn cancel_all_jobs(&mut self) -> CancelReport { let ids: Vec = self.jobs.running.iter().map(|r| r.id).collect(); + self.cancel_each(ids) + } + + fn cancel_each(&mut self, ids: Vec) -> CancelReport { + let mut report = CancelReport::default(); for id in ids { - self.cancel_job(id); + match self.try_cancel_job(id) { + Ok(()) => report.cancelled.push(id), + Err(why) => report.refused.push((id, why)), + } } + report } /// Is any job running? (Cheap: the UI asks every frame.) @@ -1152,17 +1259,17 @@ impl Session { Some(format!("“{}” is still running on this document; wait for it to finish or cancel it (Esc)", job.label)) } - /// Cancel the jobs editing document `doc` (it is closing). - pub(crate) fn cancel_jobs_on(&mut self, doc: DocId) { + /// Cancel the jobs editing document `doc` (it is closing). A job past its commit keeps + /// running and its result is still applied (see [`JobCtx::commit`]). + pub(crate) fn cancel_jobs_on(&mut self, doc: DocId) -> CancelReport { let ids: Vec = self.jobs.running.iter().filter(|r| r.doc == Some(doc)).map(|r| r.id).collect(); - for id in ids { - self.cancel_job(id); - } + let report = self.cancel_each(ids); // Workers cancelled now, or earlier and still unwinding, hold the closing document. #[cfg(not(target_arch = "wasm32"))] for d in self.jobs.draining.iter_mut().filter(|d| d.doc == Some(doc)) { d.release = true; } + report } /// Run the command `id`: inline, or (with `background`) as a job when it is one. @@ -1399,6 +1506,10 @@ impl Session { } let prev_active = self.active; let target = match doc { + // A job past its commit has done its work outside the session (a save wrote its + // file); a closed document doesn't undo that, so its result still applies, to no + // document. + Some(d) if job.ctx.committed() && !self.documents().iter().any(|st| st.doc.id == *d) => None, Some(d) => { let i = self.documents().iter().position(|st| st.doc.id == *d).ok_or_else(|| EngineError::Other(format!("`{label}`: the document was closed")))?; @@ -1524,19 +1635,15 @@ pub fn specs() -> Vec { params: r##"{"job":u64? (default: every running job)}"##, enabled: |_| Ok(()), run: |s, p| match p.get("job") { - None | Some(Value::Null) => { - let n = s.jobs.running.len(); - s.cancel_all_jobs(); - Ok(json!({"cancelled": n.saturating_sub(s.jobs.running.len())})) - } + None | Some(Value::Null) => Ok(s.cancel_all_jobs().to_json()), Some(v) => { let id = v.as_u64().ok_or_else(|| EngineError::BadParams { cmd: "jobs.cancel".into(), msg: "`job` must be a job id".into() })?; - if s.cancel_job(JobId(id)) { - Ok(json!({"cancelled": 1})) - } else if s.job(JobId(id)).is_some() { - Err(EngineError::Other("printing cannot be cancelled here; use the system print queue".into())) - } else { - Err(EngineError::Other(format!("no running job {id}"))) + match s.try_cancel_job(JobId(id)) { + Ok(()) => Ok(json!({"cancelled": 1})), + // Not an error: the job is fine and will report how it ended. + Err(why @ CancelRefused::Committing) => Ok(CancelReport { cancelled: Vec::new(), refused: vec![(JobId(id), why)] }.to_json()), + Err(CancelRefused::Printing) => Err(EngineError::Other("printing cannot be cancelled here; use the system print queue".into())), + Err(CancelRefused::NotRunning) => Err(EngineError::Other(format!("no running job {id}"))), } } }, diff --git a/crates/engine/src/jobs/tests.rs b/crates/engine/src/jobs/tests.rs index 38cca3b206..51ff64385c 100644 --- a/crates/engine/src/jobs/tests.rs +++ b/crates/engine/src/jobs/tests.rs @@ -422,6 +422,207 @@ fn jobs_cancel_command_and_bad_params() { assert_eq!(s.execute("jobs.cancel", json!({})).unwrap()["cancelled"], 0); } +/// A fake save of the active document to `target`, gated by the test: it waits for `go`, calls +/// [`JobCtx::commit`] and reports whether it got through, then waits for `finish` and replaces +/// the file. `done` disconnects once the worker has returned. +struct GatedSave { + id: JobId, + go: std::sync::mpsc::Sender<()>, + committed: std::sync::mpsc::Receiver, + finish: std::sync::mpsc::Sender<()>, + done: std::sync::mpsc::Receiver<()>, +} + +impl GatedSave { + fn start(s: &mut Session, target: &std::path::Path) -> Self { + let (go, go_rx) = std::sync::mpsc::channel(); + let (committed_tx, committed) = std::sync::mpsc::channel(); + let (finish, finish_rx) = std::sync::mpsc::channel::<()>(); + let (done_tx, done) = std::sync::mpsc::channel::<()>(); + let target = target.to_path_buf(); + let id = job(s + .start_job( + "file.save", + json!({"path": target}), + "Saving w.psd", + true, + move |ctx| { + let _done = done_tx; + go_rx.recv().ok(); + let r = ctx.commit(); + committed_tx.send(r.is_ok()).ok(); + r?; + finish_rx.recv().ok(); + std::fs::write(&target, b"new").map_err(|e| EngineError::Other(e.to_string()))?; + Ok(target.display().to_string()) + }, + |_, path: String| Ok(json!({"path": path})), + ) + .unwrap()); + Self { id, go, committed, finish, done } + } + + /// Let the worker reach its commit; true if it committed. + fn commit(&self) -> bool { + self.go.send(()).unwrap(); + self.committed.recv_timeout(Duration::from_secs(60)).unwrap() + } + + /// Let a committed worker write the file. + fn finish(&self) { + self.finish.send(()).unwrap(); + } + + /// Block until the worker has returned. + fn join(&self) { + assert!(matches!(self.done.recv_timeout(Duration::from_secs(60)), Err(std::sync::mpsc::RecvTimeoutError::Disconnected))); + } +} + +/// A job on the active document that runs until it is cancelled. +fn open_job(s: &mut Session) -> JobId { + job(s + .start_job( + "test.spin", + json!({}), + "Spinning", + true, + |ctx| loop { + ctx.check()?; + std::thread::sleep(Duration::from_millis(1)); + }, + |_, ()| Ok(json!(null)), + ) + .unwrap()) +} + +fn index_of(s: &Session, doc: photocraft_doc::DocId) -> usize { + s.documents().iter().position(|st| st.doc.id == doc).unwrap() +} + +#[test] +fn cancelling_a_save_before_it_commits_stops_it_without_touching_the_target() { + let dir = tmp_dir("cancel-before-commit"); + let target = dir.join("w.psd"); + std::fs::write(&target, b"old").unwrap(); + let mut s = session(64, 64); + let save = GatedSave::start(&mut s, &target); + assert_eq!(s.execute("jobs.cancel", json!({"job": save.id.0})).unwrap(), json!({"cancelled": 1})); + assert_eq!(wait_event(&mut s, save.id).outcome, JobOutcome::Cancelled); + // The worker learns it lost at its commit point and writes nothing. + assert!(!save.commit(), "commit after a cancel must fail"); + save.join(); + assert_eq!(std::fs::read(&target).unwrap(), b"old", "target untouched"); + assert!(s.job(save.id).is_none()); +} + +#[test] +fn cancelling_a_committed_save_is_refused_and_it_reports_its_real_outcome() { + let dir = tmp_dir("cancel-after-commit"); + let target = dir.join("w.psd"); + std::fs::write(&target, b"old").unwrap(); + let mut s = session(64, 64); + let save = GatedSave::start(&mut s, &target); + assert!(save.commit()); + // Every single-job entry point says it was not cancelled. + assert!(!s.cancel_job(save.id)); + assert_eq!(s.try_cancel_job(save.id), Err(CancelRefused::Committing)); + assert_eq!( + s.execute("jobs.cancel", json!({"job": save.id.0})).unwrap(), + json!({"cancelled": 0, "notCancelled": [{"job": save.id.0, "reason": "committing"}]}) + ); + assert!(s.job(save.id).is_some(), "still running, document still locked"); + assert!(s.poll_jobs().is_empty(), "no Cancelled event"); + save.finish(); + let e = wait_event(&mut s, save.id); + assert_eq!(e.outcome, JobOutcome::Done(json!({"path": target.display().to_string()}))); + save.join(); + assert_eq!(std::fs::read(&target).unwrap(), b"new"); +} + +#[test] +fn cancel_all_cancels_open_jobs_and_reports_a_committed_save_it_left_running() { + let dir = tmp_dir("cancel-all-mixed"); + let (committed_target, open_target) = (dir.join("a.psd"), dir.join("c.psd")); + std::fs::write(&committed_target, b"old").unwrap(); + std::fs::write(&open_target, b"old").unwrap(); + let mut s = session(64, 64); + let committed = GatedSave::start(&mut s, &committed_target); + assert!(committed.commit()); + s.execute("file.new", json!({"width": 8, "height": 8})).unwrap(); + let spinning = open_job(&mut s); + s.execute("file.new", json!({"width": 8, "height": 8})).unwrap(); + let open = GatedSave::start(&mut s, &open_target); + + let report = s.cancel_all_jobs(); + assert_eq!(report.cancelled, vec![spinning, open.id]); + assert_eq!(report.refused, vec![(committed.id, CancelRefused::Committing)]); + assert_eq!(report.to_json(), json!({"cancelled": 2, "notCancelled": [{"job": committed.id.0, "reason": "committing"}]})); + // `jobs.cancel {}` again: only the committed save is left, and it is still not cancelled. + assert_eq!(s.execute("jobs.cancel", json!({})).unwrap(), json!({"cancelled": 0, "notCancelled": [{"job": committed.id.0, "reason": "committing"}]})); + + // One poll reports both cancellations. + let ended: Vec<_> = s.poll_jobs().into_iter().map(|e| (e.id, e.outcome)).collect(); + assert_eq!(ended, vec![(spinning, JobOutcome::Cancelled), (open.id, JobOutcome::Cancelled)]); + assert!(!open.commit()); + open.join(); + assert_eq!(std::fs::read(&open_target).unwrap(), b"old"); + committed.finish(); + assert!(matches!(wait_event(&mut s, committed.id).outcome, JobOutcome::Done(_))); + committed.join(); + assert_eq!(std::fs::read(&committed_target).unwrap(), b"new"); + assert!(s.jobs().is_empty()); +} + +#[test] +fn closing_documents_cancels_open_saves_but_a_committed_save_finishes() { + let dir = tmp_dir("cancel-on-close-mixed"); + let (committed_target, open_target) = (dir.join("a.psd"), dir.join("c.psd")); + std::fs::write(&committed_target, b"old").unwrap(); + std::fs::write(&open_target, b"old").unwrap(); + let mut s = session(64, 64); + let a = s.active().unwrap().doc.id; + let committed = GatedSave::start(&mut s, &committed_target); + assert!(committed.commit()); + s.execute("file.new", json!({"width": 8, "height": 8})).unwrap(); + let b = s.active().unwrap().doc.id; + let spinning = open_job(&mut s); + s.execute("file.new", json!({"width": 8, "height": 8})).unwrap(); + let c = s.active().unwrap().doc.id; + let open = GatedSave::start(&mut s, &open_target); + + assert_eq!(s.cancel_jobs_on(a).refused, vec![(committed.id, CancelRefused::Committing)]); + s.close(index_of(&s, a)); + s.close(index_of(&s, c)); + assert_eq!(wait_event(&mut s, open.id).outcome, JobOutcome::Cancelled); + assert!(!open.commit()); + open.join(); + assert_eq!(std::fs::read(&open_target).unwrap(), b"old", "the open save's target is untouched"); + // The other document's job is not affected. + assert!(s.job(spinning).is_some()); + // The committed save outlives its document and reports what it did, not "document closed". + committed.finish(); + assert_eq!(wait_event(&mut s, committed.id).outcome, JobOutcome::Done(json!({"path": committed_target.display().to_string()}))); + committed.join(); + assert_eq!(std::fs::read(&committed_target).unwrap(), b"new"); + assert!(s.cancel_job(spinning)); + assert_eq!(s.documents().iter().map(|st| st.doc.id).collect::>(), vec![b]); +} + +#[test] +fn commit_and_cancel_race_with_exactly_one_winner() { + for _ in 0..200 { + let ctx = JobCtx::default(); + let other = ctx.clone(); + let t = std::thread::spawn(move || other.commit().is_ok()); + let cancelled = ctx.try_cancel(); + let committed = t.join().unwrap(); + assert_ne!(cancelled, committed, "exactly one of commit and cancel wins"); + assert_eq!(ctx.cancelled(), cancelled); + assert_eq!(ctx.committed(), committed); + } +} + #[test] fn non_job_commands_still_finish_inline_with_start() { let mut s = session(16, 16); diff --git a/crates/ui-egui/src/control/dispatch.rs b/crates/ui-egui/src/control/dispatch.rs index 7d9c6dd5cd..3c667480f6 100644 --- a/crates/ui-egui/src/control/dispatch.rs +++ b/crates/ui-egui/src/control/dispatch.rs @@ -5,6 +5,7 @@ use super::params::{ input_object, merged_object, modifiers_from, num_field, ok, screen_point, uint_field, whole_object, }; use super::{ControlRequest, DialogKind, Outcome, PhotocraftApp, SHELL_DIALOG, Tool, ToolEvent, Value, json, run_waiting, tool_event, wrap}; +use photocraft_engine::jobs::{CancelReport, JobId}; /// A domain handler: `Some` when it owns the request's method, `None` to let the next one try. type Handler = fn(&mut PhotocraftApp, &egui::Context, &ControlRequest) -> Option; @@ -123,20 +124,29 @@ fn execute_command(app: &mut PhotocraftApp, ctx: &egui::Context, req: &ControlRe fn jobs_cancel(app: &mut PhotocraftApp, p: &Value) -> Outcome { let all = p.get("job").is_none_or(Value::is_null); match p.get("job").and_then(Value::as_u64) { - Some(j) if app.session.job(photocraft_engine::jobs::JobId(j)).is_some() => { - let cancelled = crate::jobs_ui::cancel(app, photocraft_engine::jobs::JobId(j)); - ok(json!({"cancelled": usize::from(cancelled)})) - } + // Same reply as the engine's `jobs.cancel`: `{cancelled: n}`, plus `notCancelled: + // [{job, reason}]` for the jobs left running (a save already writing its file, a print). + Some(j) if app.session.job(JobId(j)).is_some() => ok(cancel_each(app, vec![JobId(j)]).to_json()), Some(j) => err(format!("no running job {j}")), None if all => { let ids: Vec<_> = app.session.jobs().into_iter().map(|j| j.id).collect(); - let cancelled = ids.into_iter().filter(|j| crate::jobs_ui::cancel(app, *j)).count(); - ok(json!({"cancelled": cancelled})) + ok(cancel_each(app, ids).to_json()) } None => err("`job` must be a job id"), } } +fn cancel_each(app: &mut PhotocraftApp, ids: Vec) -> CancelReport { + let mut report = CancelReport::default(); + for id in ids { + match crate::jobs_ui::try_cancel(app, id) { + Ok(()) => report.cancelled.push(id), + Err(why) => report.refused.push((id, why)), + } + } + report +} + // ---- the shell: view state, windows, GPU, focus, screenshots ---- fn shell_methods(app: &mut PhotocraftApp, ctx: &egui::Context, req: &ControlRequest) -> Option { diff --git a/crates/ui-egui/src/jobs_ui.rs b/crates/ui-egui/src/jobs_ui.rs index 760a7b6b48..a561b2438e 100644 --- a/crates/ui-egui/src/jobs_ui.rs +++ b/crates/ui-egui/src/jobs_ui.rs @@ -20,12 +20,12 @@ //! The canvas keeps showing the pre-job state: workers never touch the session's documents. use std::sync::Arc; -use std::sync::atomic::{AtomicBool, AtomicU8, Ordering}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::mpsc::Sender; use egui::{Color32, Rect, RichText, Sense, Stroke, vec2}; use photocraft_doc::DocId; -use photocraft_engine::jobs::{JobCtx, JobEvent, JobId, JobInfo, JobOutcome, OPEN_JOB, OpenSource, Started}; +use photocraft_engine::jobs::{CancelRefused, JobCtx, JobEvent, JobId, JobInfo, JobOutcome, OPEN_JOB, OpenSource, Started}; use serde_json::{Value, json}; use crate::theme::Tokens; @@ -73,23 +73,14 @@ pub(crate) struct OpenBatch { /// The job command of a background save. pub const SAVE_JOB: &str = "file.save"; -const ENCODING: u8 = 0; -const WRITING: u8 = 1; -const CANCELLED: u8 = 2; - -/// A save worker's stage, shared between the worker and the UI. +/// A save worker's state, shared between the worker and the UI. Whether it may still be +/// cancelled is its job's ([`JobCtx::commit`]), so every cancel path agrees. #[derive(Debug, Default)] pub struct SaveState { - stage: AtomicU8, finished: AtomicBool, } impl SaveState { - /// Cancel unless writing has started; false once it has. - fn try_cancel(&self) -> bool { - matches!(self.stage.compare_exchange(ENCODING, CANCELLED, Ordering::AcqRel, Ordering::Acquire), Ok(_) | Err(CANCELLED)) - } - /// The worker has returned (or unwound): nothing of this save can still touch the file. pub fn finished(&self) -> bool { self.finished.load(Ordering::Acquire) @@ -110,7 +101,6 @@ impl Drop for WorkerDone { /// be cancelled. pub struct SaveCtl { pub job: JobCtx, - state: Arc, } impl SaveCtl { @@ -120,15 +110,15 @@ impl SaveCtl { /// `Err` once the save was cancelled: stop, write nothing. pub fn check(&self) -> Result<(), String> { - if self.job.cancelled() || self.state.stage.load(Ordering::Acquire) == CANCELLED { Err(SAVE_CANCELLED.into()) } else { Ok(()) } + if self.job.cancelled() { Err(SAVE_CANCELLED.into()) } else { Ok(()) } } /// Call once, right before replacing the file. `Ok`: the save can no longer be cancelled - /// (Cancel and Esc are refused) and must run to the end; `Err`: it was cancelled first and - /// nothing may be written. + /// (Cancel, Esc, `jobs.cancel`, cancelling every job and closing the document all leave it + /// running) and must run to the end; `Err`: it was cancelled first and nothing may be + /// written. pub fn commit(&self) -> Result<(), String> { - self.check()?; - self.state.stage.compare_exchange(ENCODING, WRITING, Ordering::AcqRel, Ordering::Acquire).map(|_| ()).map_err(|_| SAVE_CANCELLED.into()) + self.job.commit().map_err(|_| SAVE_CANCELLED.into()) } } @@ -311,8 +301,8 @@ pub fn start_save( &label, true, move |ctx| { - let _done = WorkerDone(Arc::clone(&worker)); - let ctl = SaveCtl { job: ctx.clone(), state: worker }; + let _done = WorkerDone(worker); + let ctl = SaveCtl { job: ctx.clone() }; save(&snapshot, &target, &settings, &ctl) .map_err(|e| if ctl.check().is_err() { photocraft_engine::EngineError::Cancelled } else { photocraft_engine::EngineError::Other(e) }) }, @@ -447,25 +437,27 @@ pub fn job_in_view(app: &PhotocraftApp) -> Option { /// Cancel job `id` (the status bar's ×, the dialog's Cancel, Esc, closing an opening tab). pub fn cancel(app: &mut PhotocraftApp, id: JobId) -> bool { + try_cancel(app, id).is_ok() +} + +/// [`cancel`], saying why a job was left running. +pub fn try_cancel(app: &mut PhotocraftApp, id: JobId) -> Result<(), CancelRefused> { let label = app.session.job(id).map(|j| j.label); + let r = app.session.try_cancel_job(id); // A save that has started replacing the file runs to the end: cancelling it now would leave // the file written but the document marked unsaved. - if let Some(save) = app.jobs.saves.iter().find(|s| s.job == id) - && !save.state.try_cancel() - { + if r == Err(CancelRefused::Committing) { app.ui.status = crate::i18n::fmt(tl!("{label} is writing the file and can no longer be cancelled"), &[("label", label.as_deref().unwrap_or_default())]); app.ui.status_error = false; - return false; } - let cancelled = app.session.cancel_job(id); - if cancelled { + if r.is_ok() { // The event (and its status text) arrives with the next poll; say so now as well. if let Some(l) = label { app.ui.status = crate::i18n::fmt(tl!("Cancelled {label}"), &[("label", &l)]); app.ui.status_error = false; } } - cancelled + r } /// Handles one finished job: optional completion beep, waiting control replies, and the diff --git a/crates/ui-egui/src/jobs_ui/tests.rs b/crates/ui-egui/src/jobs_ui/tests.rs index ffb6207fd4..41d5e127b0 100644 --- a/crates/ui-egui/src/jobs_ui/tests.rs +++ b/crates/ui-egui/src/jobs_ui/tests.rs @@ -604,6 +604,95 @@ fn a_save_that_has_started_writing_cannot_be_cancelled() { assert!(!h.state().session.active().unwrap().is_dirty(), "the written save is recorded"); } +/// A `save_file` service held by `gates` that really replaces the file: it encodes until +/// `encode` opens, commits, then writes `new` once `write` opens. +fn file_save(gates: &Gates) -> crate::SaveFileFn { + let g = gates.clone(); + Arc::new(move |_, path, _, ctl| { + ctl.progress(0.0, "Encoding"); + while !g.encode.load(Ordering::Relaxed) { + ctl.check()?; + std::thread::sleep(Duration::from_millis(1)); + } + ctl.commit()?; + ctl.progress(0.0, "Writing"); + while !g.write.load(Ordering::Relaxed) { + std::thread::sleep(Duration::from_millis(1)); + } + std::fs::write(path, b"new").map_err(|e| e.to_string())?; + Ok(Vec::new()) + }) +} + +/// A file holding `old`, in a fresh directory named for the test. +fn old_file(test: &str) -> String { + let dir = std::env::temp_dir().join(format!("photocraft-jobs-ui-{}-{test}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("w.psd"); + std::fs::write(&path, b"old").unwrap(); + path.to_string_lossy().into_owned() +} + +fn control(h: &mut Harness<'static, PhotocraftApp>, method: &str, params: Value) -> Value { + let (req, _rx) = crate::ControlRequest::new(method, params); + let ctx = h.ctx.clone(); + let crate::control::Outcome::Done(v) = crate::control::handle(h.state_mut(), &ctx, &req) else { panic!("{method} replies at once") }; + v +} + +#[test] +fn jobs_cancel_before_a_save_writes_stops_it_and_leaves_the_target_untouched() { + let mut h = app_harness(); + let path = old_file("cancel-before-write"); + let gates = Gates::default(); + h.state_mut().services.save_file = Some(file_save(&gates)); + h.state_mut().save_as(Some(path.clone())).unwrap(); + h.run_steps(2); + let job = h.state().jobs.saves[0].job; + let v = control(&mut h, "jobs.cancel", json!({"job": job.0})); + assert_eq!(v["result"], json!({"cancelled": 1}), "{v}"); + // The worker reaches its commit point only now, and must give up there. + gates.encode.store(true, Ordering::Relaxed); + gates.write.store(true, Ordering::Relaxed); + step_until(&mut h, |a| !a.saving()); + assert_eq!(std::fs::read(&path).unwrap(), b"old", "target untouched"); + let st = h.state().session.active().unwrap(); + assert!(st.is_dirty() && st.path.is_none(), "the document is not marked saved"); + assert_eq!(h.state().ui.status, "Cancelled Saving w.psd"); +} + +#[test] +fn jobs_cancel_during_the_write_reports_the_save_as_not_cancelled_and_it_finishes() { + let mut h = app_harness(); + let path = old_file("cancel-during-write"); + let gates = Gates { encode: Arc::new(AtomicBool::new(true)), ..Gates::default() }; + h.state_mut().services.save_file = Some(file_save(&gates)); + h.state_mut().save_as(Some(path.clone())).unwrap(); + let saved_doc = h.state().session.active().unwrap().doc.id; + until_writing(&mut h); + let save = h.state().jobs.saves[0].job; + // A second document with a job that can still be cancelled. + h.state_mut().session.execute("file.new", json!({"width": 32, "height": 32})).unwrap(); + let slow = slow_job(h.state_mut(), &Arc::new(AtomicBool::new(false))); + + let refused = json!([{"job": save.0, "reason": "committing"}]); + let v = control(&mut h, "jobs.cancel", json!({"job": save.0})); + assert_eq!(v["result"], json!({"cancelled": 0, "notCancelled": refused}), "{v}"); + assert_eq!(h.state().ui.status, "Saving w.psd is writing the file and can no longer be cancelled"); + // Cancel everything: the other job stops, the save is reported as left running. + let v = control(&mut h, "jobs.cancel", json!({})); + assert_eq!(v["result"], json!({"cancelled": 1, "notCancelled": refused}), "{v}"); + assert!(h.state().session.job(slow).is_none()); + assert!(h.state().session.job(save).is_some(), "the save still runs and keeps its document locked"); + + gates.write.store(true, Ordering::Relaxed); + step_until(&mut h, |a| !a.saving()); + assert_eq!(std::fs::read(&path).unwrap(), b"new"); + let st = h.state().session.documents().iter().find(|st| st.doc.id == saved_doc).unwrap(); + assert!(!st.is_dirty() && st.path.as_deref() == Some(path.as_str()), "the save that happened is recorded"); +} + /// #3389: like Photoshop, a running save reads "Saving 45%" in the status bar and appends /// " - Saving 45%" to its document's tab title, in both tab strips; both go when it ends. #[test] diff --git a/docs/control-protocol.md b/docs/control-protocol.md index e74938dc2f..39f5d2cbc2 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -250,7 +250,7 @@ The shape is pinned by a test in `crates/automation/src/rpc.rs` (#3684). ### Background jobs (#210) -Long commands (every `filter.*`, `edit.contentAwareFill`, `edit.contentAwareScale`, `file.automate.photomerge`, `brush.presets.importAbr`), file opens and interactive saves (File › Save, Save As and the TIFF Options continuation, #2017) run as background jobs in the desktop app: the window keeps drawing, the status bar shows progress with a Cancel button, and jobs that lock the active document show a modal progress dialog (Esc cancels). `engine.execute`, `ui.menu.invoke`, `ui.dialog.confirm` and `app.save` still wait for the result by default; pass `"wait": false` to get `{job, pending: true}` at once, then poll `jobs.list` (`state`: running, done, failed, cancelled; `progress` 0–1; the result or error) and stop it with `jobs.cancel {job}`. A cancelled or failed job leaves the document unchanged. While a job runs, commands that would edit its document fail with "… is still running on this document". `ui.inspect` reports `jobs` (running jobs, opening files, saving files). Set `PHOTOCRAFT_INLINE_JOBS=1` to run everything inline. +Long commands (every `filter.*`, `edit.contentAwareFill`, `edit.contentAwareScale`, `file.automate.photomerge`, `brush.presets.importAbr`), file opens and interactive saves (File › Save, Save As and the TIFF Options continuation, #2017) run as background jobs in the desktop app: the window keeps drawing, the status bar shows progress with a Cancel button, and jobs that lock the active document show a modal progress dialog (Esc cancels). `engine.execute`, `ui.menu.invoke`, `ui.dialog.confirm` and `app.save` still wait for the result by default; pass `"wait": false` to get `{job, pending: true}` at once, then poll `jobs.list` (`state`: running, done, failed, cancelled; `progress` 0–1; the result or error) and stop it with `jobs.cancel {job}`. A cancelled or failed job leaves the document unchanged. A save that has started replacing its file can no longer be cancelled: `jobs.cancel` leaves it running and lists it in `notCancelled: [{job, reason: "committing"}]` (next to `cancelled`, the number of jobs it did cancel); the save then ends as done or failed like any other. Closing its document doesn't stop it either. While a job runs, commands that would edit its document fail with "… is still running on this document". `ui.inspect` reports `jobs` (running jobs, opening files, saving files). Set `PHOTOCRAFT_INLINE_JOBS=1` to run everything inline. ## Preferences @@ -440,7 +440,7 @@ stdio, and a value that is not a port is a usage error naming the variable. Impl | Method | Params | |---|---| | `engine.execute` | `{command, params?, wait?}`: any engine command (`wait: false` starts a long one as a background job: `{job, pending}`) | -| `jobs.list` / `jobs.cancel` | `{}` / `{job?}`: background jobs; cancel one or all. Every request (and MCP tool call) first applies the jobs that finished, so `doc.save`, `doc.inspect`, `doc.render` and `session.list` include a finished job's result without polling `jobs.list` first | +| `jobs.list` / `jobs.cancel` | `{}` / `{job?}`: background jobs; cancel one or all (`{cancelled: n}`, plus `notCancelled: [{job, reason}]` for jobs left running, e.g. a save already writing its file). Every request (and MCP tool call) first applies the jobs that finished, so `doc.save`, `doc.inspect`, `doc.render` and `session.list` include a finished job's result without polling `jobs.list` first | | `engine.commands` | `{filter?}`: registry with params docs and enablement (`enabled`, `disabledReason`, `needsDocument`); `result` is a bare array of command objects ([Command list replies](#command-list-replies)) | | `session.list` | open documents and the active index | | `doc.open` / `doc.new` | `{path, approveHighMemory? (false)}` / `file.new` params (`approveHighMemory` is the same one-shot PSD/PSB consent as the `file.openAs` command; see [development.md](development.md) for the structured refusal fields) | From f67696cb2c0ca3bcf63686f25677da7a0fa4f44c Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 16:42:18 +0100 Subject: [PATCH 19/20] Jobs tests: give the gated-save tests a temp dir on every platform The cancel-path tests used the Unix-only tmp_dir fixture, so the engine's lib tests did not compile on Windows (E0425 in CI run 41660911095). They get their own save_dir helper instead. --- crates/engine/src/jobs/tests.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/jobs/tests.rs b/crates/engine/src/jobs/tests.rs index 51ff64385c..70443bc523 100644 --- a/crates/engine/src/jobs/tests.rs +++ b/crates/engine/src/jobs/tests.rs @@ -479,6 +479,15 @@ impl GatedSave { } } +/// A fresh directory for a [`GatedSave`] test (on every platform, unlike the Unix-only +/// fixtures' `tmp_dir`). +fn save_dir(name: &str) -> std::path::PathBuf { + let d = std::env::temp_dir().join(format!("photocraft-engine-gated-save-{}-{name}", std::process::id())); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(&d).unwrap(); + d +} + /// A job on the active document that runs until it is cancelled. fn open_job(s: &mut Session) -> JobId { job(s @@ -502,7 +511,7 @@ fn index_of(s: &Session, doc: photocraft_doc::DocId) -> usize { #[test] fn cancelling_a_save_before_it_commits_stops_it_without_touching_the_target() { - let dir = tmp_dir("cancel-before-commit"); + let dir = save_dir("cancel-before-commit"); let target = dir.join("w.psd"); std::fs::write(&target, b"old").unwrap(); let mut s = session(64, 64); @@ -518,7 +527,7 @@ fn cancelling_a_save_before_it_commits_stops_it_without_touching_the_target() { #[test] fn cancelling_a_committed_save_is_refused_and_it_reports_its_real_outcome() { - let dir = tmp_dir("cancel-after-commit"); + let dir = save_dir("cancel-after-commit"); let target = dir.join("w.psd"); std::fs::write(&target, b"old").unwrap(); let mut s = session(64, 64); @@ -542,7 +551,7 @@ fn cancelling_a_committed_save_is_refused_and_it_reports_its_real_outcome() { #[test] fn cancel_all_cancels_open_jobs_and_reports_a_committed_save_it_left_running() { - let dir = tmp_dir("cancel-all-mixed"); + let dir = save_dir("cancel-all-mixed"); let (committed_target, open_target) = (dir.join("a.psd"), dir.join("c.psd")); std::fs::write(&committed_target, b"old").unwrap(); std::fs::write(&open_target, b"old").unwrap(); @@ -576,7 +585,7 @@ fn cancel_all_cancels_open_jobs_and_reports_a_committed_save_it_left_running() { #[test] fn closing_documents_cancels_open_saves_but_a_committed_save_finishes() { - let dir = tmp_dir("cancel-on-close-mixed"); + let dir = save_dir("cancel-on-close-mixed"); let (committed_target, open_target) = (dir.join("a.psd"), dir.join("c.psd")); std::fs::write(&committed_target, b"old").unwrap(); std::fs::write(&open_target, b"old").unwrap(); From 996a52cb8fc920f3b5883dd8d77d42bfa18d1302 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Tue, 3 Nov 2026 18:00:23 +0100 Subject: [PATCH 20/20] PSD export: a cancelled save stops between the bands of a banded layer A banded layer checks the save's interrupt before each band and before each channel's part of a band. Once cancelled it stops there and doesn't encode the layer again raw; the incomplete model is discarded by the caller (which returns IoError::Cancelled), so the save never reaches its commit point and the target is untouched. Ex carries the export's Interrupt for this. Addresses CodeRabbit review on crates/io/src/psd_export/banded.rs. --- crates/io/src/psd_export.rs | 7 +++-- crates/io/src/psd_export/banded.rs | 22 ++++++++++++-- crates/io/src/psd_export/document.rs | 7 +++-- crates/io/src/psd_export/tests.rs | 43 ++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 7 deletions(-) diff --git a/crates/io/src/psd_export.rs b/crates/io/src/psd_export.rs index a98cb347b1..89bf250b4e 100644 --- a/crates/io/src/psd_export.rs +++ b/crates/io/src/psd_export.rs @@ -94,7 +94,10 @@ impl Default for PsdExportOptions { } } -struct Ex { +struct Ex<'c> { + /// The save's interrupt: a layer saved in bands checks it between bands and channels, so a + /// cancelled save stops within a band (#5333). + ctl: Interrupt<'c>, fmt: PixelFormat, /// Document resolution (for generated type-layer data). dpi: f32, @@ -181,7 +184,7 @@ fn q255(v: f32) -> u8 { (v.clamp(0.0, 1.0) * 255.0).round() as u8 } -impl Ex { +impl Ex<'_> { fn compression(&self) -> Compression { if self.fmt.sample == SampleType::F32 { Compression::ZipPrediction } else { Compression::Rle } } diff --git a/crates/io/src/psd_export/banded.rs b/crates/io/src/psd_export/banded.rs index f2fc0a7751..a2aed97473 100644 --- a/crates/io/src/psd_export/banded.rs +++ b/crates/io/src/psd_export/banded.rs @@ -8,6 +8,10 @@ //! writes. ZIP with prediction (32-bit) predicts per row and feeds one zlib stream: the same //! pixels, but not always the same bytes, as zlib packs input it is given in parts differently. //! With a stage each band's parts go to the scratch disk as they are made (see [`Parts`]). +//! +//! A banded layer is minutes of work on a large save, so it checks the save's interrupt before +//! each band and each channel's part of it: once cancelled it stops there (and doesn't retry +//! raw), leaving an incomplete model that the caller discards before the save's commit point. use std::io::Write as _; @@ -30,6 +34,8 @@ const BAND_BYTES: usize = 64 << 20; thread_local! { /// `(BANDED_FROM, BAND_BYTES)` for this thread's exports, so tests band small layers. pub(crate) static BANDS_FOR_TESTS: std::cell::Cell> = const { std::cell::Cell::new(None) }; + /// Bands this thread has gathered and encoded. + pub(crate) static BANDS_DONE: std::cell::Cell = const { std::cell::Cell::new(0) }; } pub(super) fn banded_from() -> u64 { @@ -56,12 +62,18 @@ struct Channel { parts: Parts, } -impl Ex { +impl Ex<'_> { /// The channels of `s` over `r` (non-empty), alpha first, encoded band by band. pub(super) fn banded_channels(&self, s: &Surface, r: photocraft_geom::Rect) -> Vec { // RLE fails only on a row too long for its count (not within PSD/PSB limits); then the // layer is written raw, as a channel encoded whole falls back to. - self.banded_pass(s, r, self.compression()).or_else(|_| self.banded_pass(s, r, Compression::Raw)).unwrap_or_else(|_| self.empty_channels()) + match self.banded_pass(s, r, self.compression()) { + Ok(channels) => channels, + // Cancelled: the caller sees the interrupt and discards the whole model, so don't + // encode the layer again. + Err(_) if self.ctl.cancelled() => self.empty_channels(), + Err(_) => self.banded_pass(s, r, Compression::Raw).unwrap_or_else(|_| self.empty_channels()), + } } fn banded_pass(&self, s: &Surface, r: photocraft_geom::Rect, compression: Compression) -> photocraft_psd::Result> { @@ -81,7 +93,10 @@ impl Ex { .collect(); let mut invert = vec![self.cmyk; self.cc]; invert.push(false); + let ctl = self.ctl; + let check = || if ctl.cancelled() { Err(PsdError::Invalid("cancelled".into())) } else { Ok(()) }; for y in (0..h).step_by(band_rows) { + check()?; let rows = band_rows.min(h - y); let top = r.y0 + y as i32; let pieces = crate::pixels::par_map(crate::pixels::bands(rows), |b| { @@ -101,6 +116,7 @@ impl Ex { let work: Vec<(&mut Channel, Vec)> = channels.iter_mut().enumerate().map(|(k, c)| (c, planes[if k == 0 { self.cc } else { k - 1 }].take().unwrap_or_default())).collect(); let done = crate::pixels::par_map(work, |(c, mut plane)| -> photocraft_psd::Result<()> { + check()?; match compression { Compression::Rle => { let mut packed = encode_planes(Compression::Rle, &plane, &layout)?; @@ -120,6 +136,8 @@ impl Ex { Ok(()) }); done.into_iter().collect::>()?; + #[cfg(test)] + BANDS_DONE.set(BANDS_DONE.get() + 1); } channels .into_iter() diff --git a/crates/io/src/psd_export/document.rs b/crates/io/src/psd_export/document.rs index a87ed93010..b46a21fc3c 100644 --- a/crates/io/src/psd_export/document.rs +++ b/crates/io/src/psd_export/document.rs @@ -67,6 +67,7 @@ fn export_supported_document( let big = doc.size.width > 30_000 || doc.size.height > 30_000; let version = psd_version(doc, opts.force_psb); let mut ex = Ex { + ctl: *ctl, fmt, dpi: doc.resolution_dpi, document: opts.document_path.clone(), @@ -311,7 +312,7 @@ struct MergedExport<'a> { fn prepare_merged_planes<'a>( doc: &'a Document, - ex: &mut Ex, + ex: &mut Ex<'_>, opts: &PsdExportOptions, ctl: &Interrupt, fmt: PixelFormat, @@ -345,7 +346,7 @@ fn prepare_merged_planes<'a>( fn append_extra_channels<'a>( doc: &'a Document, - ex: &mut Ex, + ex: &mut Ex<'_>, planes: &mut Vec, has_alpha: bool, sample: SampleType, @@ -395,7 +396,7 @@ fn merged_image(doc: &Document, opts: &PsdExportOptions, ctl: &Interrupt, fmt: & (planes, has_alpha) } -fn index_layer_ids_and_text(doc: &Document, ex: &mut Ex) { +fn index_layer_ids_and_text(doc: &Document, ex: &mut Ex<'_>) { // Assign layer ids up front (stable across round trips): keep unique // `psd_id`s, number the rest after them; dividers come after all. { diff --git a/crates/io/src/psd_export/tests.rs b/crates/io/src/psd_export/tests.rs index c1d47c4777..fdf0288c9d 100644 --- a/crates/io/src/psd_export/tests.rs +++ b/crates/io/src/psd_export/tests.rs @@ -799,3 +799,46 @@ fn banded_layers_round_trip() { stage::DIR_FOR_TESTS.with(|d| *d.borrow_mut() = None); let _ = std::fs::remove_dir_all(&dir); } + +/// #5333: a save cancelled while a layer is saved in bands stops at the next band (it neither +/// finishes the layer nor encodes it again raw) and reports the cancel, so it never reaches its +/// commit point and the target is never written: RLE and ZIP, PSD and PSB, in memory and staged. +#[test] +fn a_cancelled_save_stops_between_the_bands_of_a_layer() { + let dir = std::env::temp_dir().join(format!("pc-psd-banded-cancel-{}", std::process::id())); + let this = std::thread::current().id(); + // Cancelled once two bands are done (they are counted on this thread). + let after_two = || std::thread::current().id() == this && banded::BANDS_DONE.get() >= 2; + let ctl = Interrupt::cancel_only(&after_two); + for depth in [SampleType::U8, SampleType::F32] { + let doc = banding_document(ColorMode::Rgb, depth, 203, 157); + let row_bytes = doc.size.width as usize * doc.pixel_format().bytes_per_pixel(); + let bands = |ctl: &Interrupt, run: &dyn Fn(&Interrupt)| { + banded::BANDS_DONE.set(0); + banded::BANDS_FOR_TESTS.set(Some((0, row_bytes + 3))); + run(ctl); + banded::BANDS_FOR_TESTS.set(None); + banded::BANDS_DONE.get() + }; + for force_psb in [false, true] { + for staged in [false, true] { + let what = format!("{depth:?}, psb {force_psb}, staged {staged}"); + stage::DIR_FOR_TESTS.with(|d| *d.borrow_mut() = staged.then(|| dir.clone())); + let options = PsdExportOptions { force_psb, stage_on_scratch_from: staged.then_some(0), ..Default::default() }; + let export = |ctl: &Interrupt| drop(document_to_psd_sized(&doc, &options, 0, ctl, PSD_MAX_FILE_SIZE)); + let rows = doc.layers[0].surface().unwrap().content_bounds().height() as usize; + assert_eq!(bands(&Interrupt::NONE, &export), rows, "{what}: one band a row"); + assert_eq!(bands(&ctl, &export), 2, "{what}: stops at the band after the cancel"); + } + } + stage::DIR_FOR_TESTS.with(|d| *d.borrow_mut() = None); + // The save sees the cancel, so it returns before its commit point and writes nothing. + let name = format!("banded-cancel-{depth:?}.psd"); + let save = |ctl: &Interrupt| { + let r = crate::encode_with(&doc, &name, &crate::ExportOptions::default(), ctl); + assert!(matches!(r, Err(crate::IoError::Cancelled)), "{depth:?}: {:?}", r.err()); + }; + assert_eq!(bands(&ctl, &save), 2, "{depth:?}: the save stops at the band after the cancel"); + } + let _ = std::fs::remove_dir_all(&dir); +}