From ca0729d66607213f90ae6a015487356656cb58fa Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:43:46 +0200 Subject: [PATCH 1/5] fix(encode): publish the output only once the render succeeds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When render_frame_task fails mid-encode (line 613 sets pipe_error and breaks), the code drops stdin and then *waits* for ffmpeg. ffmpeg sees a clean EOF on pipe:0, finalizes the frames it already received, writes the moov atom and exits 0. rustmotion then returns Err, but output_path now holds a structurally valid MP4 containing only the first N frames. Because ffmpeg_args emits -y (line 194), this also silently destroys a previously- good render at the same path. A user scripting rustmotion render who checks only file existence (or whose CI publishes the artifact) ships a truncated video. No code path anywhere in src/cli or src/encode removes the output on failure — only test code calls remove_file on outputs. Fix: On the pipe_error path, call child.kill() and child.wait() before returning, and let _ = std::fs::remove_file(output_path); on both the pipe_error and !status.success() branches. Better still, have ffmpeg write to a sibling scratch path and fs::rename onto output_path only after a clean exit — the same promote-on-success discipline video_audio.rs::partial_wav_path already applies to cached WAVs. Refs #220 --- crates/rustmotion/src/encode/video/ffmpeg.rs | 93 ++++++++- crates/rustmotion/src/encode/video/formats.rs | 180 +++++++++++++++++- crates/rustmotion/tests/audit_ws_c.rs | 117 ++++++++++++ 3 files changed, 383 insertions(+), 7 deletions(-) create mode 100644 crates/rustmotion/tests/audit_ws_c.rs diff --git a/crates/rustmotion/src/encode/video/ffmpeg.rs b/crates/rustmotion/src/encode/video/ffmpeg.rs index b91208c6..63d338ae 100644 --- a/crates/rustmotion/src/encode/video/ffmpeg.rs +++ b/crates/rustmotion/src/encode/video/ffmpeg.rs @@ -290,6 +290,27 @@ fn ffmpeg_args( args } +/// Scratch path ffmpeg actually writes to; promoted (renamed) onto the +/// caller's real `output_path` only after a clean exit with no `pipe_error`. +/// Kept as a sibling of `output_path` (same directory, same filesystem, so +/// the promotion is a plain rename) and keeps `output_path`'s own extension +/// as the *final* extension — mirrors `video_audio::partial_wav_path`'s doc: +/// ffmpeg picks its output muxer from the last extension, so a bare +/// `.partial` suffix appended after it makes ffmpeg refuse to start with +/// "Unable to choose an output format" instead of the encode failure this +/// path exists to isolate. +fn ffmpeg_partial_output_path(output_path: &std::path::Path) -> std::path::PathBuf { + let stem = output_path + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or("output"); + let name = match output_path.extension().and_then(|s| s.to_str()) { + Some(ext) => format!("{stem}.partial.{ext}"), + None => format!("{stem}.partial"), + }; + output_path.with_file_name(name) +} + /// Encode using FFmpeg subprocess (for h265, vp9, prores, webm, mov, transparency). /// /// Software-only. Kept with its original signature so existing callers @@ -531,6 +552,14 @@ fn encode_with_ffmpeg_hw_impl( } }; + let partial_output_path = ffmpeg_partial_output_path(std::path::Path::new(output_path)); + let partial_output_str = + partial_output_path + .to_str() + .ok_or_else(|| RustmotionError::NonUtf8Path { + path: partial_output_path.to_string_lossy().into_owned(), + })?; + let mut cmd = std::process::Command::new("ffmpeg"); cmd.args(ffmpeg_args( width, @@ -541,7 +570,7 @@ fn encode_with_ffmpeg_hw_impl( transparent, hw_encoder.as_deref(), audio_input.as_deref(), - output_path, + partial_output_str, )); cmd.stdin(std::process::Stdio::piped()); cmd.stdout(std::process::Stdio::null()); @@ -623,7 +652,18 @@ fn encode_with_ffmpeg_hw_impl( cb(EncodeProgress::Muxing); } - let status = child.wait().map_err(|e| RustmotionError::FfmpegWait { + // A `pipe_error` means the render already failed and `partial_output_path` + // will be discarded either way, so there is nothing left for ffmpeg to + // usefully finish — killing it here instead of waiting for it to + // gracefully encode and finalize a file nobody will ever read avoids + // burning time on a result already known to be thrown away. + let status = if pipe_error.is_some() { + let _ = child.kill(); + child.wait() + } else { + child.wait() + } + .map_err(|e| RustmotionError::FfmpegWait { reason: e.to_string(), })?; @@ -659,6 +699,7 @@ fn encode_with_ffmpeg_hw_impl( if let Some(e) = pipe_error { tee_stderr(); + let _ = std::fs::remove_file(&partial_output_path); // A broken pipe means ffmpeg is already gone — its own error says why, // ours only says we could not keep writing. Carry both. return Err(match e { @@ -672,11 +713,17 @@ fn encode_with_ffmpeg_hw_impl( if !status.success() { tee_stderr(); + let _ = std::fs::remove_file(&partial_output_path); return Err(RustmotionError::FfmpegFailed { stderr: stderr_summary, }); } + // Only now, with a clean exit and no pipe error, does `output_path` ever + // see this render's bytes — promote-on-success, the same discipline + // `video_audio::extract_audio_to_wav` already applies to its cached WAVs. + std::fs::rename(&partial_output_path, output_path)?; + Ok(()) } @@ -789,7 +836,47 @@ pub fn concat_mp4_segments(inputs: &[std::path::PathBuf], output_path: &str) -> #[cfg(test)] mod tests { - use super::{ffmpeg_args, parse_encoder_names, select_hardware_encoder, HardwareSelection}; + use super::{ + ffmpeg_args, ffmpeg_partial_output_path, parse_encoder_names, select_hardware_encoder, + HardwareSelection, + }; + + // ── partial-output-path naming (pure) ──────────────────────────────────── + + #[test] + fn partial_path_keeps_the_original_extension_as_its_last_extension() { + let cases = [ + ("/tmp/out.mp4", "/tmp/out.partial.mp4"), + ("/tmp/out.mov", "/tmp/out.partial.mov"), + ("/tmp/out.webm", "/tmp/out.partial.webm"), + ("out.mp4", "out.partial.mp4"), + ]; + for (input, expected) in cases { + let got = ffmpeg_partial_output_path(std::path::Path::new(input)); + assert_eq!( + got, + std::path::PathBuf::from(expected), + "input={input}: ffmpeg picks its muxer from the last extension, so it must \ + survive unchanged" + ); + } + } + + #[test] + fn partial_path_is_a_sibling_of_the_final_output_not_a_different_directory() { + let got = ffmpeg_partial_output_path(std::path::Path::new("/a/b/c/out.mp4")); + assert_eq!( + got.parent(), + Some(std::path::Path::new("/a/b/c")), + "the rename onto output_path must stay on the same filesystem" + ); + } + + #[test] + fn partial_path_falls_back_gracefully_with_no_extension() { + let got = ffmpeg_partial_output_path(std::path::Path::new("/tmp/out")); + assert_eq!(got, std::path::PathBuf::from("/tmp/out.partial")); + } /// Every option that describes the *output* has to sit after the last `-i`. /// Put one before it and ffmpeg attaches it to the following input instead, diff --git a/crates/rustmotion/src/encode/video/formats.rs b/crates/rustmotion/src/encode/video/formats.rs index ee06ab7a..8cad2f02 100644 --- a/crates/rustmotion/src/encode/video/formats.rs +++ b/crates/rustmotion/src/encode/video/formats.rs @@ -10,10 +10,52 @@ use crate::schema::ResolvedScenario as Scenario; use super::tasks::{build_frame_tasks, render_frame_task}; use super::EncodeProgress; -/// Encode frames as a PNG sequence (one PNG file per frame) +/// Sibling scratch directory a PNG-sequence render writes into before being +/// promoted onto `output_dir` — same reasoning as `partial_sibling_path`, +/// applied to a directory instead of a single file: directories don't have +/// an extension to preserve, so the suffix is the whole difference. +fn partial_sibling_dir(output_dir: &str) -> std::path::PathBuf { + let path = std::path::Path::new(output_dir); + let name = path + .file_name() + .and_then(|s| s.to_str()) + .unwrap_or("output"); + path.with_file_name(format!("{name}.partial")) +} + +/// Encode frames as a PNG sequence (one PNG file per frame). +/// +/// Renders into a sibling scratch directory and promotes (renames) it onto +/// `output_dir` only once every frame has been written — a mid-sequence +/// failure used to leave a partial run's files sitting directly in +/// `output_dir`, indistinguishable from a completed one to a caller that +/// only checks the directory exists (the same shape a single-file output +/// failing mid-encode has). pub fn encode_png_sequence( scenario: &Scenario, output_dir: &str, + quiet: bool, + transparent: bool, + on_progress: Option<&mut dyn FnMut(EncodeProgress)>, +) -> Result<()> { + let partial_dir = partial_sibling_dir(output_dir); + let _ = std::fs::remove_dir_all(&partial_dir); + match encode_png_sequence_to_dir(scenario, &partial_dir, quiet, transparent, on_progress) { + Ok(()) => { + let _ = std::fs::remove_dir_all(output_dir); + std::fs::rename(&partial_dir, output_dir)?; + Ok(()) + } + Err(e) => { + let _ = std::fs::remove_dir_all(&partial_dir); + Err(e) + } + } +} + +fn encode_png_sequence_to_dir( + scenario: &Scenario, + output_dir: &std::path::Path, _quiet: bool, _transparent: bool, mut on_progress: Option<&mut dyn FnMut(EncodeProgress)>, @@ -67,7 +109,7 @@ pub fn encode_png_sequence( for result in results { let (frame_num, rgba) = result?; - let path = format!("{}/frame_{:05}.png", output_dir, frame_num); + let path = output_dir.join(format!("frame_{:05}.png", frame_num)); let img = image::RgbaImage::from_raw(width, height, rgba) .ok_or(RustmotionError::PixelImage)?; img.save(&path)?; @@ -77,11 +119,59 @@ pub fn encode_png_sequence( Ok(()) } -/// Encode frames as an animated GIF +/// Sibling scratch path a single-file encoder (GIF today) writes to before +/// being promoted onto `output_path` only after every frame is written +/// successfully. Same discipline and the same reasoning as +/// `video::ffmpeg::ffmpeg_partial_output_path`: kept in the same directory +/// so the promotion is a same-filesystem rename, extension kept last since +/// some downstream consumers of the output (players, `file`) pick behavior +/// from it the way ffmpeg picks a muxer from its own output extension. +fn partial_sibling_path(output_path: &str) -> std::path::PathBuf { + let path = std::path::Path::new(output_path); + let stem = path + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or("output"); + let name = match path.extension().and_then(|s| s.to_str()) { + Some(ext) => format!("{stem}.partial.{ext}"), + None => format!("{stem}.partial"), + }; + path.with_file_name(name) +} + +/// Encode frames as an animated GIF. +/// +/// Writes to a sibling scratch path first and promotes (renames) it onto +/// `output_path` only once every frame has been written without error — +/// the same partial-then-rename discipline `video_audio::extract_audio_to_wav` +/// and the ffmpeg encoder already apply. A GIF that fails midway through the +/// frame loop (any `?` inside `encode_gif_to_path`) used to leave a +/// truncated-but-existing file sitting at `output_path`, the same shape the +/// ffmpeg-backed encoder and the PNG-sequence encoder had before adopting +/// this same discipline. pub fn encode_gif( scenario: &Scenario, output_path: &str, quiet: bool, + on_progress: Option<&mut dyn FnMut(EncodeProgress)>, +) -> Result<()> { + let partial_path = partial_sibling_path(output_path); + match encode_gif_to_path(scenario, &partial_path, quiet, on_progress) { + Ok(()) => { + std::fs::rename(&partial_path, output_path)?; + Ok(()) + } + Err(e) => { + let _ = std::fs::remove_file(&partial_path); + Err(e) + } + } +} + +fn encode_gif_to_path( + scenario: &Scenario, + partial_path: &std::path::Path, + quiet: bool, mut on_progress: Option<&mut dyn FnMut(EncodeProgress)>, ) -> Result<()> { let config = &scenario.video; @@ -103,7 +193,7 @@ pub fn encode_gif( let gif_w = width.min(65535) as u16; let gif_h = height.min(65535) as u16; - let file = File::create(output_path)?; + let file = File::create(partial_path)?; let mut encoder = gif::Encoder::new(BufWriter::new(file), gif_w, gif_h, &[]).map_err(|e| { RustmotionError::GifEncoder { reason: e.to_string(), @@ -243,6 +333,88 @@ mod tests { use crate::loader::load_scenario_from_source; use std::path::{Path, PathBuf}; + // ── partial-output naming (pure) ───────────────────────────────────────── + + #[test] + fn partial_sibling_path_keeps_the_extension_last() { + assert_eq!( + partial_sibling_path("/tmp/out.gif"), + PathBuf::from("/tmp/out.partial.gif") + ); + assert_eq!( + partial_sibling_path("/tmp/out"), + PathBuf::from("/tmp/out.partial") + ); + } + + #[test] + fn partial_sibling_dir_stays_next_to_the_final_directory() { + let got = partial_sibling_dir("/a/b/frames"); + assert_eq!(got, PathBuf::from("/a/b/frames.partial")); + assert_eq!(got.parent(), Some(Path::new("/a/b"))); + } + + /// A failed GIF encode must not leave a truncated file at `output_path` + /// — the real trigger used to prove this for the ffmpeg-backed encoder + /// (an odd width rejected by libx264) doesn't apply here (the `gif` + /// crate has no such constraint), so this drives the failure the one + /// way this pure Rust path can actually fail without external tools: + /// `total_frames == 0`. + /// That returns before any file is touched either way, so what this + /// test really pins is the *shape* of the fix — `encode_gif` must never + /// promote a partial onto `output_path` when its inner call errors — + /// exercised by asserting the scratch/partial path used internally is + /// never left behind either. + #[test] + fn encode_gif_leaves_no_partial_file_behind_on_an_empty_scenario() { + let json = r#"{"video": {"width": 8, "height": 8, "fps": 10}, "scenes": []}"#; + let scenario = load_scenario_from_source(None, Some(json)).expect("load"); + + let out = std::env::temp_dir().join(format!( + "rm_gif_no_debris_test_{}_{}.gif", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let partial = partial_sibling_path(out.to_str().unwrap()); + let _ = std::fs::remove_file(&out); + let _ = std::fs::remove_file(&partial); + + let result = encode_gif(&scenario, out.to_str().unwrap(), true, None); + assert!(result.is_err(), "an empty scenario has no frames to encode"); + assert!(!out.exists(), "no debris at the final output path"); + assert!(!partial.exists(), "no debris at the scratch path either"); + } + + /// Same property for the directory-based PNG-sequence encoder. + #[test] + fn encode_png_sequence_leaves_no_partial_dir_behind_on_an_empty_scenario() { + let json = r#"{"video": {"width": 8, "height": 8, "fps": 10}, "scenes": []}"#; + let scenario = load_scenario_from_source(None, Some(json)).expect("load"); + + let out_dir = std::env::temp_dir().join(format!( + "rm_png_seq_no_debris_test_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let partial_dir = partial_sibling_dir(out_dir.to_str().unwrap()); + let _ = std::fs::remove_dir_all(&out_dir); + let _ = std::fs::remove_dir_all(&partial_dir); + + let result = encode_png_sequence(&scenario, out_dir.to_str().unwrap(), true, false, None); + assert!(result.is_err(), "an empty scenario has no frames to encode"); + assert!(!out_dir.exists(), "no debris at the final output directory"); + assert!( + !partial_dir.exists(), + "no debris at the scratch directory either" + ); + } + /// Deterministic color for a given frame index: distinct enough across /// nearby indices that a swapped/misplaced frame is detected by a plain /// pixel comparison. diff --git a/crates/rustmotion/tests/audit_ws_c.rs b/crates/rustmotion/tests/audit_ws_c.rs new file mode 100644 index 00000000..e612794e --- /dev/null +++ b/crates/rustmotion/tests/audit_ws_c.rs @@ -0,0 +1,117 @@ +//! Regression tests — audit round chantier/audit-2026-09, workstream C +//! (encoding, audio, cancellation). +//! +//! Tests here only exercise the public API surface — `rustmotion::encode::...` +//! — since this is an external integration test crate; regression tests for +//! private helpers live next to them in their own `#[cfg(test)] mod tests` +//! inside the `src/encode/` file that owns them, and are cross-referenced +//! here by the behavior they cover. + +use std::path::PathBuf; + +/// Minimal RAII scratch file: unique per (label, pid, nanosecond timestamp), +/// removed on drop even if the test panics partway through. +struct ScratchFile(PathBuf); + +impl ScratchFile { + /// `ext` is the extension ffmpeg/the encoder picks its container format + /// from (`"mp4"`, `"gif"`, ...) — omitting it makes ffmpeg fail at + /// muxer selection before ever touching the path, which would make a + /// debris-after-failure test pass for the wrong reason. + fn new(label: &str, ext: &str) -> Self { + let unique = format!( + "rustmotion-audit-ws-c-{label}-{}-{}.{ext}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock before UNIX epoch") + .as_nanos() + ); + Self(std::env::temp_dir().join(unique)) + } + + fn path(&self) -> &std::path::Path { + &self.0 + } + + fn to_str(&self) -> &str { + self.0.to_str().expect("scratch path must be UTF-8") + } +} + +impl Drop for ScratchFile { + fn drop(&mut self) { + let _ = std::fs::remove_file(&self.0); + } +} + +fn ffmpeg_on_path() -> bool { + std::process::Command::new("ffmpeg") + .args(["-version"]) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .map(|s| s.success()) + .unwrap_or(false) +} + +// ─── A render ffmpeg rejects must leave no debris at output_path ────────── +// +// `encode_with_ffmpeg`/`encode_with_ffmpeg_hw` used to hand ffmpeg the +// user's own `output_path` directly, drop stdin and gracefully wait no +// matter how the frame loop ended. A failure mid-render (a broken pipe, or +// ffmpeg itself exiting non-zero) still let ffmpeg finalize whatever it had +// already received at that exact path, and `-y` meant it would even +// overwrite a previously-good render there. No cleanup ran on any error +// path. +// +// Forcing rustmotion's *own* frame renderer to fail deterministically isn't +// possible from a plain scenario — `Painter::paint_content` cannot return an +// `Err`, so no user-authored content can fail a frame. ffmpeg itself, +// though, refuses an odd width under the crate's default 10-bit H.264 +// profile (`yuv420p10le` needs even 4:2:0 chroma dimensions) — confirmed by +// hand: `ffmpeg -f rawvideo ... -video_size 321x240 ... -c:v libx264 +// -profile:v high10 -pix_fmt yuv420p10le out.mp4` creates a 0-byte +// `out.mp4` and then fails to open its encoder, closing stdin before +// rustmotion finishes writing frames — exactly the "ffmpeg already has a +// file open at output_path when the failure happens" shape this test cares +// about, without needing an internal render failure at all. +#[test] +fn a_render_ffmpeg_rejects_leaves_no_debris_at_the_output_path() { + if !ffmpeg_on_path() { + eprintln!( + "a_render_ffmpeg_rejects_leaves_no_debris_at_the_output_path: \ + ffmpeg not found — skipping" + ); + return; + } + + let json = r#"{"video": {"width": 321, "height": 240, "fps": 10}, + "scenes": [{"duration": 0.3, "children": []}]}"#; + let scenario = rustmotion::loader::load_scenario_from_source(None, Some(json)).expect("load"); + + let out = ScratchFile::new("odd-width", "mp4"); + let _ = std::fs::remove_file(out.path()); + + let result = rustmotion::encode::encode_with_ffmpeg( + &scenario, + out.to_str(), + true, + "h264", + None, + false, + None, + ); + + assert!( + result.is_err(), + "an odd width (321) must make libx264's high10/yuv420p10le encoder \ + init fail — this test's premise depends on ffmpeg actually rejecting it" + ); + assert!( + !out.path().exists(), + "no file — truncated, empty, or otherwise — may be left at output_path \ + after a failed render; got one at {}", + out.path().display() + ); +} From 4a6de54771f9ece03727dc6419f0507195a96e06 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:43:51 +0200 Subject: [PATCH 2/5] fix(encode): reject a non-positive playback rate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit remaining never reaches 0.5 when it starts at 0.0 (0.0/0.5 == 0.0) or at any negative value (it diverges toward -inf). The loop never terminates and pushes a fresh String on every iteration, so rustmotion hangs while consuming memory until the OOM killer fires. Reached from a plain scenario JSON: {"type":"video","src":"clip.mp4","playback_rate":0} — volume defaults to 1.0 (crates/rustmotion-components/src/video.rs:14 fn default_volume() -> f32 { 1.0 }), so collect_videos_in_child collects it (v.volume > 0.0, line 128), collect_video_audio_tracks calls extract_audio_to_wav(..., occ.playback_rate) (line 421), which calls build_atempo_filter(0.0) at line 333. playback_rate is an unvalidated Option (crates/rustmotion- components/src/video.rs:26); a repo-wide grep shows no validator constrains it. Both the ffmpeg render path (encode_with_ffmpeg_hw_impl line 432) and the native path (mux.rs:34) reach it. Fix: Guard the entry point: if !rate.is_finite() || rate <= 0.0 { return None; } (or surface a schema error naming the component), and additionally bound the stage count so no future arithmetic edge case can spin. A validation rule rejecting playback_rate <= 0 at load time would also stop preload.rs:182, which consumes the same field. Refs #220 --- crates/rustmotion/src/encode/video_audio.rs | 27 +++++++++++++- crates/rustmotion/tests/audit_ws_c.rs | 39 +++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/crates/rustmotion/src/encode/video_audio.rs b/crates/rustmotion/src/encode/video_audio.rs index 5ce3c1fb..caac9e1e 100644 --- a/crates/rustmotion/src/encode/video_audio.rs +++ b/crates/rustmotion/src/encode/video_audio.rs @@ -199,19 +199,41 @@ fn ffmpeg_available() -> bool { /// rate 4.0 → `atempo=2.0,atempo=2.0` /// rate 0.1 → `atempo=0.5,atempo=0.2` (0.5 * 0.2 = 0.1) /// -/// Returns `None` if rate == 1.0 (no filter needed). +/// Returns `None` if rate == 1.0 (no filter needed), or if `rate` cannot +/// possibly be reached by any chain of `atempo` stages (`<= 0.0` or +/// non-finite — see the guard below). pub fn build_atempo_filter(rate: f64) -> Option { const EPSILON: f64 = 1e-9; + // `remaining` only ever converges toward `[0.5, 2.0]` by repeatedly + // multiplying or dividing by 2.0 starting from a *positive, finite* + // `rate`. At `rate == 0.0`, `remaining /= 0.5` stays `0.0` forever; at a + // negative or non-finite rate it diverges away from the loop's own exit + // test. Either way the `while` below never terminates and pushes a new + // `String` on every turn — the guard has to reject these before that + // loop is ever reached, not inside it. + if !rate.is_finite() || rate <= 0.0 { + return None; + } if (rate - 1.0).abs() < EPSILON { return None; } + // A ceiling on the chain length, independent of the guard above: a + // legitimate rate as extreme as 1e9 only needs ~30 stages, so this never + // fires for real input. It exists so that a future mistake in this + // arithmetic degrades into "no atempo filter" instead of reopening the + // same unbounded loop the guard above closes. + const MAX_STAGES: usize = 64; + let mut parts: Vec = Vec::new(); let mut remaining = rate; if rate > 1.0 { // Each stage multiplies by at most 2.0 while remaining > 2.0 + EPSILON { + if parts.len() >= MAX_STAGES { + return None; + } parts.push("atempo=2.0".to_string()); remaining /= 2.0; } @@ -219,6 +241,9 @@ pub fn build_atempo_filter(rate: f64) -> Option { } else { // Each stage multiplies by at least 0.5 while remaining < 0.5 - EPSILON { + if parts.len() >= MAX_STAGES { + return None; + } parts.push("atempo=0.5".to_string()); remaining /= 0.5; } diff --git a/crates/rustmotion/tests/audit_ws_c.rs b/crates/rustmotion/tests/audit_ws_c.rs index e612794e..0b18c5b3 100644 --- a/crates/rustmotion/tests/audit_ws_c.rs +++ b/crates/rustmotion/tests/audit_ws_c.rs @@ -115,3 +115,42 @@ fn a_render_ffmpeg_rejects_leaves_no_debris_at_the_output_path() { out.path().display() ); } + +// ─── build_atempo_filter(rate) must not loop forever ─────────────────────── +// +// `remaining /= 0.5` never reaches the loop's `>= 0.5` exit test starting +// from `0.0`, and diverges away from it starting from any negative rate — +// either way the pre-fix loop pushed a fresh `String` forever. Calling the +// unguarded function directly on the test thread would hang the whole +// suite, so each candidate rate runs on its own thread with a bounded +// `recv_timeout`: a present guard returns well inside the timeout, a +// missing one times out and fails the assertion instead of the process. +#[test] +fn atempo_guard_rejects_non_positive_and_non_finite_rates_without_hanging() { + use std::sync::mpsc; + use std::time::Duration; + + for rate in [ + 0.0_f64, + -1.0, + -0.25, + f64::NAN, + f64::INFINITY, + f64::NEG_INFINITY, + ] { + let (tx, rx) = mpsc::channel(); + std::thread::spawn(move || { + let _ = tx.send(rustmotion::encode::video_audio::build_atempo_filter(rate)); + }); + match rx.recv_timeout(Duration::from_secs(2)) { + Ok(result) => assert_eq!( + result, None, + "rate={rate} must return None instead of building an atempo chain" + ), + Err(_) => panic!( + "rate={rate} did not return within 2s — the guard against the infinite loop \ + in build_atempo_filter is missing or broken" + ), + } + } +} From 43a384032d24340d26ab43a8ae9751cbffa164e4 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:43:55 +0200 Subject: [PATCH 3/5] fix(encode): trust the decoded signal spec for rate and channels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The interleaved samples appended to all_samples come from decoded.spec(), but the returned (sample_rate, channels) come from the container header with unwrap_or fallbacks. These diverge for real files: symphonia's ADTS AAC reader only sets params.with_channels(channels) when header.channels is Some (symphonia-codec-aac-0.5.5/src/adts.rs:139-141) — an ADTS stream with channel_config == 0 leaves codec_params.channels == None, so rustmotion assumes 2 while the decoder actually emits mono. to_stereo(&samples, 2) (line 205) then passes the mono buffer through untouched, so the track is half its real length and plays at double speed; mix_audio_tracks_segment sizes everything from that, and the muxed audio desyncs from video with no error anywhere. audio_analysis.rs:143-149 divides by the same wrong channels for the waveform/spectrum envelope, so the on-screen visualisation agrees with the broken mix. Fix: Capture spec.rate and spec.channels.count() from the first decoded packet and return those, falling back to codec_params only when no packet ever decodes. Also bail out (or resample per-segment) if a later packet's spec differs from the first — today, chained streams get concatenated into one flat Vec under a single assumed layout. Refs #220 --- crates/rustmotion/src/encode/audio.rs | 81 +++++++++++++++++++++++++-- 1 file changed, 75 insertions(+), 6 deletions(-) diff --git a/crates/rustmotion/src/encode/audio.rs b/crates/rustmotion/src/encode/audio.rs index 85f09796..6f284a44 100644 --- a/crates/rustmotion/src/encode/audio.rs +++ b/crates/rustmotion/src/encode/audio.rs @@ -64,12 +64,8 @@ pub(crate) fn decode_audio_file(path: &str) -> Result<(Vec, u32, u32)> { })?; let track_id = track.id; - let sample_rate = track.codec_params.sample_rate.unwrap_or(44100); - let channels = track - .codec_params - .channels - .map(|c| c.count() as u32) - .unwrap_or(2); + let header_sample_rate = track.codec_params.sample_rate; + let header_channels = track.codec_params.channels.map(|c| c.count() as u32); let mut decoder = symphonia::default::get_codecs() .make(&track.codec_params, &DecoderOptions::default()) @@ -79,6 +75,7 @@ pub(crate) fn decode_audio_file(path: &str) -> Result<(Vec, u32, u32)> { })?; let mut all_samples: Vec = Vec::new(); + let mut decoded_spec: Option<(u32, u32)> = None; loop { let packet = match format.next_packet() { @@ -107,11 +104,40 @@ pub(crate) fn decode_audio_file(path: &str) -> Result<(Vec, u32, u32)> { sample_buf.copy_interleaved_ref(decoded); all_samples.extend_from_slice(sample_buf.samples()); + + if decoded_spec.is_none() { + decoded_spec = Some((spec.rate, spec.channels.count() as u32)); + } } + let (sample_rate, channels) = + resolve_decoded_spec(decoded_spec, header_sample_rate, header_channels); + Ok((all_samples, sample_rate, channels)) } +/// `decode_audio_file`'s `(sample_rate, channels)` return value: the actual +/// spec of the first packet that decoded, when there was one — that's what +/// `all_samples` was interleaved from — falling back to the container +/// header's own declaration only when nothing ever decoded at all. Some +/// demuxers leave the header fields absent or, for formats whose per-frame +/// syntax can encode a channel count the container-level probe cannot see +/// (e.g. an ADTS/AAC stream with an implicit `channel_configuration`), +/// disagreeing with what the codec itself produces — trusting the header +/// unconditionally there silently mis-sizes every downstream stereo/mono +/// interpretation of `all_samples`. `44100`/`2` is the last-resort default, +/// unchanged from before this function tracked a decoded spec at all. +fn resolve_decoded_spec( + decoded_spec: Option<(u32, u32)>, + header_sample_rate: Option, + header_channels: Option, +) -> (u32, u32) { + decoded_spec.unwrap_or(( + header_sample_rate.unwrap_or(44100), + header_channels.unwrap_or(2), + )) +} + /// Duration, sample rate and channel count of a local audio file — the /// `rustmotion info` answer to "how long is this audio track?". #[derive(Debug, Clone, Copy, PartialEq)] @@ -479,6 +505,49 @@ fn resample_linear(samples: &[f32], src_rate: u32, dst_rate: u32) -> Vec { mod tests { use super::*; + // ── decode_audio_file must trust the decoded spec, not the header ──────── + // + // Reproducing the exact divergence through a real file needs a decoder + // whose *decoded* output can genuinely disagree with what the container + // probed ahead of time. Tracing one concrete case where this happens in + // the wild — `symphonia-codec-aac`'s ADTS reader leaving `codec_params.channels` + // `None` when `channel_configuration == 0` — into `AacDecoder::try_new` + // (symphonia-codec-aac 0.5.5, `aac/mod.rs`) shows that without an + // `extra_data`/channel-layout fallback it returns a hard + // `unsupported_error` instead of ever reaching a decoded packet: this + // crate's enabled `symphonia` features (`mp3, wav, ogg, flac, aac`, no + // `isomp4`) never populate that fallback for a bare ADTS stream. So this + // exact repro surfaces as a loud decode failure, not silent corruption — + // a real fixture can't exercise the discard/trust distinction at all. + // `resolve_decoded_spec` is exactly that distinction pulled out as a + // pure decision, tested directly with the divergence a real file could + // produce for a different codec/container pairing. + #[test] + fn resolve_decoded_spec_prefers_the_decoded_packets_own_spec_over_the_header() { + let decoded = Some((48_000, 1)); + let header_rate = Some(44_100); + let header_channels = Some(2); + assert_eq!( + resolve_decoded_spec(decoded, header_rate, header_channels), + (48_000, 1), + "a packet actually decoded — all_samples came from its spec, so the \ + return value must match it, not the header's own guess" + ); + } + + #[test] + fn resolve_decoded_spec_falls_back_to_the_header_when_nothing_ever_decoded() { + assert_eq!( + resolve_decoded_spec(None, Some(22_050), Some(1)), + (22_050, 1) + ); + } + + #[test] + fn resolve_decoded_spec_falls_back_to_the_hardcoded_default_as_a_last_resort() { + assert_eq!(resolve_decoded_spec(None, None, None), (44_100, 2)); + } + /// Write a minimal, hand-rolled canonical PCM WAV file (16-bit, mono) — /// no ffmpeg and no extra crate needed, `symphonia`'s built-in WAV demuxer /// decodes this directly. From 756d9d8bcd5920b12eb595ea55fede4745ef8eda Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:44:00 +0200 Subject: [PATCH 4/5] fix(encode): create temp files in a private directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The path is fully derived from PID plus a counter starting at 0, in a shared, world-writable directory. create_dir_all succeeds on an already- existing directory (or a symlink to one) and fs::write follows symlinks, so a local attacker who pre-creates /tmp/rustmotion_audio__0/audio.raw as a symlink gets an arbitrary file truncated and overwritten with PCM bytes under the rendering user's identity; PID space is small enough to pre-seed exhaustively. The same class exists in video_audio.rs:258, std::env::temp_dir().join(format!("rustmotion_vidaud_{:016x}.wav", hash)), where hash comes from DefaultHasher::new() (SipHash seeded with fixed keys 0,0 — deterministic across processes and machines), so the path is exactly predictable. There, if wav_path.exists() { return Some(wav_path) } (line 298) means a pre-planted file is used verbatim as the render's audio, and the .partial.wav sibling is handed to ffmpeg -y (line 339-340), which follows a symlink at that path. Fix: Create the scratch directory with std::fs::create_dir (fails with AlreadyExists rather than adopting a hostile one) plus a random suffix, or adopt the tempfile crate for O_EXCL creation with a random name and RAII cleanup. For the video-audio WAV cache, put it under a per-user, non-world- writable directory (e.g. dirs::cache_dir()) instead of temp_dir(), and verify the cache entry is a regular file rather than trusting exists(). Refs #220 --- crates/rustmotion/src/encode/video/ffmpeg.rs | 86 ++++++++++++- crates/rustmotion/src/encode/video_audio.rs | 128 ++++++++++++++++++- 2 files changed, 205 insertions(+), 9 deletions(-) diff --git a/crates/rustmotion/src/encode/video/ffmpeg.rs b/crates/rustmotion/src/encode/video/ffmpeg.rs index 63d338ae..2f79343d 100644 --- a/crates/rustmotion/src/encode/video/ffmpeg.rs +++ b/crates/rustmotion/src/encode/video/ffmpeg.rs @@ -290,6 +290,17 @@ fn ffmpeg_args( args } +/// Name of the scratch directory a single audio-bearing render call writes +/// its materialised PCM into. `pid` repeats across the machine's uptime and +/// `seq` is a small monotonic counter starting at zero, so together they are +/// a key an outside process could realistically pre-compute and occupy +/// ahead of time; folding in a nanosecond timestamp neither of those two +/// alone carries closes that gap without needing a random-number +/// dependency this crate doesn't already have. +fn audio_tmp_dir_name(pid: u32, seq: u32, nanos: u128) -> String { + format!("rustmotion_audio_{pid}_{seq}_{nanos:x}") +} + /// Scratch path ffmpeg actually writes to; promoted (renamed) onto the /// caller's real `output_path` only after a clean exit with no `pipe_error`. /// Kept as a sibling of `output_path` (same directory, same filesystem, so @@ -461,22 +472,32 @@ fn encode_with_ffmpeg_hw_impl( // encodes can run concurrently *within* one process (parallel test // threads today; `--frames` segments rendered concurrently by a future // distributed worker tomorrow — the exact shape this feature exists to - // enable). Two calls sharing a PID-only path would each `create_dir_all` + // enable). Two calls sharing a PID-only path would each try to create // the same directory, then whichever finishes first would // `remove_dir_all` it out from under the other mid-write, surfacing as // a bare `NotFound` on `std::fs::write` below. A monotonic counter on // top of PID makes every call's directory distinct regardless of - // timing. + // timing; a nanosecond timestamp on top of *that* keeps the full key + // from being small enough for something outside this process to + // pre-compute and occupy ahead of time — pid space and a + // monotonic-from-zero counter both are. `create_dir` below (not + // `_all`) is what actually refuses to proceed if something is already + // sitting at the computed path, symlink included; the timestamp only + // raises the cost of ever landing on that path in the first place. static AUDIO_TMP_DIR_SEQ: AtomicU32 = AtomicU32::new(0); let audio_tmp_dir = if !merged_audio.is_empty() { let seq = AUDIO_TMP_DIR_SEQ.fetch_add(1, Ordering::Relaxed); - Some(std::env::temp_dir().join(format!("rustmotion_audio_{}_{seq}", std::process::id()))) + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or_default(); + Some(std::env::temp_dir().join(audio_tmp_dir_name(std::process::id(), seq, nanos))) } else { None }; let pcm_data = if !merged_audio.is_empty() { if let Some(ref tmp_dir) = audio_tmp_dir { - std::fs::create_dir_all(tmp_dir)?; + std::fs::create_dir(tmp_dir)?; } super::super::audio::mix_audio_tracks_segment( &merged_audio, @@ -837,10 +858,63 @@ pub fn concat_mp4_segments(inputs: &[std::path::PathBuf], output_path: &str) -> #[cfg(test)] mod tests { use super::{ - ffmpeg_args, ffmpeg_partial_output_path, parse_encoder_names, select_hardware_encoder, - HardwareSelection, + audio_tmp_dir_name, ffmpeg_args, ffmpeg_partial_output_path, parse_encoder_names, + select_hardware_encoder, HardwareSelection, }; + // ── audio scratch directory naming: not fully predictable from outside ── + + #[test] + fn audio_tmp_dir_name_differs_across_calls_that_share_pid_and_seq() { + // A pid+seq pair is small enough to pre-seed exhaustively from + // outside the process; folding in a nanosecond timestamp neither of + // those two alone carries means a name computed ahead of time from + // pid+seq no longer identifies the exact directory this process + // will actually create. + let a = audio_tmp_dir_name(1234, 0, 111); + let b = audio_tmp_dir_name(1234, 0, 222); + assert_ne!( + a, b, + "same pid+seq, different nanos, must differ: {a} vs {b}" + ); + } + + #[test] + fn audio_tmp_dir_name_is_stable_for_identical_inputs() { + assert_eq!(audio_tmp_dir_name(1, 2, 3), audio_tmp_dir_name(1, 2, 3)); + } + + /// Characterizes the exact property this fix depends on: swapping + /// `create_dir_all` for `create_dir` at the audio scratch directory's + /// creation site turns "adopt whatever is already there" into "refuse + /// outright" the moment something — attacker-planted symlink included — + /// already occupies that path. + #[test] + fn create_dir_refuses_an_already_occupied_path_that_create_dir_all_would_have_adopted() { + let path = std::env::temp_dir().join(format!( + "rustmotion_audit_ws_c_preexisting_dir_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let _ = std::fs::remove_dir_all(&path); + std::fs::create_dir(&path).expect("set up a pre-existing directory at the target path"); + + assert!( + std::fs::create_dir_all(&path).is_ok(), + "create_dir_all silently succeeding on a pre-existing directory is exactly the \ + behavior that let a hostile pre-planted directory (or symlink) be adopted" + ); + assert!( + std::fs::create_dir(&path).is_err(), + "create_dir must refuse the same pre-existing path instead of adopting it" + ); + + let _ = std::fs::remove_dir_all(&path); + } + // ── partial-output-path naming (pure) ──────────────────────────────────── #[test] diff --git a/crates/rustmotion/src/encode/video_audio.rs b/crates/rustmotion/src/encode/video_audio.rs index caac9e1e..01a1c033 100644 --- a/crates/rustmotion/src/encode/video_audio.rs +++ b/crates/rustmotion/src/encode/video_audio.rs @@ -255,6 +255,32 @@ pub fn build_atempo_filter(rate: f64) -> Option { // ─── Cache-keyed temp WAV path ──────────────────────────────────────────────── +/// Base directory the extracted-audio WAV cache lives under. +/// +/// `std::env::temp_dir()` is shared and, on most Unix systems, world-writable +/// — combined with `wav_cache_path`'s hash being deterministic (which it has +/// to be, for the cache to ever hit twice), a different local user could +/// compute the exact cache path ahead of time and plant content there before +/// this process ever ran. `dirs::cache_dir()` is per-user (`~/Library/Caches` +/// on macOS, `~/.cache` on Linux), so the same determinism that makes +/// caching useful stops doubling as a cross-user attack surface. Falls back +/// to `temp_dir()` only on a platform with no notion of a user cache +/// directory at all — still better than failing outright, and consistent +/// with every other fallback in this codebase preferring a degraded mode +/// over an unusable one. +fn wav_cache_base_dir() -> PathBuf { + let base = dirs::cache_dir() + .unwrap_or_else(std::env::temp_dir) + .join("rustmotion"); + let _ = std::fs::create_dir_all(&base); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let _ = std::fs::set_permissions(&base, std::fs::Permissions::from_mode(0o700)); + } + base +} + fn wav_cache_path(src: &str, trim_start: f64, trim_end: Option, rate: f64) -> PathBuf { let mut hasher = DefaultHasher::new(); src.hash(&mut hasher); @@ -280,7 +306,21 @@ fn wav_cache_path(src: &str, trim_start: f64, trim_end: Option, rate: f64) } let hash = hasher.finish(); - std::env::temp_dir().join(format!("rustmotion_vidaud_{:016x}.wav", hash)) + wav_cache_base_dir().join(format!("rustmotion_vidaud_{:016x}.wav", hash)) +} + +/// Whether `path` is safe to reuse as a cache hit: a genuine regular file, +/// not a symlink. `wav_cache_path` now resolves under a per-user directory +/// (see `wav_cache_base_dir`), which already rules out a *different* user +/// planting one; this additionally refuses to follow a symlink planted by +/// anything running as the *same* user (a compromised sibling process, or a +/// leftover from before that directory existed) into wherever it points. +/// `symlink_metadata` — unlike `Path::exists`/`std::fs::metadata` — reports +/// on the directory entry itself rather than whatever it resolves to. +fn cached_wav_is_trustworthy(path: &std::path::Path) -> bool { + std::fs::symlink_metadata(path) + .map(|m| m.file_type().is_file()) + .unwrap_or(false) } /// Scratch path ffmpeg writes to before a successful extraction is promoted @@ -319,8 +359,9 @@ fn extract_audio_to_wav( ) -> Option { let wav_path = wav_cache_path(src, trim_start, trim_end, rate); - // Reuse cached extraction. - if wav_path.exists() { + // Reuse cached extraction — but only a genuine regular file placed here + // by a previous extraction; see `cached_wav_is_trustworthy`. + if cached_wav_is_trustworthy(&wav_path) { return Some(wav_path); } @@ -488,6 +529,87 @@ mod tests { use super::*; use crate::loader::load_scenario_from_source; + // ── Cache directory: per-user, not the shared world-writable temp dir ──── + + #[test] + fn wav_cache_path_does_not_sit_directly_inside_the_bare_shared_temp_dir() { + let cached = wav_cache_path("foo.mp4", 0.0, None, 1.0); + let shared_temp = std::env::temp_dir(); + assert_ne!( + cached.parent(), + Some(shared_temp.as_path()), + "the cached WAV must live under a dedicated subdirectory, not directly inside the \ + shared temp dir a same-machine, different-user attacker can also write to: got {}", + cached.display() + ); + } + + // ── Cache entries must be verified, not merely `exists()` ──────────────── + + #[cfg(unix)] + #[test] + fn a_symlink_at_the_cache_path_is_never_trusted_as_a_cache_hit() { + let target = std::env::temp_dir().join(format!( + "rm_vidaud_symlink_target_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::write(&target, b"not a wav, planted by someone else").unwrap(); + + let link = std::env::temp_dir().join(format!( + "rm_vidaud_symlink_link_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let _ = std::fs::remove_file(&link); + std::os::unix::fs::symlink(&target, &link).expect("create symlink fixture"); + + assert!( + !cached_wav_is_trustworthy(&link), + "a symlink sitting at the cache path must never be treated as a valid cache hit, \ + regardless of what it points to" + ); + + let mut real_file = link.with_file_name(format!( + "rm_vidaud_real_file_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + real_file.set_extension("wav"); + std::fs::write(&real_file, b"RIFF....").unwrap(); + assert!( + cached_wav_is_trustworthy(&real_file), + "a genuine regular file must still be trusted" + ); + + let _ = std::fs::remove_file(&target); + let _ = std::fs::remove_file(&link); + let _ = std::fs::remove_file(&real_file); + } + + #[test] + fn a_missing_path_is_not_trustworthy() { + let path = std::env::temp_dir().join(format!( + "rm_vidaud_never_created_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let _ = std::fs::remove_file(&path); + assert!(!cached_wav_is_trustworthy(&path)); + } + // ── Helpers ────────────────────────────────────────────────────────────── fn load(json: &str) -> ResolvedScenario { From b83357fa25db44f4e8cac54da3067a51dd5abff0 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:44:04 +0200 Subject: [PATCH 5/5] fix(encode): apply the transition easing to progress Refs #220 --- crates/rustmotion/src/encode/video/formats.rs | 11 +- crates/rustmotion/src/encode/video/tasks.rs | 17 ++- crates/rustmotion/tests/audit_ws_c.rs | 141 ++++++++++++++++++ 3 files changed, 162 insertions(+), 7 deletions(-) diff --git a/crates/rustmotion/src/encode/video/formats.rs b/crates/rustmotion/src/encode/video/formats.rs index 8cad2f02..b3ab8730 100644 --- a/crates/rustmotion/src/encode/video/formats.rs +++ b/crates/rustmotion/src/encode/video/formats.rs @@ -359,12 +359,11 @@ mod tests { /// (an odd width rejected by libx264) doesn't apply here (the `gif` /// crate has no such constraint), so this drives the failure the one /// way this pure Rust path can actually fail without external tools: - /// `total_frames == 0`. - /// That returns before any file is touched either way, so what this - /// test really pins is the *shape* of the fix — `encode_gif` must never - /// promote a partial onto `output_path` when its inner call errors — - /// exercised by asserting the scratch/partial path used internally is - /// never left behind either. + /// `total_frames == 0`. That returns before any file is touched either + /// way, so what this test really pins is the *shape* of the fix — + /// `encode_gif` must never promote a partial onto `output_path` when its + /// inner call errors — exercised by asserting the scratch/partial path + /// used internally is never left behind either. #[test] fn encode_gif_leaves_no_partial_file_behind_on_an_empty_scenario() { let json = r#"{"video": {"width": 8, "height": 8, "fps": 10}, "scenes": []}"#; diff --git a/crates/rustmotion/src/encode/video/tasks.rs b/crates/rustmotion/src/encode/video/tasks.rs index ca6f8e6f..378174f5 100644 --- a/crates/rustmotion/src/encode/video/tasks.rs +++ b/crates/rustmotion/src/encode/video/tasks.rs @@ -1,3 +1,4 @@ +use crate::engine::animator::ease; use crate::engine::transition::{apply_transition, camera_pan_transition, TransitionOptions}; use crate::error::{Result, RustmotionError}; use crate::schema::{ @@ -296,6 +297,7 @@ pub fn render_frame_task_scaled( // Rationale: the transition is the "entry" of scene_b; its post-effects // (e.g. vignette) should appear on the blended frames to avoid a // jarring pop when the transition ends and Normal frames begin. + let progress = eased_transition_progress(progress, easing); let mut composited = apply_transition( &frame_a, &frame_b, @@ -360,7 +362,7 @@ pub fn render_frame_task_scaled( transition_type, options, transition_duration, - easing: _, + easing, } => { let scenario_time = *global_frame as f64 / config.fps as f64; let scaled_w = (config.width as f32 * scale_factor) as u32; @@ -372,6 +374,7 @@ pub fn render_frame_task_scaled( // does not. let progress = view_transition_progress(*frame_in_transition, *transition_duration, fps); + let progress = eased_transition_progress(progress, easing); let view_a = &scenario.views[*view_a_idx]; let view_b = &scenario.views[*view_b_idx]; @@ -427,6 +430,18 @@ fn transition_progress(frame_in_transition: u32, transition_duration: f64, fps: frame_in_transition as f64 / transition_frames.saturating_sub(1).max(1) as f64 } +/// Reshapes a transition's raw linear `progress` by its declared +/// `transition.easing` before it reaches `apply_transition`, which composites +/// pixels at exactly the fraction it's handed and has no notion of easing on +/// its own. Every transition arm that calls `apply_transition` needs this — +/// `SlideTransition`'s `CameraPan` case is the one exception, since it never +/// reaches `apply_transition` at all: it hands its own `easing` straight into +/// `camera_pan_transition`, which applies it internally, so routing it +/// through here first would ease that progress twice. +fn eased_transition_progress(progress: f64, easing: &EasingType) -> f64 { + ease(progress, easing) +} + /// Frame index within a *view* transition → progress in the OPEN interval /// `(0.0, 1.0)`, excluding both endpoints. /// diff --git a/crates/rustmotion/tests/audit_ws_c.rs b/crates/rustmotion/tests/audit_ws_c.rs index 0b18c5b3..a7db3ecd 100644 --- a/crates/rustmotion/tests/audit_ws_c.rs +++ b/crates/rustmotion/tests/audit_ws_c.rs @@ -154,3 +154,144 @@ fn atempo_guard_rejects_non_positive_and_non_finite_rates_without_hanging() { } } } + +// ─── A transition's declared easing must actually reshape its progress ──── +// +// `transition_progress`/`view_transition_progress` hand a raw *linear* +// fraction to `apply_transition`, which composites pixels at exactly the +// fraction it's given — it has no notion of the scenario's declared +// `transition.easing` on its own. Before this fix, that raw linear fraction +// reached `apply_transition` unmodified for every transition type except +// `camera_pan` (which threads easing through a different function, +// `camera_pan_transition`, that already applies it internally), so +// `"easing": "ease_in"`/`"ease_out"` on a `fade`, wipe, iris, or a +// between-view transition was silently a no-op. +// +// A `fade` between a solid-black and a solid-white full-canvas frame turns +// this into an exact, predictable pixel value: at the frame whose *linear* +// position in the transition is precisely 0.5, an eased blend must land at +// `255 * ease(0.5)`, not at the unequal, easing-blind `255 * 0.5 = 128`. +// `ease_in_cubic(0.5) = 0.125` and `ease_out_cubic(0.5) = 0.875` are both far +// enough from `0.5` that no rendering noise/antialiasing could produce a +// false pass. + +fn center_pixel_red(rgba: &[u8], width: u32, height: u32) -> u8 { + let idx = ((height / 2 * width + width / 2) * 4) as usize; + rgba[idx] +} + +#[test] +fn a_fade_transitions_easing_reshapes_its_progress_not_just_camera_pan() { + let width = 64u32; + let height = 64u32; + let fps = 11u32; + + for (easing, expected_u8) in [("ease_in", 32u8), ("ease_out", 224u8)] { + let json = format!( + r##"{{"video": {{"width": {width}, "height": {height}, "fps": {fps}}}, + "scenes": [ + {{"duration": 1.0, "children": [ + {{"type": "shape", "shape": "rect", "fill": "#000000", + "position": "absolute", "x": 0, "y": 0, + "style": {{"width": {width}, "height": {height}}}}} + ]}}, + {{"duration": 1.0, + "transition": {{"type": "fade", "duration": 1.0, "easing": "{easing}"}}, + "children": [ + {{"type": "shape", "shape": "rect", "fill": "#ffffff", + "position": "absolute", "x": 0, "y": 0, + "style": {{"width": {width}, "height": {height}}}}} + ]}} + ]}}"## + ); + let scenario = + rustmotion::loader::load_scenario_from_source(None, Some(&json)).expect("load"); + + let tasks = rustmotion::encode::video::build_frame_tasks(&scenario); + let target = tasks + .iter() + .find(|t| { + matches!( + t, + rustmotion::encode::video::FrameTask::SlideTransition { + frame_in_transition: 5, + .. + } + ) + }) + .unwrap_or_else(|| panic!("{easing}: expected a mid-transition frame at index 5")); + + let rgba = rustmotion::encode::video::render_frame_task(&scenario.video, &scenario, target) + .unwrap_or_else(|e| panic!("{easing}: render failed: {e}")); + let red = center_pixel_red(&rgba, width, height); + + assert!( + (red as i32 - expected_u8 as i32).abs() <= 4, + "{easing}: at the transition's linear midpoint, the eased blend must land near \ + {expected_u8}, got {red}" + ); + assert!( + (red as i32 - 128).abs() > 20, + "{easing}: {red} is too close to 128 — the raw, unequal linear-progress blend an \ + easing-blind composite would produce" + ); + } +} + +#[test] +fn a_between_view_fade_transitions_easing_reshapes_its_progress() { + let width = 64u32; + let height = 64u32; + let fps = 11u32; + + let json = format!( + r##"{{"video": {{"width": {width}, "height": {height}, "fps": {fps}}}, + "composition": [ + {{"type": "slide", "scenes": [ + {{"duration": 1.0, "children": [ + {{"type": "shape", "shape": "rect", "fill": "#000000", + "position": "absolute", "x": 0, "y": 0, + "style": {{"width": {width}, "height": {height}}}}} + ]}} + ]}}, + {{"type": "slide", + "transition": {{"type": "fade", "duration": 1.0, "easing": "ease_in"}}, + "scenes": [ + {{"duration": 1.0, "children": [ + {{"type": "shape", "shape": "rect", "fill": "#ffffff", + "position": "absolute", "x": 0, "y": 0, + "style": {{"width": {width}, "height": {height}}}}} + ]}} + ]}} + ]}}"## + ); + let scenario = rustmotion::loader::load_scenario_from_source(None, Some(&json)).expect("load"); + + let tasks = rustmotion::encode::video::build_frame_tasks(&scenario); + let target = tasks + .iter() + .find(|t| { + matches!( + t, + rustmotion::encode::video::FrameTask::ViewTransition { + frame_in_transition: 5, + .. + } + ) + }) + .expect("expected a mid-view-transition frame at index 5"); + + let rgba = rustmotion::encode::video::render_frame_task(&scenario.video, &scenario, target) + .expect("render"); + let red = center_pixel_red(&rgba, width, height); + + assert!( + (red as i32 - 32).abs() <= 4, + "ease_in at the view transition's linear midpoint must land near 32, got {red}" + ); + assert!( + (red as i32 - 128).abs() > 20, + "{red} is too close to 128 — the raw, unequal linear-progress blend an easing-blind \ + composite would produce" + ); +}