From d8467c2d7e282e29fc225d181621a20ac04a2f64 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sun, 2 Aug 2026 11:55:14 +0200 Subject: [PATCH] fix(encode): derive png-seq frame index from task position MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The output index came from a shared atomic incremented in rayon's completion order rather than from the frame's position, so frames landed out of order within each batch. Two runs of the same input differed on 114 frames, and multi-threaded output was a strict permutation of single-threaded output with 436 of 486 frames misplaced. It raised the measured noise floor 9.4x and manufactured spikes that corrupted an analysis before being caught. The index is now batch_base + local position. Rendering stays fully parallel — only the index computation changed. Two multi-threaded runs and a single-threaded run now produce byte-identical sequences, cross-checked against the --frame N path on three sample frames. mp4 and gif were never affected and are unchanged. Closes #129 --- crates/rustmotion/src/encode/video/formats.rs | 175 +++++++++++++++++- 1 file changed, 171 insertions(+), 4 deletions(-) diff --git a/crates/rustmotion/src/encode/video/formats.rs b/crates/rustmotion/src/encode/video/formats.rs index cce5364..3774da9 100644 --- a/crates/rustmotion/src/encode/video/formats.rs +++ b/crates/rustmotion/src/encode/video/formats.rs @@ -36,21 +36,31 @@ pub fn encode_png_sequence( std::fs::create_dir_all(output_dir)?; let batch_size = (rayon::current_num_threads() * 2).max(4); - let counter = AtomicU32::new(0); + // Progress-only counter: rayon's scheduling order does not match task + // position, so this must never be used to derive the output frame index + // (that caused a frame permutation bug, see issue #129). The frame index + // is instead derived from each task's position within `tasks`, via the + // running `batch_base` offset combined with the in-batch enumerate index. + let progress_counter = AtomicU32::new(0); + let mut batch_base: u32 = 0; for batch in tasks.chunks(batch_size) { let results: Vec)>> = batch .par_iter() - .map(|task| { - let frame_num = counter.fetch_add(1, Ordering::Relaxed); + .enumerate() + .map(|(local_idx, task)| { + let frame_num = batch_base + local_idx as u32; let rgba = render_frame_task(config, scenario, task)?; + progress_counter.fetch_add(1, Ordering::Relaxed); Ok((frame_num, rgba)) }) .collect(); + batch_base += batch.len() as u32; + if let Some(ref mut cb) = on_progress { cb(EncodeProgress::Rendering( - counter.load(Ordering::Relaxed), + progress_counter.load(Ordering::Relaxed), total_frames, )); } @@ -179,3 +189,160 @@ pub fn encode_raw_stdout(scenario: &Scenario, quiet: bool) -> Result<()> { Ok(()) } + +// ─── Unit tests ─────────────────────────────────────────────────────────────── + +#[cfg(test)] +mod tests { + use super::*; + use crate::loader::load_scenario_from_source; + use std::path::{Path, PathBuf}; + + /// Deterministic color for a given frame index: distinct enough across + /// nearby indices that a swapped/misplaced frame is detected by a plain + /// pixel comparison. + fn expected_color(i: usize) -> (u8, u8, u8) { + ( + ((i * 47) % 256) as u8, + ((i * 91) % 256) as u8, + ((i * 131) % 256) as u8, + ) + } + + /// Scenario made of `n` one-frame scenes, each a solid-color full-canvas + /// rect uniquely colored by its scene index (see `expected_color`). + fn build_scenario(n: usize) -> Scenario { + let mut scenes = Vec::with_capacity(n); + for i in 0..n { + let (r, g, b) = expected_color(i); + scenes.push(format!( + r##"{{"duration": 0.1, "children": [ + {{"type": "shape", "shape": "rect", "fill": "#{:02x}{:02x}{:02x}", + "position": "absolute", "x": 0, "y": 0, + "style": {{"width": 8, "height": 8}}}} + ]}}"##, + r, g, b + )); + } + let json = format!( + r#"{{"video": {{"width": 8, "height": 8, "fps": 10}}, "scenes": [{}]}}"#, + scenes.join(",") + ); + load_scenario_from_source(None, Some(&json)).expect("load test scenario") + } + + fn read_dir_sorted(dir: &Path) -> Vec { + let mut entries: Vec = std::fs::read_dir(dir) + .expect("read output dir") + .map(|e| e.expect("dir entry").path()) + .collect(); + entries.sort(); + entries + } + + fn scratch_dir(name: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "rustmotion_png_seq_test_{}_{}_{}", + name, + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let _ = std::fs::remove_dir_all(&dir); + dir + } + + /// Regression test for issue #129: the frame index must be derived from + /// the task's position in `tasks`, not from completion order under + /// rayon's work-stealing scheduler. Every frame is rendered with a color + /// unique to its index; the saved `frame_NNNNN.png` must contain exactly + /// the color expected for index N. + #[test] + fn png_sequence_frames_are_not_permuted() { + let n = 200; + let scenario = build_scenario(n); + + let out_dir = scratch_dir("order"); + encode_png_sequence(&scenario, out_dir.to_str().unwrap(), true, false, None) + .expect("encode png sequence"); + + let files = read_dir_sorted(&out_dir); + assert_eq!(files.len(), n, "expected one PNG per frame"); + + for (i, path) in files.iter().enumerate() { + let expected_name = format!("frame_{:05}.png", i); + assert_eq!( + path.file_name().unwrap().to_str().unwrap(), + expected_name, + "frame files must be contiguously numbered" + ); + + let img = image::open(path) + .unwrap_or_else(|e| panic!("decode {}: {e}", path.display())) + .to_rgba8(); + let pixel = img.get_pixel(0, 0); + let (er, eg, eb) = expected_color(i); + assert_eq!( + (pixel[0], pixel[1], pixel[2]), + (er, eg, eb), + "frame {} has the wrong content: frame index does not match task position", + i + ); + } + + let _ = std::fs::remove_dir_all(&out_dir); + } + + /// Two multithreaded runs of the same scenario must be byte-identical, + /// and must match a run pinned to a single thread. Before the fix, the + /// shared-atomic frame index made this fail under rayon's work-stealing + /// scheduler (see issue #129). + #[test] + fn png_sequence_is_deterministic_across_thread_counts() { + let n = 200; + let scenario = build_scenario(n); + + let dir_mt1 = scratch_dir("mt1"); + let dir_mt2 = scratch_dir("mt2"); + let dir_st = scratch_dir("st1"); + + encode_png_sequence(&scenario, dir_mt1.to_str().unwrap(), true, false, None) + .expect("mt run 1"); + encode_png_sequence(&scenario, dir_mt2.to_str().unwrap(), true, false, None) + .expect("mt run 2"); + + let single_threaded_pool = rayon::ThreadPoolBuilder::new() + .num_threads(1) + .build() + .expect("build single-threaded pool"); + single_threaded_pool.install(|| { + encode_png_sequence(&scenario, dir_st.to_str().unwrap(), true, false, None) + .expect("st run") + }); + + let files_mt1 = read_dir_sorted(&dir_mt1); + let files_mt2 = read_dir_sorted(&dir_mt2); + let files_st = read_dir_sorted(&dir_st); + assert_eq!(files_mt1.len(), n); + assert_eq!(files_mt2.len(), n); + assert_eq!(files_st.len(), n); + + for i in 0..n { + let a = std::fs::read(&files_mt1[i]).unwrap(); + let b = std::fs::read(&files_mt2[i]).unwrap(); + let c = std::fs::read(&files_st[i]).unwrap(); + assert_eq!(a, b, "frame {} differs between two multithreaded runs", i); + assert_eq!( + a, c, + "frame {} differs between multithreaded and single-threaded runs", + i + ); + } + + let _ = std::fs::remove_dir_all(&dir_mt1); + let _ = std::fs::remove_dir_all(&dir_mt2); + let _ = std::fs::remove_dir_all(&dir_st); + } +}