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..3e068258fe 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(); @@ -110,9 +117,13 @@ 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(); - file.write_all(bytes)?; + write(&mut file)?; #[cfg(unix)] { use std::os::fd::AsFd; @@ -130,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)] { @@ -145,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 e96ccb369d..5ea4a5b3fe 100644 --- a/crates/automation/src/workspace/tests.rs +++ b/crates/automation/src/workspace/tests.rs @@ -122,6 +122,52 @@ 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 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() { diff --git a/crates/compose/src/viewport.rs b/crates/compose/src/viewport.rs index c7a5cc3c10..40a6accee9 100644 --- a/crates/compose/src/viewport.rs +++ b/crates/compose/src/viewport.rs @@ -25,56 +25,69 @@ 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)); + // 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)?; + 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 +97,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 +213,40 @@ 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:?}"); + } + } + } + /// 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 { 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..70443bc523 100644 --- a/crates/engine/src/jobs/tests.rs +++ b/crates/engine/src/jobs/tests.rs @@ -422,6 +422,216 @@ 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 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 + .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 = save_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 = save_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 = 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(); + 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 = 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(); + 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/io/src/lib.rs b/crates/io/src/lib.rs index d133832ce2..9364287344 100644 --- a/crates/io/src/lib.rs +++ b/crates/io/src/lib.rs @@ -1010,6 +1010,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..89bf250b4e 100644 --- a/crates/io/src/psd_export.rs +++ b/crates/io/src/psd_export.rs @@ -15,11 +15,14 @@ use crate::blocks; use crate::pixels::{deinterleave, encode_be, psd_depth}; use crate::psd_import::REGENERATED; +mod banded; mod document; 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)] @@ -41,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)] @@ -74,6 +73,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,11 +89,15 @@ impl Default for PsdExportOptions { maximize_compatibility: true, document_path: None, read_linked_files: true, + stage_on_scratch_from: None, } } } -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, @@ -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 @@ -175,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 } } @@ -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 { @@ -208,11 +221,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..a2aed97473 --- /dev/null +++ b/crates/io/src/psd_export/banded.rs @@ -0,0 +1,152 @@ +//! 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`]). +//! +//! 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 _; + +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) }; + /// 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 { + #[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. + 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> { + 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); + 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| { + 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<()> { + check()?; + 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::>()?; + #[cfg(test)] + BANDS_DONE.set(BANDS_DONE.get() + 1); + } + 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/document.rs b/crates/io/src/psd_export/document.rs index f6c36c2d42..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(), @@ -88,6 +89,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); @@ -307,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, @@ -331,13 +336,17 @@ 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 } } fn append_extra_channels<'a>( doc: &'a Document, - ex: &mut Ex, + ex: &mut Ex<'_>, planes: &mut Vec, has_alpha: bool, sample: SampleType, @@ -387,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/stage.rs b/crates/io/src/psd_export/stage.rs new file mode 100644 index 0000000000..869208e319 --- /dev/null +++ b/crates/io/src/psd_export/stage.rs @@ -0,0 +1,223 @@ +//! 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 + } + + /// 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 c53f2c06f3..fdf0288c9d 100644 --- a/crates/io/src/psd_export/tests.rs +++ b/crates/io/src/psd_export/tests.rs @@ -619,3 +619,226 @@ 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); +} + +/// 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); +} + +/// #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); +} diff --git a/crates/io/src/tiff_layers.rs b/crates/io/src/tiff_layers.rs index fdb31c93b5..619e9e53c9 100644 --- a/crates/io/src/tiff_layers.rs +++ b/crates/io/src/tiff_layers.rs @@ -366,6 +366,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 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 1e2c60f827..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; @@ -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_encoded(&self.image_data.data, self.image_data.stored.as_ref(), rows), } } @@ -415,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)?)), } } @@ -464,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)) } @@ -719,6 +718,77 @@ 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()); + } + + /// #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 eb7185bde2..8537b74649 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; @@ -273,16 +275,58 @@ 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. + 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..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); } } @@ -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_encoded(&c.data, c.stored.as_ref(), rows)?, } } } 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 188c0cbcd1..19a9a1056b 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..aff858c279 --- /dev/null +++ b/crates/raster/src/stage_file.rs @@ -0,0 +1,178 @@ +//! 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. +//! +//! 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}; + +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, + /// 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 { + 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), extents: Mutex::default() }) + } + + /// 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)?; + 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. 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<()> { + 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 + } + 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. + 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); + } + + /// 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); + } +} diff --git a/crates/ui-egui/src/canvas/background_redraw/tests.rs b/crates/ui-egui/src/canvas/background_redraw/tests.rs index a5397c77dd..ce47c0c003 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. @@ -696,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 155bb21c91..e1c2389bfb 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); @@ -73,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/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..bc62505d7e 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,49 @@ 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()); + // 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]; - 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); @@ -176,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 72483f9c0e..07933ede30 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. @@ -396,10 +416,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). @@ -426,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; @@ -482,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); } @@ -506,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, @@ -596,6 +627,21 @@ pub(super) fn show(app: &mut PhotocraftApp, ctx: &egui::Context, idx: usize, xf: } return false; } + let key = stream_key(id.0); + // 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 + // 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 }; @@ -606,11 +652,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. @@ -618,9 +659,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); @@ -637,6 +677,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) @@ -926,6 +967,71 @@ 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)); + } + + /// #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)); 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/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 { diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 9227b37f44..c3ea55a6d7 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) |