diff --git a/crates/rustmotion-core/src/error.rs b/crates/rustmotion-core/src/error.rs index 55e4a3f..02da951 100644 --- a/crates/rustmotion-core/src/error.rs +++ b/crates/rustmotion-core/src/error.rs @@ -161,8 +161,14 @@ pub enum RustmotionError { #[error("Failed to open FFmpeg stdin pipe")] FfmpegPipe, - #[error("Failed to write to FFmpeg pipe: {reason}")] - FfmpegWrite { reason: String }, + // A broken pipe here nearly always means ffmpeg already died on its own + // arguments, so the useful diagnostic is ffmpeg's stderr rather than our + // write error. Carry it in the error so it survives `--quiet`. + #[error("Failed to write to FFmpeg pipe: {reason}{}", .stderr.as_ref().map(|s| format!("\nffmpeg reported:\n{}", s)).unwrap_or_default())] + FfmpegWrite { + reason: String, + stderr: Option, + }, #[error("Failed to wait for FFmpeg: {reason}")] FfmpegWait { reason: String }, diff --git a/crates/rustmotion/src/encode/video/ffmpeg.rs b/crates/rustmotion/src/encode/video/ffmpeg.rs index af6fa2e..0884f76 100644 --- a/crates/rustmotion/src/encode/video/ffmpeg.rs +++ b/crates/rustmotion/src/encode/video/ffmpeg.rs @@ -10,6 +10,108 @@ use crate::schema::ResolvedScenario as Scenario; use super::tasks::{build_frame_tasks, render_frame_task}; use super::EncodeProgress; +/// Assemble FFmpeg's argument vector. +/// +/// The order is load-bearing. FFmpeg parses argv positionally: an option applies +/// to the *next* `-i` that follows it, or to the output when no input follows. So +/// the whole input section — including the audio input and its `-f s16le -ar -ac` +/// — has to be emitted before the first output option. Emitting the codec block +/// between the two inputs makes ffmpeg reject `-profile:v` as an input option for +/// audio.raw and refuse to start, which silently broke every scenario carrying an +/// audio track. +/// +/// Kept separate from the spawn so the ordering invariant is unit-testable without +/// an ffmpeg binary on the machine. +#[allow(clippy::too_many_arguments)] +fn ffmpeg_args( + width: u32, + height: u32, + fps: u32, + codec: &str, + crf_val: u8, + transparent: bool, + audio_input: Option<&str>, + output_path: &str, +) -> Vec { + let size = format!("{}x{}", width, height); + let framerate = fps.to_string(); + let crf = crf_val.to_string(); + let mut args: Vec = Vec::new(); + fn push(xs: &[&str], out: &mut Vec) { + out.extend(xs.iter().map(|s| (*s).to_string())) + } + + // ---- inputs ------------------------------------------------------------ + push(&["-y", "-loglevel", "error"], &mut args); + push(&["-f", "rawvideo", "-pixel_format", "rgba"], &mut args); + push(&["-video_size", &size], &mut args); + push(&["-framerate", &framerate], &mut args); + push(&["-i", "pipe:0"], &mut args); + + if let Some(path) = audio_input { + push( + &["-f", "s16le", "-ar", "44100", "-ac", "2", "-i", path], + &mut args, + ); + } + + // ---- output options ---------------------------------------------------- + let alpha_fmt = |with: &'static str, without: &'static str| { + if transparent { + with + } else { + without + } + }; + match codec { + "h265" | "hevc" => { + push( + &["-c:v", "libx265", "-crf", &crf, "-preset", "medium"], + &mut args, + ); + push(&["-pix_fmt", alpha_fmt("yuva420p", "yuv420p")], &mut args); + } + "vp9" => { + push( + &["-c:v", "libvpx-vp9", "-crf", &crf, "-b:v", "0"], + &mut args, + ); + push(&["-pix_fmt", alpha_fmt("yuva420p", "yuv420p")], &mut args); + } + "prores" => { + push(&["-c:v", "prores_ks", "-profile:v", "4"], &mut args); + push( + &["-pix_fmt", alpha_fmt("yuva444p10le", "yuv422p10le")], + &mut args, + ); + } + _ => { + push( + &[ + "-c:v", + "libx264", + "-crf", + &crf, + "-preset", + "medium", + "-profile:v", + "high10", + "-pix_fmt", + "yuv420p10le", + ], + &mut args, + ); + } + } + + if audio_input.is_some() { + push(&["-c:a", "aac", "-b:a", "128k"], &mut args); + } + + args.push(output_path.to_string()); + args +} + /// Encode using FFmpeg subprocess (for h265, vp9, prores, webm, mov, transparency) pub fn encode_with_ffmpeg( scenario: &Scenario, @@ -61,106 +163,38 @@ pub fn encode_with_ffmpeg( None }; + // Materialise the mixed PCM before the command is assembled: the audio input + // has to be declared next to the video input, ahead of every output option. + let audio_input: Option = match (&pcm_data, &audio_tmp_dir) { + (Some(pcm), Some(tmp_dir)) => { + let audio_path = tmp_dir.join("audio.raw"); + std::fs::write(&audio_path, pcm)?; + Some( + audio_path + .to_str() + .ok_or_else(|| RustmotionError::NonUtf8Path { + path: audio_path.to_string_lossy().into_owned(), + })? + .to_owned(), + ) + } + _ => None, + }; + // Build FFmpeg command let crf_val = crf.unwrap_or(23); let mut cmd = std::process::Command::new("ffmpeg"); - cmd.args([ - "-y", - "-loglevel", - "error", - "-f", - "rawvideo", - "-pixel_format", - "rgba", - "-video_size", - &format!("{}x{}", width, height), - "-framerate", - &fps.to_string(), - "-i", - "pipe:0", - ]); - - match codec { - "h265" | "hevc" => { - cmd.args([ - "-c:v", - "libx265", - "-crf", - &crf_val.to_string(), - "-preset", - "medium", - ]); - if transparent { - cmd.args(["-pix_fmt", "yuva420p"]); - } else { - cmd.args(["-pix_fmt", "yuv420p"]); - } - } - "vp9" => { - cmd.args([ - "-c:v", - "libvpx-vp9", - "-crf", - &crf_val.to_string(), - "-b:v", - "0", - ]); - if transparent { - cmd.args(["-pix_fmt", "yuva420p"]); - } else { - cmd.args(["-pix_fmt", "yuv420p"]); - } - } - "prores" => { - cmd.args(["-c:v", "prores_ks", "-profile:v", "4"]); - if transparent { - cmd.args(["-pix_fmt", "yuva444p10le"]); - } else { - cmd.args(["-pix_fmt", "yuv422p10le"]); - } - } - _ => { - cmd.args([ - "-c:v", - "libx264", - "-crf", - &crf_val.to_string(), - "-preset", - "medium", - "-profile:v", - "high10", - "-pix_fmt", - "yuv420p10le", - ]); - } - } - - if let Some(ref pcm) = pcm_data { - let audio_path = audio_tmp_dir.as_ref().unwrap().join("audio.raw"); - std::fs::write(&audio_path, pcm)?; - let audio_path_str = audio_path - .to_str() - .ok_or_else(|| RustmotionError::NonUtf8Path { - path: audio_path.to_string_lossy().into_owned(), - })?; - cmd.args([ - "-f", - "s16le", - "-ar", - "44100", - "-ac", - "2", - "-i", - audio_path_str, - "-c:a", - "aac", - "-b:a", - "128k", - ]); - } - - cmd.arg(output_path); + cmd.args(ffmpeg_args( + width, + height, + fps, + codec, + crf_val, + transparent, + audio_input.as_deref(), + output_path, + )); cmd.stdin(std::process::Stdio::piped()); cmd.stdout(std::process::Stdio::null()); // Always capture stderr so failures surface a useful diagnostic. We tee to @@ -205,6 +239,7 @@ pub fn encode_with_ffmpeg( if let Err(e) = stdin.write_all(&rgba) { pipe_error = Some(RustmotionError::FfmpegWrite { reason: e.to_string(), + stderr: None, // filled in below, once stderr is drained }); break; } @@ -239,7 +274,18 @@ pub fn encode_with_ffmpeg( let _ = std::fs::remove_dir_all(tmp_dir); } - if let Some(e) = pipe_error { + // ffmpeg's actual complaint sits in the last few lines of stderr. Build the + // summary once: every failure path needs it, and `--quiet` must not be the + // difference between a diagnosable error and "Broken pipe". + let stderr_summary = stderr_text + .as_ref() + .map(|s| { + let lines: Vec<&str> = s.lines().rev().take(8).collect(); + lines.into_iter().rev().collect::>().join("\n") + }) + .filter(|s| !s.trim().is_empty()); + + let tee_stderr = || { if !quiet { if let Some(ref text) = stderr_text { if !text.trim().is_empty() { @@ -247,27 +293,23 @@ pub fn encode_with_ffmpeg( } } } - return Err(e); + }; + + if let Some(e) = pipe_error { + tee_stderr(); + // 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 { + RustmotionError::FfmpegWrite { reason, .. } => RustmotionError::FfmpegWrite { + reason, + stderr: stderr_summary, + }, + other => other, + }); } if !status.success() { - // Extract the last few lines of stderr — ffmpeg's actual error message - // typically appears in the final 5-10 lines. - let stderr_summary = stderr_text - .as_ref() - .map(|s| { - let lines: Vec<&str> = s.lines().rev().take(8).collect(); - lines.into_iter().rev().collect::>().join("\n") - }) - .filter(|s| !s.trim().is_empty()); - - if !quiet { - if let Some(ref text) = stderr_text { - if !text.trim().is_empty() { - eprintln!("{}", text); - } - } - } + tee_stderr(); return Err(RustmotionError::FfmpegFailed { stderr: stderr_summary, }); @@ -275,3 +317,79 @@ pub fn encode_with_ffmpeg( Ok(()) } + +#[cfg(test)] +mod tests { + use super::ffmpeg_args; + + /// 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, + /// then aborts with "Option ... cannot be applied to input url". + const OUTPUT_OPTS: [&str; 6] = ["-c:v", "-crf", "-preset", "-profile:v", "-c:a", "-b:a"]; + + fn input_positions(args: &[String]) -> Vec { + args.iter() + .enumerate() + .filter(|(_, s)| s.as_str() == "-i") + .map(|(i, _)| i) + .collect() + } + + #[test] + fn the_audio_input_is_declared_before_every_output_option() { + for codec in ["h264", "h265", "vp9", "prores"] { + let args = ffmpeg_args(320, 240, 30, codec, 23, false, Some("/tmp/a.raw"), "o.mp4"); + let inputs = input_positions(&args); + assert_eq!( + inputs.len(), + 2, + "{codec}: expected a video and an audio input" + ); + + // The audio input keeps its own format options immediately ahead of it. + let audio_i = inputs[1]; + assert_eq!(args[audio_i - 1], "2", "{codec}: -ac lost before audio -i"); + assert_eq!(args[audio_i + 1], "/tmp/a.raw"); + + for opt in OUTPUT_OPTS { + if let Some(pos) = args.iter().position(|s| s == opt) { + assert!( + pos > audio_i, + "{codec}: {opt} is emitted at {pos}, before the audio input at {audio_i} — \ + ffmpeg would read it as an option of audio.raw and refuse to start" + ); + } + } + assert_eq!( + args.last().unwrap(), + "o.mp4", + "{codec}: output must be last" + ); + } + } + + #[test] + fn a_silent_scenario_declares_a_single_input_and_no_audio_codec() { + let args = ffmpeg_args(320, 240, 30, "h264", 23, false, None, "o.mp4"); + assert_eq!(input_positions(&args).len(), 1); + assert!(!args.iter().any(|s| s == "-c:a" || s == "-b:a")); + assert_eq!(args.last().unwrap(), "o.mp4"); + } + + #[test] + fn transparency_selects_an_alpha_pixel_format() { + for (codec, opaque, alpha) in [ + ("h265", "yuv420p", "yuva420p"), + ("vp9", "yuv420p", "yuva420p"), + ("prores", "yuv422p10le", "yuva444p10le"), + ] { + let pix = |t: bool| { + let a = ffmpeg_args(320, 240, 30, codec, 23, t, None, "o.mov"); + let i = a.iter().position(|s| s == "-pix_fmt").unwrap(); + a[i + 1].clone() + }; + assert_eq!(pix(false), opaque, "{codec} opaque"); + assert_eq!(pix(true), alpha, "{codec} transparent"); + } + } +}