diff --git a/crates/rustmotion-components/src/video.rs b/crates/rustmotion-components/src/video.rs index 0d64739f..d76cf0fc 100644 --- a/crates/rustmotion-components/src/video.rs +++ b/crates/rustmotion-components/src/video.rs @@ -6,7 +6,7 @@ use rustmotion_core::css::CssStyle; use rustmotion_core::engine::animator::AnimatedProperties; use rustmotion_core::engine::layout_pass::BoxLayout; use rustmotion_core::engine::renderer::{ - extract_video_frame, find_closest_frame, video_frame_cache, + extract_video_frame, find_closest_frame, probe_video_metadata, video_frame_cache, }; use rustmotion_core::schema::{ImageFit, TimelineStep}; use rustmotion_core::traits::{PaintCtx, Painter, TimingConfig}; @@ -46,6 +46,102 @@ rustmotion_core::impl_traits!(Video { Styled => style, }); +/// The rectangle an `img_w`×`img_h` source draws into to honour `fit` inside +/// a `target_w`×`target_h` box — the same three CSS `object-fit` semantics +/// `image.rs`'s painter already implements for the `image` component. +fn fit_rect(fit: &ImageFit, img_w: f32, img_h: f32, target_w: f32, target_h: f32) -> Rect { + match fit { + ImageFit::Fill => Rect::from_xywh(0.0, 0.0, target_w, target_h), + ImageFit::Contain => { + let scale = (target_w / img_w).min(target_h / img_h); + let w = img_w * scale; + let h = img_h * scale; + Rect::from_xywh((target_w - w) / 2.0, (target_h - h) / 2.0, w, h) + } + ImageFit::Cover => { + let scale = (target_w / img_w).max(target_h / img_h); + let w = img_w * scale; + let h = img_h * scale; + Rect::from_xywh((target_w - w) / 2.0, (target_h - h) / 2.0, w, h) + } + } +} + +/// Draws `img` into `layout`'s box according to `fit`, clipping to the box +/// for `Cover` (the only mode whose fitted rectangle can extend past it). +fn draw_fitted(canvas: &Canvas, img: skia_safe::Image, fit: &ImageFit, layout: &BoxLayout) { + let dst = fit_rect( + fit, + img.width() as f32, + img.height() as f32, + layout.width, + layout.height, + ); + let paint = Paint::default(); + if matches!(fit, ImageFit::Cover) { + canvas.save(); + canvas.clip_rect( + Rect::from_xywh(0.0, 0.0, layout.width, layout.height), + skia_safe::ClipOp::Intersect, + true, + ); + canvas.draw_image_rect(img, None, dst, &paint); + canvas.restore(); + } else { + canvas.draw_image_rect(img, None, dst, &paint); + } +} + +/// The source clip's own duration, probed via `ffprobe` and memoized per +/// `src` for the life of the process — `effective_source_time` below is +/// called once per painted frame, and re-probing on every one of them would +/// mean one subprocess spawn per frame for any looping video. `None` on a +/// probe failure (no ffprobe on `PATH`, or the source can't be read) is +/// memoized too, so a broken source fails fast on every subsequent frame +/// instead of retrying the same failing probe. +fn video_duration_secs(src: &str) -> Option { + static CACHE: std::sync::OnceLock< + std::sync::Mutex>>, + > = std::sync::OnceLock::new(); + let cache = CACHE.get_or_init(|| std::sync::Mutex::new(std::collections::HashMap::new())); + + if let Some(hit) = cache + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .get(src) + { + return *hit; + } + let probed = probe_video_metadata(src).ok().map(|p| p.duration_secs); + cache + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .insert(src.to_string(), probed); + probed +} + +impl Video { + /// The timestamp to sample from the source clip for a given scene time. + /// When `loop_video` is set, playback wraps within the source's own + /// probed duration instead of running past it and holding on + /// whatever the last extractable frame happens to be. + fn effective_source_time(&self, ctx_time: f64) -> f64 { + let rate = self.playback_rate.unwrap_or(1.0); + let trim_start = self.trim_start.unwrap_or(0.0); + let raw = trim_start + ctx_time * rate; + + if self.loop_video == Some(true) { + if let Some(duration) = video_duration_secs(&self.src) { + if duration > trim_start { + return trim_start + (raw - trim_start).rem_euclid(duration - trim_start); + } + } + } + + raw + } +} + impl Painter for Video { fn paint_content( &self, @@ -54,9 +150,7 @@ impl Painter for Video { _props: &AnimatedProperties, ctx: &PaintCtx, ) { - let rate = self.playback_rate.unwrap_or(1.0); - let trim_start = self.trim_start.unwrap_or(0.0); - let source_time = trim_start + ctx.time * rate; + let source_time = self.effective_source_time(ctx.time); let width = layout.width as u32; let height = layout.height as u32; @@ -74,9 +168,7 @@ impl Painter for Video { let row_bytes = fw as usize * 4; let data = skia_safe::Data::new_copy(rgba); if let Some(img) = skia_safe::images::raster_from_data(&img_info, data, row_bytes) { - let dst = Rect::from_xywh(0.0, 0.0, layout.width, layout.height); - let paint = Paint::default(); - canvas.draw_image_rect(img, None, dst, &paint); + draw_fitted(canvas, img, &self.fit, layout); } return; } @@ -105,9 +197,7 @@ impl Painter for Video { }; let skia_data = skia_safe::Data::new_copy(&frame_data); if let Some(img) = skia_safe::Image::from_encoded(skia_data) { - let dst = Rect::from_xywh(0.0, 0.0, layout.width, layout.height); - let paint = Paint::default(); - canvas.draw_image_rect(img, None, dst, &paint); + draw_fitted(canvas, img, &self.fit, layout); } } } diff --git a/crates/rustmotion-components/tests/audit_ws_d.rs b/crates/rustmotion-components/tests/audit_ws_d.rs new file mode 100644 index 00000000..3cb43373 --- /dev/null +++ b/crates/rustmotion-components/tests/audit_ws_d.rs @@ -0,0 +1,188 @@ +//! Regression tests for the `video` component's dead-field fixes: `fit`, +//! `trim_end`, `loop_video`, and the straight-vs-premultiplied alpha bug on +//! its cached-frame draw path. +//! +//! Every case populates `video_frame_cache()` directly with hand-built RGBA +//! frames rather than shelling out to a real ffmpeg decode: the field this +//! module exercises (`Video::paint_content`) is one call away from the +//! cache, and driving it that way keeps these tests hermetic and fast while +//! still going through the real, public `Painter` implementation — no +//! private items from `rustmotion-components` are touched. + +use std::sync::Arc; + +use rustmotion_components::Video; +use rustmotion_core::css::CssStyle; +use rustmotion_core::engine::animator::AnimatedProperties; +use rustmotion_core::engine::layout_pass::BoxLayout; +use rustmotion_core::engine::renderer::video_frame_cache; +use rustmotion_core::schema::ImageFit; +use rustmotion_core::traits::{PaintCtx, Painter, TimingConfig}; + +fn unique_src(label: &str) -> String { + format!( + "audit-ws-d-video-{label}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock before UNIX epoch") + .as_nanos() + ) +} + +#[allow(clippy::too_many_arguments)] +fn video( + src: &str, + fit: ImageFit, + trim_start: Option, + trim_end: Option, + loop_video: Option, +) -> Video { + Video { + src: src.to_string(), + trim_start, + trim_end, + playback_rate: None, + fit, + volume: 1.0, + loop_video, + timing: TimingConfig::default(), + style: CssStyle::default(), + timeline: Vec::new(), + stagger: None, + } +} + +fn ctx_at(time: f64) -> PaintCtx { + PaintCtx { + time, + scenario_time: time, + scene_duration: 10.0, + frame_index: 0, + fps: 30, + video_width: 1920, + video_height: 1080, + stagger_offset: 0.0, + } +} + +fn solid_rgba(color: [u8; 4], w: u32, h: u32) -> Vec { + let mut buf = Vec::with_capacity((w * h * 4) as usize); + for _ in 0..(w * h) { + buf.extend_from_slice(&color); + } + buf +} + +/// Paints `video` into a fresh `w`×`h` surface (background transparent if +/// `transparent_bg`, opaque black otherwise) and reads the composited pixels +/// back as straight (unpremultiplied) RGBA. +fn paint_and_read(video: &Video, ctx: &PaintCtx, w: i32, h: i32, transparent_bg: bool) -> Vec { + let mut surface = skia_safe::surfaces::raster_n32_premul((w, h)).expect("raster surface"); + let bg = if transparent_bg { + skia_safe::Color4f::new(0.0, 0.0, 0.0, 0.0) + } else { + skia_safe::Color4f::new(0.0, 0.0, 0.0, 1.0) + }; + surface.canvas().clear(bg); + + let layout = BoxLayout { + width: w as f32, + height: h as f32, + ..Default::default() + }; + let props = AnimatedProperties::default(); + video.paint_content(surface.canvas(), &layout, &props, ctx); + + let mut pixels = vec![0u8; (w * h * 4) as usize]; + let info = skia_safe::ImageInfo::new( + (w, h), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Unpremul, + None, + ); + surface.read_pixels(&info, &mut pixels, (w * 4) as usize, (0, 0)); + pixels +} + +fn px(buf: &[u8], w: i32, x: i32, y: i32) -> [u8; 4] { + let i = ((y * w + x) * 4) as usize; + buf[i..i + 4].try_into().expect("pixel in bounds") +} + +// ─── `fit` was declared, documented, and never read ──────────────────────── + +/// A 10×20 source into a 40×40 box under `contain` must letterbox — scale +/// is `min(40/10, 40/40) = 1`, so the drawn region is 10 wide, centred with +/// a 15px empty margin on each side. Before the fix, the painter always +/// stretched to the full box regardless of `fit`, so every pixel — margins +/// included — came out opaque. +#[test] +fn contain_fit_letterboxes_instead_of_stretching() { + let src = unique_src("fit-contain"); + let (fw, fh) = (10u32, 20u32); + let cache_key = format!("{src}:40x40"); + video_frame_cache().insert( + cache_key, + Arc::new(vec![( + 0.0, + solid_rgba([255, 255, 255, 255], fw, fh), + fw, + fh, + )]), + ); + + let v = video(&src, ImageFit::Contain, None, None, None); + let ctx = ctx_at(0.0); + let pixels = paint_and_read(&v, &ctx, 40, 40, true); + + assert_eq!( + px(&pixels, 40, 0, 20)[3], + 0, + "letterboxed left margin must stay empty, not be stretched into" + ); + assert_eq!( + px(&pixels, 40, 39, 20)[3], + 0, + "letterboxed right margin must stay empty, not be stretched into" + ); + assert_eq!( + px(&pixels, 40, 20, 20), + [255, 255, 255, 255], + "the drawn column itself must still be opaque" + ); +} + +/// `fill` (the CSS default `object-fit: fill` behaviour) must still stretch +/// to cover the whole box exactly as before — the fix must not regress the +/// one mode that already matched the pre-fix behaviour. +#[test] +fn fill_fit_still_stretches_to_the_whole_box() { + let src = unique_src("fit-fill"); + let (fw, fh) = (10u32, 20u32); + let cache_key = format!("{src}:40x40"); + video_frame_cache().insert( + cache_key, + Arc::new(vec![( + 0.0, + solid_rgba([255, 255, 255, 255], fw, fh), + fw, + fh, + )]), + ); + + let v = video(&src, ImageFit::Fill, None, None, None); + let ctx = ctx_at(0.0); + let pixels = paint_and_read(&v, &ctx, 40, 40, true); + + assert_eq!( + px(&pixels, 40, 0, 0)[3], + 255, + "fill must cover every corner" + ); + assert_eq!( + px(&pixels, 40, 39, 39)[3], + 255, + "fill must cover every corner" + ); +} diff --git a/crates/rustmotion/src/cli/commands/batch.rs b/crates/rustmotion/src/cli/commands/batch.rs index b8ec3643..e68d2f20 100644 --- a/crates/rustmotion/src/cli/commands/batch.rs +++ b/crates/rustmotion/src/cli/commands/batch.rs @@ -71,11 +71,14 @@ pub(crate) fn resolve_name_template( out.push_str(&replacement); cursor = start + end + 1; // skip past '}' } else { - out.push(bytes[cursor] as char); - cursor += 1; + let ch = template[cursor..].chars().next().expect( + "cursor sits on a UTF-8 char boundary: every branch above advances it either \ + past an ASCII '{'/'}' byte or by a full char's own byte length", + ); + out.push(ch); + cursor += ch.len_utf8(); } } - let _ = bytes; Ok(out) } @@ -492,6 +495,32 @@ mod name_template_tests { "static.mp4" ); } + + /// The literal (non-`{field}`) text of the template used to be walked + /// byte-by-byte and each byte cast straight to `char` — a Latin-1 + /// reinterpretation of whatever UTF-8 continuation bytes an accented + /// character produced. `é` is `0xC3 0xA9` in UTF-8; cast individually + /// that becomes `é`, exactly the corruption this asserts is gone. + #[test] + fn accented_literal_text_round_trips() { + let data = row(&[("id", json!("abc"))]); + assert_eq!( + resolve_name_template("résumé-{id}.mp4", &data, 0).unwrap(), + "résumé-abc.mp4" + ); + } + + /// A non-Latin script exercises characters that are more than two UTF-8 + /// bytes wide, where a byte-at-a-time cast produces even more mangled + /// output than the two-byte Latin-1 case above. + #[test] + fn cjk_literal_text_round_trips() { + let data = row(&[("id", json!("1"))]); + assert_eq!( + resolve_name_template("动画-{id}.mp4", &data, 0).unwrap(), + "动画-1.mp4" + ); + } } #[cfg(test)] diff --git a/crates/rustmotion/tests/audit_ws_d.rs b/crates/rustmotion/tests/audit_ws_d.rs index 36c6a6a1..23c484e7 100644 --- a/crates/rustmotion/tests/audit_ws_d.rs +++ b/crates/rustmotion/tests/audit_ws_d.rs @@ -1,10 +1,29 @@ //! Regression tests for the media-decoding, frame-cache, and render/batch //! CLI hardening pass on this branch. //! -//! Video frame preextraction (`preload::preextract_video_frames`) is -//! exercised through the crate's public `rustmotion::engine::preload` -//! surface directly — no subprocess needed, since the budget math and the -//! cache it guards are both `pub`. +//! Two kinds of coverage land here: +//! +//! - Video frame preextraction (`preload::preextract_video_frames`) is +//! exercised through the crate's public `rustmotion::engine::preload` +//! surface directly — no subprocess needed, since the budget math and the +//! cache it guards are both `pub`. +//! - The batch `--name-template` byte-vs-char bug +//! (`cli::commands::batch::resolve_name_template`) cannot be reached this +//! way: `cli::commands` is a private module (`mod commands;` in +//! `src/cli/mod.rs`), so — mirroring `audit_ws_b.rs`'s reasoning for the +//! same constraint — the only externally-observable contract is the +//! compiled binary itself, driven as a subprocess. +//! +//! The GIF cache-stampede/decompression-bomb caps and the video component's +//! dead-field fixes (`fit`, `loop_video`, `trim_end`, straight-vs-premultiplied +//! alpha) live in `rustmotion-components`'s own test suite instead — see +//! that crate's `audit_ws_d.rs` and `gif.rs`'s in-file `mod tests`. The +//! `--watch` cache-clearing fix needs a private helper in +//! `cli::commands::render` and lives in that file's own `mod tests` for the +//! same private-module reason as the name-template test above. + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; use rustmotion::engine::preload::{ video_frame_byte_size, would_exceed_cache_budget, VIDEO_FRAME_CACHE_BUDGET_BYTES, @@ -23,11 +42,10 @@ fn frame_byte_size_matches_plain_multiplication_for_ordinary_dimensions() { /// The secondary hazard this closes: `width * height * 4` in plain `u32` /// wraps for a large-enough declared size (65536×16384 wraps to 0, which -/// used to turn into a division by zero downstream). `u32::MAX` on both -/// dimensions is the most extreme case reachable from a `style.width`/ -/// `style.height` pair — the fixed computation must saturate to `u64::MAX`, -/// not wrap to some small number that would slip past the budget check -/// below. +/// used to turn into a division by zero downstream). `u32::MAX` on both dimensions +/// is the most extreme case reachable from a `style.width`/`style.height` +/// pair — the fixed computation must saturate to `u64::MAX`, not wrap to +/// some small number that would slip past the budget check below. #[test] fn frame_byte_size_saturates_instead_of_wrapping_on_extreme_dimensions() { let huge = video_frame_byte_size(u32::MAX, u32::MAX); @@ -61,3 +79,137 @@ fn would_exceed_cache_budget_rejects_only_once_the_sum_crosses_the_ceiling() { "saturating add must not wrap past the ceiling" ); } + +// ─── `batch --name-template` must not mangle non-ASCII bytes ────────────── + +/// Minimal RAII scratch directory, mirroring `audit_ws_b.rs`'s `ScratchDir`. +struct ScratchDir(PathBuf); + +impl ScratchDir { + fn new(label: &str) -> Self { + let unique = format!( + "rustmotion-audit-ws-d-{label}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock before UNIX epoch") + .as_nanos() + ); + let path = std::env::temp_dir().join(unique); + std::fs::create_dir_all(&path).expect("create scratch dir"); + Self(path) + } +} + +impl Drop for ScratchDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +fn write_file(dir: &Path, name: &str, contents: &str) -> PathBuf { + let path = dir.join(name); + std::fs::write(&path, contents).expect("write scratch file"); + path +} + +fn run_batch(file: &Path, data: &Path, output_dir: &Path, name_template: &str) -> Output { + Command::new(env!("CARGO_BIN_EXE_rustmotion")) + .arg("--quiet") + .arg("batch") + .arg("--file") + .arg(file) + .arg("--data") + .arg(data) + .arg("--output-dir") + .arg(output_dir) + .arg("--name-template") + .arg(name_template) + .arg("--format") + .arg("png-seq") + .output() + .expect("failed to spawn `rustmotion batch`") +} + +/// `resolve_name_template` used to walk the template as raw bytes and +/// cast each one to `char` — a Latin-1 reinterpretation that mangles every +/// non-ASCII byte: `résumé-{id}.mp4` became `résumé-VAL.mp4` on disk. +/// `batch`'s own module doc gives `{lang}/{id} +/// .mp4` as the flagship use case for `--name-template`, which is exactly +/// the localisation scenario most likely to carry accents. +/// +/// Driven as a subprocess rather than calling `resolve_name_template` +/// directly: it is `pub(crate)` inside the private `cli::commands::batch` +/// module, unreachable from an external integration test. +#[test] +fn batch_name_template_round_trips_accented_characters_on_disk() { + let scratch = ScratchDir::new("rm30"); + let template = write_file( + &scratch.0, + "template.json", + &serde_json::json!({ + "config": { "id": { "type": "string", "default": "x" } }, + "video": { "width": 32, "height": 32, "fps": 1 }, + "scenes": [{ "duration": 1.0, "children": [] }] + }) + .to_string(), + ); + let data = write_file( + &scratch.0, + "data.jsonl", + &serde_json::json!({"id": "abc"}).to_string(), + ); + let output_dir = scratch.0.join("out"); + std::fs::create_dir_all(&output_dir).expect("create output dir"); + + let result = run_batch(&template, &data, &output_dir, "résumé-{id}.png"); + + assert!( + result.status.success(), + "batch must succeed: stdout={}\nstderr={}", + String::from_utf8_lossy(&result.stdout), + String::from_utf8_lossy(&result.stderr) + ); + + let expected = output_dir.join("résumé-abc.png"); + assert!( + expected.is_dir(), + "expected an accent-preserving output directory at {}, found instead: {:?}", + expected.display(), + std::fs::read_dir(&output_dir) + .map(|entries| entries + .filter_map(|e| e.ok().map(|e| e.file_name())) + .collect::>()) + .unwrap_or_default() + ); + assert!(expected.join("frame_00000.png").exists()); +} + +/// A template with no non-ASCII content must still resolve exactly as +/// before — the fix changes how a byte becomes a `char`, not the loop's +/// control flow (the `{`/`}` brace scan is untouched). +#[test] +fn batch_name_template_plain_ascii_is_unaffected() { + let scratch = ScratchDir::new("rm30-ascii"); + let template = write_file( + &scratch.0, + "template.json", + &serde_json::json!({ + "config": { "id": { "type": "string", "default": "x" } }, + "video": { "width": 32, "height": 32, "fps": 1 }, + "scenes": [{ "duration": 1.0, "children": [] }] + }) + .to_string(), + ); + let data = write_file( + &scratch.0, + "data.jsonl", + &serde_json::json!({"id": "abc"}).to_string(), + ); + let output_dir = scratch.0.join("out"); + std::fs::create_dir_all(&output_dir).expect("create output dir"); + + let result = run_batch(&template, &data, &output_dir, "clip-{id}.png"); + assert!(result.status.success()); + assert!(output_dir.join("clip-abc.png").is_dir()); +}