From d2930b2554b3ae541efcede7399cbee811703804 Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Sun, 18 Oct 2026 01:21:48 +0200 Subject: [PATCH] Fix #1743 (follow-up): commands that write to a given path never replace footage, a proxy or the project file Session::write_output (and refuse_export) is the one place that checks; Save Frame As (PNG/PSD/EXR), Export Lottie, Save a Copy (+XML), Collect Files, Export Timeline (and its pre-rendered movies), Export Template, Save Animation Preset, Export Guides and Export Shortcuts write through it. The control channel's render.frame/ui.screenshot, MCP render_frame and CLI render-frame --out check first (new query file.checkOutput for the bridged cases). Save/Save As/auto-save are untouched. Co-Authored-By: Claude Sonnet 5.5 --- apps/effectcraft-cli/src/main.rs | 2 + apps/effectcraft-cli/tests/it/cli.rs | 29 ++ crates/automation/src/backend.rs | 13 + crates/automation/src/tests.rs | 105 +++++++ crates/automation/src/tools.rs | 11 +- crates/engine/src/cmd/animation.rs | 3 +- crates/engine/src/cmd/app_more.rs | 3 +- crates/engine/src/cmd/comp_more.rs | 3 +- crates/engine/src/cmd/essential.rs | 3 +- crates/engine/src/cmd/file_more.rs | 115 +++++--- crates/engine/src/cmd/frame_export.rs | 5 +- crates/engine/src/cmd/lottie.rs | 3 +- crates/engine/src/cmd/settings.rs | 2 +- crates/engine/src/cmd/timeline.rs | 11 + crates/engine/src/cmd/view.rs | 3 +- crates/engine/src/output_guard.rs | 91 +++++- crates/engine/src/tests.rs | 1 + crates/engine/src/tests/export_overwrite.rs | 271 ++++++++++++++++++ crates/engine/src/tests/render_overwrite.rs | 33 ++- crates/ui-egui/src/control.rs | 12 + crates/ui-egui/tests/it/main.rs | 1 + .../ui-egui/tests/it/ui_export_overwrite.rs | 107 +++++++ docs/agents.md | 12 + 23 files changed, 767 insertions(+), 72 deletions(-) create mode 100644 crates/engine/src/tests/export_overwrite.rs create mode 100644 crates/ui-egui/tests/it/ui_export_overwrite.rs diff --git a/apps/effectcraft-cli/src/main.rs b/apps/effectcraft-cli/src/main.rs index fd77162ed..2befb5c4b 100644 --- a/apps/effectcraft-cli/src/main.rs +++ b/apps/effectcraft-cli/src/main.rs @@ -726,6 +726,8 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> { } } let out = args.opt("--out").unwrap_or("frame.png").to_string(); + // Never over footage, a proxy or the project file (checked before rendering). + b.check_output(&out)?; let t0 = std::time::Instant::now(); let f = b.render_with(comp.as_ref(), time, max_side, args.flag("--transparent"))?; let ms = t0.elapsed().as_secs_f64() * 1000.0; diff --git a/apps/effectcraft-cli/tests/it/cli.rs b/apps/effectcraft-cli/tests/it/cli.rs index 7a770fa87..12a9bf4a3 100644 --- a/apps/effectcraft-cli/tests/it/cli.rs +++ b/apps/effectcraft-cli/tests/it/cli.rs @@ -419,6 +419,35 @@ fn render_out_is_relative_to_the_working_directory() { let _ = std::fs::remove_dir_all(&root); } +/// #1743: `render-frame --out` writes a PNG to the path it is given, and never over footage or +/// the project file. +#[test] +fn render_frame_never_writes_over_footage_or_the_project() { + let root = tmp("render-frame-overwrite"); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + let (clip, project) = (root.join("clip.png"), root.join("p.ecproj")); + let (c, p) = (clip.to_str().unwrap(), project.to_str().unwrap()); + // A real PNG to import: a frame of the demo project. + ok_json(&["render-frame", "--max-side", "64", "--out", c]); + let imported = json!({"paths": [c]}).to_string(); + ok_json(&["run", "file.import", &imported, "comp.new", r#"{"name":"T","width":64,"height":36,"duration":1}"#, "--empty", "--save-as", p]); + let (footage, saved) = (std::fs::read(&clip).unwrap(), std::fs::read(&project).unwrap()); + for (out, what) in [(c, "footage"), (p, "project")] { + let (code, v) = run_json(&["render-frame", "--project", p, "--out", out]); + assert_eq!(code, 1, "{what}: {v}"); + let error = v["error"].as_str().unwrap(); + assert!(error.contains(out) && error.contains(what), "{what}: {v}"); + } + assert_eq!(std::fs::read(&clip).unwrap(), footage, "the footage is as it was"); + assert_eq!(std::fs::read(&project).unwrap(), saved, "the project file is as it was"); + // Anywhere else it writes. + let free = root.join("frame.png"); + ok_json(&["render-frame", "--project", p, "--out", free.to_str().unwrap()]); + assert_eq!(&std::fs::read(&free).unwrap()[..8], b"\x89PNG\r\n\x1a\n"); + let _ = std::fs::remove_dir_all(&root); +} + /// A real stdio client: each edit's reply is read before disconnecting or killing the server. struct McpChild { child: std::process::Child, diff --git a/crates/automation/src/backend.rs b/crates/automation/src/backend.rs index 5bba7172a..c0dca6c9a 100644 --- a/crates/automation/src/backend.rs +++ b/crates/automation/src/backend.rs @@ -85,6 +85,19 @@ impl Backend { } } + /// Refuse `path`, a file this caller is about to write itself (a frame, a screenshot), when + /// the open project reads it: footage, a proxy or the project file. The project is the + /// session's or the app's, so both are asked through `file.checkOutput`; a relative path is + /// made absolute here, against this process's folder, because the app has its own. + pub fn check_output(&mut self, path: &str) -> Result<()> { + let absolute = std::path::absolute(path).map_err(|e| Error::BadArgs(format!("cannot resolve {path}: {e}")))?; + let found = self.exec("file.checkOutput", json!({"path": absolute.to_string_lossy()}))?; + match found.get("blocked").and_then(Value::as_bool) { + Some(false) => Ok(()), + _ => Err(Error::BadArgs(found.get("reason").and_then(Value::as_str).unwrap_or("this file is one the project reads").to_string())), + } + } + /// Call a raw control-channel method (bridge only). pub fn control(&mut self, method: &str, params: Value) -> Result { match &mut self.target { diff --git a/crates/automation/src/tests.rs b/crates/automation/src/tests.rs index ff7eedef8..5e846dd3a 100644 --- a/crates/automation/src/tests.rs +++ b/crates/automation/src/tests.rs @@ -1615,3 +1615,108 @@ fn blender_export_requires_an_absolute_path_in_a_write_root() { } std::fs::remove_dir_all(&base).unwrap(); } + +/// `render_frame` writes the frame to the `path` it is given, and never over a file the open +/// project reads (#1743): footage, a proxy or the project file. The refusal comes before the +/// frame is rendered, and the file is untouched. +#[test] +fn render_frame_never_writes_over_a_file_the_project_reads() { + use effectcraft_engine::{ + color::Label, + project::{Footage, FootageKind, ItemKind}, + }; + use std::sync::Arc; + let dir = std::env::temp_dir().join(format!("effectcraft-mcp-overwrite-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let (clip, project) = (dir.join("clip.png"), dir.join("Project.ecproj")); + std::fs::write(&clip, b"SOURCE FOOTAGE").unwrap(); + let mut session = Session::default(); + session.execute("comp.new", json!({"width": 16, "height": 16})).unwrap(); + session.execute("file.saveAs", json!({"path": project.to_string_lossy()})).unwrap(); + Arc::make_mut(&mut session.project).add_item( + "clip.png", + Label::Aqua, + None, + ItemKind::Footage(Footage { + path: clip.to_string_lossy().into_owned(), + kind: FootageKind::Still, + width: 16, + height: 16, + has_video: true, + ..Default::default() + }), + ); + let saved = std::fs::read(&project).unwrap(); + let mut s = McpServer::new(Backend::headless(session)); + for (file, what) in [(&clip, "footage"), (&project, "project")] { + let (c, err) = call(&mut s, "render_frame", json!({"path": file.to_string_lossy(), "inline": false})); + let text = c[0]["text"].as_str().unwrap(); + assert!(err && text.contains(&*file.to_string_lossy()) && text.contains(what), "{what}: {text}"); + } + assert_eq!(std::fs::read(&clip).unwrap(), b"SOURCE FOOTAGE", "the footage is as it was"); + assert_eq!(std::fs::read(&project).unwrap(), saved, "the project file is as it was"); + // Anywhere else it writes the PNG. + let free = dir.join("frame.png"); + let (c, err) = call(&mut s, "render_frame", json!({"path": free.to_string_lossy(), "inline": false})); + assert!(!err, "{c:?}"); + assert_eq!(&std::fs::read(&free).unwrap()[..8], b"\x89PNG\r\n\x1a\n"); + let _ = std::fs::remove_dir_all(&dir); +} + +/// The same through the desktop app: the app knows the project's files, so the tool asks it +/// (`file.checkOutput`) before it writes anything. +#[test] +fn bridged_render_frame_asks_the_app_before_it_writes() { + use effectcraft_engine::project::{Footage, FootageKind}; + struct Probe; + impl effectcraft_engine::Importer for Probe { + fn probe(&self, path: &str) -> Result { + Ok(Footage { path: path.to_string(), kind: FootageKind::Still, width: 4, height: 4, has_video: true, ..Default::default() }) + } + } + let dir = std::env::temp_dir().join(format!("effectcraft-mcp-bridge-overwrite-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let clip = dir.join("clip.png"); + std::fs::write(&clip, b"SOURCE FOOTAGE").unwrap(); + + let l = TcpListener::bind("127.0.0.1:0").unwrap(); + let port = l.local_addr().unwrap().port(); + std::thread::spawn(move || { + let (stream, _) = l.accept().unwrap(); + let mut out = stream.try_clone().unwrap(); + let mut session = Session { importer: Some(std::sync::Arc::new(Probe)), ..Default::default() }; + for line in BufReader::new(stream).lines() { + let msg: Value = serde_json::from_str(&line.unwrap()).unwrap(); + let p = &msg["params"]; + let mut reply = match msg["method"].as_str().unwrap() { + "engine.execute" => match session.execute(p["command"].as_str().unwrap(), p["params"].clone()) { + Ok(v) => json!({"ok": true, "result": v}), + Err(e) => json!({"ok": false, "error": e.to_string()}), + }, + "render.frame" => { + let png = crate::encode_png(4, 2, vec![255; 32], 0).unwrap(); + json!({"ok": true, "result": {"comp": 1, "time": 0.5, "width": 4, "height": 2, "png": base64::encode(&png)}}) + } + m => json!({"ok": false, "error": format!("unknown method `{m}`")}), + }; + reply["id"] = msg["id"].clone(); + writeln!(out, "{reply}").unwrap(); + } + }); + let mut s = McpServer::new(Backend::bridge(&port.to_string()).unwrap()); + call_json(&mut s, "execute_command", json!({"command": "comp.new", "params": {"width": 16, "height": 16}})); + call_json(&mut s, "execute_command", json!({"command": "file.import", "params": {"paths": [clip.to_string_lossy()]}})); + + let (c, err) = call(&mut s, "render_frame", json!({"path": clip.to_string_lossy(), "inline": false})); + let text = c[0]["text"].as_str().unwrap(); + assert!(err && text.contains("clip.png") && text.contains("footage"), "{text}"); + assert_eq!(std::fs::read(&clip).unwrap(), b"SOURCE FOOTAGE", "the footage is as it was"); + // Anywhere else it writes the PNG. + let free = dir.join("frame.png"); + let (c, err) = call(&mut s, "render_frame", json!({"path": free.to_string_lossy(), "inline": false})); + assert!(!err, "{c:?}"); + assert_eq!(&std::fs::read(&free).unwrap()[..8], b"\x89PNG\r\n\x1a\n"); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index df7eb133b..aeffa464c 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -406,12 +406,17 @@ fn add_keyframe(b: &mut Backend, a: &Value) -> Result { fn render_frame(b: &mut Backend, a: &Value) -> Result { let max_side = get(a, "max_side").and_then(Value::as_u64).unwrap_or(960) as u32; let transparent = get(a, "transparent").and_then(Value::as_bool).unwrap_or(false); - let f = b.render_with(get(a, "comp"), get(a, "time").and_then(Value::as_f64), max_side, transparent)?; - let mut info = json!({"comp": f.comp, "time": f.time, "width": f.width, "height": f.height}); - if let Some(p) = get(a, "path").and_then(Value::as_str) { + let path = get(a, "path").and_then(Value::as_str); + if let Some(p) = path { if let Some(c) = b.confinement() { c.check(p, crate::confine::Access::Write)?; } + // Before the frame is rendered: never over footage, a proxy or the project file. + b.check_output(p)?; + } + let f = b.render_with(get(a, "comp"), get(a, "time").and_then(Value::as_f64), max_side, transparent)?; + let mut info = json!({"comp": f.comp, "time": f.time, "width": f.width, "height": f.height}); + if let Some(p) = path { std::fs::write(p, &f.png).map_err(|e| Error::Other(format!("cannot write {p}: {e}")))?; info["path"] = json!(p); } diff --git a/crates/engine/src/cmd/animation.rs b/crates/engine/src/cmd/animation.rs index 4808d7538..2c2f23f1a 100644 --- a/crates/engine/src/cmd/animation.rs +++ b/crates/engine/src/cmd/animation.rs @@ -67,6 +67,7 @@ fn earliest_key<'a>(groups: impl IntoIterator) -> Option Result { let path = str_p(p, "path").ok_or_else(|| bad("anim.savePreset", "missing `path`"))?.to_string(); + s.refuse_export(&path)?; let comp = s.active_comp().ok_or(EngineError::NoComp)?; let t = s.time(); let mut effects: Vec<(LayerId, PropGroup)> = selected_effects(s); @@ -106,7 +107,7 @@ fn save_preset(s: &mut Session, p: &Value) -> Result { .unwrap_or_else(|| std::path::Path::new(&path).file_stem().map(|s| s.to_string_lossy().to_string()).unwrap_or_default()); let doc = json!({PRESET_TAG: PRESET_VERSION, "name": name, "properties": props_json, "effects": fx_json}); let text = serde_json::to_string_pretty(&doc).unwrap_or_default(); - s.services.write_file(&path, text.as_bytes()).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(&path, text.as_bytes())?; Ok(json!({"path": path, "properties": props.len(), "effects": effects.len()})) } diff --git a/crates/engine/src/cmd/app_more.rs b/crates/engine/src/cmd/app_more.rs index 29597a3bc..ea4230852 100644 --- a/crates/engine/src/cmd/app_more.rs +++ b/crates/engine/src/cmd/app_more.rs @@ -152,8 +152,9 @@ fn clear_recent_presets(s: &mut Session, _: &Value) -> Result { fn save_copy_xml(s: &mut Session, p: &Value) -> Result { let path = str_p(p, "path").ok_or_else(|| bad("file.saveCopyAsXml", "missing `path` (.ecprojx)"))?; + s.refuse_export(path)?; let xml = crate::xml_project::to_xml(&s.project.to_file_json_with_view(&s.saved_view(), false)?).map_err(EngineError::Other)?; - s.services.write_file(path, xml.as_bytes()).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(path, xml.as_bytes())?; s.toast(format!("Saved an XML copy to {path}")); Ok(json!({"path": path, "bytes": xml.len()})) } diff --git a/crates/engine/src/cmd/comp_more.rs b/crates/engine/src/cmd/comp_more.rs index 449444ed1..8b8340e78 100644 --- a/crates/engine/src/cmd/comp_more.rs +++ b/crates/engine/src/cmd/comp_more.rs @@ -275,6 +275,7 @@ fn save_frame(s: &mut Session, p: &Value) -> Result { return queue_frame(s, p); } let path = str_p(p, "path").ok_or_else(|| bad("comp.saveFrameAs", "missing `path` (.png)"))?.to_string(); + s.refuse_export(&path)?; let cid = comp_id(s, p)?; let comp = s.project.comp(cid).ok_or(EngineError::NoComp)?; let t = f_p(p, "time").map(Tick::from_seconds_f64).unwrap_or(s.time()); @@ -283,7 +284,7 @@ fn save_frame(s: &mut Session, p: &Value) -> Result { let img = s.render(cid, t, effectcraft_render::RenderOpts { scale, backend: effectcraft_render::Backend::Auto, ..Default::default() }); let rgba = if b_p(p, "transparent") == Some(false) { img.to_rgba8_over(bg) } else { img.to_rgba8() }; let png = encode_png(&rgba, img.width, img.height).map_err(|e| EngineError::Other(format!("PNG encoding failed: {e}")))?; - s.services.write_file(&path, &png).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(&path, &png)?; s.toast(format!("Saved frame to {path}")); Ok(json!({"path": path, "width": img.width, "height": img.height, "time": t.seconds()})) } diff --git a/crates/engine/src/cmd/essential.rs b/crates/engine/src/cmd/essential.rs index cb6702660..419bd7316 100644 --- a/crates/engine/src/cmd/essential.rs +++ b/crates/engine/src/cmd/essential.rs @@ -807,6 +807,7 @@ fn export_template(s: &mut Session, p: &Value) -> Result { const C: &str = "essential.exportTemplate"; let cid = eg_comp(s, p)?; let path = str_p(p, "path").ok_or_else(|| bad(C, "missing `path` (.ectemplate)"))?.to_string(); + s.refuse_export(&path)?; let comp = s.project.comp(cid).ok_or(EngineError::NoComp)?.clone(); let eg = comp.essential.clone().filter(|e| !e.controls.is_empty()).ok_or_else(|| bad(C, "the composition has no Essential Graphics controls"))?; let comp_name = s.project.item(cid).map(|i| i.name.clone()).unwrap_or_default(); @@ -868,7 +869,7 @@ fn export_template(s: &mut Session, p: &Value) -> Result { entries.insert(0, ("project.ecproj".into(), sub.to_json().into_bytes())); entries.insert(0, ("manifest.json".into(), serde_json::to_vec_pretty(&manifest).unwrap_or_default())); let bytes = effectcraft_lottie::zip::store(&entries); - s.services.write_file(&path, &bytes).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(&path, &bytes)?; s.toast(format!("Exported template “{name}” to {path}")); Ok(json!({"path": path, "name": name, "bytes": bytes.len(), "controls": eg.flat().len(), "items": deps.len()})) } diff --git a/crates/engine/src/cmd/file_more.rs b/crates/engine/src/cmd/file_more.rs index c5018ae7c..168529c47 100644 --- a/crates/engine/src/cmd/file_more.rs +++ b/crates/engine/src/cmd/file_more.rs @@ -11,7 +11,7 @@ use serde_json::{Value, json}; use super::{CommandSpec, always, b_p, bad, f_p, has_comp, has_project_selection, str_p}; use crate::sequence::SequenceOptions; -use crate::{EngineError, Result, Session, cmd}; +use crate::{EngineError, Result, Session, cmd, query}; fn has_footage_selection(s: &Session) -> std::result::Result<(), String> { if selected_footage(s).is_empty() { Err("select footage in the Project panel".into()) } else { Ok(()) } @@ -78,11 +78,19 @@ fn close_project(s: &mut Session, _: &Value) -> Result { fn save_copy(s: &mut Session, p: &Value) -> Result { let path = str_p(p, "path").ok_or_else(|| bad("file.saveCopy", "missing `path`"))?; + s.refuse_export(path)?; let json = super::file::file_text(s, path)?; - s.services.write_file(path, json.as_bytes()).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(path, json.as_bytes())?; Ok(json!({"path": path, "bytes": json.len()})) } +/// `file.checkOutput {path}`: the commands that write to a path never replace footage, a proxy +/// or the project file; this says beforehand, for callers that write the file themselves. +fn check_output(s: &mut Session, p: &Value) -> Result { + let path = str_p(p, "path").ok_or_else(|| bad("file.checkOutput", "missing `path`"))?; + Ok(s.output_status(path)) +} + fn rate_of(p: &Value) -> FrameRate { f_p(p, "frameRate").map(|f| limits::frame_rate(FrameRate::from_f64(f), FrameRate::FPS_29_97)).unwrap_or(FrameRate::FPS_29_97) } @@ -376,41 +384,6 @@ fn find_missing(s: &mut Session, p: &Value) -> Result { fn collect_files(s: &mut Session, p: &Value) -> Result { let folder = str_p(p, "folder").ok_or_else(|| bad("file.collectFiles", "missing `folder`"))?.trim_end_matches('/').to_string(); - // Collect Files makes its folder (and the footage folder inside it). - #[cfg(not(target_arch = "wasm32"))] - std::fs::create_dir_all(format!("{folder}/(Footage)")).map_err(|e| EngineError::Other(format!("cannot create {folder}: {e}")))?; - let mut proj = (*s.project).clone(); - let mut copied = 0; - let mut errors = vec![]; - let mut used_names = BTreeSet::new(); - for it in proj.items.values_mut() { - let ItemKind::Footage(f) = &mut it.kind else { continue }; - if f.path.is_empty() { - continue; - } - let files: Vec = if f.sequence.is_empty() { vec![f.path.clone()] } else { f.sequence.clone() }; - let mut new_paths = vec![]; - for src in &files { - let mut name = std::path::Path::new(src).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| "footage".into()); - let mut k = 2; - while !used_names.insert(name.clone()) { - name = format!("{k}_{name}"); - k += 1; - } - let dst = format!("{folder}/(Footage)/{name}"); - match s.services.read_file(src).and_then(|bytes| s.services.write_file(&dst, &bytes)) { - Ok(()) => copied += 1, - Err(e) => errors.push(format!("{src}: {e}")), - } - new_paths.push(dst); - } - if f.sequence.is_empty() { - f.path = new_paths.remove(0); - } else { - f.path = new_paths.first().cloned().unwrap_or_default(); - f.sequence = new_paths; - } - } let stem = s .path .as_deref() @@ -418,7 +391,67 @@ fn collect_files(s: &mut Session, p: &Value) -> Result { .map(|n| n.to_string_lossy().to_string()) .unwrap_or_else(|| "Untitled Project".into()); let out = format!("{folder}/{stem}.ecproj"); - s.services.write_file(&out, proj.to_json().as_bytes()).map_err(|e| EngineError::Other(format!("cannot write {out}: {e}")))?; + // Where every copy goes, before anything is made: a copy never lands on a file the project + // reads (other footage, a proxy, the project file itself), and then nothing is written. + let protected = s.protected_files(); + let mut used_names = BTreeSet::new(); + let mut plan: Vec<(ItemId, Vec<(String, String)>)> = vec![]; + for it in s.project.items.values() { + let ItemKind::Footage(f) = &it.kind else { continue }; + if f.path.is_empty() { + continue; + } + let files: Vec<&String> = if f.sequence.is_empty() { vec![&f.path] } else { f.sequence.iter().collect() }; + let mut pairs = vec![]; + for src in files { + let mut name = std::path::Path::new(src).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| "footage".into()); + let mut k = 2; + while !used_names.insert(name.clone()) { + name = format!("{k}_{name}"); + k += 1; + } + let dst = format!("{folder}/(Footage)/{name}"); + // A file that is already where its copy goes stays (it is not copied onto itself). + if !crate::output_guard::same_file(std::path::Path::new(src), std::path::Path::new(&dst)) + && let Some(source) = protected.find(std::path::Path::new(&dst)) + { + return Err(EngineError::Other(format!("Nothing was collected. {}", source.refusal(&dst, crate::output_guard::Use::Export)))); + } + pairs.push((src.clone(), dst)); + } + plan.push((it.id, pairs)); + } + if let Some(source) = protected.find(std::path::Path::new(&out)) { + return Err(EngineError::Other(format!("Nothing was collected. {}", source.refusal(&out, crate::output_guard::Use::Export)))); + } + // Collect Files makes its folder (and the footage folder inside it). + #[cfg(not(target_arch = "wasm32"))] + std::fs::create_dir_all(format!("{folder}/(Footage)")).map_err(|e| EngineError::Other(format!("cannot create {folder}: {e}")))?; + let mut proj = (*s.project).clone(); + let mut copied = 0; + let mut errors = vec![]; + for (id, pairs) in plan { + let mut new_paths = vec![]; + for (src, dst) in &pairs { + if crate::output_guard::same_file(std::path::Path::new(src), std::path::Path::new(dst)) { + copied += 1; + } else { + match s.services.read_file(src).and_then(|bytes| s.write_output_with(&protected, dst, &bytes).map_err(std::io::Error::other)) { + Ok(()) => copied += 1, + Err(e) => errors.push(format!("{src}: {e}")), + } + } + new_paths.push(dst.clone()); + } + let Some(ItemKind::Footage(f)) = proj.items.get_mut(&id).map(|it| &mut it.kind) else { continue }; + if f.sequence.is_empty() { + f.path = new_paths.pop().unwrap_or_default(); + } else { + f.path = new_paths.first().cloned().unwrap_or_default(); + f.sequence = new_paths; + } + } + s.write_output_with(&protected, &out, proj.to_json().as_bytes())?; s.toast(format!("Collected {copied} file(s) into {folder}")); Ok(json!({"project": out, "copied": copied, "errors": errors})) } @@ -1173,6 +1206,12 @@ pub fn specs() -> Vec { cmd!("file.close", "Close", ["File"], Some("Cmd+W"), "{comp?}", has_comp, close), cmd!("file.closeProject", "Close Project", ["File"], None, "{}", always, close_project), cmd!("file.saveCopy", "Save a Copy...", ["File", "Save As"], None, "{path}", always, save_copy), + query!( + "file.checkOutput", + "Check Output File", + "{path} → {path, blocked, source?: footage|proxy|project, reason?}: whether an export may be written to path (it may not replace a file the project reads)", + check_output + ), cmd!("file.importMultiple", "Multiple Files...", ["File", "Import"], Some("Cmd+Alt+I"), "{paths: [string]}", always, |s, p| s .execute("file.import", p.clone())), cmd!( diff --git a/crates/engine/src/cmd/frame_export.rs b/crates/engine/src/cmd/frame_export.rs index baad9b6ef..d8b49159c 100644 --- a/crates/engine/src/cmd/frame_export.rs +++ b/crates/engine/src/cmd/frame_export.rs @@ -195,6 +195,7 @@ pub fn layered_exr(s: &Session, cid: ItemId, t: Tick, scale: f64) -> Result<(Vec fn frame_args(s: &Session, p: &Value, cmd: &str, ext: &str) -> Result<(String, ItemId, Tick, f64)> { let path = str_p(p, "path").ok_or_else(|| bad(cmd, format!("missing `path` (.{ext})")))?.to_string(); + s.refuse_export(&path)?; let cid = comp_id(s, p)?; let t = f_p(p, "time").map(Tick::from_seconds_f64).unwrap_or(s.time()); let scale = f_p(p, "scale").unwrap_or(1.0).clamp(0.01, 4.0); @@ -204,7 +205,7 @@ fn frame_args(s: &Session, p: &Value, cmd: &str, ext: &str) -> Result<(String, I fn save_psd(s: &mut Session, p: &Value) -> Result { let (path, cid, t, scale) = frame_args(s, p, "comp.saveFrameAsPsd", "psd")?; let (bytes, n, w, h) = layered_psd(s, cid, t, scale)?; - s.services.write_file(&path, &bytes).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(&path, &bytes)?; s.toast(format!("Saved {n} layers to {path}")); Ok(json!({"path": path, "layers": n, "width": w, "height": h, "time": t.seconds()})) } @@ -212,7 +213,7 @@ fn save_psd(s: &mut Session, p: &Value) -> Result { fn save_exr(s: &mut Session, p: &Value) -> Result { let (path, cid, t, scale) = frame_args(s, p, "comp.saveFrameAsExr", "exr")?; let (bytes, channels, w, h) = layered_exr(s, cid, t, scale)?; - s.services.write_file(&path, &bytes).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(&path, &bytes)?; s.toast(format!("Saved a multi-layer EXR to {path}")); Ok(json!({"path": path, "channels": channels, "width": w, "height": h, "time": t.seconds()})) } diff --git a/crates/engine/src/cmd/lottie.rs b/crates/engine/src/cmd/lottie.rs index 03a99b994..288d2e56e 100644 --- a/crates/engine/src/cmd/lottie.rs +++ b/crates/engine/src/cmd/lottie.rs @@ -8,6 +8,7 @@ use crate::{EngineError, Result, Session, cmd}; fn export_lottie(s: &mut Session, p: &Value) -> Result { let path = str_p(p, "path").ok_or_else(|| bad("file.exportLottie", "missing `path`"))?.to_string(); + s.refuse_export(&path)?; let cid = comp_id(s, p)?; let opts = ExportOptions { include_expressions: b_p(p, "includeExpressions").unwrap_or(false), text_as_shapes: b_p(p, "textAsShapes").unwrap_or(false) }; let services = s.services.clone(); @@ -20,7 +21,7 @@ fn export_lottie(s: &mut Session, p: &Value) -> Result { } else { res.to_string_compact().into_bytes() }; - s.services.write_file(&path, &bytes).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(&path, &bytes)?; let n = res.warnings.len(); s.toast(if n == 0 { format!("Exported Lottie to {path}") } else { format!("Exported Lottie to {path} ({n} warning{})", if n == 1 { "" } else { "s" }) }); Ok(json!({"path": path, "bytes": bytes.len(), "warnings": res.warnings})) diff --git a/crates/engine/src/cmd/settings.rs b/crates/engine/src/cmd/settings.rs index 272903c69..3673a9550 100644 --- a/crates/engine/src/cmd/settings.rs +++ b/crates/engine/src/cmd/settings.rs @@ -195,7 +195,7 @@ fn sc_export(s: &mut Session, p: &Value) -> Result { let doc = s.keymaps.export(str_p(p, "preset")).map_err(|e| bad("shortcuts.export", e))?; if let Some(path) = str_p(p, "path") { let text = serde_json::to_string_pretty(&doc).unwrap_or_default(); - s.services.write_file(path, text.as_bytes()).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(path, text.as_bytes())?; return Ok(json!({"path": path, "preset": doc["name"]})); } Ok(doc) diff --git a/crates/engine/src/cmd/timeline.rs b/crates/engine/src/cmd/timeline.rs index ae57a3b88..f607a36dc 100644 --- a/crates/engine/src/cmd/timeline.rs +++ b/crates/engine/src/cmd/timeline.rs @@ -99,6 +99,7 @@ fn isolate(project: &Project, cid: ItemId, lid: LayerId) -> Option { /// What a pre-render needs from the session, so it can run on a background thread. struct PrerenderCx { + protected: crate::output_guard::Protected, exporter: Option>, footage: Arc, expr: Option>, @@ -157,6 +158,7 @@ fn export_timeline(s: &mut Session, p: &Value) -> Result { } path = format!("{path}.{}", format.extension()); } + s.refuse_export(&path)?; let cid = comp_id(s, p)?; let prerender_mode = match str_p(p, "prerender") { None => match p.get("prerender").and_then(Value::as_bool) { @@ -178,6 +180,7 @@ fn export_timeline(s: &mut Session, p: &Value) -> Result { let project = s.project.clone(); let services = s.services.clone(); let cx = PrerenderCx { + protected: crate::output_guard::Protected::of(s), exporter: s.exporter.clone(), footage: s.footage.clone(), expr: s.expr.clone(), @@ -189,6 +192,9 @@ fn export_timeline(s: &mut Session, p: &Value) -> Result { let wait = !b_p(p, "background").unwrap_or(false); s.spawn_task("exportTimeline", format!("Export {}", format.label()), wait, move |ctl| { let (bytes, mut result) = export_timeline_work(&cx, &project, cid, &opts, &dir, &stem, ctl)?; + if let Some(source) = cx.protected.find(std::path::Path::new(&path)) { + return Err(source.refusal(&path, crate::output_guard::Use::Export)); + } services.write_file(&path, &bytes).map_err(|e| format!("cannot write {path}: {e}"))?; result["path"] = json!(path); result["toast"] = json!(match format { @@ -220,6 +226,11 @@ fn export_timeline_work( let layer_name = project.comp(c).and_then(|cc| cc.layer(l)).map(|x| x.name.clone()).unwrap_or_default(); ctl.message(format!("Pre-render {layer_name}")); let file = dir.join(format!("{stem}_{}_{}_{}.mov", safe(&comp_name), safe(&layer_name), l.0)).to_string_lossy().to_string(); + // The movies are outputs too: never over footage, a proxy or the project file. + if let Some(source) = cx.protected.find(std::path::Path::new(&file)) { + errors.push(format!("{comp_name} ▸ {layer_name}: {}", source.refusal(&file, crate::output_guard::Use::Export))); + continue; + } // Progress over all the layers, each one's frames a share of it. let mut progress = |done: u64, total: u64| ctl.progress(i * 1000 + done.saturating_mul(1000) / total.max(1), n * 1000); match prerender(cx, project, c, l, &file, &mut progress) { diff --git a/crates/engine/src/cmd/view.rs b/crates/engine/src/cmd/view.rs index a36c53933..115da8667 100644 --- a/crates/engine/src/cmd/view.rs +++ b/crates/engine/src/cmd/view.rs @@ -83,10 +83,11 @@ fn clear_guides(s: &mut Session, p: &Value) -> Result { fn export_guides(s: &mut Session, p: &Value) -> Result { let path = str_p(p, "path").ok_or_else(|| bad("view.exportGuides", "missing `path`"))?; + s.refuse_export(path)?; let comp = s.project.comp(comp_id(s, p)?).ok_or(EngineError::NoComp)?; let doc = json!({"effectcraftGuides": 1, "width": comp.width, "height": comp.height, "guides": comp.guides}); let text = serde_json::to_string_pretty(&doc).unwrap_or_default(); - s.services.write_file(path, text.as_bytes()).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(path, text.as_bytes())?; Ok(json!({"path": path, "guides": comp.guides.len()})) } diff --git a/crates/engine/src/output_guard.rs b/crates/engine/src/output_guard.rs index 5d4dc289e..40dcc1e56 100644 --- a/crates/engine/src/output_guard.rs +++ b/crates/engine/src/output_guard.rs @@ -5,6 +5,11 @@ //! exporter. Existing aliases are compared by native file identity, including hard links. //! Missing sources remain protected by their normalized path. Checks happen before dispatch; //! filesystem changes between the check and the write require exporter-side revalidation. +//! +//! The commands that write to a path they are given (Save Frame As, Export Lottie, Save a Copy, +//! Collect Files, Export Timeline…) never replace one of those files either, with no flag: +//! they write through [`Session::write_output`], the one place that checks. Save and Save As +//! write the project's own file and do not. use std::collections::{HashMap, HashSet}; use std::path::{Component, Path, PathBuf}; @@ -12,6 +17,7 @@ use std::sync::Arc; use effectcraft_project::render_queue::{RenderQueueItem, log_path, numbered_template, sequence_frame, sequence_path}; use effectcraft_project::{Footage, ItemKind}; +use serde_json::{Value, json}; use crate::Session; @@ -26,6 +32,15 @@ const PROBE_LIMIT: u64 = 100_000; /// render time. const MAX_NUMBER: u32 = 1_000; +/// What a file is written for, which words a refusal. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Use { + /// A Render Queue output (or a preview render): the fix is Output To. + Render, + /// Anything else written to a path: a frame, an export, a copy. + Export, +} + /// What reads a file the project depends on. #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum Source { @@ -38,12 +53,28 @@ pub(crate) enum Source { } impl Source { - /// Why `path`, a file this reads, cannot be a render's output. - pub(crate) fn refusal(&self, path: &str) -> String { + /// `footage`, `proxy` or `project`: the `source` of a blocked output. + pub(crate) fn kind(&self) -> &'static str { match self { - Source::Footage(name) => format!("{path} is the footage “{name}” of this project, and a render never replaces footage; choose another Output To"), - Source::Proxy(name) => format!("{path} is the proxy of “{name}”, and a render never replaces a proxy; choose another Output To"), - Source::Project => format!("{path} is this project's own file, and a render never replaces it; choose another Output To"), + Source::Footage(_) => "footage", + Source::Proxy(_) => "proxy", + Source::Project => "project", + } + } + + /// Why `path`, a file this reads, cannot be written for `used`. + pub(crate) fn refusal(&self, path: &str, used: Use) -> String { + match (used, self) { + (Use::Render, Source::Footage(name)) => { + format!("{path} is the footage “{name}” of this project, and a render never replaces footage; choose another Output To") + } + (Use::Render, Source::Proxy(name)) => format!("{path} is the proxy of “{name}”, and a render never replaces a proxy; choose another Output To"), + (Use::Render, Source::Project) => format!("{path} is this project's own file, and a render never replaces it; choose another Output To"), + (Use::Export, Source::Footage(name)) => { + format!("{path} is the footage “{name}” of this project, and footage is never written over; choose another file") + } + (Use::Export, Source::Proxy(name)) => format!("{path} is the proxy of “{name}”, and a proxy is never written over; choose another file"), + (Use::Export, Source::Project) => format!("{path} is this project's own file, which only Save writes; choose another file"), } } } @@ -178,8 +209,8 @@ fn fold(name: &str) -> String { if IGNORES_CASE { name.to_lowercase() } else { name.to_string() } } -#[cfg(test)] -fn same_file(a: &Path, b: &Path) -> bool { +/// Whether two paths are one file: the same name once resolved, or the same file on disk. +pub(crate) fn same_file(a: &Path, b: &Path) -> bool { if key(a) == key(b) { return true; } @@ -241,6 +272,50 @@ impl Session { Protected::of(self) } + /// What reads the file at `path`, if the project does. + fn blocking(&self, path: &str) -> Option { + if cfg!(target_arch = "wasm32") { + return None; + } + self.protected_files().find(Path::new(path)).cloned() + } + + /// `Err` when `path` is a file the project reads (footage, a proxy, the project file) and so + /// is not for an export to replace. For callers that write the file themselves; call it + /// before the work, not after it. + pub fn refuse_export(&self, path: &str) -> crate::Result<()> { + match self.blocking(path) { + Some(source) => Err(crate::EngineError::Other(source.refusal(path, Use::Export))), + None => Ok(()), + } + } + + /// Write `bytes` to `path` through the host's services, unless the project reads the file + /// there ([`Session::refuse_export`]). Every command that writes to a path it is given + /// writes through this; Save and Save As (the project's own file) do not. + pub fn write_output(&self, path: &str, bytes: &[u8]) -> crate::Result<()> { + self.write_output_with(&self.protected_files(), path, bytes) + } + + /// [`Session::write_output`] against files already found ([`Session::protected_files`]), + /// for a command that writes many. + pub(crate) fn write_output_with(&self, protected: &Protected, path: &str, bytes: &[u8]) -> crate::Result<()> { + if !cfg!(target_arch = "wasm32") + && let Some(source) = protected.find(Path::new(path)) + { + return Err(crate::EngineError::Other(source.refusal(path, Use::Export))); + } + self.services.write_file(path, bytes).map_err(|e| crate::EngineError::Other(format!("cannot write {path}: {e}"))) + } + + /// `file.checkOutput`'s reply for `path`. + pub(crate) fn output_status(&self, path: &str) -> Value { + match self.blocking(path) { + Some(source) => json!({"path": path, "blocked": true, "source": source.kind(), "reason": source.refusal(path, Use::Export)}), + None => json!({"path": path, "blocked": false}), + } + } + /// Reject protected outputs and ranges that cannot be checked completely. Ordinary-file /// replacement consent is left to the exporter, independently of this source guard. pub(crate) fn check_outputs(&self, items: &[Vec<(RenderQueueItem, String)>]) -> Vec { @@ -287,7 +362,7 @@ impl Session { .map(PathBuf::as_path) .chain(movie) .chain(Some(Path::new(&log))) - .find_map(|path| protected.find(path).map(|source| source.refusal(&path.to_string_lossy())))) + .find_map(|path| protected.find(path).map(|source| source.refusal(&path.to_string_lossy(), Use::Render)))) } /// A new item's default output must not be a file the project reads: `[compName]_1`, diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index aee5e8550..e6ab33bd6 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -10,6 +10,7 @@ mod disk_cache; mod effects; mod essential; mod essential_more; +mod export_overwrite; mod face; mod fidelity; mod frame_export; diff --git a/crates/engine/src/tests/export_overwrite.rs b/crates/engine/src/tests/export_overwrite.rs new file mode 100644 index 000000000..c27febd2b --- /dev/null +++ b/crates/engine/src/tests/export_overwrite.rs @@ -0,0 +1,271 @@ +//! The commands that write to a path they are given leave a file the project reads alone (#1743, +//! beyond the Render Queue): footage, a proxy, a frame of an imported image sequence and the +//! project file are refused with nothing changed, and any other path is written. Save and Save As +//! write the project's own file and are not among them. + +use std::sync::Arc; + +use effectcraft_project::{Footage, FootageKind, ItemId, Proxy}; +use serde_json::{Value, json}; + +use super::render_overwrite::{Fixture, SOURCE}; + +/// Footage that is a file named `path`. +fn video(path: String) -> Footage { + Footage { path, kind: FootageKind::Video, has_video: true, ..Default::default() } +} + +/// `item` gets the file `file` of the project's folder as its proxy. +fn give_proxy(f: &mut Fixture, item: ItemId, file: &str) { + f.write(file, SOURCE); + let footage = video(f.text(file)); + Arc::make_mut(&mut f.s.project).item_mut(item).unwrap().proxy = Some(Box::new(Proxy { footage, enabled: true })); +} + +/// A project, saved as `Project.ecproj`, that reads `clip.` (footage) with `proxy.` as +/// its proxy and the frames `shot_00000.` to `shot_00004.` of an image sequence, all +/// holding `SOURCE`. Its composition `Main` holds a red solid. +fn reading(name: &str, ext: &str) -> Fixture { + let mut f = Fixture::new(name); + let clip = f.footage(&format!("clip.{ext}")); + give_proxy(&mut f, clip, &format!("proxy.{ext}")); + f.sequence_as("shot", ext, 0..5); + f.comp("Main"); + f.s.execute("layer.newSolid", json!({"name": "Red", "color": "#ff0000"})).unwrap(); + f +} + +/// The names in `dir`, sorted. +fn listing(dir: &std::path::Path) -> Vec { + let mut names: Vec = std::fs::read_dir(dir).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().into_owned()).collect(); + names.sort(); + names +} + +/// A command that writes to a path. +struct Case { + command: &'static str, + /// The extension of a path for it. + ext: &'static str, + /// What it needs beyond a composition holding a solid. + setup: fn(&mut Fixture), + /// Its parameters for `path`. + params: fn(&str) -> Value, +} + +fn nothing(_: &mut Fixture) {} + +/// The first layer of the active composition. +fn first_layer(f: &Fixture) -> u64 { + f.s.active_comp().unwrap().layers[0].id.0 +} + +/// The composition shows one control, for `essential.exportTemplate`. +fn expose_opacity(f: &mut Fixture) { + let layer = first_layer(f); + f.s.execute("essential.addProperty", json!({"layer": layer, "path": "transform/opacity"})).unwrap(); +} + +/// A property is selected, for `anim.savePreset`. +fn select_opacity(f: &mut Fixture) { + let layer = first_layer(f); + f.s.execute("prop.select", json!({"layer": layer, "path": "transform/opacity"})).unwrap(); +} + +/// The composition has a guide, for `view.exportGuides`. +fn add_guide(f: &mut Fixture) { + f.s.execute("view.addGuide", json!({"orientation": "vertical"})).unwrap(); +} + +/// Run `case` on every file the project reads: each is refused, the project's folder is as it was +/// and nothing was rendered; then on a free path, where it writes. +fn never_replaces_what_the_project_reads(case: &Case) { + let ext = case.ext; + let mut f = reading(&format!("{}-{ext}", case.command), ext); + (case.setup)(&mut f); + let mut reads = vec![(format!("clip.{ext}"), "footage"), (format!("proxy.{ext}"), "proxy"), (format!("shot_00002.{ext}"), "footage")]; + // (A timeline export adds its extension to `Project.ecproj`: it has a test of its own.) + if case.command != "file.exportTimeline" { + reads.push(("Project.ecproj".to_string(), "project")); + } + let before: Vec> = reads.iter().map(|(file, _)| f.read(file)).collect(); + let listed = listing(&f.dir); + for ((file, source), bytes) in reads.iter().zip(&before) { + let path = f.text(file); + let e = f.s.execute(case.command, (case.params)(&path)).expect_err(&format!("{} wrote over {file}", case.command)).to_string(); + assert!(e.contains(file.as_str()) && e.contains(source), "{}: {file}: {e}", case.command); + assert_eq!(&f.read(file), bytes, "{}: {file} is as it was", case.command); + } + assert_eq!(listing(&f.dir), listed, "{}: refusing changed nothing in the folder", case.command); + assert!(f.exported().is_empty(), "{}: nothing was rendered for it", case.command); + + let free = format!("out.{ext}"); + f.s.execute(case.command, (case.params)(&f.text(&free))).unwrap_or_else(|e| panic!("{}: {e}", case.command)); + assert_ne!(f.read(&free), SOURCE, "{}: wrote its file", case.command); + for ((file, _), bytes) in reads.iter().zip(&before) { + assert_eq!(&f.read(file), bytes, "{}: {file} is still as it was", case.command); + } +} + +macro_rules! never_replaces { + ($name:ident, $command:literal, $ext:literal, $setup:expr) => { + #[test] + fn $name() { + never_replaces_what_the_project_reads(&Case { command: $command, ext: $ext, setup: $setup, params: |path| json!({"path": path}) }); + } + }; +} + +never_replaces!(save_frame_as_png, "comp.saveFrameAs", "png", nothing); +never_replaces!(save_frame_as_photoshop_layers, "comp.saveFrameAsPsd", "psd", nothing); +never_replaces!(save_frame_as_proexr, "comp.saveFrameAsExr", "exr", nothing); +never_replaces!(export_lottie_json, "file.exportLottie", "json", nothing); +never_replaces!(export_lottie_dotlottie, "file.exportLottie", "lottie", nothing); +never_replaces!(save_a_copy, "file.saveCopy", "ecproj", nothing); +never_replaces!(save_a_copy_as_xml, "file.saveCopyAsXml", "ecprojx", nothing); +never_replaces!(export_timeline, "file.exportTimeline", "xml", nothing); +never_replaces!(export_template, "essential.exportTemplate", "ectemplate", expose_opacity); +never_replaces!(save_animation_preset, "anim.savePreset", "ecpreset", select_opacity); +never_replaces!(export_guides, "view.exportGuides", "json", add_guide); +never_replaces!(export_shortcuts, "shortcuts.export", "json", nothing); + +// ---------------------------------------------------------------- Collect Files + +/// A project reading the footage `in/`, with an empty `out/(Footage)` where Collect Files +/// would put its copy. +fn collecting(name: &str, file: &str) -> Fixture { + let mut f = Fixture::new(name); + std::fs::create_dir_all(f.path("in")).unwrap(); + std::fs::create_dir_all(f.path("out/(Footage)")).unwrap(); + f.footage(&format!("in/{file}")); + f +} + +/// Collect Files into the project folder's `out`. +fn collect(f: &mut Fixture) -> crate::Result { + let folder = f.text("out"); + f.s.execute("file.collectFiles", json!({"folder": folder})) +} + +/// `there`, the place the copy of the footage `in/` would go, is something the project +/// reads (`source`): Collect Files is refused before it copies or writes anything. +fn collect_is_refused(mut f: Fixture, original: &str, there: &str, source: &str) { + f.write(there, b"NOT TO BE REPLACED"); + let listed = listing(&f.path("out/(Footage)")); + let e = collect(&mut f).unwrap_err().to_string(); + assert!(e.contains(original) && e.contains(source), "{e}"); + assert_eq!(f.read(there), b"NOT TO BE REPLACED", "the file the project reads is as it was"); + assert_eq!(f.read(&format!("in/{original}")), SOURCE, "the original is as it was"); + assert!(!f.path("out/Project.ecproj").exists(), "the collected project was not written"); + assert_eq!(listing(&f.path("out/(Footage)")), listed, "nothing else was copied"); +} + +#[test] +fn collect_files_does_not_copy_over_other_footage() { + let mut f = collecting("collect-footage", "a.mov"); + f.footage("out/(Footage)/a.mov"); + collect_is_refused(f, "a.mov", "out/(Footage)/a.mov", "footage"); +} + +#[test] +fn collect_files_does_not_copy_over_a_proxy() { + let mut f = collecting("collect-proxy", "a.mov"); + let a = f.s.project.items.keys().next().copied().unwrap(); + give_proxy(&mut f, a, "out/(Footage)/a.mov"); + collect_is_refused(f, "a.mov", "out/(Footage)/a.mov", "proxy"); +} + +#[test] +fn collect_files_does_not_copy_over_a_sequence_frame() { + let mut f = collecting("collect-frame", "shot_00002.png"); + f.sequence_as("out/(Footage)/shot", "png", 0..5); + collect_is_refused(f, "shot_00002.png", "out/(Footage)/shot_00002.png", "footage"); +} + +/// Collecting into the project's own folder would write the collected copy over the project. +#[test] +fn collect_files_does_not_write_over_the_project_file() { + let mut f = collecting("collect-project", "a.mov"); + let project = f.read("Project.ecproj"); + let folder = f.dir.to_string_lossy().into_owned(); + let e = f.s.execute("file.collectFiles", json!({"folder": folder})).unwrap_err().to_string(); + assert!(e.contains("Project.ecproj") && e.contains("project"), "{e}"); + assert_eq!(f.read("Project.ecproj"), project, "the project file is as it was"); + assert!(!f.path("(Footage)").exists(), "no folder was made, nothing was copied"); +} + +#[test] +fn collect_files_copies_into_a_free_folder() { + let mut f = collecting("collect-free", "a.mov"); + let r = collect(&mut f).unwrap(); + assert_eq!((r["copied"].as_u64(), &r["errors"]), (Some(1), &json!([])), "{r}"); + assert_eq!(f.read("out/(Footage)/a.mov"), SOURCE); + assert_eq!(f.read("in/a.mov"), SOURCE, "the original stays"); + assert!(f.path("out/Project.ecproj").exists()); +} + +/// Footage that is where its copy would go is already collected: it is not copied onto itself. +#[test] +fn collect_files_leaves_footage_that_is_already_in_the_folder() { + let mut f = collecting("collect-in-place", "a.mov"); + f.footage("out/(Footage)/c.mov"); + f.write("out/(Footage)/c.mov", b"ALREADY COLLECTED"); + let r = collect(&mut f).unwrap(); + assert_eq!((r["copied"].as_u64(), &r["errors"]), (Some(2), &json!([])), "{r}"); + assert_eq!(f.read("out/(Footage)/c.mov"), b"ALREADY COLLECTED"); + assert_eq!(f.read("out/(Footage)/a.mov"), SOURCE); +} + +// ---------------------------------------------------------------- the rest + +/// The movies a timeline export renders beside it are written like any other output. +#[test] +fn a_timeline_export_does_not_pre_render_over_footage() { + let mut f = Fixture::new("timeline-prerender"); + f.comp("Main"); + f.s.execute("layer.newSolid", json!({"name": "Red", "color": "#ff0000"})).unwrap(); + // OpenTimelineIO has no solids: the solid is rendered to a movie beside the document. + let params = json!({"path": f.text("Main.otio"), "prerender": "unsupported"}); + let r = f.s.execute("file.exportTimeline", params.clone()).unwrap(); + let movies = f.exported(); + assert_eq!(movies.len(), 1, "{r}"); + assert_eq!(r["prerendered"].as_array().map(Vec::len), Some(1), "{r}"); + // That movie is footage now. + let movie = std::path::Path::new(&movies[0]).file_name().unwrap().to_string_lossy().into_owned(); + f.footage(&movie); + let r = f.s.execute("file.exportTimeline", params).unwrap(); + assert_eq!(f.read(&movie), SOURCE, "the footage is as it was"); + assert_eq!(f.exported().len(), 1, "no second render was started"); + assert_eq!(r["prerendered"], json!([]), "{r}"); + let warnings = r["warnings"].as_array().unwrap().iter().filter_map(Value::as_str).collect::>().join("\n"); + assert!(warnings.contains(&movie) && warnings.contains("footage"), "the export says why: {r}"); +} + +/// `file.checkOutput` answers before anything is written, for callers that write the file +/// themselves (the CLI, MCP's `render_frame`). +#[test] +fn check_output_names_the_files_an_export_may_not_use() { + let mut f = reading("check-output", "png"); + for (file, source) in [("clip.png", "footage"), ("proxy.png", "proxy"), ("shot_00003.png", "footage"), ("Project.ecproj", "project")] { + let r = f.s.execute("file.checkOutput", json!({"path": f.text(file)})).unwrap(); + assert_eq!((r["blocked"].as_bool(), r["source"].as_str()), (Some(true), Some(source)), "{file}: {r}"); + assert!(r["reason"].as_str().unwrap().contains(file), "{file}: {r}"); + assert_eq!(r["path"], json!(f.text(file)), "{r}"); + } + let r = f.s.execute("file.checkOutput", json!({"path": f.text("free.png")})).unwrap(); + assert_eq!((r["blocked"].as_bool(), r["source"].is_null()), (Some(false), true), "{r}"); + assert!(f.s.execute("file.checkOutput", json!({})).is_err(), "a path is needed"); + assert!(!f.path("free.png").exists(), "it only looks"); +} + +/// A project saved under a timeline's name is not overwritten by the timeline. +#[test] +fn a_timeline_export_does_not_write_over_the_project_file() { + let mut f = Fixture::new("timeline-project"); + f.comp("Main"); + f.s.execute("file.saveAs", json!({"path": f.text("Edit.xml")})).unwrap(); + let saved = f.read("Edit.xml"); + let e = f.s.execute("file.exportTimeline", json!({"path": f.text("Edit.xml")})).unwrap_err().to_string(); + assert!(e.contains("Edit.xml") && e.contains("project"), "{e}"); + assert_eq!(f.read("Edit.xml"), saved); +} diff --git a/crates/engine/src/tests/render_overwrite.rs b/crates/engine/src/tests/render_overwrite.rs index 420763da2..93c2ec45c 100644 --- a/crates/engine/src/tests/render_overwrite.rs +++ b/crates/engine/src/tests/render_overwrite.rs @@ -12,7 +12,7 @@ use serde_json::{Value, json}; use crate::{ExportJob, ExportResult, Exporter, Session}; -const SOURCE: &[u8] = b"SOURCE FOOTAGE"; +pub(crate) const SOURCE: &[u8] = b"SOURCE FOOTAGE"; const EARLIER: &[u8] = b"AN EARLIER RENDER"; const RENDERED: &[u8] = b"RENDERED"; @@ -51,14 +51,14 @@ impl Exporter for Writer { /// A session whose project is saved in a fresh folder (the footage goes there too), with an /// exporter that writes real files. -struct Fixture { - s: Session, +pub(crate) struct Fixture { + pub(crate) s: Session, writer: Arc, - dir: PathBuf, + pub(crate) dir: PathBuf, } impl Fixture { - fn new(name: &str) -> Fixture { + pub(crate) fn new(name: &str) -> Fixture { let dir = std::env::temp_dir().join("effectcraft-render-overwrite").join(format!("{}-{name}", std::process::id())); let _ = std::fs::remove_dir_all(&dir); std::fs::create_dir_all(&dir).unwrap(); @@ -69,24 +69,24 @@ impl Fixture { Fixture { s, writer, dir } } - fn path(&self, file: &str) -> PathBuf { + pub(crate) fn path(&self, file: &str) -> PathBuf { self.dir.join(file) } - fn text(&self, file: &str) -> String { + pub(crate) fn text(&self, file: &str) -> String { self.path(file).to_string_lossy().into_owned() } - fn read(&self, file: &str) -> Vec { + pub(crate) fn read(&self, file: &str) -> Vec { std::fs::read(self.path(file)).unwrap() } - fn write(&self, file: &str, bytes: &[u8]) { + pub(crate) fn write(&self, file: &str, bytes: &[u8]) { std::fs::write(self.path(file), bytes).unwrap(); } /// A video file in the project's folder, imported. - fn footage(&mut self, file: &str) -> ItemId { + pub(crate) fn footage(&mut self, file: &str) -> ItemId { self.write(file, SOURCE); let f = Footage { path: self.text(file), @@ -102,9 +102,14 @@ impl Fixture { /// Numbered stills in the project's folder, imported as one image sequence. fn sequence(&mut self, name: &str, frames: std::ops::Range) -> ItemId { + self.sequence_as(name, "png", frames) + } + + /// [`Fixture::sequence`] of files with extension `ext`. + pub(crate) fn sequence_as(&mut self, name: &str, ext: &str, frames: std::ops::Range) -> ItemId { let files: Vec = frames .map(|n| { - let file = format!("{name}_{n:05}.png"); + let file = format!("{name}_{n:05}.{ext}"); self.write(&file, SOURCE); self.text(&file) }) @@ -119,11 +124,11 @@ impl Fixture { duration: Tick::from_seconds_f64(0.5), ..Default::default() }; - Arc::make_mut(&mut self.s.project).add_item(&format!("{name}_[00000-00004].png"), Label::None, None, ItemKind::Footage(f)) + Arc::make_mut(&mut self.s.project).add_item(&format!("{name}_[00000-00004].{ext}"), Label::None, None, ItemKind::Footage(f)) } /// A five-frame composition. - fn comp(&mut self, name: &str) { + pub(crate) fn comp(&mut self, name: &str) { self.s.execute("comp.new", json!({"name": name, "width": 16, "height": 16, "frameRate": 10, "duration": 0.5})).unwrap(); } @@ -133,7 +138,7 @@ impl Fixture { } /// The paths the exporter was started for. - fn exported(&self) -> Vec { + pub(crate) fn exported(&self) -> Vec { self.writer.jobs.lock().unwrap().clone() } diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index f67591bee..023bfa8b3 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -619,6 +619,12 @@ fn playback(app: &mut EffectcraftApp, p: &Value, now: f64) -> Outcome { /// when the capture is issued. fn screenshot(app: &mut EffectcraftApp, ctx: &egui::Context, p: &Value) -> Outcome { let s = |k: &str| p.get(k).and_then(Value::as_str); + // A file the project reads is never the output. + if let Some(path) = s("path") + && let Err(e) = app.session.refuse_export(path) + { + return err(e); + } // The panel/element is an identity, not a rectangle: it is resolved when the capture // is issued (after the UI settles), so a panel that moves meanwhile is not cropped // with its old rectangle. @@ -672,6 +678,12 @@ fn quit(app: &mut EffectcraftApp, ctx: &egui::Context, p: &Value) -> Outcome { /// temp file) or inline as base64 when `base64: true`. fn render_frame(app: &EffectcraftApp, p: &Value) -> Outcome { let s = &app.session; + // A file the project reads is never the output (checked before rendering). + if let Some(path) = p.get("path").and_then(Value::as_str) + && let Err(e) = s.refuse_export(path) + { + return err(e); + } let cid = match s.resolve_comp(p.get("comp")) { Ok(c) => c, Err(e) => return err(e), diff --git a/crates/ui-egui/tests/it/main.rs b/crates/ui-egui/tests/it/main.rs index 180b56232..47758810b 100644 --- a/crates/ui-egui/tests/it/main.rs +++ b/crates/ui-egui/tests/it/main.rs @@ -26,6 +26,7 @@ mod ui_effect_controls; mod ui_effect_drop_highlight; mod ui_effect_names; mod ui_env_light_gpu; +mod ui_export_overwrite; mod ui_expression_values; mod ui_font_search; mod ui_form_fit; diff --git a/crates/ui-egui/tests/it/ui_export_overwrite.rs b/crates/ui-egui/tests/it/ui_export_overwrite.rs new file mode 100644 index 000000000..e14d9a7e8 --- /dev/null +++ b/crates/ui-egui/tests/it/ui_export_overwrite.rs @@ -0,0 +1,107 @@ +//! The control channel's `render.frame` and `ui.screenshot` write a PNG to the `path` they are +//! given, and never over a file the project reads (#1743): footage, a proxy or the project file. + +use std::path::PathBuf; + +use effectcraft_engine::Session; +use effectcraft_engine::color::Label; +use effectcraft_engine::project::{Footage, FootageKind, ItemKind}; +use effectcraft_ui_egui::EffectcraftApp; +use effectcraft_ui_egui::control::{self, ControlRequest, Outcome}; +use egui_kittest::Harness; +use serde_json::{Value, json}; + +const SOURCE: &[u8] = b"SOURCE FOOTAGE"; + +struct Rig { + h: Harness<'static, EffectcraftApp>, + dir: PathBuf, +} + +impl Rig { + /// A project saved as `Project.ecproj` in a fresh folder, reading `clip.png` beside it. + fn new(name: &str) -> Rig { + let dir = std::env::temp_dir().join("effectcraft-ui-export-overwrite").join(format!("{}-{name}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("clip.png"), SOURCE).unwrap(); + let mut s = Session::default(); + s.execute("comp.new", json!({"name": "Main", "width": 16, "height": 16, "frameRate": 10, "duration": 0.5})).unwrap(); + s.execute("file.saveAs", json!({"path": dir.join("Project.ecproj").to_string_lossy()})).unwrap(); + let clip = Footage { + path: dir.join("clip.png").to_string_lossy().into_owned(), + kind: FootageKind::Still, + width: 16, + height: 16, + has_video: true, + ..Default::default() + }; + std::sync::Arc::make_mut(&mut s.project).add_item("clip.png", Label::Aqua, None, ItemKind::Footage(clip)); + let mut app = Some(EffectcraftApp::new(s)); + let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).build_eframe(|_| app.take().expect("app")); + h.run_steps(2); + Rig { h, dir } + } + + fn text(&self, file: &str) -> String { + self.dir.join(file).to_string_lossy().into_owned() + } + + fn read(&self, file: &str) -> Vec { + std::fs::read(self.dir.join(file)).unwrap() + } + + /// A control-channel request, answered at once or not. + fn send(&mut self, method: &str, params: Value) -> Outcome { + let ctx = self.h.ctx.clone(); + let (req, _rx) = ControlRequest::new(method, params); + let now = self.h.state().now(); + control::handle(self.h.state_mut(), &ctx, &req, now) + } + + /// The reply of a request that is answered at once. + fn reply(&mut self, method: &str, params: Value) -> Value { + match self.send(method, params) { + Outcome::Done(v) => v, + _ => panic!("{method}: no immediate reply"), + } + } +} + +impl Drop for Rig { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.dir); + } +} + +#[test] +fn render_frame_never_writes_over_a_file_the_project_reads() { + let mut r = Rig::new("render-frame"); + let project = r.read("Project.ecproj"); + for (file, what) in [("clip.png", "footage"), ("Project.ecproj", "project")] { + let v = r.reply("render.frame", json!({"path": r.text(file)})); + let error = v["error"].as_str().unwrap_or_default(); + assert!(v["ok"] == false && error.contains(file) && error.contains(what), "{file}: {v}"); + } + assert_eq!(r.read("clip.png"), SOURCE, "the footage is as it was"); + assert_eq!(r.read("Project.ecproj"), project, "the project file is as it was"); + // Anywhere else it writes the PNG. + let v = r.reply("render.frame", json!({"path": r.text("frame.png")})); + assert_eq!(v["ok"], true, "{v}"); + assert_eq!(&r.read("frame.png")[..8], b"\x89PNG\r\n\x1a\n"); +} + +#[test] +fn screenshot_never_writes_over_a_file_the_project_reads() { + let mut r = Rig::new("screenshot"); + let project = r.read("Project.ecproj"); + for (file, what) in [("clip.png", "footage"), ("Project.ecproj", "project")] { + let Outcome::Done(v) = r.send("ui.screenshot", json!({"path": r.text(file)})) else { panic!("{file}: the screenshot was taken for it") }; + let error = v["error"].as_str().unwrap_or_default(); + assert!(v["ok"] == false && error.contains(file) && error.contains(what), "{file}: {v}"); + } + assert_eq!(r.read("clip.png"), SOURCE, "the footage is as it was"); + assert_eq!(r.read("Project.ecproj"), project, "the project file is as it was"); + // Anywhere else it is taken (the frame that carries it comes later). + assert!(matches!(r.send("ui.screenshot", json!({"path": r.text("shot.png")})), Outcome::Screenshot { .. })); +} diff --git a/docs/agents.md b/docs/agents.md index d801fcec6..c23c20ec5 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -319,6 +319,18 @@ build real projects through these interfaces; they are worked examples of everyt proresProfile: 4444, output}` then `renderQueue.render {"wait": true}`. Check a render by importing it (`file.import`, `file.newCompFromSelection`) and `render_frame {transparent: true}` for its alpha (ProRes 4444 and WebM VP9 alpha both read back as straight alpha). +* **Other files written to a path you give** follow the same footage rule, with no flag and no + prompt: `comp.saveFrameAs` (also `…Psd`, `…Exr`), `file.exportLottie`, `file.saveCopy`, + `file.saveCopyAsXml`, `file.collectFiles` (every copy and the collected project; nothing is + written if one would land on a file the project reads), `file.exportTimeline` (and the movies it + pre-renders), `essential.exportTemplate`, `anim.savePreset`, `view.exportGuides`, + `shortcuts.export`, the control channel's `render.frame` / `ui.screenshot` with a `path`, the + MCP `render_frame` tool and `effectcraft-cli render-frame --out`. Footage (every frame of a + sequence), a proxy and the project file are refused with the reason and left untouched. + `file.checkOutput {path}` says beforehand (`{blocked, source, reason}`). Not covered: Save / + Save As / Increment and Save / auto-save (they write the project's own file), script `File` + writes (a script may rewrite a data file it also imports), and files the app extracts for + itself (template media, vector images). * **Essential Graphics**: controls can be addressed by name (`essential.set {"layer":"#1", "control":"Title","value":"John Smith"}`); a command that targets an explicit `comp` runs even when the active comp would disable it (`essential.exportTemplate {"comp":"Lower Third", …}`).