diff --git a/crates/horizon-client/src/game/film.rs b/crates/horizon-client/src/game/film.rs index 68cd2a940..2c51c4175 100644 --- a/crates/horizon-client/src/game/film.rs +++ b/crates/horizon-client/src/game/film.rs @@ -166,7 +166,8 @@ fn encoder(ffmpeg: &Path, crf: u32) -> Option> { } /// The writer thread: frames to `ffmpeg`'s pipe (or PNG files), sound to a raw file, then the -/// file is closed and the sound muxed in. +/// file is closed and the sound muxed in. `.mp4` is only ever produced by a successful +/// finish, so a failed or cancelled take cannot damage an earlier film of the same name. fn write( rx: mpsc::Receiver, dir: PathBuf, @@ -176,32 +177,38 @@ fn write( crf: u32, audio: bool, outcome: Arc>>, +) { + let ffmpeg = find_ffmpeg(); + let encode = ffmpeg.as_deref().and_then(|f| encoder(f, crf)); + write_with(rx, dir, name, fps, size, crf, audio, outcome, ffmpeg, encode); +} + +/// [`write`], with the `ffmpeg` and its encoder arguments given: a test passes a failing one. +fn write_with( + rx: mpsc::Receiver, + dir: PathBuf, + name: String, + fps: u32, + size: [u32; 2], + crf: u32, + audio: bool, + outcome: Arc>>, + ffmpeg: Option, + encode: Option>, ) { let result = (|| -> Result<(String, String), String> { + // This capture's own files: the encoder's picture, the raw sound, the log and the PNG + // frames. The finished `.mp4` is not among them. + let video_path = dir.join(format!("{name}.video.mp4")); let pcm_path = dir.join(format!("{name}.audio.raw")); - let mut pcm = if audio { - Some(std::io::BufWriter::new( - std::fs::File::create(&pcm_path).map_err(|e| format!("audio file: {e}"))?, - )) - } else { - None - }; - let ffmpeg = find_ffmpeg(); - let encode = ffmpeg.as_deref().and_then(|f| encoder(f, crf)); - let video_path = dir.join(if audio { - format!("{name}.video.mp4") - } else { - format!("{name}.mp4") - }); + let log_path = dir.join(format!("{name}.ffmpeg.log")); let frames_dir = dir.join(format!("{name}_frames")); + let out = dir.join(format!("{name}.mp4")); let mut child: Option = None; - let mut log_path = None; if let (Some(ffmpeg), Some(encode)) = (&ffmpeg, &encode) { - let log = dir.join(format!("{name}.ffmpeg.log")); - let stderr = std::fs::File::create(&log).map_err(|e| format!("log file: {e}"))?; - log_path = Some(log); + let stderr = std::fs::File::create(&log_path).map_err(|e| format!("log file: {e}"))?; let bgra = true; - let c = Command::new(ffmpeg) + let spawned = Command::new(ffmpeg) .args(["-y", "-hide_banner", "-loglevel", "error", "-f", "rawvideo"]) .args(["-pix_fmt", if bgra { "bgra" } else { "rgba" }]) .args(["-video_size", &format!("{}x{}", size[0], size[1])]) @@ -225,11 +232,37 @@ fn write( .stdin(Stdio::piped()) .stdout(Stdio::null()) .stderr(stderr) - .spawn() - .map_err(|e| format!("could not run ffmpeg: {e}"))?; - child = Some(c); + .spawn(); + match spawned { + Ok(c) => child = Some(c), + Err(e) => { + let _ = std::fs::remove_file(&log_path); + return Err(format!("could not run ffmpeg: {e}")); + } + } + } + let mut pcm = if audio { + match std::fs::File::create(&pcm_path) { + Ok(f) => Some(std::io::BufWriter::new(f)), + Err(e) => { + if let Some(mut c) = child.take() { + let _ = c.kill(); + let _ = c.wait(); + } + let _ = std::fs::remove_file(&video_path); + let _ = std::fs::remove_file(&log_path); + return Err(format!("audio file: {e}")); + } + } } else { - std::fs::create_dir_all(&frames_dir).map_err(|e| format!("frames folder: {e}"))?; + None + }; + if child.is_none() + && let Err(e) = std::fs::create_dir_all(&frames_dir) + { + let _ = std::fs::remove_file(&pcm_path); + let _ = std::fs::remove_dir_all(&frames_dir); + return Err(format!("frames folder: {e}")); } let mut stdin = child.as_mut().and_then(|c| c.stdin.take()); let mut n = 0u32; @@ -251,7 +284,7 @@ fn write( } } if let Err(e) = pipe.write_all(&data) { - failed = Some(format!("ffmpeg stopped (see {name}.ffmpeg.log): {e}")); + failed = Some(format!("ffmpeg stopped: {e}")); } } else if let Err(e) = write_png(&frames_dir.join(format!("frame_{n:06}.png")), &f) @@ -289,25 +322,48 @@ fn write( let _ = std::fs::remove_file(p); } }; + // A cancelled or failed take keeps nothing of its own: the encoder's picture, the raw + // sound and the log go. A finished film of an earlier export with the same name is not + // among these paths and stays (a PNG sequence from an earlier one stays too: an encoder + // take never writes into it). + let discard = || { + tidy(&[&video_path, &pcm_path, &log_path]); + }; + let discard_frames = || { + tidy(&[&pcm_path]); + let _ = std::fs::remove_dir_all(&frames_dir); + }; if let Some(mut c) = child { - let status = c.wait().map_err(|e| format!("ffmpeg: {e}"))?; + let status = match c.wait() { + Ok(status) => status, + Err(e) => { + let _ = c.kill(); + discard(); + return Err(format!("ffmpeg: {e}")); + } + }; if cancelled || failed.is_some() || !status.success() { - let why = failed.unwrap_or_else(|| { + let mut why = failed.unwrap_or_else(|| { if cancelled { "cancelled".into() } else { - format!("ffmpeg failed (see {name}.ffmpeg.log)") + "ffmpeg failed".into() } }); - if cancelled { - tidy(&[&video_path, &pcm_path]); - if let Some(l) = &log_path { - tidy(&[l]); + if !cancelled { + // The log goes with the take; what it said is the only trace left, so the + // error carries it. + let log = std::fs::read(&log_path) + .map(|b| String::from_utf8_lossy(&b).into_owned()) + .unwrap_or_default(); + let log = log.split_whitespace().collect::>().join(" "); + if !log.is_empty() { + why.push_str(&format!(": {}", log.chars().take(400).collect::())); } } + discard(); return Err(why); } - let out = dir.join(format!("{name}.mp4")); let mut note = String::new(); if audio { let mux = Command::new(ffmpeg.as_ref().expect("ffmpeg")) @@ -336,10 +392,16 @@ fn write( ); } } + } else { + // The finished picture replaces a film of an earlier export with this name; + // Windows cannot rename over an existing file. + let _ = std::fs::remove_file(&out); + if let Err(e) = std::fs::rename(&video_path, &out) { + discard(); + return Err(format!("the film could not be finished: {e}")); + } } - if let Some(l) = &log_path { - tidy(&[l]); - } + tidy(&[&log_path]); let encoder_note = if encode .as_ref() .is_some_and(|e| e.iter().any(|a| a == "h264_videotoolbox")) @@ -357,11 +419,11 @@ fn write( Ok((out.display().to_string(), note)) } else { if cancelled { - let _ = std::fs::remove_dir_all(&frames_dir); - tidy(&[&pcm_path]); + discard_frames(); return Err("cancelled".into()); } if let Some(why) = failed { + discard_frames(); return Err(why); } let wav = if audio { @@ -878,6 +940,146 @@ mod tests { } } + /// A writer's folder, removed with the test. + #[cfg(unix)] + struct TakeDir(PathBuf); + + #[cfg(unix)] + impl TakeDir { + fn new() -> Self { + static NEXT: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + let n = NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!( + "horizon-film-take-{}-{n}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + Self(dir) + } + + fn file(&self, name: &str, bytes: &[u8]) -> PathBuf { + let p = self.0.join(name); + std::fs::write(&p, bytes).unwrap(); + p + } + + fn gone(&self, name: &str) -> bool { + !self.0.join(name).exists() + } + } + + #[cfg(unix)] + impl Drop for TakeDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + + /// One take of `name` into `dir` with the frames given, then the ending `end`. + #[cfg(unix)] + fn take(dir: &Path, name: &str, audio: bool, ffmpeg: PathBuf, end: Msg, frames: &[u8]) -> Outcome { + let (tx, rx) = mpsc::sync_channel(8); + for &n in frames { + tx.send(Msg::Frame(frame(n))).unwrap(); + } + tx.send(end).unwrap(); + let outcome = Arc::new(Mutex::new(None)); + write_with( + rx, + dir.to_path_buf(), + name.into(), + 30, + [1, 1], + 20, + audio, + outcome.clone(), + Some(ffmpeg), + Some(vec!["-c:v".into(), "libx264".into()]), + ); + let mut o = outcome.lock().unwrap(); + o.take().expect("the writer always reports") + } + + /// A failed encoder leaves no picture, raw sound or log of this take, and neither deletes the + /// finished film of an earlier export with the same name nor another take's files. + #[cfg(unix)] + #[test] + fn a_failed_take_removes_its_own_files_and_keeps_the_film() { + if !Path::new("/bin/false").is_file() { + return; + } + for audio in [true, false] { + let take_dir = TakeDir::new(); + let earlier = take_dir.file("cap.mp4", b"earlier film"); + let other = take_dir.file("other.audio.raw", b"other take"); + let other_log = take_dir.file("other.ffmpeg.log", b"other log"); + let o = take(&take_dir.0, "cap", audio, PathBuf::from("/bin/false"), Msg::Finish, &[1]); + assert!(!o.ok, "audio={audio}"); + assert_eq!(std::fs::read(&earlier).unwrap(), b"earlier film", "audio={audio}"); + assert_eq!(std::fs::read(&other).unwrap(), b"other take"); + assert_eq!(std::fs::read(&other_log).unwrap(), b"other log"); + for name in ["cap.video.mp4", "cap.audio.raw", "cap.ffmpeg.log", "cap_frames"] { + assert!(take_dir.gone(name), "{name} left behind (audio={audio})"); + } + } + } + + /// The log is removed with the failed take, so its message is carried in the error instead. + #[cfg(unix)] + #[test] + fn an_encoder_error_carries_its_log_and_removes_it() { + use std::os::unix::fs::PermissionsExt; + let take_dir = TakeDir::new(); + let encoder = take_dir.file( + "ffmpeg", + b"#!/bin/sh\nfor last; do :; done\ncat > /dev/null\necho 'Unknown encoder libx264' >&2\nexit 1\n", + ); + std::fs::set_permissions(&encoder, std::fs::Permissions::from_mode(0o755)).unwrap(); + let o = take(&take_dir.0, "cap", true, encoder, Msg::Finish, &[1]); + assert!(!o.ok); + assert!(o.message.contains("Unknown encoder libx264"), "{}", o.message); + assert!(take_dir.gone("cap.ffmpeg.log")); + assert!(take_dir.gone("cap.video.mp4")); + assert!(take_dir.gone("cap.audio.raw")); + } + + /// A cancelled take removes the same files, and the earlier film stays as it was. + #[cfg(unix)] + #[test] + fn a_cancelled_take_removes_its_own_files_and_keeps_the_film() { + if !Path::new("/bin/false").is_file() { + return; + } + let take_dir = TakeDir::new(); + let earlier = take_dir.file("cap.mp4", b"earlier film"); + let o = take(&take_dir.0, "cap", true, PathBuf::from("/bin/false"), Msg::Cancel, &[1]); + assert!(!o.ok); + assert_eq!(std::fs::read(&earlier).unwrap(), b"earlier film"); + for name in ["cap.video.mp4", "cap.audio.raw", "cap.ffmpeg.log", "cap_frames"] { + assert!(take_dir.gone(name), "{name} left behind"); + } + } + + /// A take that finishes keeps the film and drops its temporaries; the film is the only file + /// left for this name. + #[cfg(unix)] + #[test] + fn a_finished_take_keeps_the_film_and_drops_its_temporaries() { + use std::os::unix::fs::PermissionsExt; + let take_dir = TakeDir::new(); + let encoder = take_dir.file( + "ffmpeg", + b"#!/bin/sh\nfor last; do :; done\ncat > /dev/null\nprintf 'film' > \"$last\"\nexit 0\n", + ); + std::fs::set_permissions(&encoder, std::fs::Permissions::from_mode(0o755)).unwrap(); + let o = take(&take_dir.0, "cap", false, encoder, Msg::Finish, &[1, 2]); + assert!(o.ok, "{}", o.message); + assert_eq!(std::fs::read(take_dir.0.join("cap.mp4")).unwrap(), b"film"); + for name in ["cap.video.mp4", "cap.audio.raw", "cap.ffmpeg.log", "cap_frames"] { + assert!(take_dir.gone(name), "{name} left behind"); + } + } + fn drain(rx: &mpsc::Receiver) -> Vec { rx.try_iter() .filter_map(|m| match m {