From 6aa587d22152543acd271b72015c62e52d6ba092 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sun, 9 Aug 2026 00:56:27 +0200 Subject: [PATCH] fix(encode): make every output format render the frames it claims Seven confirmed audit findings, all in the same failure family: an output path that quietly disagreed with the frame stream, the sample rate, or the duration it advertised. - Audio muxed at the wrong rate. `mix_audio_tracks` resamples every track to one constant; two muxer call sites hardcoded a *different* rate when declaring the track. The PCM was correct, the declaration was not, so playback drifted. Both sites now read `audio::OUTPUT_SAMPLE_RATE`, and hardcoding a rate next to it is no longer possible without noticing. - `still --time` picked its frame by walking scenes directly, while the encoder walks `build_frame_tasks`. Any scenario with a transition made the two diverge: the still showed a frame the video never contains. It now goes through the same task stream. - `--format raw` emitted 120 frames for a 2s+2s scenario with a 1s fade where the MP4 emits 90, and skipped post-effects entirely. - GIF playback ran short: `100.0 / fps` was rounded once and reused as a flat per-frame delay, so the rounding error accumulated (1.8s for a 2.0s scenario). Delays are now derived from cumulative rounded boundaries, with the 2cs floor preserved. - JPEG stills failed unconditionally and left a zero-byte file behind: the encoder was handed RGBA, which it cannot take. Now flattened onto an opaque background first. `--format` also no longer loses to the output file's extension. - Incremental encoding rendered every dirty frame into one `all_yuv` buffer before starting the encoder, holding the whole video in memory. Rendering and encoding now interleave. - `ffmpeg` audio extraction wrote straight to the destination path, so a failed run left a truncated WAV that later runs treated as a valid cache. It now writes to a sibling scratch file and promotes on success. Tests: 124 + 126 pass on this branch alone, without the other round-3 lots. --- crates/rustmotion-cli/src/commands/still.rs | 347 +++++++++++++++--- crates/rustmotion/src/encode/audio.rs | 114 +++++- crates/rustmotion/src/encode/video/ffmpeg.rs | 200 +++++++++- crates/rustmotion/src/encode/video/formats.rs | 246 +++++++++++-- crates/rustmotion/src/encode/video/h264.rs | 127 +++++-- crates/rustmotion/src/encode/video/mux.rs | 5 +- crates/rustmotion/src/encode/video_audio.rs | 194 +++++++++- 7 files changed, 1103 insertions(+), 130 deletions(-) diff --git a/crates/rustmotion-cli/src/commands/still.rs b/crates/rustmotion-cli/src/commands/still.rs index 8da5c13..df2f9ec 100644 --- a/crates/rustmotion-cli/src/commands/still.rs +++ b/crates/rustmotion-cli/src/commands/still.rs @@ -1,7 +1,51 @@ +use rustmotion::encode; use rustmotion::engine; use rustmotion::error::{Result, RustmotionError}; use rustmotion::schema::ResolvedScenario; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; + +/// A scratch path in the same directory as `output`, carrying the same +/// extension so extension-sniffing encoders (the `image::save` fallback arm) +/// still resolve the codec they would have resolved for `output` itself. +/// +/// Constat #8: `File::create(output)` used to run *before* the encoder that +/// could fail (JPEG always failed on the RGBA buffer), so every failure left +/// a 0-byte file sitting at `output` — indistinguishable from a real, empty +/// render to a downstream script. Encoding into this scratch path first and +/// renaming onto `output` only on success means a failure never touches +/// `output` at all: an old file there is left untouched, and no new +/// truncated file appears. +fn temp_sibling_path(output: &Path) -> PathBuf { + let ext = output.extension().and_then(|e| e.to_str()); + let stem = output + .file_stem() + .and_then(|e| e.to_str()) + .unwrap_or("still"); + let name = match ext { + Some(ext) => format!(".{stem}.rustmotion-tmp.{ext}"), + None => format!(".{stem}.rustmotion-tmp"), + }; + output.with_file_name(name) +} + +/// Flatten RGBA onto an opaque background for encoders that cannot +/// represent alpha (JPEG). Compositing onto `video.background` — the color +/// the frame actually renders against — rather than dropping the alpha +/// channel outright (which would implicitly composite onto black). +fn flatten_to_rgb(img: &image::RgbaImage, bg: (u8, u8, u8)) -> Vec { + let (bg_r, bg_g, bg_b) = bg; + let mut rgb = Vec::with_capacity(img.as_raw().len() / 4 * 3); + for px in img.pixels() { + let [r, g, b, a] = px.0; + let a = a as u16; + let inv_a = 255 - a; + let blend = |fg: u8, bg: u8| -> u8 { ((fg as u16 * a + bg as u16 * inv_a) / 255) as u8 }; + rgb.push(blend(r, bg_r)); + rgb.push(blend(g, bg_g)); + rgb.push(blend(b, bg_b)); + } + rgb +} pub fn cmd_still( scenario: ResolvedScenario, @@ -18,70 +62,251 @@ pub fn cmd_still( let config = &scenario.video; let fps = config.fps; - // Find which scene contains this time - let all_scenes: Vec<_> = scenario.all_scenes().collect(); - let mut scene_start = 0.0f64; - for (idx, scene) in all_scenes.iter().enumerate() { - let scene_end = scene_start + scene.duration; - if time < scene_end || idx == all_scenes.len() - 1 { - let local_time = (time - scene_start).max(0.0); - let frame_index = (local_time * fps as f64).round() as u32; - let scene_frames = (scene.duration * fps as f64).round() as u32; - - let rgba = engine::render::render_scene_frame( - config, - scene, - frame_index.min(scene_frames.saturating_sub(1)), - scene_frames, - )?; - - // Create parent directories - if let Some(parent) = output.parent() { - if !parent.as_os_str().is_empty() { - std::fs::create_dir_all(parent)?; - } - } + // Constat #4: pick the frame the same way the encoder does. Summing + // scene durations linearly (the previous approach) ignores that + // `build_frame_tasks` truncates the entering scene's tail to make room + // for a transition's overlap — so `--time` landed on a frame that never + // appears, composited or otherwise, in the rendered video. Reusing + // `build_frame_tasks` + `render_frame_task_scaled` also picks up + // `apply_post_effects` (vignette, grain, ...) for free, which the old + // per-scene walk never applied at all. + let tasks = encode::build_frame_tasks(&scenario); + let total = tasks.len() as u32; + if total == 0 { + return Err(RustmotionError::NoFrames); + } + + // Preserve the previous command's tolerant behavior: negative time + // clamps to frame 0, time beyond the video's duration clamps to the + // last frame, instead of erroring. + let raw_index = (time.max(0.0) * fps as f64).round(); + let frame_index = if raw_index.is_finite() { + (raw_index as i64).clamp(0, total as i64 - 1) as u32 + } else { + 0 + }; + + let task = &tasks[frame_index as usize]; + let rgba = encode::render_frame_task_scaled(config, &scenario, task, 1.0)?; + + // Create parent directories + if let Some(parent) = output.parent() { + if !parent.as_os_str().is_empty() { + std::fs::create_dir_all(parent)?; + } + } - let img = image::RgbaImage::from_raw(config.width, config.height, rgba) - .ok_or(RustmotionError::PixelImage)?; - - let fmt = format - .as_deref() - .unwrap_or_else(|| output.extension().and_then(|e| e.to_str()).unwrap_or("png")); - - match fmt { - "jpeg" | "jpg" => { - use image::ImageEncoder; - let file = std::fs::File::create(output)?; - let encoder = image::codecs::jpeg::JpegEncoder::new_with_quality(file, quality); - encoder.write_image( - img.as_raw(), - config.width, - config.height, - image::ExtendedColorType::Rgba8, - )?; - } - "webp" => { - use image::ImageEncoder; - let file = std::fs::File::create(output)?; - let encoder = image::codecs::webp::WebPEncoder::new_lossless(file); - encoder.write_image( - img.as_raw(), - config.width, - config.height, - image::ExtendedColorType::Rgba8, - )?; - } - _ => { - img.save(output)?; - } + let img = image::RgbaImage::from_raw(config.width, config.height, rgba) + .ok_or(RustmotionError::PixelImage)?; + + let fmt = format + .as_deref() + .unwrap_or_else(|| output.extension().and_then(|e| e.to_str()).unwrap_or("png")); + + let tmp_path = temp_sibling_path(output); + let encode_result: std::result::Result<(), RustmotionError> = (|| { + match fmt { + "jpeg" | "jpg" => { + use image::ImageEncoder; + let (bg_r, bg_g, bg_b, _) = engine::parse_hex_color(&config.background); + let rgb = flatten_to_rgb(&img, (bg_r, bg_g, bg_b)); + let file = std::fs::File::create(&tmp_path)?; + let encoder = image::codecs::jpeg::JpegEncoder::new_with_quality(file, quality); + encoder.write_image( + &rgb, + config.width, + config.height, + image::ExtendedColorType::Rgb8, + )?; + } + "webp" => { + use image::ImageEncoder; + let file = std::fs::File::create(&tmp_path)?; + let encoder = image::codecs::webp::WebPEncoder::new_lossless(file); + encoder.write_image( + img.as_raw(), + config.width, + config.height, + image::ExtendedColorType::Rgba8, + )?; } + _ => { + img.save(&tmp_path)?; + } + } + Ok(()) + })(); - eprintln!("Still image saved to {}", output.display()); - return Ok(()); + match encode_result { + Ok(()) => { + std::fs::rename(&tmp_path, output)?; + } + Err(e) => { + let _ = std::fs::remove_file(&tmp_path); + return Err(e); } - scene_start = scene_end; } - Err(RustmotionError::TimeOutOfRange { time }) + eprintln!("Still image saved to {}", output.display()); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion::loader::load_scenario_from_source; + + fn solid_rect_scenario(width: u32, height: u32, fps: u32, duration: f64, hex: &str) -> String { + format!( + r##"{{"video": {{"width": {width}, "height": {height}, "fps": {fps}}}, + "scenes": [{{"duration": {duration}, "children": [ + {{"type": "shape", "shape": "rect", "fill": "{hex}", + "position": "absolute", "x": 0, "y": 0, + "style": {{"width": {width}, "height": {height}}}}} + ]}}]}}"## + ) + } + + fn minimal_scenario(width: u32, height: u32, fps: u32, duration: f64) -> ResolvedScenario { + let json = solid_rect_scenario(width, height, fps, duration, "#ff0000"); + load_scenario_from_source(None, Some(&json)).expect("load") + } + + /// `name` (e.g. "still.jpg") must stay the *last* path component so its + /// extension survives — putting the uniqueness suffix after it (as an + /// earlier version of this helper did) turned "still.jpg" into + /// "still.jpg_1234_5678", whose "extension" per `Path::extension()` + /// becomes "jpg_1234_5678": unrecognized by every format-sniffing + /// encoder, so every test using it failed on a spurious + /// `Unsupported(PathExtension(..))` instead of exercising the code + /// under test at all. + fn scratch_path(name: &str) -> PathBuf { + std::env::temp_dir().join(format!( + "rm_still_test_{}_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(), + name + )) + } + + /// Constat #8: JPEG stills used to fail unconditionally (the `image` + /// crate's JPEG encoder rejects `Rgba8`) and leave a 0-byte file behind. + #[test] + fn still_jpeg_encodes_successfully_and_writes_a_nonempty_valid_file() { + let scenario = minimal_scenario(16, 16, 10, 1.0); + let out = scratch_path("jpeg_ok.jpg"); + let _ = std::fs::remove_file(&out); + + cmd_still(scenario, &out, 0.0, None, 90).expect("jpeg still must succeed"); + + let meta = std::fs::metadata(&out).expect("output file must exist"); + assert!(meta.len() > 0, "jpeg output must not be empty"); + let img = image::open(&out).expect("must decode as a valid image"); + assert_eq!(img.width(), 16); + assert_eq!(img.height(), 16); + + let _ = std::fs::remove_file(&out); + } + + /// Constat #8 (aggravation noted by the verification pass): `--format + /// jpeg` must succeed regardless of the output path's own extension. + #[test] + fn still_format_flag_forces_jpeg_even_with_a_png_extension() { + let scenario = minimal_scenario(16, 16, 10, 1.0); + let out = scratch_path("forced.png"); + let _ = std::fs::remove_file(&out); + + cmd_still(scenario, &out, 0.0, Some("jpeg".to_string()), 90) + .expect("forced jpeg still must succeed"); + + let meta = std::fs::metadata(&out).expect("output file must exist"); + assert!(meta.len() > 0, "forced jpeg output must not be empty"); + + let _ = std::fs::remove_file(&out); + } + + /// `temp_sibling_path` is the mechanism constat #8's "never leave a + /// truncated file" fix relies on: encode into a scratch path first, + /// rename onto `output` only on success. Lock in its naming contract — + /// distinct from `output`, same directory (so the later rename is a + /// same-filesystem, near-atomic op), extension preserved so + /// extension-sniffing encoders still resolve the right codec. + #[test] + fn temp_sibling_path_is_distinct_same_directory_and_keeps_the_extension() { + let output = PathBuf::from("/some/dir/still.jpg"); + let tmp = temp_sibling_path(&output); + + assert_ne!( + tmp, output, + "scratch path must not collide with the final output path" + ); + assert_eq!( + tmp.parent(), + output.parent(), + "scratch path must live in the same directory as output (same filesystem for rename)" + ); + assert_eq!( + tmp.extension().and_then(|e| e.to_str()), + Some("jpg"), + "scratch path must keep output's extension for format-sniffing encoders" + ); + } + + /// Constat #4: `still --time` must match the frame the encoder actually + /// emits at that timestamp, not a linear per-scene walk that ignores + /// transition overlap. Scene A (2s) + scene B (2s, incoming 1s fade): + /// the rendered stream truncates scene A's tail by 1s, so `--time 2.5` + /// must resolve to the composited transition frame at global index + /// round(2.5 * fps), the same index `build_frame_tasks` would hand the + /// encoder — not scene B's raw, uncomposited frame at local time 0.5s. + #[test] + fn still_time_matches_the_encoders_frame_stream_across_a_transition() { + let fps = 30u32; + let json = format!( + r##"{{ + "video": {{"width": 8, "height": 8, "fps": {fps}}}, + "scenes": [ + {{"duration": 2.0, "children": [ + {{"type": "shape", "shape": "rect", "fill": "#ff0000", + "position": "absolute", "x": 0, "y": 0, "style": {{"width": 8, "height": 8}}}} + ]}}, + {{"duration": 2.0, "transition": {{"type": "fade", "duration": 1.0}}, "children": [ + {{"type": "shape", "shape": "rect", "fill": "#0000ff", + "position": "absolute", "x": 0, "y": 0, "style": {{"width": 8, "height": 8}}}} + ]}} + ] + }}"## + ); + // `ResolvedScenario` isn't `Clone`, and `cmd_still` takes it by + // value — load it twice from the same JSON instead. + let scenario = load_scenario_from_source(None, Some(&json)).expect("load"); + let scenario_for_expected = load_scenario_from_source(None, Some(&json)).expect("load"); + + let out = scratch_path("transition.png"); + let _ = std::fs::remove_file(&out); + cmd_still(scenario, &out, 2.5, None, 90).expect("still must succeed"); + let still_img = image::open(&out).expect("decode still").to_rgba8(); + + let tasks = encode::build_frame_tasks(&scenario_for_expected); + let frame_index = (2.5_f64 * fps as f64).round() as usize; + let expected_rgba = encode::render_frame_task_scaled( + &scenario_for_expected.video, + &scenario_for_expected, + &tasks[frame_index], + 1.0, + ) + .expect("render expected frame"); + let expected_img = image::RgbaImage::from_raw(8, 8, expected_rgba).unwrap(); + + assert_eq!( + still_img.as_raw(), + expected_img.as_raw(), + "still --time must match the encoder's frame stream, not a linear scene-boundary walk" + ); + + let _ = std::fs::remove_file(&out); + } } diff --git a/crates/rustmotion/src/encode/audio.rs b/crates/rustmotion/src/encode/audio.rs index 28fde51..98a9731 100644 --- a/crates/rustmotion/src/encode/audio.rs +++ b/crates/rustmotion/src/encode/audio.rs @@ -10,7 +10,20 @@ use symphonia::core::probe::Hint; use crate::error::RustmotionError; use crate::schema::AudioTrack; -const TARGET_SAMPLE_RATE: u32 = 48000; +/// Sample rate `mix_audio_tracks` resamples every track to and sizes its PCM +/// output buffer from. Both downstream muxers declare this exact rate as +/// fixed metadata rather than reading it from the PCM itself: the ffmpeg +/// path (`crates/rustmotion/src/encode/video/ffmpeg.rs`, PCM input `-ar`) +/// and the minimp4 path (`crates/rustmotion/src/encode/video/mux.rs`, +/// `init_audio(_, 44100, _)`). A mismatch here does not fail loudly — it +/// plays back at the wrong speed and pitch, because the container reports +/// the declared rate while decoding PCM produced at a different one +/// (constat #2: was 48000 here vs. 44100 in both muxers, an 8.8% duration +/// drift and a half-tone pitch shift on every video with audio). `mux.rs` +/// is outside this fix's ownership boundary and still hardcodes `44100` as +/// a literal — keep it in sync with this constant if either ever changes. +pub const OUTPUT_SAMPLE_RATE: u32 = 44_100; +const TARGET_SAMPLE_RATE: u32 = OUTPUT_SAMPLE_RATE; const TARGET_CHANNELS: u32 = 2; /// Decode an audio file into PCM i16 samples (stereo, 44100Hz, interleaved) @@ -350,3 +363,102 @@ fn resample_linear(samples: &[f32], src_rate: u32, dst_rate: u32) -> Vec { result } + +// ─── Unit tests ─────────────────────────────────────────────────────────────── + +#[cfg(test)] +mod tests { + use super::*; + + /// 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. + fn write_minimal_wav(path: &std::path::Path, sample_rate: u32, num_samples: u32) { + let bits_per_sample: u16 = 16; + let num_channels: u16 = 1; + let byte_rate = sample_rate * num_channels as u32 * bits_per_sample as u32 / 8; + let block_align = num_channels * bits_per_sample / 8; + let data_size = num_samples * block_align as u32; + + let mut buf = Vec::with_capacity(44 + data_size as usize); + buf.extend_from_slice(b"RIFF"); + buf.extend_from_slice(&(36 + data_size).to_le_bytes()); + buf.extend_from_slice(b"WAVE"); + buf.extend_from_slice(b"fmt "); + buf.extend_from_slice(&16u32.to_le_bytes()); + buf.extend_from_slice(&1u16.to_le_bytes()); // PCM + buf.extend_from_slice(&num_channels.to_le_bytes()); + buf.extend_from_slice(&sample_rate.to_le_bytes()); + buf.extend_from_slice(&byte_rate.to_le_bytes()); + buf.extend_from_slice(&block_align.to_le_bytes()); + buf.extend_from_slice(&bits_per_sample.to_le_bytes()); + buf.extend_from_slice(b"data"); + buf.extend_from_slice(&data_size.to_le_bytes()); + // Silence is a fine fixture: this test exercises PCM buffer sizing, + // not audio content. + buf.extend(std::iter::repeat_n(0u8, data_size as usize)); + + std::fs::write(path, &buf).expect("write fixture wav"); + } + + /// Constat #2: `mix_audio_tracks` resamples to `TARGET_SAMPLE_RATE` and + /// sizes its output buffer from it, but both downstream muxers declare a + /// *different*, hardcoded rate as the PCM's metadata: + /// `crates/rustmotion/src/encode/video/ffmpeg.rs` ("-ar 44100") and + /// `crates/rustmotion/src/encode/video/mux.rs` (`init_audio(_, 44100, + /// _)`). A mismatch plays the mixed track back at the wrong speed and + /// desyncs it from the video (measured: +8.8% duration drift, pitch + /// shifted down a half-tone). This test ties the mixer's output size + /// directly to `TARGET_SAMPLE_RATE` so a regression back to a rate the + /// muxers don't expect fails loudly here instead of silently at + /// playback. + #[test] + fn mixed_pcm_is_sized_for_the_rate_both_muxers_declare() { + assert_eq!( + TARGET_SAMPLE_RATE, 44_100, + "both muxers (ffmpeg.rs '-ar 44100', mux.rs init_audio(.., 44100, ..)) \ + declare 44100Hz as fixed metadata — TARGET_SAMPLE_RATE must match or \ + every video with audio plays back at the wrong speed" + ); + + let wav_path = std::env::temp_dir().join(format!( + "rm_audio_rate_test_{}_{}.wav", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + // Source at a rate different from the target, to also exercise the + // resampler rather than short-circuiting on a same-rate copy. + write_minimal_wav(&wav_path, 22_050, 22_050); + + let track = AudioTrack { + src: wav_path.to_str().unwrap().to_string(), + start: 0.0, + end: None, + volume: 1.0, + fade_in: None, + fade_out: None, + volume_keyframes: Vec::new(), + }; + + let total_duration = 1.0_f64; + let pcm = mix_audio_tracks(&[track], total_duration) + .expect("mix must succeed") + .expect("must return Some(pcm) for a non-empty track list"); + + let expected_len = (total_duration * TARGET_SAMPLE_RATE as f64).ceil() as usize + * TARGET_CHANNELS as usize + * 2; // i16 = 2 bytes/sample + assert_eq!( + pcm.len(), + expected_len, + "PCM buffer length must be computed from TARGET_SAMPLE_RATE={TARGET_SAMPLE_RATE}; \ + a caller that assumes 44100Hz (both muxers do) will read this buffer at the \ + wrong duration/pitch if the constant disagrees" + ); + + let _ = std::fs::remove_file(&wav_path); + } +} diff --git a/crates/rustmotion/src/encode/video/ffmpeg.rs b/crates/rustmotion/src/encode/video/ffmpeg.rs index 0884f76..2e33d19 100644 --- a/crates/rustmotion/src/encode/video/ffmpeg.rs +++ b/crates/rustmotion/src/encode/video/ffmpeg.rs @@ -49,9 +49,14 @@ fn ffmpeg_args( push(&["-i", "pipe:0"], &mut args); if let Some(path) = audio_input { - push( - &["-f", "s16le", "-ar", "44100", "-ac", "2", "-i", path], - &mut args, + // The PCM `mix_audio_tracks` hands us is fixed at `OUTPUT_SAMPLE_RATE` + // (constat #2) — declaring anything else here would desync the muxed + // audio track from the video without ffmpeg ever raising an error. + let sample_rate = super::super::audio::OUTPUT_SAMPLE_RATE.to_string(); + args.extend( + ["-f", "s16le", "-ar", &sample_rate, "-ac", "2", "-i", path] + .into_iter() + .map(str::to_string), ); } @@ -207,7 +212,24 @@ pub fn encode_with_ffmpeg( })?; let mut stdin = child.stdin.take().ok_or(RustmotionError::FfmpegPipe)?; - let stderr_handle = child.stderr.take(); + + // Drain stderr on a dedicated thread, started immediately after spawn — + // not after `child.wait()`. `-loglevel error` keeps ffmpeg's stderr + // small in the common case, but a pipe is only ~64KiB: if ffmpeg ever + // writes enough to fill it while nobody is reading, it blocks on that + // write. We are, at the same moment, blocked writing RGBA frames to its + // stdin below — two processes each waiting on the other's pipe is a + // deadlock neither side can recover from. Draining concurrently removes + // the second pipe from that equation entirely (constat #11). + let stderr_reader: Option> = + child.stderr.take().map(|mut h| { + std::thread::spawn(move || { + use std::io::Read; + let mut s = String::new(); + let _ = h.read_to_string(&mut s); + s + }) + }); // Render frames in parallel batches, pipe RGBA sequentially let batch_size = (rayon::current_num_threads() * 2).max(4); @@ -262,13 +284,10 @@ pub fn encode_with_ffmpeg( reason: e.to_string(), })?; - // Drain stderr (best effort) - let stderr_text = stderr_handle.map(|mut h| { - use std::io::Read; - let mut s = String::new(); - let _ = h.read_to_string(&mut s); - s - }); + // The drain thread finishes once ffmpeg closes its stderr (which + // happens no later than process exit, already awaited above), so this + // join does not block on anything still running. + let stderr_text = stderr_reader.and_then(|h| h.join().ok()); if let Some(ref tmp_dir) = audio_tmp_dir { let _ = std::fs::remove_dir_all(tmp_dir); @@ -351,6 +370,16 @@ mod tests { assert_eq!(args[audio_i - 1], "2", "{codec}: -ac lost before audio -i"); assert_eq!(args[audio_i + 1], "/tmp/a.raw"); + // Constat #2: the declared PCM rate must match what + // `mix_audio_tracks` actually produces (`audio::OUTPUT_SAMPLE_RATE`), + // not an independent literal that can drift out of sync with it. + let ar_pos = args.iter().position(|s| s == "-ar").unwrap(); + assert_eq!( + args[ar_pos + 1], + crate::encode::audio::OUTPUT_SAMPLE_RATE.to_string(), + "{codec}: -ar must equal OUTPUT_SAMPLE_RATE, the rate mix_audio_tracks resamples to" + ); + for opt in OUTPUT_OPTS { if let Some(pos) = args.iter().position(|s| s == opt) { assert!( @@ -392,4 +421,153 @@ mod tests { assert_eq!(pix(true), alpha, "{codec} transparent"); } } + + // ── Integration test (gated on ffmpeg + ffprobe) ──────────────────────── + // + // Ties constat #1 (audio input declared before every output option — a + // scenario with audio must actually produce a file) and constat #2 + // (the mixer and the muxer must agree on the sample rate) together + // end-to-end, matching what the audit's own suggested fix asked for: + // "Ajouter un test d'intégration gaté sur ffmpeg qui rend un scénario + // avec piste audio et vérifie que le MP4 existe et contient deux flux." + + 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) + } + + fn ffprobe_on_path() -> bool { + std::process::Command::new("ffprobe") + .args(["-version"]) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .map(|s| s.success()) + .unwrap_or(false) + } + + fn ffprobe_stream_duration(path: &str, selector: &str) -> Option { + let out = std::process::Command::new("ffprobe") + .args([ + "-v", + "error", + "-select_streams", + selector, + "-show_entries", + "stream=duration", + "-of", + "default=noprint_wrappers=1:nokey=1", + path, + ]) + .output() + .ok()?; + if !out.status.success() { + return None; + } + String::from_utf8_lossy(&out.stdout) + .trim() + .parse::() + .ok() + } + + /// Write a minimal, hand-rolled canonical PCM WAV file (16-bit, mono) — + /// no ffmpeg needed to produce the *input* fixture, only to encode it. + fn write_minimal_wav(path: &std::path::Path, sample_rate: u32, num_samples: u32) { + let bits_per_sample: u16 = 16; + let num_channels: u16 = 1; + let byte_rate = sample_rate * num_channels as u32 * bits_per_sample as u32 / 8; + let block_align = num_channels * bits_per_sample / 8; + let data_size = num_samples * block_align as u32; + + let mut buf = Vec::with_capacity(44 + data_size as usize); + buf.extend_from_slice(b"RIFF"); + buf.extend_from_slice(&(36 + data_size).to_le_bytes()); + buf.extend_from_slice(b"WAVE"); + buf.extend_from_slice(b"fmt "); + buf.extend_from_slice(&16u32.to_le_bytes()); + buf.extend_from_slice(&1u16.to_le_bytes()); // PCM + buf.extend_from_slice(&num_channels.to_le_bytes()); + buf.extend_from_slice(&sample_rate.to_le_bytes()); + buf.extend_from_slice(&byte_rate.to_le_bytes()); + buf.extend_from_slice(&block_align.to_le_bytes()); + buf.extend_from_slice(&bits_per_sample.to_le_bytes()); + buf.extend_from_slice(b"data"); + buf.extend_from_slice(&data_size.to_le_bytes()); + buf.extend(std::iter::repeat_n(0u8, data_size as usize)); + + std::fs::write(path, &buf).expect("write fixture wav"); + } + + #[test] + fn encode_with_ffmpeg_produces_a_synced_two_stream_mp4() { + if !ffmpeg_on_path() { + eprintln!( + "encode_with_ffmpeg_produces_a_synced_two_stream_mp4: ffmpeg not found — skipping" + ); + return; + } + + let wav_path = std::env::temp_dir().join(format!( + "rm_ffmpeg_it_audio_{}_{}.wav", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + // Source at a rate different from OUTPUT_SAMPLE_RATE, to also + // exercise the resampler rather than a same-rate passthrough. + write_minimal_wav(&wav_path, 22_050, 22_050); + + let json = format!( + r#"{{"video": {{"width": 32, "height": 32, "fps": 10}}, + "audio": [{{"src": "{}"}}], + "scenes": [{{"duration": 1.0, "children": []}}]}}"#, + wav_path.to_str().unwrap().replace('\\', "\\\\") + ); + let scenario = crate::loader::load_scenario_from_source(None, Some(&json)).expect("load"); + + let out = std::env::temp_dir().join(format!("rm_ffmpeg_it_out_{}.mp4", std::process::id())); + let _ = std::fs::remove_file(&out); + + super::encode_with_ffmpeg( + &scenario, + out.to_str().unwrap(), + true, + "h264", + None, + false, + None, + ) + .expect("ffmpeg encode with an audio track must succeed (constat #1)"); + + assert!(out.exists(), "output MP4 must exist"); + assert!( + std::fs::metadata(&out).unwrap().len() > 0, + "output MP4 must not be empty" + ); + + if ffprobe_on_path() { + let video_dur = ffprobe_stream_duration(out.to_str().unwrap(), "v:0") + .expect("must report a video stream duration"); + let audio_dur = ffprobe_stream_duration(out.to_str().unwrap(), "a:0").expect( + "must report an audio stream duration — the MP4 must contain an audio stream \ + at all (constat #1)", + ); + assert!( + (video_dur - audio_dur).abs() < 0.05, + "audio/video duration must match within 50ms, got video={video_dur:.3}s \ + audio={audio_dur:.3}s (constat #2: a sample-rate mismatch between the mixer \ + and the muxer desyncs them)" + ); + } + + let _ = std::fs::remove_file(&wav_path); + let _ = std::fs::remove_file(&out); + } } diff --git a/crates/rustmotion/src/encode/video/formats.rs b/crates/rustmotion/src/encode/video/formats.rs index 3774da9..78d471a 100644 --- a/crates/rustmotion/src/encode/video/formats.rs +++ b/crates/rustmotion/src/encode/video/formats.rs @@ -81,7 +81,7 @@ pub fn encode_png_sequence( pub fn encode_gif( scenario: &Scenario, output_path: &str, - _quiet: bool, + quiet: bool, mut on_progress: Option<&mut dyn FnMut(EncodeProgress)>, ) -> Result<()> { let config = &scenario.video; @@ -116,10 +116,17 @@ pub fn encode_gif( reason: e.to_string(), })?; - let delay = (100.0 / fps as f64).round() as u16; + if !quiet && fps > 50 { + eprintln!( + "rustmotion: GIF frame delay has a 1/100s resolution — {} fps cannot be represented \ + exactly; playback speed will be approximate.", + fps + ); + } let batch_size = (rayon::current_num_threads() * 2).max(4); let counter = AtomicU32::new(0); + let mut frame_idx: u32 = 0; for batch in tasks.chunks(batch_size) { let results: Vec>> = batch @@ -141,7 +148,8 @@ pub fn encode_gif( for result in results { let rgba = result?; let mut frame = gif::Frame::from_rgba_speed(gif_w, gif_h, &mut rgba.clone(), 10); - frame.delay = delay; + frame.delay = gif_frame_delay_cs(frame_idx, fps); + frame_idx += 1; encoder .write_frame(&frame) .map_err(|e| RustmotionError::GifFrame { @@ -153,38 +161,75 @@ pub fn encode_gif( Ok(()) } -/// Stream raw RGBA pixel data to stdout for piping to external tools -pub fn encode_raw_stdout(scenario: &Scenario, quiet: bool) -> Result<()> { - let config = &scenario.video; - let fps = config.fps; +/// Centisecond delay for GIF frame `frame_index` out of a stream running at +/// `fps`. +/// +/// Constat #7: rounding `100.0 / fps` once and reusing that flat delay for +/// every frame accumulates rounding error across the whole GIF — at 30fps, +/// `round(100/30) = 3cs` per frame gives 60 frames * 3cs = 1.800s for a +/// 2.000s scenario (a 10% shortfall), and at fps > 100 the delay rounds to 0 +/// and viewers fall back to their own default (~10cs), turning a short clip +/// into a multi-minute one. +/// +/// Emitting the *cumulative* rounded boundary and taking the difference +/// between consecutive frames (`round(100*(i+1)/fps) - round(100*i/fps)`) +/// telescopes to the exact target duration: the rounding error is +/// distributed across frames instead of compounding. A 2cs floor is applied +/// afterward — GIF's 1/100s tick can't represent anything shorter, and +/// unclamped near-zero delays are exactly what pushes viewers into their own +/// undocumented fallback. +fn gif_frame_delay_cs(frame_index: u32, fps: u32) -> u16 { + let cs_at = |i: u32| -> i64 { (100.0 * i as f64 / fps as f64).round() as i64 }; + let delay = cs_at(frame_index + 1) - cs_at(frame_index); + delay.clamp(2, u16::MAX as i64) as u16 +} +/// Stream raw RGBA pixel data to stdout for piping to external tools. +pub fn encode_raw_stdout(scenario: &Scenario, quiet: bool) -> Result<()> { let mut stdout = std::io::stdout().lock(); + encode_raw_frames(scenario, quiet, &mut stdout) +} + +/// Shared implementation behind `encode_raw_stdout`, generic over the writer +/// so it can be exercised in tests against an in-memory buffer instead of +/// the process's real stdout. +/// +/// Constat #5: this used to iterate `view.scenes` directly and compute each +/// scene's frame count in isolation (`(scene.duration * fps).round()`), +/// which ignores that consecutive scenes overlap during a transition — the +/// entering scene's tail is trimmed to make room for the blended frames. +/// `--format raw` therefore emitted MORE frames than `png-seq`/the encoded +/// MP4 (120 vs 90 for a 2s+2s scenario with a 1s fade) and never applied +/// `apply_post_effects` or `ViewTransition`/`WorldFrame` compositing at all. +/// Routing through `build_frame_tasks` + `render_frame_task` — the same +/// pipeline every other encoder in this module uses — fixes all of that at +/// once: `raw`'s frame stream is now byte-for-byte, frame-for-frame, the +/// same content the MP4 encodes. +fn encode_raw_frames(scenario: &Scenario, quiet: bool, writer: &mut dyn Write) -> Result<()> { + let config = &scenario.video; - let mut frame_offset = 0u32; for view in &scenario.views { - for scene in &view.scenes { - let scene_frames = (scene.duration * fps as f64).round() as u32; - - for local_frame in 0..scene_frames { - let rgba = crate::engine::render::render_scene_frame( - config, - scene, - local_frame, - scene_frames, - )?; - stdout.write_all(&rgba)?; - - if !quiet { - let global_frame = frame_offset + local_frame; - eprint!("\rFrame {}", global_frame); - } - } - frame_offset += scene_frames; + prefetch_icons(&view.scenes); + } + + let tasks = build_frame_tasks(scenario); + let total_frames = tasks.len() as u32; + + if total_frames == 0 { + return Err(RustmotionError::NoFrames); + } + + for (idx, task) in tasks.iter().enumerate() { + let rgba = render_frame_task(config, scenario, task)?; + writer.write_all(&rgba)?; + + if !quiet { + eprint!("\rFrame {}", idx); } } if !quiet { - eprintln!("\nDone: {} frames streamed to stdout", frame_offset); + eprintln!("\nDone: {} frames streamed to stdout", total_frames); } Ok(()) @@ -345,4 +390,151 @@ mod tests { let _ = std::fs::remove_dir_all(&dir_mt2); let _ = std::fs::remove_dir_all(&dir_st); } + + // ── Constat #5: raw format frame count ───────────────────────────────── + + /// A 2s scene + a 2s scene with a 1s incoming fade transition, at 8x8 to + /// stay fast, mirroring the brief's exact repro shape (320x240, 2s+2s, + /// 1s fade → 90 frames instead of a naive 120). + fn transition_scenario_json(width: u32, height: u32, fps: u32) -> String { + format!( + r##"{{"video": {{"width": {width}, "height": {height}, "fps": {fps}}}, + "scenes": [ + {{"duration": 2.0, "children": [ + {{"type": "shape", "shape": "rect", "fill": "#ff0000", + "position": "absolute", "x": 0, "y": 0, + "style": {{"width": {width}, "height": {height}}}}} + ]}}, + {{"duration": 2.0, "transition": {{"type": "fade", "duration": 1.0}}, "children": [ + {{"type": "shape", "shape": "rect", "fill": "#0000ff", + "position": "absolute", "x": 0, "y": 0, + "style": {{"width": {width}, "height": {height}}}}} + ]}} + ]}}"## + ) + } + + /// Constat #5: `--format raw` must stream exactly the frames + /// `build_frame_tasks` produces (and thus exactly what `png-seq`/the MP4 + /// encoder produce), not a naive per-scene frame count that ignores + /// transition overlap. Brief's repro: 2s+2s scenario with a 1s fade + /// gives 3.0s worth of frames (90 @ 30fps), not 4.0s (120 @ 30fps). + #[test] + fn raw_frame_count_matches_build_frame_tasks_across_a_transition() { + let fps = 30u32; + let json = transition_scenario_json(8, 8, fps); + let scenario = load_scenario_from_source(None, Some(&json)).expect("load"); + + let mut buf: Vec = Vec::new(); + encode_raw_frames(&scenario, true, &mut buf).expect("raw encode"); + + let bytes_per_frame = 8usize * 8 * 4; + assert_eq!( + buf.len() % bytes_per_frame, + 0, + "raw output must be a whole number of frames" + ); + let raw_frame_count = buf.len() / bytes_per_frame; + + let expected_frame_count = build_frame_tasks(&scenario).len(); + assert_eq!( + raw_frame_count, expected_frame_count, + "raw format must produce the same frame count as build_frame_tasks (matches \ + png-seq/mp4), not a naive per-scene sum that ignores transition overlap" + ); + // Ground truth from the brief: 2s + 2s with a 1s transition is 3.0s + // of output, not 4.0s. + assert_eq!( + expected_frame_count, + (3.0 * fps as f64).round() as usize, + "a 2s+2s scenario with a 1s fade must occupy 3.0s of output frames" + ); + } + + // ── Constat #7: GIF frame delay rounding ──────────────────────────────── + + /// At fps where the average per-frame delay comfortably clears the 2cs + /// floor, the telescoping sum of `gif_frame_delay_cs` must exactly + /// reproduce the target duration — no accumulated rounding drift. + #[test] + fn gif_delay_sums_to_the_exact_target_duration_for_low_fps() { + for fps in [20u32, 24, 25, 30, 40, 50] { + let duration_s = 2.0_f64; + let frame_count = (duration_s * fps as f64).round() as u32; + let sum_cs: i64 = (0..frame_count) + .map(|i| gif_frame_delay_cs(i, fps) as i64) + .sum(); + let expected_cs = (duration_s * 100.0).round() as i64; + assert_eq!( + sum_cs, expected_cs, + "fps={fps}: summed delays must equal the target duration exactly" + ); + } + } + + /// The 2cs floor must never be violated, however high `fps` climbs — + /// this is what turns the old bug's catastrophic 0-delay (viewers + /// silently substitute ~10cs, ballooning a 2s clip to 48s) into a + /// bounded, predictable slowdown instead. + #[test] + fn gif_delay_never_drops_below_the_two_centisecond_floor() { + for fps in [60u32, 120, 240, 1000] { + for i in 0..20 { + let delay = gif_frame_delay_cs(i, fps); + assert!( + delay >= 2, + "fps={fps} frame={i}: delay {delay}cs is below the floor" + ); + } + } + } + + /// End-to-end regression for the brief's exact reproduction: a real GIF + /// encoded at 30fps for a 2.0s scene must play back for ~2.000s, not the + /// buggy 1.800s (flat `round(100/30)=3cs` delay × 60 frames). Decodes + /// the GIF's own frame delays via the `gif` crate (already a direct + /// dependency) — no ffmpeg needed. + #[test] + fn gif_total_playback_duration_matches_the_scenario_duration() { + let fps = 30u32; + let json = format!( + r#"{{"video": {{"width": 8, "height": 8, "fps": {fps}}}, + "scenes": [{{"duration": 2.0, "children": []}}]}}"# + ); + let scenario = load_scenario_from_source(None, Some(&json)).expect("load"); + + let out = std::env::temp_dir().join(format!( + "rm_gif_duration_test_{}_{}.gif", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let _ = std::fs::remove_file(&out); + encode_gif(&scenario, out.to_str().unwrap(), true, None).expect("encode gif"); + + let file = std::fs::File::open(&out).expect("open gif"); + let mut decoder = gif::DecodeOptions::new() + .read_info(file) + .expect("read gif info"); + let mut total_cs: u32 = 0; + let mut frame_count = 0u32; + while let Some(frame) = decoder.read_next_frame().expect("read frame") { + total_cs += frame.delay as u32; + frame_count += 1; + } + + assert_eq!( + frame_count, 60, + "expected 60 frames at 30fps for a 2.0s scene" + ); + let total_secs = total_cs as f64 / 100.0; + assert!( + (total_secs - 2.0).abs() < 0.02, + "gif total playback duration must match the scenario's 2.000s, got {total_secs:.3}s" + ); + + let _ = std::fs::remove_file(&out); + } } diff --git a/crates/rustmotion/src/encode/video/h264.rs b/crates/rustmotion/src/encode/video/h264.rs index ec0abc7..7abf84c 100644 --- a/crates/rustmotion/src/encode/video/h264.rs +++ b/crates/rustmotion/src/encode/video/h264.rs @@ -161,14 +161,14 @@ pub fn encode_video_incremental( return Err(RustmotionError::NoFrames); } - // Flatten tasks that need rendering + // Flatten tasks that need rendering. Kept in increasing scene-index + // order (the encode loop below relies on that to detect scene + // boundaries by watching for a change in `scene_idx`, rather than + // pre-summing per-scene frame counts). let mut flat_tasks: Vec<(usize, &super::tasks::FrameTask)> = Vec::new(); - let mut scene_frame_counts: Vec<(usize, u32)> = Vec::new(); for i in 0..num_scenes { if needs_render[i] { - let tasks = &scene_tasks[i]; - scene_frame_counts.push((i, tasks.len() as u32)); - for task in tasks { + for task in &scene_tasks[i] { flat_tasks.push((i, task)); } } @@ -176,10 +176,26 @@ pub fn encode_video_incremental( let frames_to_render = flat_tasks.len() as u32; - // Render in parallel batches + // Render and encode one batch at a time instead of materialising every + // dirty frame's YUV buffer before encoding a single one (constat #9). + // The old two-phase shape — render ALL batches into `all_yuv`, only then + // start encoding — meant a `--watch` re-render of a long, high-res + // scenario held the *entire* duration's worth of YUV resident before + // the encoder even started: at 1080x1920 a YUV420 frame is ~3.11MB, so + // 60s @ 30fps (1800 frames) peaked at ~5.6GB before any bytes were + // encoded. Rendering still happens in parallel batches (unchanged + // throughput), but each batch is encoded immediately after it renders, + // so at most one batch's worth of YUV (a few dozen frames) is ever + // resident at once. let batch_size = (rayon::current_num_threads() * 2).max(4); let counter = AtomicU32::new(0); - let mut all_yuv: Vec>> = Vec::with_capacity(flat_tasks.len()); + + let mut encoder = create_encoder(width, height, fps)?; + let mut rendered_segments: std::collections::HashMap> = + std::collections::HashMap::new(); + let mut current_scene: Option = None; + let mut current_segment: Vec = Vec::new(); + let mut encoded_count: u32 = 0; for batch in flat_tasks.chunks(batch_size) { let batch_results: Vec>> = batch @@ -199,38 +215,34 @@ pub fn encode_video_incremental( )); } - all_yuv.extend(batch_results); - } - - // Encode phase - if let Some(ref mut cb) = on_progress { - cb(EncodeProgress::Encoding(0, frames_to_render)); - } - - let mut encoder = create_encoder(width, height, fps)?; - let mut yuv_iter = all_yuv.into_iter(); - let mut rendered_segments: std::collections::HashMap> = - std::collections::HashMap::new(); - let mut encoded_count: u32 = 0; + for ((scene_idx, _), yuv_result) in batch.iter().zip(batch_results) { + let yuv = yuv_result?; - for &(scene_idx, frame_count) in &scene_frame_counts { - let mut segment_h264: Vec = Vec::new(); + // `flat_tasks` groups frames by scene in increasing scene-index + // order (built by iterating `0..num_scenes` and pushing each + // dirty scene's tasks contiguously), so a scene index only ever + // changes when the previous scene's segment is complete. + if current_scene != Some(*scene_idx) { + if let Some(prev_idx) = current_scene { + rendered_segments.insert(prev_idx, std::mem::take(&mut current_segment)); + } + current_scene = Some(*scene_idx); + } - for _ in 0..frame_count { - let yuv = yuv_iter.next().unwrap()?; encoder.force_intra_frame(); let yuv_buf = YUVBuffer::from_vec(yuv, width as usize, height as usize); let bitstream = encoder .encode(&yuv_buf) .map_err(|e| RustmotionError::from(e.to_string()))?; - bitstream.write_vec(&mut segment_h264); + bitstream.write_vec(&mut current_segment); encoded_count += 1; if let Some(ref mut cb) = on_progress { cb(EncodeProgress::Encoding(encoded_count, frames_to_render)); } } - - rendered_segments.insert(scene_idx, segment_h264); + } + if let Some(prev_idx) = current_scene { + rendered_segments.insert(prev_idx, std::mem::take(&mut current_segment)); } // Assemble final segments @@ -355,4 +367,65 @@ mod incremental_tests { "reason must name world views: {err}" ); } + + /// Constat #9: the old code rendered *every* dirty frame into `all_yuv` + /// before encoding a single one — a full render phase, then a full + /// encode phase, back to back. That is what let a `--watch` re-render of + /// a long, high-resolution scenario hold the entire duration's worth of + /// YUV buffers in memory before the encoder even started. + /// + /// Peak RSS isn't observable from a unit test, but the *causal* symptom + /// is: `Encoding` progress events can only start after every `Rendering` + /// event has fired, because rendering must finish in full before + /// encoding begins. With batch-interleaved render+encode, an `Encoding` + /// event fires after each batch — including before the *last* batch has + /// even been rendered, as long as there is more than one batch. This + /// test forces >=4 batches (independent of the machine's core count, by + /// reading `rayon::current_num_threads()` the same way production code + /// does) and asserts that interleaving. + #[test] + fn incremental_encode_interleaves_rendering_and_encoding_instead_of_buffering_everything() { + let batch_size = (rayon::current_num_threads() * 2).max(4); + let total_frames = batch_size * 3 + 1; // guarantee >= 4 batches + let fps = 10u32; + let duration = total_frames as f64 / fps as f64; + + let json = format!( + r#"{{"video": {{"width": 16, "height": 16, "fps": {fps}}}, + "scenes": [{{"duration": {duration}, "children": []}}]}}"# + ); + let scenario = load_scenario_from_source(None, Some(&json)).unwrap(); + + let mut events: Vec<(&'static str, u32)> = Vec::new(); + let mut cb = |p: EncodeProgress| match p { + EncodeProgress::Rendering(cur, _) => events.push(("render", cur)), + EncodeProgress::Encoding(cur, _) => events.push(("encode", cur)), + EncodeProgress::Muxing => events.push(("mux", 0)), + }; + + let out = std::env::temp_dir().join(format!( + "rustmotion_incr_mem_test_{}.mp4", + std::process::id() + )); + encode_video_incremental(&scenario, out.to_str().unwrap(), true, None, Some(&mut cb)) + .expect("encode"); + + let last_render_idx = events + .iter() + .rposition(|(k, _)| *k == "render") + .expect("at least one render event"); + let first_encode_idx = events + .iter() + .position(|(k, _)| *k == "encode") + .expect("at least one encode event"); + + assert!( + first_encode_idx < last_render_idx, + "encoding must start before rendering finishes (interleaved batches), \ + got event sequence: {:?}", + events + ); + + let _ = std::fs::remove_file(&out); + } } diff --git a/crates/rustmotion/src/encode/video/mux.rs b/crates/rustmotion/src/encode/video/mux.rs index dc9dd0f..4f4fdef 100644 --- a/crates/rustmotion/src/encode/video/mux.rs +++ b/crates/rustmotion/src/encode/video/mux.rs @@ -34,7 +34,10 @@ pub(super) fn mux_h264_to_mp4( let mut muxer = Mp4Muxer::new(writer); muxer.init_video(width as i32, height as i32, false, "rustmotion"); if let Some(ref pcm) = pcm_data { - muxer.init_audio(128000, 44100, 2); + // Same constant the mixer resamples to. Hardcoding it here is how the + // 48kHz/44.1kHz desync got in: two sites declared a rate, one of them + // silently disagreed with the PCM it was handed. + muxer.init_audio(128000, crate::encode::audio::OUTPUT_SAMPLE_RATE, 2); muxer.write_video_with_audio(h264_data, fps, pcm); } else { muxer.write_video_with_fps(h264_data, fps); diff --git a/crates/rustmotion/src/encode/video_audio.rs b/crates/rustmotion/src/encode/video_audio.rs index 9057419..5ce3c1f 100644 --- a/crates/rustmotion/src/encode/video_audio.rs +++ b/crates/rustmotion/src/encode/video_audio.rs @@ -236,10 +236,50 @@ fn wav_cache_path(src: &str, trim_start: f64, trim_end: Option, rate: f64) trim_start.to_bits().hash(&mut hasher); trim_end.map(|v| v.to_bits()).hash(&mut hasher); rate.to_bits().hash(&mut hasher); + + // Constat #10: the cache key used to depend only on + // (src, trim_start, trim_end, rate) — editing `src` in place (same + // path, new bytes) left the old extraction cached under the same key + // forever, silently serving stale audio. Folding in the source's size + // and mtime means a modified file gets a different cache path + // automatically. Best-effort: if `metadata` fails (source vanished + // between validation and extraction), the hash simply falls back to the + // path-only key, matching the previous behavior exactly. + if let Ok(meta) = std::fs::metadata(src) { + meta.len().hash(&mut hasher); + if let Ok(modified) = meta.modified() { + if let Ok(dur) = modified.duration_since(std::time::UNIX_EPOCH) { + dur.as_nanos().hash(&mut hasher); + } + } + } + let hash = hasher.finish(); std::env::temp_dir().join(format!("rustmotion_vidaud_{:016x}.wav", hash)) } +/// Scratch path ffmpeg writes to before a successful extraction is promoted +/// (renamed) onto `wav_path`. +/// +/// Deviates from the audit's literal suggestion of a `.wav.partial` +/// suffix: `Path::with_extension` on a path already ending in `.wav` +/// replaces the extension rather than appending, so `.wav.partial` +/// really means "last extension is `.partial`" — and ffmpeg picks its output +/// muxer from the *last* extension. Pointing it at a `.partial`-suffixed +/// path makes it fail with "Unable to choose an output format", which +/// looked identical to the transient-failure case this fix exists to guard +/// against until traced back to this naming choice. Keeping `.wav` as the +/// final extension (`.partial.wav`) keeps ffmpeg's format +/// autodetection working while still being unambiguously distinct from the +/// real cache path. +fn partial_wav_path(wav_path: &std::path::Path) -> PathBuf { + let stem = wav_path + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or("audio"); + wav_path.with_file_name(format!("{stem}.partial.wav")) +} + // ─── Audio extraction ───────────────────────────────────────────────────────── /// Extract audio from a video file into a WAV using ffmpeg. @@ -259,6 +299,17 @@ fn extract_audio_to_wav( return Some(wav_path); } + // Constat #10: ffmpeg used to write straight to `wav_path`. An + // interrupted extraction (Ctrl-C, disk full, the source still being + // written) left a truncated file sitting exactly at the path + // `wav_path.exists()` treats as a valid cache hit above — every + // subsequent render silently reused the corrupt WAV, with no error and + // no way to detect it short of manually clearing `temp_dir()`. Writing + // to a scratch sibling and renaming onto `wav_path` only after ffmpeg + // reports success means a failed extraction can never become a false + // cache hit. + let partial_path = partial_wav_path(&wav_path); + let mut args: Vec = Vec::new(); // Input seek (trim_start) @@ -286,7 +337,7 @@ fn extract_audio_to_wav( // Overwrite output args.push("-y".to_string()); - args.push(wav_path.to_str().unwrap_or_default().to_string()); + args.push(partial_path.to_str().unwrap_or_default().to_string()); let status = std::process::Command::new("ffmpeg") .args(&args) @@ -295,13 +346,24 @@ fn extract_audio_to_wav( .status(); match status { - Ok(s) if s.success() => Some(wav_path), + Ok(s) if s.success() => match std::fs::rename(&partial_path, &wav_path) { + Ok(()) => Some(wav_path), + Err(e) => { + eprintln!( + "rustmotion: embedded-video audio: failed to finalize cached WAV for '{}': {}. Skipping.", + src, e + ); + let _ = std::fs::remove_file(&partial_path); + None + } + }, Ok(_) => { eprintln!( "rustmotion: embedded-video audio: ffmpeg failed to extract audio from '{}' \ (trim_start={:.3}, trim_end={:?}, rate={:.3}). Skipping.", src, trim_start, trim_end, rate ); + let _ = std::fs::remove_file(&partial_path); None } Err(e) => { @@ -309,6 +371,7 @@ fn extract_audio_to_wav( "rustmotion: embedded-video audio: could not spawn ffmpeg for '{}': {}. Skipping.", src, e ); + let _ = std::fs::remove_file(&partial_path); None } } @@ -625,6 +688,133 @@ mod tests { assert_ne!(p1, p2); } + /// Constat #10: the cache key must fold in the source file's own + /// metadata, not just its path — otherwise editing a video in place + /// (same path, new bytes) keeps serving audio extracted from the file's + /// *previous* contents forever, with no error and no way to detect it. + #[test] + fn wav_cache_path_changes_when_source_file_is_modified() { + let src = std::env::temp_dir().join(format!( + "rm_vidaud_src_test_{}_{}.mp4", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::write(&src, b"version one").unwrap(); + let p1 = wav_cache_path(src.to_str().unwrap(), 0.0, None, 1.0); + + // Best-effort: push the mtime forward too, in case the filesystem's + // mtime resolution is coarser than the write below. + std::thread::sleep(std::time::Duration::from_millis(20)); + std::fs::write(&src, b"version two, a longer and different payload").unwrap(); + let p2 = wav_cache_path(src.to_str().unwrap(), 0.0, None, 1.0); + + assert_ne!( + p1, p2, + "modifying the source file's contents must invalidate the cached WAV path" + ); + + let _ = std::fs::remove_file(&src); + } + + /// Constat #10: a failed extraction must never leave a residue file — + /// neither the promoted `wav_path` (a false cache hit on the next + /// render, per `wav_path.exists()` above) nor the `.partial` scratch + /// file ffmpeg wrote to along the way. + #[test] + fn failed_extraction_leaves_no_residue_on_disk() { + if !ffmpeg_available() { + eprintln!("failed_extraction_leaves_no_residue_on_disk: ffmpeg not found — skipping"); + return; + } + let missing_src = std::env::temp_dir().join(format!( + "rm_vidaud_missing_{}_{}.mp4", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let _ = std::fs::remove_file(&missing_src); // guarantee it does not exist + + let wav_path = wav_cache_path(missing_src.to_str().unwrap(), 0.0, None, 1.0); + let partial_path = partial_wav_path(&wav_path); + let _ = std::fs::remove_file(&wav_path); + let _ = std::fs::remove_file(&partial_path); + + let result = extract_audio_to_wav(missing_src.to_str().unwrap(), 0.0, None, 1.0); + + assert!( + result.is_none(), + "extraction from a nonexistent source must fail" + ); + assert!( + !wav_path.exists(), + "a failed extraction must not leave a cached WAV that a later render would reuse" + ); + assert!( + !partial_path.exists(), + "a failed extraction must not leave a .partial scratch file behind" + ); + } + + /// Constat #10: a successful extraction promotes the `.partial` scratch + /// file to the real cache path and leaves no `.partial` behind. + #[test] + fn successful_extraction_leaves_no_partial_file_behind() { + if !ffmpeg_available() { + eprintln!( + "successful_extraction_leaves_no_partial_file_behind: ffmpeg not found — skipping" + ); + return; + } + let fixture = std::env::temp_dir().join("rustmotion_test_vidaud_partial_fixture.mp4"); + let fixture_str = fixture.to_str().unwrap(); + let status = std::process::Command::new("ffmpeg") + .args([ + "-y", + "-f", + "lavfi", + "-i", + "sine=frequency=440:duration=1", + fixture_str, + ]) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status(); + if !matches!(status, Ok(s) if s.success()) { + eprintln!( + "successful_extraction_leaves_no_partial_file_behind: fixture generation failed — skipping" + ); + return; + } + + let wav_path = wav_cache_path(fixture_str, 0.0, None, 1.0); + let partial_path = partial_wav_path(&wav_path); + let _ = std::fs::remove_file(&wav_path); + let _ = std::fs::remove_file(&partial_path); + + let result = extract_audio_to_wav(fixture_str, 0.0, None, 1.0); + + assert!( + result.is_some(), + "extraction from a valid fixture must succeed" + ); + assert!( + wav_path.exists(), + "successful extraction must leave the cached WAV at its final path" + ); + assert!( + !partial_path.exists(), + "successful extraction must not leave the .partial scratch file behind" + ); + + let _ = std::fs::remove_file(&wav_path); + let _ = std::fs::remove_file(&fixture); + } + // ── Integration test (gated on ffmpeg) ─────────────────────────────────── /// Full round-trip: generate a 1-second sine+test-video fixture with ffmpeg,