From d30f49a69c9d17e7ebfb905b006b59cc625e5b5d Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:59:30 +0200 Subject: [PATCH 01/25] fix(encode): reject a non-positive playback rate (#287) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `remaining` never reaches 0.5 when it starts at 0.0 (0.0/0.5 == 0.0) or at any negative value (it diverges toward -inf). The loop never terminates and pushes a fresh `String` on every iteration, so rustmotion hangs while consuming memory until the OOM killer fires. Reached from a plain scenario JSON: `{"type":"video","src":"clip.mp4","playback_rate":0}` — `volume` defaults to 1.0 (`crates/rustmotion-components/src/video.rs:14` `fn default_volume() -> f32 { 1.0 }`), so `collect_videos_in_child` collects it (`v.volume > 0.0`, line 128), `collect_video_audio_tracks` calls `extract_audio_to_wav(..., occ.playback_rate)` (line 421), which calls `build_atempo_filter(0.0)` at line 333. `playback_rate` is an unvalidated `Option` (`crates/rustmotion-components/src/video.rs:26`); a repo-wide grep shows no validator constrains it. Both the ffmpeg render path (`encode_with_ffmpeg_hw_impl` line 432) and the native path (`mux.rs:34`) reach it. Refs #220 --- crates/rustmotion/src/encode/video_audio.rs | 27 +++++++++++++- crates/rustmotion/tests/audit_ws_c.rs | 39 +++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/crates/rustmotion/src/encode/video_audio.rs b/crates/rustmotion/src/encode/video_audio.rs index 5ce3c1fb..caac9e1e 100644 --- a/crates/rustmotion/src/encode/video_audio.rs +++ b/crates/rustmotion/src/encode/video_audio.rs @@ -199,19 +199,41 @@ fn ffmpeg_available() -> bool { /// rate 4.0 → `atempo=2.0,atempo=2.0` /// rate 0.1 → `atempo=0.5,atempo=0.2` (0.5 * 0.2 = 0.1) /// -/// Returns `None` if rate == 1.0 (no filter needed). +/// Returns `None` if rate == 1.0 (no filter needed), or if `rate` cannot +/// possibly be reached by any chain of `atempo` stages (`<= 0.0` or +/// non-finite — see the guard below). pub fn build_atempo_filter(rate: f64) -> Option { const EPSILON: f64 = 1e-9; + // `remaining` only ever converges toward `[0.5, 2.0]` by repeatedly + // multiplying or dividing by 2.0 starting from a *positive, finite* + // `rate`. At `rate == 0.0`, `remaining /= 0.5` stays `0.0` forever; at a + // negative or non-finite rate it diverges away from the loop's own exit + // test. Either way the `while` below never terminates and pushes a new + // `String` on every turn — the guard has to reject these before that + // loop is ever reached, not inside it. + if !rate.is_finite() || rate <= 0.0 { + return None; + } if (rate - 1.0).abs() < EPSILON { return None; } + // A ceiling on the chain length, independent of the guard above: a + // legitimate rate as extreme as 1e9 only needs ~30 stages, so this never + // fires for real input. It exists so that a future mistake in this + // arithmetic degrades into "no atempo filter" instead of reopening the + // same unbounded loop the guard above closes. + const MAX_STAGES: usize = 64; + let mut parts: Vec = Vec::new(); let mut remaining = rate; if rate > 1.0 { // Each stage multiplies by at most 2.0 while remaining > 2.0 + EPSILON { + if parts.len() >= MAX_STAGES { + return None; + } parts.push("atempo=2.0".to_string()); remaining /= 2.0; } @@ -219,6 +241,9 @@ pub fn build_atempo_filter(rate: f64) -> Option { } else { // Each stage multiplies by at least 0.5 while remaining < 0.5 - EPSILON { + if parts.len() >= MAX_STAGES { + return None; + } parts.push("atempo=0.5".to_string()); remaining /= 0.5; } diff --git a/crates/rustmotion/tests/audit_ws_c.rs b/crates/rustmotion/tests/audit_ws_c.rs index e612794e..0b18c5b3 100644 --- a/crates/rustmotion/tests/audit_ws_c.rs +++ b/crates/rustmotion/tests/audit_ws_c.rs @@ -115,3 +115,42 @@ fn a_render_ffmpeg_rejects_leaves_no_debris_at_the_output_path() { out.path().display() ); } + +// ─── build_atempo_filter(rate) must not loop forever ─────────────────────── +// +// `remaining /= 0.5` never reaches the loop's `>= 0.5` exit test starting +// from `0.0`, and diverges away from it starting from any negative rate — +// either way the pre-fix loop pushed a fresh `String` forever. Calling the +// unguarded function directly on the test thread would hang the whole +// suite, so each candidate rate runs on its own thread with a bounded +// `recv_timeout`: a present guard returns well inside the timeout, a +// missing one times out and fails the assertion instead of the process. +#[test] +fn atempo_guard_rejects_non_positive_and_non_finite_rates_without_hanging() { + use std::sync::mpsc; + use std::time::Duration; + + for rate in [ + 0.0_f64, + -1.0, + -0.25, + f64::NAN, + f64::INFINITY, + f64::NEG_INFINITY, + ] { + let (tx, rx) = mpsc::channel(); + std::thread::spawn(move || { + let _ = tx.send(rustmotion::encode::video_audio::build_atempo_filter(rate)); + }); + match rx.recv_timeout(Duration::from_secs(2)) { + Ok(result) => assert_eq!( + result, None, + "rate={rate} must return None instead of building an atempo chain" + ), + Err(_) => panic!( + "rate={rate} did not return within 2s — the guard against the infinite loop \ + in build_atempo_filter is missing or broken" + ), + } + } +} From 928f9efc80812cf4b602c64d10f450423996546c Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:59:54 +0200 Subject: [PATCH 02/25] perf(preload): bound the extracted video frame cache (#288) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Triggering scenario: a scenario JSON declaring a `video` component with `style.width/height` of 1920x1080 inside a 60 s scene at 30 fps. `.output()` (line 234) collects ffmpeg's entire rawvideo stdout into one `Vec`: 1800 frames x 8.3 MB = 15 GB. `data` then stays borrowed for the whole copy loop, so the per-frame `.to_vec()` copies build a second 15 GB structure alongside it — 30 GB peak before `output.stdout` is dropped. The result is inserted into the process-global `VIDEO_FRAME_CACHE` (assets.rs:168), which has no eviction and no `clear` function at all. There is no size, duration, or resolution cap anywhere on this path, and the inputs are entirely scenario-controlled. Under `--watch` this compounds: changing the component's `style.width` mints a new `"src:WxH"` key while the old multi-GB entry is never freed. Secondary hazard on the same line: `width * height * 4` is plain u32 arithmetic, so a declared 65536x16384 wraps to 0 and `data.len() / frame_size` panics with a divide-by-zero. Refs #220 --- crates/rustmotion/src/engine/preload.rs | 163 +++++++++++++++++++++--- crates/rustmotion/tests/audit_ws_d.rs | 63 +++++++++ 2 files changed, 210 insertions(+), 16 deletions(-) create mode 100644 crates/rustmotion/tests/audit_ws_d.rs diff --git a/crates/rustmotion/src/engine/preload.rs b/crates/rustmotion/src/engine/preload.rs index 8f19c877..dc132c5b 100644 --- a/crates/rustmotion/src/engine/preload.rs +++ b/crates/rustmotion/src/engine/preload.rs @@ -1,3 +1,4 @@ +use std::io::Read; use std::sync::Arc; use crate::components::{ChildComponent, Component}; @@ -8,6 +9,54 @@ use rustmotion_core::engine::renderer::{ }; use rustmotion_core::traits::{Styled, Timed}; +/// Total bytes `VIDEO_FRAME_CACHE` may hold across every distinct +/// `(src, width, height)` entry combined. `preextract_video_frames` refuses +/// to add an entry that would push the cache past this ceiling rather than +/// caching it anyway — a video past the budget renders blank for the +/// affected frames, the same degraded outcome an ffmpeg failure already +/// produces on this path, instead of the process exhausting memory (a single +/// 1080p 30s embed alone reaches ~7.5 GB of raw decoded RGBA held in memory +/// forever, with no eviction). +pub const VIDEO_FRAME_CACHE_BUDGET_BYTES: u64 = 1024 * 1024 * 1024; + +/// Bytes one raw RGBA frame at `width`×`height` occupies, computed in `u64` +/// and saturating rather than the plain `u32` multiplication this used to be: +/// `width * height * 4` in `u32` wraps for a large-enough declared size +/// (65536×16384 wraps to 0), which downstream turned into a division by +/// zero. Saturating instead of panicking means an absurd declared size still +/// fails the budget check below rather than crashing the preload pass. +pub fn video_frame_byte_size(width: u32, height: u32) -> u64 { + u64::from(width) + .saturating_mul(u64::from(height)) + .saturating_mul(4) +} + +/// Whether caching `additional_bytes` more on top of `already_cached_bytes` +/// would cross [`VIDEO_FRAME_CACHE_BUDGET_BYTES`]. Saturating so a caller +/// that already (somehow) exceeds the budget, or an `additional_bytes` at +/// `u64::MAX` from a saturated [`video_frame_byte_size`], still reports +/// "over budget" instead of wrapping back under it. +pub fn would_exceed_cache_budget(already_cached_bytes: u64, additional_bytes: u64) -> bool { + already_cached_bytes.saturating_add(additional_bytes) > VIDEO_FRAME_CACHE_BUDGET_BYTES +} + +/// Bytes currently held across every entry of the process-global video-frame +/// cache. `VIDEO_FRAME_CACHE` has no eviction (see `assets.rs`), so this is a +/// running total the caller checks before adding to it, not a size taken +/// from any single-entry accounting the map itself keeps. +fn video_frame_cache_bytes() -> u64 { + video_frame_cache() + .iter() + .map(|entry| { + entry + .value() + .iter() + .map(|(_, data, _, _)| data.len() as u64) + .sum::() + }) + .sum() +} + /// Pre-fetch and cache all icon components before rendering. /// Call this before the render loop to avoid HTTP requests during parallel rendering. pub fn prefetch_icons(scenes: &[Scene]) { @@ -157,6 +206,16 @@ pub fn prefetch_icons(scenes: &[Scene]) { /// already established for embedded-video *audio* extraction /// (`encode::video_audio::collect_video_audio_tracks`), which this frame /// path never inherited. +/// +/// ffmpeg's rawvideo stdout is read directly off the pipe in +/// `frame_byte_size` chunks (`Read::read_exact`) rather than buffered whole +/// via `Command::output` and then copied frame-by-frame out of that buffer — +/// the old shape held the full decode in memory twice at its peak. A byte +/// budget (`would_exceed_cache_budget`) is checked before ffmpeg is even +/// spawned, and the read loop itself stops at `expected_frames` regardless, +/// so a source that would blow the budget is refused up front and one that +/// somehow outputs more frames than the requested time range implies cannot +/// grow the cache past what was budgeted for it. pub fn preextract_video_frames(scenes: &[Scene], fps: u32) { if !ffmpeg_available() { eprintln!( @@ -212,7 +271,34 @@ pub fn preextract_video_frames(scenes: &[Scene], fps: u32) { let max_time = times.last().copied().unwrap_or(0.0); let duration = max_time - min_time + (1.0 / fps as f64); - let output = std::process::Command::new("ffmpeg") + let frame_byte_size = video_frame_byte_size(width, height); + if frame_byte_size == 0 { + eprintln!( + "rustmotion: video frame preextraction: '{}' resolved to a zero-byte \ + frame size ({width}x{height}) — refusing to preextract. This video will \ + render blank for the affected frames.", + video.src + ); + return; + } + let expected_frames = (times.len() as u64).saturating_add(1); + let expected_bytes = frame_byte_size.saturating_mul(expected_frames); + let already_cached = video_frame_cache_bytes(); + if would_exceed_cache_budget(already_cached, expected_bytes) { + eprintln!( + "rustmotion: video frame preextraction: caching '{}' at {width}x{height} \ + would need ~{} MiB on top of the {} MiB already cached, over the {} MiB \ + budget — refusing to preextract. This video will render blank for the \ + affected frames.", + video.src, + expected_bytes / (1024 * 1024), + already_cached / (1024 * 1024), + VIDEO_FRAME_CACHE_BUDGET_BYTES / (1024 * 1024), + ); + return; + } + + let mut child = match std::process::Command::new("ffmpeg") .args([ "-ss", &format!("{:.3}", min_time), @@ -231,35 +317,80 @@ pub fn preextract_video_frames(scenes: &[Scene], fps: u32) { ]) .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::null()) - .output(); + .spawn() + { + Ok(child) => child, + Err(e) => { + eprintln!( + "rustmotion: video frame preextraction: could not spawn ffmpeg for \ + '{}': {}. This video will render blank for the affected frames.", + video.src, e + ); + return; + } + }; - match output { - Ok(output) if output.status.success() => { - let frame_size = (width * height * 4) as usize; - let data = &output.stdout; - let num_frames = data.len() / frame_size; - let mut frames: Vec<(f64, Vec, u32, u32)> = Vec::with_capacity(num_frames); + let Some(mut stdout) = child.stdout.take() else { + eprintln!( + "rustmotion: video frame preextraction: ffmpeg for '{}' produced no \ + stdout pipe. This video will render blank for the affected frames.", + video.src + ); + let _ = child.wait(); + return; + }; - for idx in 0..num_frames { - let start = idx * frame_size; - let frame_data = data[start..start + frame_size].to_vec(); - let time = min_time + idx as f64 / fps as f64; - frames.push((time, frame_data, width, height)); + let frame_size = frame_byte_size as usize; + let max_frames = expected_frames as usize; + let mut frames: Vec<(f64, Vec, u32, u32)> = Vec::with_capacity(times.len()); + loop { + if frames.len() >= max_frames { + break; + } + let mut buf = vec![0u8; frame_size]; + match stdout.read_exact(&mut buf) { + Ok(()) => { + let time = min_time + frames.len() as f64 / fps as f64; + frames.push((time, buf, width, height)); + } + Err(e) if e.kind() == std::io::ErrorKind::UnexpectedEof => break, + Err(e) => { + eprintln!( + "rustmotion: video frame preextraction: reading ffmpeg output for \ + '{}' failed: {}. Keeping the {} frame(s) decoded so far.", + video.src, + e, + frames.len() + ); + break; } + } + } + drop(stdout); + match child.wait() { + Ok(status) if status.success() => { + if frames.is_empty() { + eprintln!( + "rustmotion: video frame preextraction: ffmpeg produced no frames \ + for '{}'. This video will render blank for the affected frames.", + video.src + ); + return; + } cache.insert(cache_key, Arc::new(frames)); } - Ok(output) => { + Ok(status) => { eprintln!( "rustmotion: video frame preextraction: ffmpeg failed to decode \ frames from '{}' (exit status: {}). This video will render blank \ for the affected frames.", - video.src, output.status + video.src, status ); } Err(e) => { eprintln!( - "rustmotion: video frame preextraction: could not spawn ffmpeg for \ + "rustmotion: video frame preextraction: could not wait on ffmpeg for \ '{}': {}. This video will render blank for the affected frames.", video.src, e ); diff --git a/crates/rustmotion/tests/audit_ws_d.rs b/crates/rustmotion/tests/audit_ws_d.rs new file mode 100644 index 00000000..36c6a6a1 --- /dev/null +++ b/crates/rustmotion/tests/audit_ws_d.rs @@ -0,0 +1,63 @@ +//! 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`. + +use rustmotion::engine::preload::{ + video_frame_byte_size, would_exceed_cache_budget, VIDEO_FRAME_CACHE_BUDGET_BYTES, +}; + +// ─── Video frame cache: byte-size arithmetic and budget accounting ──────── + +/// Ordinary dimensions must still compute the exact byte count a naive +/// `width * height * 4` would — the fix only changes what happens past +/// `u32::MAX`, not everyday results. +#[test] +fn frame_byte_size_matches_plain_multiplication_for_ordinary_dimensions() { + assert_eq!(video_frame_byte_size(1920, 1080), 1920u64 * 1080 * 4); + assert_eq!(video_frame_byte_size(0, 0), 0); +} + +/// 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. +#[test] +fn frame_byte_size_saturates_instead_of_wrapping_on_extreme_dimensions() { + let huge = video_frame_byte_size(u32::MAX, u32::MAX); + assert_eq!( + huge, + u64::MAX, + "must saturate at u64::MAX, never wrap silently to a small number" + ); + assert!( + would_exceed_cache_budget(0, huge), + "a saturated size must always fail the budget check" + ); +} + +/// The exact policy `preextract_video_frames` applies before spawning +/// ffmpeg: refuse only once the *sum* crosses the ceiling, and never let a +/// saturated `u64::MAX` addend wrap the sum back under it. +#[test] +fn would_exceed_cache_budget_rejects_only_once_the_sum_crosses_the_ceiling() { + assert!(!would_exceed_cache_budget( + 0, + VIDEO_FRAME_CACHE_BUDGET_BYTES + )); + assert!(would_exceed_cache_budget( + 0, + VIDEO_FRAME_CACHE_BUDGET_BYTES + 1 + )); + assert!(would_exceed_cache_budget(VIDEO_FRAME_CACHE_BUDGET_BYTES, 1)); + assert!( + would_exceed_cache_budget(u64::MAX, 1), + "saturating add must not wrap past the ceiling" + ); +} From c029b9f27e7b2cb409c10384e13b3d91a7995394 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:00:10 +0200 Subject: [PATCH 03/25] fix(render): clear asset caches between watch renders (#242) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Triggering scenario: `rustmotion render deck.json -o out.mp4 --watch` left open for an afternoon, with an `image` component pointing at `logo.png`. The author re-exports `logo.png`. `ASSET_CACHE` is keyed on the path alone (image.rs:41-54) with no mtime or size component, and in the incremental branch `clear_asset_cache()` only runs when the video config hash changes (resolution/fps) — editing scene content never clears it. Every subsequent re-render silently re-uses the stale decoded bitmap; the author sees the old logo indefinitely with nothing on stderr. The adjacent audio path solved exactly this and documents why (`encode/audio_analysis.rs:29-39`: fingerprint on len+mtime because 'someone re-exports a mix while the studio is open') — the image/GIF/video caches never got the same treatment. Memory side: `clear_asset_cache` touches only `ASSET_CACHE`; `GIF_CACHE`, `VIDEO_FRAME_CACHE` and `AUDIO_ANALYSIS_CACHE` have no clear function at all, so a long `--watch` session that touches several asset variants accumulates every decode for the process lifetime. Refs #220 --- crates/rustmotion-components/src/gif.rs | 174 ++++++++++++++++++- crates/rustmotion/src/cli/commands/render.rs | 75 +++++++- 2 files changed, 238 insertions(+), 11 deletions(-) diff --git a/crates/rustmotion-components/src/gif.rs b/crates/rustmotion-components/src/gif.rs index 529624ca..b5698604 100644 --- a/crates/rustmotion-components/src/gif.rs +++ b/crates/rustmotion-components/src/gif.rs @@ -102,8 +102,26 @@ fn stall_decode_for_tests() { } } +/// Hard ceiling on one composed GIF canvas frame's byte size +/// (`width × height × 4`), independent of the render's own video dimensions. +/// A GIF's logical-screen descriptor is two `u16` fields straight from the +/// file header — up to 65535×65535, a 17.2 GiB single allocation — and +/// nothing validated them before this cap existed. +const MAX_GIF_CANVAS_BYTES: u64 = 128 * 1024 * 1024; + +/// Hard ceiling on the number of animation frames one GIF decodes into. +/// Real GIFs rarely exceed a few hundred; this keeps a maliciously (or just +/// accidentally) long frame count from growing the decoded frame list +/// without bound even when each individual frame is well under the canvas +/// budget above. +const MAX_GIF_FRAMES: usize = 600; + /// Decode a GIF into full-canvas RGBA frames, their cumulative end times, and -/// the total duration. +/// the total duration. `max_canvas_w`/`max_canvas_h` are the render's own +/// video dimensions: a GIF canvas larger than that can never be usefully +/// drawn (it only ever gets scaled into the component's layout box, which is +/// at most the video frame), so it is rejected the same way an +/// over-budget canvas is. /// /// Every frame after the first is usually a *sub-rectangle* holding only the /// pixels that changed, so frames must be composed onto a persistent canvas @@ -113,7 +131,7 @@ fn stall_decode_for_tests() { /// why only the first frame ever appeared (issue #185). /// /// `None` means nothing can be drawn, and the reason has already been reported. -fn decode_composed_frames(src: &str) -> Option { +fn decode_composed_frames(src: &str, max_canvas_w: u32, max_canvas_h: u32) -> Option { let file = match std::fs::File::open(src) { Ok(f) => f, Err(e) => { @@ -139,6 +157,18 @@ fn decode_composed_frames(src: &str) -> Option { let canvas_w = decoder.width() as u32; let canvas_h = decoder.height() as u32; + let canvas_bytes = canvas_w as u64 * canvas_h as u64 * 4; + if canvas_bytes > MAX_GIF_CANVAS_BYTES || canvas_w > max_canvas_w || canvas_h > max_canvas_h { + if crate::warn_once_for(&format!("gif-oversized:{src}")) { + eprintln!( + "rustmotion: gif '{src}' declares a {canvas_w}x{canvas_h} canvas ({canvas_bytes} \ + bytes/frame), over the {MAX_GIF_CANVAS_BYTES}-byte budget or larger than this \ + render's own {max_canvas_w}x{max_canvas_h} video — refusing to decode it." + ); + } + return None; + } + #[cfg(test)] stall_decode_for_tests(); @@ -148,6 +178,16 @@ fn decode_composed_frames(src: &str) -> Option { let mut composed = vec![0u8; canvas_w as usize * canvas_h as usize * 4]; while let Ok(Some(frame)) = decoder.read_next_frame() { + if frames.len() >= MAX_GIF_FRAMES { + if crate::warn_once_for(&format!("gif-frame-cap:{src}")) { + eprintln!( + "rustmotion: gif '{src}' has more than {MAX_GIF_FRAMES} frames — truncating \ + the decoded animation at the cap." + ); + } + break; + } + // `Previous` disposal restores what was there before this frame, so it // has to be captured before compositing. let restore = (frame.dispose == gif::DisposalMethod::Previous).then(|| composed.clone()); @@ -189,10 +229,14 @@ fn decode_composed_frames(src: &str) -> Option { /// failure (`decode_composed_frames` returning `None`) leaves the entry /// vacant — `or_try_insert_with` never calls `insert` on its `Err` path — /// so a broken source is retried rather than permanently cached as absent. -fn cached_decode(src: &str) -> Option> { +fn cached_decode(src: &str, max_canvas_w: u32, max_canvas_h: u32) -> Option> { gif_cache() .entry(src.to_string()) - .or_try_insert_with(|| decode_composed_frames(src).map(Arc::new).ok_or(())) + .or_try_insert_with(|| { + decode_composed_frames(src, max_canvas_w, max_canvas_h) + .map(Arc::new) + .ok_or(()) + }) .ok() .map(|entry| entry.clone()) } @@ -205,7 +249,7 @@ impl Painter for Gif { _props: &AnimatedProperties, ctx: &PaintCtx, ) { - let Some(cached) = cached_decode(&self.src) else { + let Some(cached) = cached_decode(&self.src, ctx.video_width, ctx.video_height) else { return; }; @@ -282,7 +326,8 @@ mod tests { write_two_frame_gif(&path); let (frames, times, total) = - decode_composed_frames(path.to_str().expect("utf-8 path")).expect("gif must decode"); + decode_composed_frames(path.to_str().expect("utf-8 path"), 1920, 1080) + .expect("gif must decode"); std::fs::remove_file(&path).ok(); assert_eq!(frames.len(), 2, "both frames must be drawable"); @@ -312,7 +357,7 @@ mod tests { #[test] fn a_missing_file_reports_instead_of_returning_nothing() { let missing = std::env::temp_dir().join("rustmotion_gif_absent_xyz.gif"); - assert!(decode_composed_frames(missing.to_str().expect("utf-8")).is_none()); + assert!(decode_composed_frames(missing.to_str().expect("utf-8"), 1920, 1080).is_none()); // The warn-once slot must have been claimed — silence is the bug. assert!( !crate::warn_once_for(&format!("gif-open:{}", missing.to_str().expect("utf-8"))), @@ -371,7 +416,7 @@ mod tests { let src = src.clone(); std::thread::spawn(move || { barrier.wait(); - cached_decode(&src) + cached_decode(&src, 4, 2) }) }) .collect(); @@ -392,4 +437,117 @@ mod tests { ); } } + + /// Writes a GIF whose logical-screen descriptor declares `w`×`h` but + /// whose only frame is 1×1 — the crafted-header shape a decompression + /// bomb takes: a file of a few hundred bytes that asks the decoder to commit to a + /// canvas orders of magnitude larger than anything it actually encodes. + fn write_oversized_header_gif(path: &std::path::Path, w: u16, h: u16) { + let palette: &[u8] = &[0, 0, 0, 255, 255, 255]; + let mut file = std::fs::File::create(path).expect("create gif fixture"); + let mut encoder = gif::Encoder::new(&mut file, w, h, palette).expect("gif encoder"); + let frame = gif::Frame::from_indexed_pixels(1, 1, vec![0], None); + encoder.write_frame(&frame).expect("write frame"); + } + + /// A 6000×6000 canvas is 144 MiB per frame — comfortably over + /// `MAX_GIF_CANVAS_BYTES` (128 MiB) regardless of how large the render's + /// own video is, so passing generous `max_w`/`max_h` here isolates the + /// byte-budget check from the video-dimensions check exercised by the + /// next test. 144 MiB is deliberately far short of the 65535×65535 + /// (~17 GiB) header a real crafted file could declare — large enough to + /// prove the budget check fires, small enough that running this test + /// never risks the allocation it is asserting never happens. + #[test] + fn a_canvas_over_the_byte_budget_is_rejected_without_allocating_it() { + let path = std::env::temp_dir().join(format!( + "rustmotion_gif_bomb_budget_{}_{}.gif", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(), + )); + write_oversized_header_gif(&path, 6000, 6000); + + let result = decode_composed_frames(path.to_str().expect("utf-8"), 8192, 8192); + std::fs::remove_file(&path).ok(); + + assert!( + result.is_none(), + "a 144 MiB canvas must be refused, not decoded" + ); + assert!( + !crate::warn_once_for(&format!("gif-oversized:{}", path.to_str().unwrap())), + "the rejection must have reported once" + ); + } + + /// The other half of the same guard: a canvas larger than the render's + /// own video dimensions can never be usefully drawn either. A + /// 3000×2000 canvas is only 24 MiB (well under the byte budget alone) + /// but bigger than the render's own 1920×1080 video, so this isolates + /// the video-dimensions check from the byte-budget one above. + #[test] + fn a_canvas_larger_than_the_video_is_rejected_even_under_the_byte_budget() { + let path = std::env::temp_dir().join(format!( + "rustmotion_gif_bomb_dims_{}_{}.gif", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(), + )); + write_oversized_header_gif(&path, 3000, 2000); + + let result = decode_composed_frames(path.to_str().expect("utf-8"), 1920, 1080); + std::fs::remove_file(&path).ok(); + + assert!( + result.is_none(), + "a canvas bigger than the video's own dimensions must be refused" + ); + } + + /// Writes `count` tiny (2×2) frames — cheap regardless of `count`, so + /// the frame-count cap can be tested without the per-frame size mattering. + fn write_many_frame_gif(path: &std::path::Path, count: u32) { + let palette: &[u8] = &[0, 0, 0, 255, 255, 255]; + let mut file = std::fs::File::create(path).expect("create gif fixture"); + let mut encoder = gif::Encoder::new(&mut file, 2, 2, palette).expect("gif encoder"); + for _ in 0..count { + let mut frame = gif::Frame::from_indexed_pixels(2, 2, vec![0, 1, 1, 0], None); + frame.delay = 1; + encoder.write_frame(&frame).expect("write frame"); + } + } + + /// The decode loop had no frame-count limit at all — a + /// small canvas with a pathologically large frame count still grows the + /// decoded animation without bound. `MAX_GIF_FRAMES` truncates it + /// instead of rejecting the whole GIF: a real, merely-too-long animation + /// still plays (up to the cap), it just doesn't keep growing memory. + #[test] + fn frame_count_beyond_the_cap_is_truncated_not_unbounded() { + let path = std::env::temp_dir().join(format!( + "rustmotion_gif_many_frames_{}_{}.gif", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(), + )); + write_many_frame_gif(&path, MAX_GIF_FRAMES as u32 + 50); + + let (frames, times, _total) = decode_composed_frames(path.to_str().expect("utf-8"), 10, 10) + .expect("a small, merely-long gif must still decode"); + std::fs::remove_file(&path).ok(); + + assert_eq!( + frames.len(), + MAX_GIF_FRAMES, + "frame count must be truncated to the cap, not grow past it" + ); + assert_eq!(times.len(), MAX_GIF_FRAMES); + } } diff --git a/crates/rustmotion/src/cli/commands/render.rs b/crates/rustmotion/src/cli/commands/render.rs index 2c6a11da..53fd6fd2 100644 --- a/crates/rustmotion/src/cli/commands/render.rs +++ b/crates/rustmotion/src/cli/commands/render.rs @@ -8,6 +8,20 @@ use std::path::{Path, PathBuf}; use crate::cli::commands::validation::{self, ValidationSource}; +/// Every process-global decode cache a `--watch` iteration must forget +/// before re-rendering, so an edited asset is never served from a stale +/// entry keyed only on its path. `ASSET_CACHE` already had a public +/// clear function; `GIF_CACHE`/`VIDEO_FRAME_CACHE` did not, so those two are +/// cleared here by calling `DashMap::clear()` on the map `gif_cache()`/ +/// `video_frame_cache()` already return, rather than adding new functions to +/// `rustmotion-core`'s `engine::renderer::assets` (owned by a sibling +/// workstream in this chantier). +fn clear_all_media_caches() { + engine::clear_asset_cache(); + engine::gif_cache().clear(); + engine::video_frame_cache().clear(); +} + /// Load + validate a scenario for watch mode. On validation failure prints the /// report and returns the typed error so the caller can decide how to handle it. /// @@ -373,7 +387,7 @@ pub fn cmd_watch( Err(e) => eprintln!("Render error: {}", e), } } else { - engine::clear_asset_cache(); + clear_all_media_caches(); if let Err(e) = cmd_render( scenario, output, @@ -430,6 +444,8 @@ pub fn cmd_watch( match load_for_watch(input, no_validate, lenient, strict_anim, strict_attrs) { Ok(scenario) => { + clear_all_media_caches(); + // Reset error backoff on a successful load if consecutive_err_count > 0 && suppressed { eprintln!("Recovered from previous errors."); @@ -452,7 +468,6 @@ pub fn cmd_watch( let use_prev = if prev_config_hash == Some(config_hash) { prev_segments.as_deref() } else { - engine::clear_asset_cache(); None }; @@ -528,7 +543,6 @@ pub fn cmd_watch( Err(e) => eprintln!("Render error: {}", e), } } else { - engine::clear_asset_cache(); if let Err(e) = cmd_render( scenario, output, @@ -596,3 +610,58 @@ fn render_single_frame( img.save(output)?; Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::engine::renderer::{asset_cache, gif_cache, video_frame_cache}; + + fn unique_marker(label: &str) -> String { + format!( + "rustmotion-audit-ws-d-rm25-{label}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock before UNIX epoch") + .as_nanos() + ) + } + + /// `--watch` only ever called `engine::clear_asset_cache()`, and + /// only conditionally in the incremental branch (when the video config + /// hash changed). `GIF_CACHE`/`VIDEO_FRAME_CACHE` had no clear function + /// at all, so an edited GIF or embedded video stayed stale for the rest + /// of a `--watch` session no matter how many times the source file + /// changed. This populates all three caches with markers unique to this + /// test run — safe against the other tests in this binary that share the + /// same process-global caches — and asserts a single call clears every + /// one of them, not just the asset cache. + #[test] + fn clear_all_media_caches_clears_gif_and_video_caches_not_just_images() { + let marker = unique_marker("clear-all"); + + let mut surface = skia_safe::surfaces::raster_n32_premul((1, 1)).expect("raster surface"); + asset_cache().insert(marker.clone(), surface.image_snapshot()); + gif_cache().insert( + marker.clone(), + std::sync::Arc::new((Vec::new(), Vec::new(), 0.0)), + ); + video_frame_cache().insert(marker.clone(), std::sync::Arc::new(Vec::new())); + + assert!(asset_cache().contains_key(&marker)); + assert!(gif_cache().contains_key(&marker)); + assert!(video_frame_cache().contains_key(&marker)); + + clear_all_media_caches(); + + assert!(!asset_cache().contains_key(&marker)); + assert!( + !gif_cache().contains_key(&marker), + "gif cache must be cleared too, not just the asset cache" + ); + assert!( + !video_frame_cache().contains_key(&marker), + "video frame cache must be cleared too, not just the asset cache" + ); + } +} From 9890365bd95be1dca2b53324bead964517830f50 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:00:33 +0200 Subject: [PATCH 04/25] fix(video): loop the picture with the audio (#245) Refs #220 --- crates/rustmotion-components/src/video.rs | 110 +++++++++- .../rustmotion-components/tests/audit_ws_d.rs | 188 ++++++++++++++++++ crates/rustmotion/src/cli/commands/batch.rs | 35 +++- crates/rustmotion/tests/audit_ws_d.rs | 170 +++++++++++++++- 4 files changed, 481 insertions(+), 22 deletions(-) create mode 100644 crates/rustmotion-components/tests/audit_ws_d.rs 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()); +} From 7298fd82e2cdcf311d44e19db21d7a555b2f0ee0 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:00:53 +0200 Subject: [PATCH 05/25] fix(video): treat decoded frames as unpremultiplied (#247) Refs #220 --- crates/rustmotion-components/src/gif.rs | 6 +- crates/rustmotion-components/src/video.rs | 32 +++-- .../rustmotion-components/tests/audit_ws_d.rs | 135 ++++++++++++++++++ 3 files changed, 161 insertions(+), 12 deletions(-) diff --git a/crates/rustmotion-components/src/gif.rs b/crates/rustmotion-components/src/gif.rs index b5698604..5a0d918d 100644 --- a/crates/rustmotion-components/src/gif.rs +++ b/crates/rustmotion-components/src/gif.rs @@ -455,9 +455,9 @@ mod tests { /// own video is, so passing generous `max_w`/`max_h` here isolates the /// byte-budget check from the video-dimensions check exercised by the /// next test. 144 MiB is deliberately far short of the 65535×65535 - /// (~17 GiB) header a real crafted file could declare — large enough to - /// prove the budget check fires, small enough that running this test - /// never risks the allocation it is asserting never happens. + /// (~17 GiB) header the audit's own crafted file could declare — large + /// enough to prove the budget check fires, small enough that running + /// this test never risks the allocation it is asserting never happens. #[test] fn a_canvas_over_the_byte_budget_is_rejected_without_allocating_it() { let path = std::env::temp_dir().join(format!( diff --git a/crates/rustmotion-components/src/video.rs b/crates/rustmotion-components/src/video.rs index d76cf0fc..cae7b018 100644 --- a/crates/rustmotion-components/src/video.rs +++ b/crates/rustmotion-components/src/video.rs @@ -95,10 +95,10 @@ fn draw_fitted(canvas: &Canvas, img: skia_safe::Image, fit: &ImageFit, layout: & /// 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. +/// mean one subprocess spawn per frame for any looping video with no +/// explicit `trim_end`. `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>>, @@ -122,15 +122,29 @@ fn video_duration_secs(src: &str) -> Option { 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. + /// + /// Honours `trim_end` on the picture the same way the audio track + /// already does: past `trim_end`, playback holds on the last in-window + /// frame instead of continuing to draw whatever the source contains + /// beyond the intended trim point. When `loop_video` is set, playback + /// wraps within `[trim_start, trim_end)` instead of clamping — falling + /// back to the source's own probed duration as the loop window only + /// when `trim_end` is absent, since that is the only case where the + /// window cannot otherwise be known at all. 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(end) = self.trim_end { + if end > trim_start { + return if self.loop_video == Some(true) { + trim_start + (raw - trim_start).rem_euclid(end - trim_start) + } else { + raw.min(end) + }; + } + } else 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); @@ -162,7 +176,7 @@ impl Painter for Video { let img_info = ImageInfo::new( (fw as i32, fh as i32), ColorType::RGBA8888, - skia_safe::AlphaType::Premul, + skia_safe::AlphaType::Unpremul, None, ); let row_bytes = fw as usize * 4; diff --git a/crates/rustmotion-components/tests/audit_ws_d.rs b/crates/rustmotion-components/tests/audit_ws_d.rs index 3cb43373..7ac9dbd8 100644 --- a/crates/rustmotion-components/tests/audit_ws_d.rs +++ b/crates/rustmotion-components/tests/audit_ws_d.rs @@ -186,3 +186,138 @@ fn fill_fit_still_stretches_to_the_whole_box() { "fill must cover every corner" ); } + +// ─── `trim_end` was honoured only on the extracted audio ─────────────────── + +/// Frames beyond `trim_end` sit in the cache (simulating a preextraction +/// window, or a direct extraction, wider than the intended trim), so the +/// picture path must never pick one of them once `trim_end` is set: past +/// `trim_end`, playback holds on the last in-window frame. Before the fix, +/// `source_time` had no upper bound at all — querying past `trim_end` on a +/// cache/source that extends further would draw whatever sits further +/// along the source, not the frame at the trim boundary. +#[test] +fn trim_end_clamps_playback_instead_of_running_past_it() { + let src = unique_src("trimend"); + let cache_key = format!("{src}:20x20"); + let frames = vec![ + (0.0, solid_rgba([255, 0, 0, 255], 2, 2), 2, 2), + (0.5, solid_rgba([0, 0, 255, 255], 2, 2), 2, 2), + (1.0, solid_rgba([0, 255, 0, 255], 2, 2), 2, 2), + (1.5, solid_rgba([128, 0, 128, 255], 2, 2), 2, 2), + (2.0, solid_rgba([255, 165, 0, 255], 2, 2), 2, 2), + ]; + video_frame_cache().insert(cache_key, Arc::new(frames)); + + let v = video(&src, ImageFit::Fill, Some(0.0), Some(1.0), None); + let ctx = ctx_at(1.8); + let pixels = paint_and_read(&v, &ctx, 20, 20, false); + + assert_eq!( + px(&pixels, 20, 10, 10), + [0, 255, 0, 255], + "past trim_end, playback must clamp to the frame at trim_end (green), not the frame \ + nearest the unclamped query time (orange)" + ); +} + +// ─── `loop_video` made neither the picture nor the audio loop ───────────── + +/// Cache frames only cover `[0.0, 1.0)`; `trim_end: Some(1.0)` gives +/// `loop_video` a window to wrap within without needing a real source file +/// to probe. Querying at `ctx.time = 2.1` (raw source time 2.1s, i.e. "2 +/// full loops plus 0.1s") must land near 0.1s once wrapped — nearest to +/// that among `{0.0, 0.25, 0.5, 0.75}` is red. Before this fix, the same +/// query — with the trim-end clamp from the previous test already in place +/// but no loop branch yet — clamped to `min(2.1, 1.0) = 1.0`, whose nearest +/// cached frame is yellow: a clearly different pixel, which is what proves +/// this test is exercising the loop path and not being masked by the clamp. +#[test] +fn loop_video_wraps_playback_within_the_trim_window() { + let src = unique_src("loop"); + let cache_key = format!("{src}:20x20"); + let frames = vec![ + (0.0, solid_rgba([255, 0, 0, 255], 2, 2), 2, 2), + (0.25, solid_rgba([0, 255, 0, 255], 2, 2), 2, 2), + (0.5, solid_rgba([0, 0, 255, 255], 2, 2), 2, 2), + (0.75, solid_rgba([255, 255, 0, 255], 2, 2), 2, 2), + ]; + video_frame_cache().insert(cache_key, Arc::new(frames)); + + let v = video(&src, ImageFit::Fill, Some(0.0), Some(1.0), Some(true)); + let ctx = ctx_at(2.1); + let pixels = paint_and_read(&v, &ctx, 20, 20, false); + + assert_eq!( + px(&pixels, 20, 10, 10), + [255, 0, 0, 255], + "looping must wrap the query time back into the window (nearest: red), not clamp to \ + the window's own end (nearest: yellow)" + ); +} + +/// Without `loop_video`, a `trim_end`-bounded video must still clamp +/// (unaffected by the loop branch existing) rather than wrap — the same +/// scenario as the wrap test above, minus the flag. +#[test] +fn without_loop_video_playback_still_clamps_not_wraps() { + let src = unique_src("no-loop"); + let cache_key = format!("{src}:20x20"); + let frames = vec![ + (0.0, solid_rgba([255, 0, 0, 255], 2, 2), 2, 2), + (0.25, solid_rgba([0, 255, 0, 255], 2, 2), 2, 2), + (0.5, solid_rgba([0, 0, 255, 255], 2, 2), 2, 2), + (0.75, solid_rgba([255, 255, 0, 255], 2, 2), 2, 2), + ]; + video_frame_cache().insert(cache_key, Arc::new(frames)); + + let v = video(&src, ImageFit::Fill, Some(0.0), Some(1.0), None); + let ctx = ctx_at(2.1); + let pixels = paint_and_read(&v, &ctx, 20, 20, false); + + assert_eq!( + px(&pixels, 20, 10, 10), + [255, 255, 0, 255], + "no loop_video: must clamp to the window's end (nearest: yellow), not wrap" + ); +} + +// ─── cached-frame draw path mistagged straight alpha as premultiplied ────── + +/// ffmpeg's `-pix_fmt rgba` output — what fills the video-frame cache — is +/// straight (unpremultiplied) alpha. Tagging that buffer `AlphaType::Premul` +/// makes Skia treat the RGB channels as already scaled by alpha instead of +/// scaling them itself, which brightens (here: doubles) every +/// semi-transparent pixel's channels once composited. +/// +/// A straight-alpha (200, 100, 50, 128) pixel, composited over black: +/// correctly tagged `Unpremul`, Skia premultiplies it to +/// (200×128/255, 100×128/255, 50×128/255) ≈ (100, 50, 25) before compositing +/// over black, landing there almost exactly (the `(1 - alpha) * 0` background +/// term vanishes either way). Mistagged `Premul`, Skia uses the raw channel +/// values directly as if already scaled — (200, 100, 50) — composited over +/// black with no further scaling, landing at roughly double the correct +/// result. +#[test] +fn cached_frame_straight_alpha_composites_correctly_not_doubled() { + let src = unique_src("alpha"); + let cache_key = format!("{src}:10x10"); + let straight = [200u8, 100, 50, 128]; + video_frame_cache().insert( + cache_key, + Arc::new(vec![(0.0, solid_rgba(straight, 2, 2), 2, 2)]), + ); + + let v = video(&src, ImageFit::Fill, None, None, None); + let ctx = ctx_at(0.0); + let pixels = paint_and_read(&v, &ctx, 10, 10, false); + let composited = px(&pixels, 10, 5, 5); + + let close = |actual: u8, expected: u8| (actual as i16 - expected as i16).abs() <= 4; + assert!( + close(composited[0], 100) && close(composited[1], 50) && close(composited[2], 25), + "straight-alpha (200,100,50,128) over black must composite to roughly (100,50,25), \ + got {composited:?} — a value near (200,100,50) means the buffer is still mistagged \ + as premultiplied and its channels are being used unscaled" + ); +} From 05e0074501b01f855f644bd435ae09d7a472bb07 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:01:33 +0200 Subject: [PATCH 06/25] fix(html): emit lengths the core parser can read (#289) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two parsers for the same syntax have drifted. Executed: `style="padding: 24px 48px; margin: 0 auto; grid-template-columns: repeat(3, 1fr)"` transpiles to `"padding":"24px 48px"`, `"margin":"0 auto"`, `"grid-template-columns":["repeat(3,","1fr)"]`. All three deserialize successfully — `Edges::Uniform(LengthPercentage::String)` and `GridTrack::Length(LengthPercentage::String)` are untagged string variants — and then core's `parse_length` (rustmotion-core/src/css/units.rs:125) only understands a single number+unit token, so `parse_length_or_warn` (units.rs:262) falls back to `Px(0.0)`: zero padding, zero margin, two zero-width grid tracks. The only signal is an `eprintln!` warning; `validate` exits 0 and the render is wrong. Per-side padding is entirely unexpressible from HTML (`padding-top:32` is rejected by `CssStyle`'s `deny_unknown_fields`, `padding:32px 48px` silently becomes 0), and the skill rule html-css-mental-model.md:97 only documents the JSON object form, which inline `style=""` cannot produce. Refs #220 --- crates/rustmotion-html/src/element.rs | 9 +- crates/rustmotion-html/src/style.rs | 217 +++++++++++++++++++-- crates/rustmotion-html/tests/audit_ws_f.rs | 92 +++++++++ 3 files changed, 298 insertions(+), 20 deletions(-) diff --git a/crates/rustmotion-html/src/element.rs b/crates/rustmotion-html/src/element.rs index b370f658..337c8b2e 100644 --- a/crates/rustmotion-html/src/element.rs +++ b/crates/rustmotion-html/src/element.rs @@ -58,11 +58,10 @@ fn collect_text(handle: &Handle, out: &mut String) { /// [`crate::style::parse_anim_attr`]) lands in `style.animation` — inline CSS /// cannot express animation arrays, so `anim` is the only writer of that key. fn style_object(attrs: &[(String, String)]) -> Result, HtmlError> { - let mut map = attrs - .iter() - .find(|(k, _)| k == "style") - .map(|(_, raw)| parse_inline_style(raw)) - .unwrap_or_default(); + let mut map = match attrs.iter().find(|(k, _)| k == "style") { + Some((_, raw)) => parse_inline_style(raw)?, + None => Map::new(), + }; if let Some((_, anim)) = attrs.iter().find(|(k, _)| k == "anim") { map.insert("animation".into(), parse_anim_attr(anim)?); } diff --git a/crates/rustmotion-html/src/style.rs b/crates/rustmotion-html/src/style.rs index d27a71e4..e80a239c 100644 --- a/crates/rustmotion-html/src/style.rs +++ b/crates/rustmotion-html/src/style.rs @@ -28,10 +28,16 @@ pub fn coerce_value(raw: &str) -> Value { Value::from(t.to_string()) } -/// Parse an inline `style="a:b; c:d"` declaration list into a JSON style object. -/// `grid-template-columns`/`-rows` are split into string arrays; all other -/// properties pass through their kebab-case name with a coerced value. -pub fn parse_inline_style(decls: &str) -> Map { +/// Parse an inline `style="a:b; c:d"` declaration list into a JSON style +/// object. `padding`/`margin`/`border-radius` accept the CSS 1/2/3/4-value +/// box shorthand, expanded into the `{top,right,bottom,left}` / +/// `{top-left,top-right,bottom-right,bottom-left}` object the core CSS +/// engine's `Edges`/`BorderRadius` types deserialize. `grid-template-columns`/ +/// `-rows` accept a track list, `repeat()`/`minmax()` included. Any other +/// property whose value is more than one top-level (paren-aware) token is +/// refused rather than passed through as an opaque string the core length +/// parser cannot read (see [`HtmlError::UnsupportedStyleShorthand`]). +pub fn parse_inline_style(decls: &str) -> Result, HtmlError> { let mut map = Map::new(); for decl in decls.split(';') { let decl = decl.trim(); @@ -43,17 +49,198 @@ pub fn parse_inline_style(decls: &str) -> Map { }; let prop = prop.trim().to_string(); let value = value.trim(); - if prop == "grid-template-columns" || prop == "grid-template-rows" { - let arr: Vec = value - .split_whitespace() - .map(|t| Value::from(t.to_string())) - .collect(); - map.insert(prop, Value::Array(arr)); - } else { - map.insert(prop, coerce_value(value)); + match prop.as_str() { + "grid-template-columns" | "grid-template-rows" => { + map.insert( + prop.clone(), + Value::Array(parse_grid_template(&prop, value)?), + ); + } + "padding" | "margin" => { + let tokens = split_top_level_tokens(value); + match tokens.len() { + 1 => { + map.insert(prop, coerce_value(value)); + } + 2..=4 => { + map.insert(prop, expand_box_edges(&tokens)); + } + _ => { + return Err(HtmlError::UnsupportedStyleShorthand { + prop, + value: value.to_string(), + }) + } + } + } + "border-radius" => { + let tokens = split_top_level_tokens(value); + match tokens.len() { + 1 => { + map.insert(prop, coerce_value(value)); + } + 2..=4 => { + map.insert(prop, expand_border_radius_corners(&tokens)); + } + _ => { + return Err(HtmlError::UnsupportedStyleShorthand { + prop, + value: value.to_string(), + }) + } + } + } + _ => { + if split_top_level_tokens(value).len() > 1 { + return Err(HtmlError::UnsupportedStyleShorthand { + prop, + value: value.to_string(), + }); + } + map.insert(prop, coerce_value(value)); + } + } + } + Ok(map) +} + +/// Split a CSS value on top-level whitespace: whitespace inside a `(...)` +/// span (e.g. the argument list of `rgba(0, 0, 0, 0.5)` or `repeat(3, 1fr)`) +/// does not count as a separator, so a single functional-notation value +/// stays one token while a genuine multi-value shorthand (`24px 48px`) +/// splits into its parts. +fn split_top_level_tokens(s: &str) -> Vec<&str> { + let mut tokens = Vec::new(); + let mut depth = 0i32; + let mut token_start: Option = None; + for (i, c) in s.char_indices() { + match c { + '(' => depth += 1, + ')' => depth = depth.saturating_sub(1), + _ => {} + } + if c.is_whitespace() && depth == 0 { + if let Some(start) = token_start.take() { + tokens.push(&s[start..i]); + } + } else if token_start.is_none() { + token_start = Some(i); + } + } + if let Some(start) = token_start { + tokens.push(&s[start..]); + } + tokens +} + +/// Expand a 2/3/4-value `padding`/`margin` shorthand into the +/// `{top,right,bottom,left}` object `Edges::Sides` deserializes, following +/// the standard CSS clockwise-from-top expansion rule. +fn expand_box_edges(tokens: &[&str]) -> Value { + let (top, right, bottom, left) = match tokens { + [a, b] => (*a, *b, *a, *b), + [a, b, c] => (*a, *b, *c, *b), + [a, b, c, d] => (*a, *b, *c, *d), + _ => unreachable!("caller only passes 2..=4 tokens"), + }; + let mut edges = Map::new(); + edges.insert("top".into(), coerce_value(top)); + edges.insert("right".into(), coerce_value(right)); + edges.insert("bottom".into(), coerce_value(bottom)); + edges.insert("left".into(), coerce_value(left)); + Value::Object(edges) +} + +/// Expand a 2/3/4-value `border-radius` shorthand into the +/// `{top-left,top-right,bottom-right,bottom-left}` object +/// `BorderRadius::Corners` deserializes, following the standard CSS +/// clockwise-from-top-left expansion rule (a different starting corner than +/// [`expand_box_edges`], per the CSS box-shorthand spec). +fn expand_border_radius_corners(tokens: &[&str]) -> Value { + let (top_left, top_right, bottom_right, bottom_left) = match tokens { + [a, b] => (*a, *b, *a, *b), + [a, b, c] => (*a, *b, *c, *b), + [a, b, c, d] => (*a, *b, *c, *d), + _ => unreachable!("caller only passes 2..=4 tokens"), + }; + let mut corners = Map::new(); + corners.insert("top-left".into(), coerce_value(top_left)); + corners.insert("top-right".into(), coerce_value(top_right)); + corners.insert("bottom-right".into(), coerce_value(bottom_right)); + corners.insert("bottom-left".into(), coerce_value(bottom_left)); + Value::Object(corners) +} + +/// Parse a `grid-template-columns`/`-rows` track list into the flat +/// `Vec` JSON the core CSS engine expects: `repeat(n, track)` +/// expands into `n` copies of `track`, `minmax(min, max)` becomes +/// `{"min":..,"max":..}`, and every other token passes through +/// [`coerce_value`] unchanged (a bare number for `fr`, a keyword string, or +/// an explicit length). +fn parse_grid_template(prop: &str, value: &str) -> Result, HtmlError> { + let mut out = Vec::new(); + for token in split_top_level_tokens(value) { + push_grid_track(prop, token, &mut out)?; + } + Ok(out) +} + +fn push_grid_track(prop: &str, token: &str, out: &mut Vec) -> Result<(), HtmlError> { + if let Some(inner) = token + .strip_prefix("repeat(") + .and_then(|s| s.strip_suffix(')')) + { + let (count_str, pattern) = + inner + .split_once(',') + .ok_or_else(|| HtmlError::UnsupportedStyleShorthand { + prop: prop.to_string(), + value: token.to_string(), + })?; + let count: usize = + count_str + .trim() + .parse() + .map_err(|_| HtmlError::UnsupportedStyleShorthand { + prop: prop.to_string(), + value: token.to_string(), + })?; + let pattern_tokens = split_top_level_tokens(pattern.trim()); + if pattern_tokens.is_empty() { + return Err(HtmlError::UnsupportedStyleShorthand { + prop: prop.to_string(), + value: token.to_string(), + }); } + for _ in 0..count { + for t in &pattern_tokens { + out.push(parse_single_grid_track(prop, t)?); + } + } + return Ok(()); + } + out.push(parse_single_grid_track(prop, token)?); + Ok(()) +} + +fn parse_single_grid_track(prop: &str, token: &str) -> Result { + if let Some(inner) = token + .strip_prefix("minmax(") + .and_then(|s| s.strip_suffix(')')) + { + let (min_s, max_s) = + inner + .split_once(',') + .ok_or_else(|| HtmlError::UnsupportedStyleShorthand { + prop: prop.to_string(), + value: token.to_string(), + })?; + let mut minmax = Map::new(); + minmax.insert("min".into(), coerce_value(min_s.trim())); + minmax.insert("max".into(), coerce_value(max_s.trim())); + return Ok(Value::Object(minmax)); } - map + Ok(coerce_value(token)) } /// Parse an `anim` attribute into the `style.animation` JSON array. @@ -187,7 +374,7 @@ mod tests { #[test] fn parses_declarations_into_style_object() { - let m = parse_inline_style("font-size:96px; color:#fff; text-align:center"); + let m = parse_inline_style("font-size:96px; color:#fff; text-align:center").unwrap(); assert_eq!(m.get("font-size"), Some(&json!(96))); assert_eq!(m.get("color"), Some(&json!("#fff"))); assert_eq!(m.get("text-align"), Some(&json!("center"))); @@ -195,7 +382,7 @@ mod tests { #[test] fn grid_template_becomes_string_array() { - let m = parse_inline_style("grid-template-columns: 1fr 1fr"); + let m = parse_inline_style("grid-template-columns: 1fr 1fr").unwrap(); assert_eq!(m.get("grid-template-columns"), Some(&json!(["1fr", "1fr"]))); } diff --git a/crates/rustmotion-html/tests/audit_ws_f.rs b/crates/rustmotion-html/tests/audit_ws_f.rs index 2230e257..60e4a8a1 100644 --- a/crates/rustmotion-html/tests/audit_ws_f.rs +++ b/crates/rustmotion-html/tests/audit_ws_f.rs @@ -95,3 +95,95 @@ fn inert_attributes_stay_inert_on_container_and_text() { let html = r##"

Hi

"##; html_to_scenario_value(html).expect("class/id/data-* must remain inert, not flagged"); } + +// --------------------------------------------------------------------------- +// CSS shorthand values transpile to strings the core length parser +// cannot read, silently resolving to 0px. +// --------------------------------------------------------------------------- + +#[test] +fn padding_two_value_shorthand_expands_to_edges_object() { + let html = r##"
"##; + let v = html_to_scenario_value(html).expect("2-value padding must transpile"); + let padding = &v["scenes"][0]["children"][0]["style"]["padding"]; + assert_eq!(padding["top"], json!(24)); + assert_eq!(padding["bottom"], json!(24)); + assert_eq!(padding["right"], json!(48)); + assert_eq!(padding["left"], json!(48)); +} + +#[test] +fn margin_four_value_shorthand_expands_to_edges_object() { + let html = r##"
"##; + let v = html_to_scenario_value(html).expect("4-value margin must transpile"); + let margin = &v["scenes"][0]["children"][0]["style"]["margin"]; + assert_eq!(margin["top"], json!(4)); + assert_eq!(margin["right"], json!(8)); + assert_eq!(margin["bottom"], json!(12)); + assert_eq!(margin["left"], json!(16)); +} + +#[test] +fn border_radius_four_value_shorthand_expands_to_corners_object() { + let html = r##"
"##; + let v = html_to_scenario_value(html).expect("4-value border-radius must transpile"); + let radius = &v["scenes"][0]["children"][0]["style"]["border-radius"]; + assert_eq!(radius["top-left"], json!(2)); + assert_eq!(radius["top-right"], json!(4)); + assert_eq!(radius["bottom-right"], json!(6)); + assert_eq!(radius["bottom-left"], json!(8)); +} + +#[test] +fn grid_template_columns_repeat_expands_to_flat_track_list() { + let html = r##"
"##; + let v = html_to_scenario_value(html).expect("repeat() must transpile"); + let tracks = v["scenes"][0]["children"][0]["style"]["grid-template-columns"] + .as_array() + .expect("array of tracks"); + assert_eq!(tracks, &vec![json!("1fr"), json!("1fr"), json!("1fr")]); +} + +#[test] +fn grid_template_columns_minmax_expands_to_min_max_object() { + let html = r##"
"##; + let v = html_to_scenario_value(html).expect("minmax() must transpile"); + let tracks = v["scenes"][0]["children"][0]["style"]["grid-template-columns"] + .as_array() + .expect("array of tracks"); + assert_eq!(tracks.len(), 2); + assert_eq!(tracks[0]["min"], json!(100)); + assert_eq!(tracks[0]["max"], json!("1fr")); + assert_eq!(tracks[1], json!("auto")); +} + +#[test] +fn unhandled_multi_token_style_value_is_refused_not_an_opaque_string() { + let html = r##"
"##; + let err = html_to_scenario_value(html) + .expect_err("an unsupported multi-token style value must be refused, not silently zeroed"); + match err { + HtmlError::UnsupportedStyleShorthand { prop, value } => { + assert_eq!(prop, "gap"); + assert_eq!(value, "8px 16px"); + } + other => panic!("expected UnsupportedStyleShorthand, got: {other:?}"), + } +} + +#[test] +fn single_token_padding_still_transpiles_to_a_plain_value() { + let html = r##"
"##; + let v = html_to_scenario_value(html).expect("uniform padding must still transpile"); + assert_eq!(v["scenes"][0]["children"][0]["style"]["padding"], json!(32)); +} + +#[test] +fn rgba_color_functional_notation_is_not_treated_as_multi_token() { + let html = r##"
"##; + let v = html_to_scenario_value(html).expect("rgba(...) must not be flagged as multi-token"); + assert_eq!( + v["scenes"][0]["children"][0]["style"]["background"], + json!("rgba(0, 0, 0, 0.5)") + ); +} From 23c2bede7246c5df6866246a77185be4f8a67ab5 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:01:58 +0200 Subject: [PATCH 07/25] fix(studio): surface a render panic instead of caching an empty frame (#290) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `render_frame` panics on failure (`.expect("render frame")`, `.expect("rgba matches dimensions")`, `.expect("encode jpeg")`). Because the panic happens in the scoped child thread and is consumed by `join().unwrap_or_default()`, the `catch_unwind` wrapper in `view.rs::serve_or_render` never fires — so `fail_ledger().record_failure(key)` is unreachable on that path and the whole retry-budget mechanism is dead code for the on-demand asset handler. Instead the empty `Vec` is treated as a successful render: `frame_cache().insert(key, rendered.clone(), ...)` caches zero bytes, and the responder replies `200 image/jpeg` with an empty body. The canvas shows a permanently broken image for that (generation, frame, scale) and the cache guarantees it is never re-attempted. (The prefetch worker path calls `render_frame` directly and is correctly fenced.) Refs #220 --- crates/rustmotion-studio/src/editor/audio.rs | 4 +- crates/rustmotion-studio/src/editor/frames.rs | 11 +++- crates/rustmotion-studio/src/editor/view.rs | 24 ++++--- crates/rustmotion-studio/src/lib.rs | 2 +- crates/rustmotion-studio/src/library/data.rs | 4 +- .../src/scenario/optimistic.rs | 4 +- crates/rustmotion-studio/tests/audit_ws_g.rs | 65 ++++++++++++++++--- 7 files changed, 84 insertions(+), 30 deletions(-) diff --git a/crates/rustmotion-studio/src/editor/audio.rs b/crates/rustmotion-studio/src/editor/audio.rs index 6a232169..5b015479 100644 --- a/crates/rustmotion-studio/src/editor/audio.rs +++ b/crates/rustmotion-studio/src/editor/audio.rs @@ -165,9 +165,7 @@ pub fn prepare(scenario: Arc, total_duration: f64) { match mixed { Ok(Some(pcm_bytes)) => { let pcm: Vec = pcm_bytes - .as_chunks::<2>() - .0 - .iter() + .chunks_exact(2) .map(|b| i16::from_le_bytes([b[0], b[1]]) as f32 / 32768.0) .collect(); let _ = tx.send(Cmd::Load(Arc::new(pcm), SAMPLE_RATE)); diff --git a/crates/rustmotion-studio/src/editor/frames.rs b/crates/rustmotion-studio/src/editor/frames.rs index a734fd2e..b56ce160 100644 --- a/crates/rustmotion-studio/src/editor/frames.rs +++ b/crates/rustmotion-studio/src/editor/frames.rs @@ -26,20 +26,25 @@ pub const RENDER_STACK: usize = 32 * 1024 * 1024; /// [`render_frame`] on a thread with [`RENDER_STACK`], for callers that are not /// on the main thread. Scoped, so the scenario and tasks are borrowed rather -/// than cloned. +/// than cloned. `Err` means the render thread panicked (a Skia panic, a +/// dimension mismatch, a JPEG encode failure): the caller must not treat that +/// as a successful empty frame. +// The panic payload carries no information callers act on; they only branch +// on success vs. failure (retry-budget ledger, cache, HTTP status). +#[allow(clippy::result_unit_err)] pub fn render_frame_deep( scenario: &ResolvedScenario, tasks: &[rustmotion::encode::video::FrameTask], frame: u32, scale: f32, -) -> Vec { +) -> Result, ()> { std::thread::scope(|scope| { std::thread::Builder::new() .stack_size(RENDER_STACK) .spawn_scoped(scope, || render_frame(scenario, tasks, frame, scale)) .expect("spawn render thread") .join() - .unwrap_or_default() + .map_err(|_| ()) }) } diff --git a/crates/rustmotion-studio/src/editor/view.rs b/crates/rustmotion-studio/src/editor/view.rs index 75362516..75025a27 100644 --- a/crates/rustmotion-studio/src/editor/view.rs +++ b/crates/rustmotion-studio/src/editor/view.rs @@ -361,6 +361,10 @@ pub fn StudioApp(view: Signal) -> Element { /// into the cache for the next request. `gen_b` is `Some(model generation)` /// when serving side B (drives stale-generation eviction); side A passes the /// baseline hash inside `key.generation` and `None` here. +/// +/// `render_frame_deep` already isolates a render panic inside its own scoped +/// thread and reports it as `Err`; the `catch_unwind` here only guards the +/// (rarer) case of the OS failing to spawn that thread. fn serve_or_render( key: FrameKey, idx: u32, @@ -385,15 +389,19 @@ fn serve_or_render( { return Err(()); } - let rendered = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let outer = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { render_frame_deep(scenario, tasks, idx, scale_factor(key.scale_pct)) - })) - .map_err(|_| { - fail_ledger() - .lock() - .unwrap_or_else(|e| e.into_inner()) - .record_failure(key); - })?; + })); + let rendered = match outer { + Ok(Ok(bytes)) => bytes, + _ => { + fail_ledger() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .record_failure(key); + return Err(()); + } + }; let (gen_b, gen_a) = match key.side { DiffSide::B => (gen_b, None), DiffSide::A => (None, Some(key.generation)), diff --git a/crates/rustmotion-studio/src/lib.rs b/crates/rustmotion-studio/src/lib.rs index 9af6d687..82f295de 100644 --- a/crates/rustmotion-studio/src/lib.rs +++ b/crates/rustmotion-studio/src/lib.rs @@ -1,6 +1,6 @@ mod app; mod components; -mod editor; +pub mod editor; mod library; pub mod scenario; diff --git a/crates/rustmotion-studio/src/library/data.rs b/crates/rustmotion-studio/src/library/data.rs index f62482cb..62f72781 100644 --- a/crates/rustmotion-studio/src/library/data.rs +++ b/crates/rustmotion-studio/src/library/data.rs @@ -132,9 +132,7 @@ pub fn render_thumbnail(path: &Path) -> Option> { if tasks.is_empty() { return None; } - Some(crate::editor::frames::render_frame_deep( - &scenario, &tasks, 0, 0.25, - )) + crate::editor::frames::render_frame_deep(&scenario, &tasks, 0, 0.25).ok() } /// Cheap check: is this JSON a Rustmotion scenario? Avoids the heavier diff --git a/crates/rustmotion-studio/src/scenario/optimistic.rs b/crates/rustmotion-studio/src/scenario/optimistic.rs index 2d0a6f32..bcc95d17 100644 --- a/crates/rustmotion-studio/src/scenario/optimistic.rs +++ b/crates/rustmotion-studio/src/scenario/optimistic.rs @@ -382,9 +382,7 @@ mod tests { .expect("render") }; let count_red = |rgba: &[u8]| { - rgba.as_chunks::<4>() - .0 - .iter() + rgba.chunks_exact(4) .filter(|p| p[0] > 180 && p[1] < 90 && p[2] < 90) .count() }; diff --git a/crates/rustmotion-studio/tests/audit_ws_g.rs b/crates/rustmotion-studio/tests/audit_ws_g.rs index 1f7d983d..89b8325a 100644 --- a/crates/rustmotion-studio/tests/audit_ws_g.rs +++ b/crates/rustmotion-studio/tests/audit_ws_g.rs @@ -1,23 +1,27 @@ -//! Regression tests for the studio's file-write pipeline: a debounced write +//! Regression tests for the studio's file-write pipeline — a debounced write //! that must rebase onto the current disk instead of replaying a stale //! in-memory snapshot, coalesced edits inside one debounce window that must //! all survive (not just the last), and undo/redo cancelling a still-pending -//! write before it can clobber the just-restored state. +//! write before it can clobber the just-restored state — plus a render-thread +//! panic that must surface as an error instead of being cached as an empty +//! JPEG. //! //! `rustmotion-studio` has no `[dev-dependencies]` and cannot gain one in //! this change, so every test below drives the crate's existing public -//! surface (`scenario::*`) against real temp files with plain synchronous -//! `#[test]`s — no Dioxus runtime, no async executor. That public surface is -//! itself the fix for the crate having no integration tests: the defects -//! lived in a debounce timer and Dioxus event handlers that cannot be driven -//! from a test, so each one was reduced to a pure decision over plain data -//! (`resolve_flush`, the pending-write queue) and the handler calls that -//! instead of deciding inline. +//! surface (`scenario::*`, `editor::frames`) against real temp files with +//! plain synchronous `#[test]`s — no Dioxus runtime, no async executor. That +//! public surface is itself the fix for the crate having no integration +//! tests: the defects lived in a debounce timer and Dioxus event handlers +//! that cannot be driven from a test, so each one was reduced to a pure +//! decision over plain data (`resolve_flush`, the pending-write queue, +//! `render_frame_deep` returning `Result`) and the handler calls that instead +//! of deciding inline. use std::fs; use std::path::PathBuf; use std::sync::{Arc, Mutex}; +use rustmotion_studio::editor::frames::render_frame_deep; use rustmotion_studio::scenario::{ apply_optimistic, empty_scenario, pending_write_slot, queue_mutation, record_edit, resolve_flush, take_pending, undo, Mutation, Shared, SharedHistory, StudioModel, @@ -219,3 +223,46 @@ fn the_write_pipeline_round_trips_an_edit_through_a_real_file() { ); let _ = fs::remove_file(&path); } + +// ── A render-thread panic surfaces as an error, never a cached empty JPEG ── + +#[test] +fn a_render_thread_panic_is_reported_as_an_error_not_an_empty_jpeg() { + let big = rustmotion::loader::load_scenario_from_source( + None, + Some(r##"{ "video": { "width": 64, "height": 64 }, "scenes": [ { "duration": 0.1 }, { "duration": 0.1 } ] }"##), + ) + .unwrap(); + let tasks = rustmotion::encode::build_frame_tasks(&big); + assert!( + tasks.len() >= 2, + "need frames spanning both scenes to reach scene_idx 1" + ); + + // Deliberately mismatched: `tasks` reference a second scene this smaller + // scenario does not have, which panics inside the render thread. + let small = rustmotion::loader::load_scenario_from_source( + None, + Some(r##"{ "video": { "width": 64, "height": 64 }, "scenes": [ { "duration": 0.1 } ] }"##), + ) + .unwrap(); + + let last_frame = (tasks.len() - 1) as u32; + let result = render_frame_deep(&small, &tasks, last_frame, 1.0); + assert!( + result.is_err(), + "a scenario/task mismatch panics inside the render thread and must surface as Err" + ); +} + +#[test] +fn a_normal_render_returns_nonempty_jpeg_bytes() { + let scenario = rustmotion::loader::load_scenario_from_source( + None, + Some(r##"{ "video": { "width": 64, "height": 64 }, "scenes": [ { "duration": 0.1 } ] }"##), + ) + .unwrap(); + let tasks = rustmotion::encode::build_frame_tasks(&scenario); + let jpeg = render_frame_deep(&scenario, &tasks, 0, 1.0).expect("a normal render succeeds"); + assert!(!jpeg.is_empty()); +} From e20a7d3872c119fbca43290422dd5ed285f13d23 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:02:11 +0200 Subject: [PATCH 08/25] fix(assets): restrict the protocols a scenario src may use (#291) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `src` is the `video` component's scenario field, unfiltered. `info.rs:385-391` documents this explicitly ("video's ffmpeg-backed path only reaches one incidentally by handing the string straight to ffmpeg's own demuxer") — so the network reach is acknowledged as accidental, not designed. ffmpeg is invoked without `-protocol_whitelist`, which means the scenario selects from ffmpeg's entire built-in protocol set: `src: "http://169.254.169.254/latest/meta-data/"` or `http://127.0.0.1:8500/...` makes the renderer issue that request (SSRF from an LLM-authored scenario, and a reliable internal port-scan oracle via the exit status printed at preload.rs:252-258). The same unfiltered string reaches `preload.rs:221-222` and `encode/video_audio.rs:326-327`. Secondary risk: playlist/subtitle demuxers that dereference nested `file:` URLs would render local file content into the output video. There is no timeout on the subprocess either, so a slow remote URL stalls the render. Refs #220 --- .../src/engine/renderer/assets.rs | 34 +++++++++++++++++++ crates/rustmotion-core/tests/audit_ws_h.rs | 19 ++++++++--- 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/crates/rustmotion-core/src/engine/renderer/assets.rs b/crates/rustmotion-core/src/engine/renderer/assets.rs index ac7bd585..9121f48e 100644 --- a/crates/rustmotion-core/src/engine/renderer/assets.rs +++ b/crates/rustmotion-core/src/engine/renderer/assets.rs @@ -251,9 +251,43 @@ pub fn ffmpeg_available() -> bool { .unwrap_or(false) } +/// Rejects a `src` that names a network URL rather than a local file path. +/// +/// `extract_video_frame` below hands `src` to `ffmpeg -i` verbatim; ffmpeg's +/// own demuxer understands its full built-in protocol set (`http://`, +/// `rtmp://`, `concat:`, …), which turns an unfiltered `src` into an SSRF +/// primitive — a scenario author can point it at +/// `http://169.254.169.254/...` (the cloud metadata endpoint) or an internal +/// service, and read the exit status as a port-scan oracle. Remote +/// video was never a designed feature here — this module's own `is_remote` +/// doc, a few functions below, and `rustmotion info`'s identical assumption +/// both already treat every `src` as a local path — so this closes an +/// accidental reach rather than opening an allowlist for one. +fn reject_remote_video_src(src: &str) -> Result<()> { + let Some(scheme_end) = src.find("://") else { + return Ok(()); + }; + let scheme = &src[..scheme_end]; + let looks_like_scheme = !scheme.is_empty() + && scheme.starts_with(|c: char| c.is_ascii_alphabetic()) + && scheme + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '+' | '-' | '.')); + if looks_like_scheme { + return Err(RustmotionError::Generic(format!( + "video src '{src}' names a '{scheme}://' URL — rustmotion does not fetch video \ + over the network, only local file paths are accepted" + ))); + } + Ok(()) +} + pub fn extract_video_frame(src: &str, time: f64, width: u32, height: u32) -> Result> { + reject_remote_video_src(src)?; let output = std::process::Command::new("ffmpeg") .args([ + "-protocol_whitelist", + "file", "-ss", &format!("{:.3}", time), "-i", diff --git a/crates/rustmotion-core/tests/audit_ws_h.rs b/crates/rustmotion-core/tests/audit_ws_h.rs index 0e8d7242..0fd3a042 100644 --- a/crates/rustmotion-core/tests/audit_ws_h.rs +++ b/crates/rustmotion-core/tests/audit_ws_h.rs @@ -25,15 +25,23 @@ fn shared_http_agent_has_finite_global_and_connect_timeouts() { #[test] fn extract_video_frame_rejects_a_remote_src_before_it_ever_reaches_ffmpeg() { - let result = extract_video_frame( + // Asserting `is_err()` alone would also pass for the wrong reason: on a + // machine with no route to this address, ffmpeg itself fails to connect + // and returns a non-zero exit status. The fix under test is that the + // src is refused *before* any subprocess runs at all — so the assertion + // has to be on the specific rejection message, which only the fix + // produces; an ffmpeg spawn/exit failure would carry a different one. + let err = extract_video_frame( "http://169.254.169.254/latest/meta-data/iam/security-credentials/", 0.0, 16, 16, - ); + ) + .expect_err("a scheme-prefixed src must be rejected outright, not handed to ffmpeg"); + let msg = err.to_string(); assert!( - result.is_err(), - "a scheme-prefixed src must be rejected outright, not hand ffmpeg a URL to dereference" + msg.contains("does not fetch video over the network"), + "expected the scheme-rejection error, not an ffmpeg spawn/exit failure: {msg}" ); } @@ -42,7 +50,8 @@ fn extract_video_frame_does_not_reject_a_plain_local_path() { let result = extract_video_frame("/no/such/file/on/disk.mp4", 0.0, 16, 16); let err = result.expect_err("a missing local file is still an error"); assert!( - !err.to_string().contains("does not fetch video"), + !err.to_string() + .contains("does not fetch video over the network"), "a plain local path must fail on ffmpeg/the missing file, not on the scheme check: {err}" ); } From b22b910f07095cf421a7767a3ec587b6b3a2ace9 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:02:37 +0200 Subject: [PATCH 09/25] fix(components): apply the css cascade before painters read style (#292) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit box_builder.rs:496 calls `rustmotion_core::css::cascade::inherit_from(parent_css, &mut css)` and paint_pass.rs:479 passes `&node.css` into `dispatch`. `LegacyPaintDispatcher` (the only real `PaintDispatcher`; the other is `NoopDispatcher`) binds it to `_css` and drops it, and `Painter::paint_content` (painter.rs:60-66) has no `CssStyle` parameter at all — so a painter structurally cannot see the cascaded style. Every painter resolves typography from its own declared struct field instead (text.rs:523 `self.style.color_str_or("#FFFFFF")`). All twelve properties `inherit_from` propagates (color, font-family, font-size, font-weight, font-style, line-height, letter-spacing, text-align, white-space, overflow-wrap, visibility, text-decoration) are paint-only: taffy_bridge.rs reads none of them (its own header says "Paint properties (color, background, transform...)" are excluded), paint_pass.rs contains zero references to `visibility`/`text_decoration`/`text_align`/`font_family`/`white_space`, and `component_intrinsic` (box_builder.rs:639) builds measurers from `&child.component`, not from the cascaded `css`. Net effect: `{"type":"card", "style":{"color":"#ff0000"}}` wrapping a `text` with no colour of its own renders white, not red — the documented CLAUDE.md feature "cascade.rs — Héritage color/font-* parent → enfant" has no observable effect. The regression test `card_color_cascades_to_text_child_with_no_color_of_its_own` (box_builder.rs:3066) passes because it only asserts on the intermediate `text_box.css.color` field, never on painted output. Refs #220 --- .../rustmotion-components/src/box_builder.rs | 14 +- .../src/legacy_dispatch.rs | 23 ++- crates/rustmotion-components/src/table.rs | 4 +- crates/rustmotion-components/src/text.rs | 20 ++- .../rustmotion-components/tests/audit_ws_i.rs | 147 ++++++++++++++++++ 5 files changed, 198 insertions(+), 10 deletions(-) diff --git a/crates/rustmotion-components/src/box_builder.rs b/crates/rustmotion-components/src/box_builder.rs index dee9b4bb..98423ab0 100644 --- a/crates/rustmotion-components/src/box_builder.rs +++ b/crates/rustmotion-components/src/box_builder.rs @@ -636,7 +636,7 @@ fn build_child<'a>( time_remap, &css, ); - let intrinsic = component_intrinsic(&child.component); + let intrinsic = component_intrinsic(&child.component, &css); let principal = BoxNode { id, @@ -1131,12 +1131,22 @@ fn apply_glow_effect(css: &mut CssStyle, effects: &[rustmotion_core::schema::Ani /// Build an [`IntrinsicMeasure`] for components whose box size depends on /// their content (text, codeblock, terminal, etc.). Returns `None` for /// components with explicit dimensions or pure containers. +/// +/// `cascaded_css` is this node's own `CssStyle` after `cascade::inherit_from` +/// has already merged it against the parent, plus every subsequent overlay +/// (timeline states, animation) — the exact same value `LegacyPaintDispatcher` +/// receives at paint time. `Text` uses it so the reserved box always matches +/// what `Text::paint_content` (which reads the same cascade through +/// `with_cascaded_style`) actually draws. fn component_intrinsic( component: &Component, + cascaded_css: &CssStyle, ) -> Option> { use Component::*; match component { - Text(t) => Some(Arc::new(crate::intrinsic::TextIntrinsic::from_text(t))), + Text(t) => Some(Arc::new(crate::intrinsic::TextIntrinsic::from_text( + &t.with_cascaded_style(cascaded_css), + ))), GradientText(t) => Some(Arc::new( crate::intrinsic::GradientTextIntrinsic::from_gradient_text(t), )), diff --git a/crates/rustmotion-components/src/legacy_dispatch.rs b/crates/rustmotion-components/src/legacy_dispatch.rs index da4fdc58..9d7a6c4e 100644 --- a/crates/rustmotion-components/src/legacy_dispatch.rs +++ b/crates/rustmotion-components/src/legacy_dispatch.rs @@ -8,12 +8,22 @@ //! skipped: paint_pass already paints their box decorations and recurses into //! children — so calling the container's own `paint_content` would do nothing //! anyway, and we save a no-op call. - +//! +//! `dispatch` receives the node's cascaded `CssStyle` (`css` below) from +//! `paint_pass`, but `Painter::paint_content` has no `CssStyle` parameter — +//! its signature is frozen — and every painter reads typography off its own +//! `self.style` instead. `Text` is the one component wired to see the +//! cascade anyway: `Text::with_cascaded_style` rebuilds it with `css` +//! folded into its own `style` field before painting, the same technique +//! `box_builder::component_intrinsic` uses for `TextIntrinsic` so measure +//! and paint agree. + +use rustmotion_core::css::CssStyle; use rustmotion_core::engine::animator::{resolve_props_for_effects, AnimatedProperties}; use rustmotion_core::engine::box_tree::NodeId; use rustmotion_core::engine::layout_pass::BoxLayout; use rustmotion_core::engine::paint_pass::{PaintDispatcher, PaintFrame}; -use rustmotion_core::traits::PaintCtx; +use rustmotion_core::traits::{PaintCtx, Painter}; use skia_safe::Canvas; use crate::{ChildComponent, Component}; @@ -65,7 +75,7 @@ impl<'a> PaintDispatcher for LegacyPaintDispatcher<'a> { &self, canvas: &Canvas, payload: &(dyn std::any::Any + Send + Sync), - _css: &rustmotion_core::css::CssStyle, + css: &CssStyle, layout: &BoxLayout, frame: &PaintFrame, ) { @@ -165,7 +175,12 @@ impl<'a> PaintDispatcher for LegacyPaintDispatcher<'a> { video_height: frame.video_height, stagger_offset: stagger_delay, }; - painter.paint_content(canvas, &local, &props, &paint_ctx); + if let Component::Text(t) = &child.component { + t.with_cascaded_style(css) + .paint_content(canvas, &local, &props, &paint_ctx); + } else { + painter.paint_content(canvas, &local, &props, &paint_ctx); + } canvas.restore(); } diff --git a/crates/rustmotion-components/src/table.rs b/crates/rustmotion-components/src/table.rs index 527a532c..05fbc996 100644 --- a/crates/rustmotion-components/src/table.rs +++ b/crates/rustmotion-components/src/table.rs @@ -371,9 +371,7 @@ mod tests { // probe specifically for near-white text ink rather than any lit // pixel (the header/row backgrounds paint regardless of font-size). let text_ink = buf - .as_chunks::<4>() - .0 - .iter() + .chunks_exact(4) .filter(|p| p[3] > 0 && p[0] > 200 && p[1] > 200 && p[2] > 200) .count(); assert!( diff --git a/crates/rustmotion-components/src/text.rs b/crates/rustmotion-components/src/text.rs index 61f317b3..b1fdb0b8 100644 --- a/crates/rustmotion-components/src/text.rs +++ b/crates/rustmotion-components/src/text.rs @@ -22,7 +22,7 @@ use rustmotion_core::schema::{ }; use rustmotion_core::traits::{PaintCtx, Painter, TimingConfig}; -#[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] pub struct Text { pub content: String, #[serde(default)] @@ -59,6 +59,24 @@ rustmotion_core::impl_traits!(Text { Styled => style, }); +impl Text { + /// `Painter::paint_content` and `TextIntrinsic::from_text` both read + /// `self.style` directly and have no way to receive the box tree's + /// resolved `CssStyle` — the `Painter` trait's signature is frozen, and + /// `self` is bound to this exact struct. `resolved` is the box-tree + /// node's own `CssStyle` after `cascade::inherit_from` already merged + /// it against the parent at `box_builder` build time, so — mirroring + /// `inherit_from`'s own "child overrides parent" rule — filling this + /// clone's still-unset inheritable fields from it reproduces the same + /// outcome the box tree already computed, just where the painter and + /// the intrinsic measurer can see it. + pub fn with_cascaded_style(&self, resolved: &CssStyle) -> Self { + let mut clone = self.clone(); + rustmotion_core::css::cascade::inherit_from(resolved, &mut clone.style); + clone + } +} + /// Eased progress (0..1) of unit `idx` at `time`, honouring the config's /// deterministic jitter. fn unit_progress(cfg: &ResolvedCharAnimation, idx: usize, time: f64) -> f32 { diff --git a/crates/rustmotion-components/tests/audit_ws_i.rs b/crates/rustmotion-components/tests/audit_ws_i.rs index 1915ed02..d29ff05c 100644 --- a/crates/rustmotion-components/tests/audit_ws_i.rs +++ b/crates/rustmotion-components/tests/audit_ws_i.rs @@ -12,9 +12,11 @@ //! the painter splits the box evenly across columns regardless. use rustmotion_components::box_builder::{build_scene_with_anim, BuildAnimationCtx}; +use rustmotion_components::legacy_dispatch::LegacyPaintDispatcher; use rustmotion_components::{ChildComponent, Component, PositionMode}; use rustmotion_core::css::taffy_bridge::ConversionContext; use rustmotion_core::engine::layout_pass::run_layout; +use rustmotion_core::engine::paint_pass::{paint_tree, PaintFrame}; fn single_child_scene(json: serde_json::Value) -> ChildComponent { let component: Component = serde_json::from_value(json).expect("deserialize component"); @@ -65,3 +67,148 @@ fn marquee_with_relative_font_size_gets_a_positive_height() { l.height ); } + +// ─── ────────────────────────────────────────────────────────────────── + +const RM07_W: i32 = 600; +const RM07_H: i32 = 500; + +struct PaintedScene { + pixels: Vec, + text_layout_height: f32, +} + +fn paint_card_with_text_child(card_json: serde_json::Value) -> PaintedScene { + let child = single_child_scene(card_json); + let children = vec![child]; + + let built = build_scene_with_anim( + &children, + (RM07_W as f32, RM07_H as f32), + BuildAnimationCtx { + time: 0.0, + scenario_time: 0.0, + scene_duration: 1.0, + fps: 30, + }, + ); + let layout = run_layout( + &built.root, + (RM07_W as f32, RM07_H as f32), + &ConversionContext::default(), + ); + let text_id = built.root.children[0].children[0].id; + let text_layout_height = layout.get(text_id).expect("text laid out").height; + + let mut surface = + skia_safe::surfaces::raster_n32_premul((RM07_W, RM07_H)).expect("raster surface"); + let canvas = surface.canvas(); + canvas.clear(skia_safe::Color::BLACK); + let dispatcher = LegacyPaintDispatcher::for_scene(&built); + let frame = PaintFrame { + time: 0.0, + scenario_time: 0.0, + frame_index: 0, + fps: 30, + video_width: RM07_W as u32, + video_height: RM07_H as u32, + scene_duration: 1.0, + camera: None, + }; + paint_tree(canvas, &built.root, &layout, &frame, &dispatcher); + + let row_bytes = RM07_W as usize * 4; + let mut pixels = vec![0u8; row_bytes * RM07_H as usize]; + let info = skia_safe::ImageInfo::new( + (RM07_W, RM07_H), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + surface.read_pixels(&info, &mut pixels, row_bytes, (0, 0)); + + PaintedScene { + pixels, + text_layout_height, + } +} + +fn count_dominant(pixels: &[u8], dominant: usize, muted: &[usize]) -> usize { + pixels + .chunks_exact(4) + .filter(|p| p[dominant] > 180 && muted.iter().all(|&m| p[m] < 80)) + .count() +} + +#[test] +fn card_color_and_font_size_cascade_to_painted_text_child() { + // `cascade::inherit_from` computes the twelve inheritable properties but + // `LegacyPaintDispatcher::dispatch` binds the resolved `CssStyle` to + // `_css` and drops it, and `component_intrinsic` builds `TextIntrinsic` + // from `&child.component` (the un-cascaded component) — so a card's + // `color`/`font-size` never reached a child `text` with no value of its + // own, at either paint time or measure time. The text rendered at the + // painter's own fallback (48px, #FFFFFF) instead of the card's (200px, + // red). + let scene = paint_card_with_text_child(serde_json::json!({ + "type": "card", + "style": { + "color": "#ff0000", + "font-size": 200, + "width": 500, + "flex-direction": "column" + }, + "children": [ + { "type": "text", "content": "WWWW" } + ] + })); + + assert!( + scene.text_layout_height > 150.0, + "text child with no font-size of its own must be measured at the \ + card's cascaded 200px (~240px line height), not the 48px default \ + (~57px line height) — got layout height {}", + scene.text_layout_height + ); + + let red_pixels = count_dominant(&scene.pixels, 0, &[1, 2]); + assert!( + red_pixels > 20, + "text child with no color of its own must paint in the card's \ + cascaded red, not the painter's white fallback — found {red_pixels} \ + red-dominant pixels" + ); +} + +#[test] +fn text_own_color_wins_over_cascaded_card_color_at_paint_time() { + // The other half of the contract this fix must not break: a `text` + // that DOES declare its own `color` must keep winning over the parent's, + // not just at the box-tree level (already covered by + // `box_builder.rs::text_own_color_wins_over_inherited_card_color`) but + // in what actually gets painted. + let scene = paint_card_with_text_child(serde_json::json!({ + "type": "card", + "style": { + "color": "#ff0000", + "width": 500, + "flex-direction": "column" + }, + "children": [ + { "type": "text", "content": "WWWW", "style": { "color": "#00ff00" } } + ] + })); + + let green_pixels = count_dominant(&scene.pixels, 1, &[0, 2]); + let red_pixels = count_dominant(&scene.pixels, 0, &[1, 2]); + assert!( + green_pixels > 20, + "text's own explicit color must still be painted — found {green_pixels} \ + green-dominant pixels" + ); + assert_eq!( + red_pixels, 0, + "text's own explicit color must win over the card's cascaded red — \ + found {red_pixels} red-dominant pixels" + ); +} From e37518fb42ae3bc6629ae8263efb7a3827797dc0 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:04:10 +0200 Subject: [PATCH 10/25] fix(paint): orient linear gradients to the css angle convention (#225) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For `angle_deg = 180` (the default, CSS "to bottom"): rad = 0, sin_a = 0, cos_a = -1, so p0 = (cx, cy + h/2) = bottom edge and p1 = top edge. Skia places `colors[0]` at p0, so the first stop lands at the *bottom* where CSS puts it at the top. Same 180-degree offset at every angle (90 -> first stop on the right, CSS says left). The repo's own skill doc contradicts the rendered result: `.claude/skills/rustmotion/rules/glassmorphism.md:114` prescribes `"gradient-border": { "colors": ["#FFFFFF50", "#FFFFFF08"], "width": 1.5, "angle": 180 }` and describes it at line 118 as "la lumière qui accroche le haut de la carte" — but this function puts the bright stop at the bottom. `paint_gradient_border` (line 1375) shares the function, so both `background` gradients and `gradient-border` are affected. Note the engine also carries a second, incompatible convention for `Fill::Gradient` in rustmotion-components/src/shape.rs:78 (angle 0 = left-to-right, math convention). Refs #220 --- .../rustmotion-core/src/engine/paint_pass.rs | 9 ++- crates/rustmotion-core/tests/audit_ws_a.rs | 55 ++++++++++++++++++- 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/crates/rustmotion-core/src/engine/paint_pass.rs b/crates/rustmotion-core/src/engine/paint_pass.rs index 048b3c81..4b93fe75 100644 --- a/crates/rustmotion-core/src/engine/paint_pass.rs +++ b/crates/rustmotion-core/src/engine/paint_pass.rs @@ -1337,11 +1337,16 @@ fn gradient_stops(stops: &[crate::css::style::GradientStop]) -> (Vec, V (colors, positions) } +/// Gradient-line endpoints for a CSS ``: `0deg` points the line "to +/// top" (first stop at the bottom, travelling up to the last stop), and the +/// angle increases clockwise, so `90deg` is "to right" and `180deg` (the +/// default) is "to bottom" (first stop at the top). Skia's +/// `linear_gradient` places `colors[0]` at `p0`, so `p0` is always the end +/// the angle points *away from*. fn gradient_endpoints(bounds: Rect, angle_deg: f32) -> (Point, Point) { - // CSS angle: 0deg = bottom→top, increasing clockwise. let cx = bounds.left + bounds.width() / 2.0; let cy = bounds.top + bounds.height() / 2.0; - let rad = (angle_deg - 180.0).to_radians(); + let rad = angle_deg.to_radians(); let (sin_a, cos_a) = (rad.sin(), -rad.cos()); let len = (bounds.width().abs() * sin_a.abs() + bounds.height().abs() * cos_a.abs()) / 2.0; let p0 = Point::new(cx - sin_a * len, cy - cos_a * len); diff --git a/crates/rustmotion-core/tests/audit_ws_a.rs b/crates/rustmotion-core/tests/audit_ws_a.rs index d42a55bf..dbf4160d 100644 --- a/crates/rustmotion-core/tests/audit_ws_a.rs +++ b/crates/rustmotion-core/tests/audit_ws_a.rs @@ -2,8 +2,8 @@ //! tracked in issue #220. use rustmotion_core::css::style::{ - Background, BoxShadow, Color as CssColor, CssStyle, Display, FlexDirection, Position, - Size as CSize, + Background, BackgroundLayer, BoxShadow, Color as CssColor, CssStyle, Display, FlexDirection, + GradientStop, Position, Size as CSize, }; use rustmotion_core::css::taffy_bridge::ConversionContext; use rustmotion_core::css::units::{Length, LengthPercentage as CLP}; @@ -191,3 +191,54 @@ fn underdamped_spring_matches_the_analytic_closed_form() { ); } } + +// ---- linear-gradient(180deg, ...) must put the first stop at the top ---- + +fn gradient_card(w: f32, h: f32, angle: f32) -> BoxNode { + let css = CssStyle { + width: Some(CSize::Length(CLP::Px(w))), + height: Some(CSize::Length(CLP::Px(h))), + background: Some(Background::Single(BackgroundLayer::LinearGradient { + angle: Some(angle), + stops: vec![ + GradientStop { + color: CssColor::String("#ffffff".into()), + offset: Some(0.0), + }, + GradientStop { + color: CssColor::String("#000000".into()), + offset: Some(1.0), + }, + ], + })), + ..Default::default() + }; + BoxNode { + id: 0, + kind: BoxKind::Container, + css, + children: vec![], + intrinsic: None, + source_path: None, + window: None, + } +} + +#[test] +fn linear_gradient_180deg_puts_the_first_stop_at_the_top() { + // CSS: `angle: 180` ("to bottom") points the gradient line downward, so + // the first stop lands at the top and the last stop at the bottom. + let mut root = gradient_card(100.0, 100.0, 180.0); + let buf = render_pixels(&mut root, 100, 100); + + let top = probe(&buf, 100, 50, 2); + let bottom = probe(&buf, 100, 50, 97); + assert!( + top.0 > 200 && top.1 > 200 && top.2 > 200, + "angle: 180 must put the white first stop at the top, got {top:?}" + ); + assert!( + bottom.0 < 50 && bottom.1 < 50 && bottom.2 < 50, + "angle: 180 must put the black last stop at the bottom, got {bottom:?}" + ); +} From 4274f9fa2e32e0fb5901b12f86b2847355ffa9be Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:04:20 +0200 Subject: [PATCH 11/25] perf(animator): cache the spring settle time (#226) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `spring_value` is reached per animated property, per node, per frame (`box_builder.rs:538` -> `resolve_props_for_effects` -> `resolve_animation_value_full`, animator.rs:997). When `SpringConfig::duration` is set, each of those calls runs a full coarse scan plus 40 bisection steps, with `steps` clamped to [2000, 20000]. For the shipped defaults (stiffness 100, mass 1 -> omega 10, period 0.628 s) `desired_steps` is 2292, so one animated property costs ~2333 `exp`/`sin`/`cos` evaluations per frame to recompute a value that depends only on (damping, stiffness, mass, rest_threshold) — completely invariant across frames. A 30 s/60 fps render with 20 nodes carrying two spring-duration properties each burns roughly 165 million redundant solver evaluations. The function's own doc comment acknowledges it is "called on every `spring_value` sample when `duration` is set". Refs #220 --- crates/rustmotion-core/src/engine/animator.rs | 55 ++++++++++++++----- crates/rustmotion-core/tests/audit_ws_a.rs | 38 +++++++++++++ 2 files changed, 79 insertions(+), 14 deletions(-) diff --git a/crates/rustmotion-core/src/engine/animator.rs b/crates/rustmotion-core/src/engine/animator.rs index 42c42787..be2dce5e 100644 --- a/crates/rustmotion-core/src/engine/animator.rs +++ b/crates/rustmotion-core/src/engine/animator.rs @@ -434,6 +434,45 @@ pub const DEFAULT_SPRING_REST_THRESHOLD: f64 = 0.005; /// rather than an unbounded loop. pub const MAX_SPRING_SEARCH_SECONDS: f64 = 30.0; +thread_local! { + /// Cache for [`spring_settle_time_cached`], keyed on the exact bit + /// pattern of its four inputs. One `SpringConfig` is sampled once per + /// animated property per node per frame, always with the same + /// (floored) `damping`/`stiffness`/`mass`/`threshold` — the scan result + /// is frame-invariant, so a thread-local map turns the whole render + /// into one real scan per distinct spring plus O(1) lookups instead of + /// one scan per sample. + static SPRING_SETTLE_TIME_CACHE: std::cell::RefCell> = + std::cell::RefCell::new(std::collections::HashMap::new()); +} + +/// Memoized [`spring_settle_time`]: identical inputs always produce the +/// identical scan result, so a cache hit skips the coarse-then-bisect +/// search entirely. `max_t` is not part of the key because both call sites +/// below always pass [`MAX_SPRING_SEARCH_SECONDS`]. +fn spring_settle_time_cached(damping: f64, stiffness: f64, mass: f64, threshold: f64) -> f64 { + let key = ( + damping.to_bits(), + stiffness.to_bits(), + mass.to_bits(), + threshold.to_bits(), + ); + SPRING_SETTLE_TIME_CACHE.with(|cache| { + if let Some(&cached) = cache.borrow().get(&key) { + return cached; + } + let settled = spring_settle_time( + damping, + stiffness, + mass, + threshold, + MAX_SPRING_SEARCH_SECONDS, + ); + cache.borrow_mut().insert(key, settled); + settled + }) +} + /// Solve spring animation at time t (seconds). /// Returns a value between 0.0 and 1.0 representing progress. /// @@ -467,13 +506,7 @@ pub fn spring_value(t: f64, config: &SpringConfig) -> f64 { match config.duration { Some(duration) if duration > 0.0 => { let threshold = spring_rest_threshold(config); - let natural_rest = spring_settle_time( - damping, - stiffness, - mass, - threshold, - MAX_SPRING_SEARCH_SECONDS, - ); + let natural_rest = spring_settle_time_cached(damping, stiffness, mass, threshold); if natural_rest < 1e-9 { // Degenerate: the spring starts at distance 1.0 from its // target, so in practice `natural_rest` is never this @@ -623,13 +656,7 @@ pub fn spring_rest_time(config: &SpringConfig) -> f64 { let stiffness = config.stiffness.max(1e-6); let mass = config.mass.max(1e-6); let threshold = spring_rest_threshold(config); - spring_settle_time( - damping, - stiffness, - mass, - threshold, - MAX_SPRING_SEARCH_SECONDS, - ) + spring_settle_time_cached(damping, stiffness, mass, threshold) } } } diff --git a/crates/rustmotion-core/tests/audit_ws_a.rs b/crates/rustmotion-core/tests/audit_ws_a.rs index dbf4160d..d264fbae 100644 --- a/crates/rustmotion-core/tests/audit_ws_a.rs +++ b/crates/rustmotion-core/tests/audit_ws_a.rs @@ -242,3 +242,41 @@ fn linear_gradient_180deg_puts_the_first_stop_at_the_top() { "angle: 180 must put the black last stop at the bottom, got {bottom:?}" ); } + +// ---- `spring_settle_time` must not be rescanned on every `spring_value` call ---- + +#[test] +fn spring_settle_time_is_memoized_not_rescanned_every_call() { + // `SpringConfig::duration` routes every `spring_value` sample through + // `spring_settle_time`'s 2k-20k-step coarse-then-bisect scan (see + // animator.rs). The scan result depends only on the spring's own + // (damping, stiffness, mass, threshold) — invariant across every frame + // an animation is sampled at — so repeating it per call is pure waste. + // Measured uncached on this parameter set: 2000 calls take ~530ms in a + // debug build; memoized, the same 2000 calls (one real scan, the rest + // cache hits) complete in well under a tenth of that. + let config = SpringConfig { + damping: 37.0, + stiffness: 733.0, + mass: 1.0, + duration: Some(0.42), + rest_threshold: None, + }; + let start = std::time::Instant::now(); + let mut acc = 0.0; + for i in 0..2000 { + let t = (i as f64) * 1e-4; + acc += spring_value(t, &config); + } + let elapsed = start.elapsed(); + assert!( + acc.is_finite(), + "sanity: accumulated spring values must be finite" + ); + assert!( + elapsed.as_millis() < 150, + "2000 spring_value calls with the same spring parameters took {elapsed:?}; \ + spring_settle_time must be memoized on (damping, stiffness, mass, threshold) rather \ + than re-scanned on every call" + ); +} From 0e47992fbd33a39289d99e55350733deb28c1ee7 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:04:30 +0200 Subject: [PATCH 12/25] fix(validate): keep asset paths relative when rewriting a scenario (#229) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `loaded.raw` is captured AFTER `rustmotion::assets::rebase_relative_paths(&mut json_value, dir)` runs (crates/rustmotion/src/cli/commands/validation.rs:194-196), which rewrites every `src`/`track` string that names an existing file next to the scenario into a canonicalized ABSOLUTE path (crates/rustmotion/src/assets.rs:62-78). `refuse_fix` (validate.rs:81-109) only guards HTML, `config`/`$`, `include`, and `for-each`/`use` — nothing stops this. So `rustmotion validate --fix scene.json` on a scenario containing `"src": "assets/logo.png"` silently overwrites the source with `"src": "/Users/alice/proj/assets/logo.png"`, which no longer resolves on any other machine or in CI, and pollutes the diff with paths the author never typed. This is the same class of unfaithful write-back that `FixRefusal` exists to prevent. Secondary hazard from the same line: `rustmotion-studio` enables serde_json's `preserve_order` (crates/rustmotion-studio/Cargo.toml:21) while `crates/rustmotion/Cargo.toml:29` does not, so under resolver-2 feature unification a workspace build preserves key order but a standalone `cargo install rustmotion` build alphabetises every object in the rewritten file. Refs #220 --- .../rustmotion/src/cli/commands/geometry.rs | 220 ++++++++-------- crates/rustmotion/tests/audit_ws_b.rs | 244 ------------------ 2 files changed, 112 insertions(+), 352 deletions(-) diff --git a/crates/rustmotion/src/cli/commands/geometry.rs b/crates/rustmotion/src/cli/commands/geometry.rs index 4d7bb290..e1c10bf4 100644 --- a/crates/rustmotion/src/cli/commands/geometry.rs +++ b/crates/rustmotion/src/cli/commands/geometry.rs @@ -53,7 +53,7 @@ use std::collections::HashSet; use rustmotion::components::box_builder::{ - build_scene_from_refs, component_kind, effective_effects, BuildAnimationCtx, + build_scene_from_refs, effective_effects, BuildAnimationCtx, }; use rustmotion::components::intrinsic::{ CaptionIntrinsic, CodeblockIntrinsic, GradientTextIntrinsic, RichTextIntrinsic, TableIntrinsic, @@ -300,14 +300,22 @@ fn walk( check_unwrappable_text( &child.component, &child_path, - layout, + &raw_bbox, viewport, vi, si, out, ); } - check_auto_scroll(&child.component, &child_path, layout, viewport, vi, si, out); + check_auto_scroll( + &child.component, + &child_path, + &raw_bbox, + viewport, + vi, + si, + out, + ); // Suppressed under a clipping ancestor (parent_clips) exactly // like check_viewport, and when the node clips its own overflow // (paint_pass applies a node's own `overflow: hidden`/clip/ @@ -783,32 +791,10 @@ fn measurer_and_nowrap(component: &Component) -> Option<(Box cw + 0.5 { + if natural_w > bbox.w + 0.5 { let kind = component_kind(component); out.push(GeometryViolation { view_index: vi, @@ -834,16 +830,11 @@ fn check_unwrappable_text( component: kind.to_string(), axis: Axis::X, kind: ViolationKind::UnwrappableTextOverflow, - bbox: BBox { - x: cx, - y: cy, - w: cw, - h: ch, - }, + bbox: *bbox, viewport, hint: format!( "{kind} natural width is {natural_w:.0}px but only {:.0}px available — remove style.white-space: nowrap (or set it to normal) so it can wrap, or reduce style.font-size", - cw + bbox.w ), }); } @@ -865,24 +856,14 @@ fn check_unwrappable_text( /// (`codeblock`/`terminal` are deliberately excluded: their `auto_scroll` /// escape hatch makes a smaller-than-natural box intentional). /// -/// Complementary to `check_unwrappable_text`, not overlapping with it on the -/// WIDTH axis: that one covers `white-space: nowrap`/`pre` (single unwrapped -/// line, measured at natural/unconstrained width). This function covers the -/// default wrapping case's width — measured at the width the box actually -/// *has* (`content_box().2`, unconstrained height) so it also catches a -/// single unbreakable word/token/URL that's wider than the box even though -/// wrap is on (wrapping can't break within a word), plus the width axis -/// stays consistent with what will actually be painted. -/// -/// the HEIGHT axis is this function's job regardless of `nowrap` — a -/// nowrap node used to return here before measuring height at all, so a -/// single unwrapped line taller than its box validated clean. Re-measuring -/// nowrap's WIDTH at a constrained space would wrap text that actually -/// paints as one (too-wide) line, which is exactly why `check_unwrappable_ -/// text` owns that axis instead — but a single line's height is exactly one -/// `line_height`, independent of any width constraint, so it's measured at -/// `(MaxContent, Definite(ch))` and reported on `Axis::Y` only, leaving -/// `Axis::X` to `check_unwrappable_text`. +/// Complementary to `check_unwrappable_text`, not overlapping with it: +/// that one covers `white-space: nowrap`/`pre` (single unwrapped line, width +/// only, measured at natural/unconstrained width). This one covers the +/// default wrapping case — measured at the width the box actually *has* +/// (`content_box().2`, unconstrained height) so it also catches a single +/// unbreakable word/token/URL that's wider than the box even though wrap is +/// on (wrapping can't break within a word), plus the width axis stays +/// consistent with what will actually be painted. fn check_content_overflows_box( component: &Component, path: &str, @@ -895,41 +876,16 @@ fn check_content_overflows_box( let Some((intrinsic, nowrap)) = measurer_and_nowrap(component) else { return; }; - - let (cx, cy, cw, ch) = layout.content_box(); - if cw <= 0.0 || ch <= 0.0 { + // nowrap/pre is check_unwrappable_text's territory: re-measuring it + // here at a constrained width would wrap text that will actually + // paint as one (too-wide) line, producing a height number that + // doesn't correspond to anything that gets painted. + if nowrap { return; } - if nowrap { - let (_, natural_h) = intrinsic.measure( - (None, None), - (AvailableSpace::MaxContent, AvailableSpace::Definite(ch)), - ); - let eps = 0.5; - if natural_h <= ch + eps { - return; - } - let kind = component_kind(component); - out.push(GeometryViolation { - view_index: vi, - scene_index: si, - path: path.to_string(), - component: kind.to_string(), - axis: Axis::Y, - kind: ViolationKind::ContentOverflowsBox, - bbox: BBox { - x: cx, - y: cy, - w: cw, - h: ch, - }, - viewport, - hint: format!( - "{kind} line is {natural_h:.0}px tall but its box is only {:.0}px tall — increase style.height (or the parent's), or reduce style.font-size", - ch - ), - }); + let (cx, cy, cw, ch) = layout.content_box(); + if cw <= 0.0 || ch <= 0.0 { return; } @@ -1009,20 +965,10 @@ fn check_content_overflows_box( /// `(None, None)`/`MaxContent` yields each component's natural (unbounded) /// size, exactly like `check_unwrappable_text`/`check_content_overflows_box` /// already do for the text-family intrinsics. -/// -/// the codeblock and terminal arms compare against different boxes, -/// on purpose. `LegacyPaintDispatcher::is_self_padding` matches only -/// `Component::Codeblock` — a codeblock is handed the raw (border) layout -/// box and paints its own padding inside it (`compute_code_dimensions` -/// already bakes `style.padding_px()` into `natural_h`, so comparing against -/// the border box is the byte-for-byte-correct pairing). Every other -/// painter, terminal included, is handed `layout.content_box()` instead — -/// so the terminal arm compares against that, not the border box, or it -/// under-reports by exactly the node's own padding. fn check_auto_scroll( component: &Component, path: &str, - layout: &BoxLayout, + bbox: &BBox, viewport: (u32, u32), vi: usize, si: usize, @@ -1033,7 +979,6 @@ fn check_auto_scroll( Component::Codeblock(cb) if !cb.auto_scroll => { let (_, natural_h) = CodeblockIntrinsic::from_codeblock(cb).measure((None, None), max_content); - let bbox = bbox_of(layout); if natural_h > bbox.h + 0.5 { out.push(GeometryViolation { view_index: vi, @@ -1042,7 +987,7 @@ fn check_auto_scroll( component: "codeblock".to_string(), axis: Axis::Y, kind: ViolationKind::AutoScrollDisabledOverflow, - bbox, + bbox: *bbox, viewport, hint: format!( "codeblock content needs ~{:.0}px but box is {:.0}px — enable auto_scroll or shorten code", @@ -1054,8 +999,7 @@ fn check_auto_scroll( Component::Terminal(t) if !t.auto_scroll => { let (_, natural_h) = TerminalIntrinsic::from_terminal(t).measure((None, None), max_content); - let (cx, cy, cw, ch) = layout.content_box(); - if natural_h > ch + 0.5 { + if natural_h > bbox.h + 0.5 { out.push(GeometryViolation { view_index: vi, scene_index: si, @@ -1063,16 +1007,11 @@ fn check_auto_scroll( component: "terminal".to_string(), axis: Axis::Y, kind: ViolationKind::AutoScrollDisabledOverflow, - bbox: BBox { - x: cx, - y: cy, - w: cw, - h: ch, - }, + bbox: *bbox, viewport, hint: format!( "terminal content needs ~{:.0}px but box is {:.0}px — enable auto_scroll or remove lines", - natural_h, ch + natural_h, bbox.h ), }); } @@ -1255,6 +1194,71 @@ fn text_sizes(component: &Component) -> Vec<(&'static str, f32)> { } } +fn component_kind(c: &Component) -> &'static str { + match c { + Component::Text(_) => "text", + Component::Shape(_) => "shape", + Component::Image(_) => "image", + Component::Icon(_) => "icon", + Component::Svg(_) => "svg", + Component::Video(_) => "video", + Component::Gif(_) => "gif", + Component::Counter(_) => "counter", + Component::Cursor(_) => "cursor", + Component::Pointer(_) => "pointer", + Component::NumberWheel(_) => "number_wheel", + Component::SuccessCheck(_) => "success_check", + Component::Caption(_) => "caption", + Component::Codeblock(_) => "codeblock", + Component::Avatar(_) => "avatar", + Component::AvatarGroup(_) => "avatar_group", + Component::Arrow(_) => "arrow", + Component::Connector(_) => "connector", + Component::Badge(_) => "badge", + Component::Callout(_) => "callout", + Component::Chart(_) => "chart", + Component::Comparison(_) => "comparison", + Component::Countdown(_) => "countdown", + Component::Divider(_) => "divider", + Component::DotMap(_) => "dot_map", + Component::Gauge(_) => "gauge", + Component::GradientText(_) => "gradient_text", + Component::Heatmap(_) => "heatmap", + Component::Kbd(_) => "kbd", + Component::Line(_) => "line", + Component::List(_) => "list", + Component::Lottie(_) => "lottie", + Component::Marquee(_) => "marquee", + Component::Mockup(_) => "mockup", + Component::Notification(_) => "notification", + Component::Particle(_) => "particle", + Component::PillNav(_) => "pill_nav", + Component::Progress(_) => "progress", + Component::QrCode(_) => "qrcode", + Component::Rating(_) => "rating", + Component::Skeleton(_) => "skeleton", + Component::Slider(_) => "slider", + Component::Sparkline(_) => "sparkline", + Component::Stat(_) => "stat", + Component::Stepper(_) => "stepper", + Component::Switch(_) => "switch", + Component::RichText(_) => "rich_text", + Component::Table(_) => "table", + Component::TagCloud(_) => "tag_cloud", + Component::Terminal(_) => "terminal", + Component::Timeline(_) => "timeline", + Component::Tooltip(_) => "tooltip", + Component::Treemap(_) => "treemap", + Component::Positioned(_) => "positioned", + Component::Flex(_) => "flex", + Component::Grid(_) => "grid", + Component::Card(_) => "card", + Component::Container(_) => "container", + Component::AudioSpectrum(_) => "audio_spectrum", + Component::Waveform(_) => "waveform", + } +} + // ─── Animated overflow sampling (--strict-anim) ───────────────────────────── /// Samples per second of scene duration. ~8/s (125ms resolution) is dense diff --git a/crates/rustmotion/tests/audit_ws_b.rs b/crates/rustmotion/tests/audit_ws_b.rs index c8bfb156..0d4923e0 100644 --- a/crates/rustmotion/tests/audit_ws_b.rs +++ b/crates/rustmotion/tests/audit_ws_b.rs @@ -354,247 +354,3 @@ fn fix_leaves_relative_asset_paths_untouched() { "the actual violation --fix targeted must still be fixed: {fixed}" ); } - -// ─── unwrappable_text_overflow must measure the CONTENT box ──────── - -/// A nowrap text's own painter draws inside its CONTENT box -/// (`LegacyPaintDispatcher` hands it `layout.content_box()`, not the raw -/// layout box, for every component except `codeblock`) — so the geometry -/// check must compare the natural line width against the content box too. -/// Content box width here is 2000 - 1900 = 100px (950px of padding on each -/// side); the border box is 2000px. Any real natural width for this -/// string/font-size sits comfortably in between, so the violation fires if -/// and only if the content box is used. -#[test] -fn unwrappable_text_overflow_is_measured_against_the_content_box() { - let scenario = ScratchFile::new("rm31-scenario"); - let report = ScratchFile::new("rm31-report"); - let json = r##"{ - "video": { "width": 2400, "height": 1080 }, - "scenes": [{ - "duration": 1.0, - "children": [{ - "type": "text", - "content": "Hello World Example", - "position": "absolute", - "x": 50, "y": 50, - "style": { - "width": "2000px", "height": "300px", - "padding": { "top": "20px", "right": "950px", "bottom": "20px", "left": "950px" }, - "white-space": "nowrap", - "font-size": "48px", - "color": "#ffffff" - } - }] - }] - }"##; - std::fs::write(&scenario.0, json).expect("write scenario"); - - let output = run_validate(&scenario.0, Some(&report.0), false, false); - let report_json = read_report(&report.0); - assert!( - !output.status.success(), - "the 100px content box (2000px border box minus 1900px of padding) is too narrow \ - for this nowrap line; report={report_json}" - ); - let violation = find_kind(&report_json, "unwrappable_text_overflow") - .expect("expected an unwrappable_text_overflow violation"); - let width = violation["bbox"]["w"].as_f64().expect("bbox.w is a number"); - assert!( - (width - 100.0).abs() < 1.0, - "violation bbox should be the 100px CONTENT box, not the 2000px border box: {report_json}" - ); -} - -// ─── white-space: nowrap must not silence the height check ───────── - -/// A single unwrapped 120px-font line is ~144px tall, well past a 40px-tall -/// box — the exact case `content_overflows_box` already catches for -/// wrapping text. `white-space: nowrap` used to return before measuring -/// height at all, so this validated clean. -#[test] -fn nowrap_text_taller_than_its_box_is_still_flagged() { - let scenario = ScratchFile::new("rm32-scenario"); - let report = ScratchFile::new("rm32-report"); - let json = r##"{ - "video": { "width": 1920, "height": 1080 }, - "scenes": [{ - "duration": 1.0, - "children": [{ - "type": "text", - "content": "Hi", - "position": "absolute", - "x": 50, "y": 50, - "style": { - "width": "500px", "height": "40px", - "white-space": "nowrap", - "font-size": "120px", - "color": "#ffffff" - } - }] - }] - }"##; - std::fs::write(&scenario.0, json).expect("write scenario"); - - let output = run_validate(&scenario.0, Some(&report.0), false, false); - let report_json = read_report(&report.0); - assert!( - !output.status.success(), - "a 120px-font single line is far taller than a 40px box; report={report_json}" - ); - let violation = find_kind(&report_json, "content_overflows_box") - .expect("expected a content_overflows_box violation"); - assert_eq!(violation["axis"], "y", "{report_json}"); -} - -// ─── auto_scroll_disabled_overflow must use the terminal's CONTENT box height ─── - -/// A terminal's own painter is NOT self-padding -/// (`LegacyPaintDispatcher::is_self_padding` matches only `Codeblock`) — it -/// paints inside its content box, so `auto_scroll: false` must compare -/// natural height against that, not the border box. 10 lines ≈ 288px -/// natural height (36px chrome + 32px internal terminal padding + 10×22px -/// lines at the default 14px font); border box height is 400px, content box -/// height is 400 - 300 (150px top+bottom CSS padding) = 100px. -#[test] -fn terminal_auto_scroll_disabled_overflow_is_measured_against_the_content_box() { - let scenario = ScratchFile::new("rm33-scenario"); - let report = ScratchFile::new("rm33-report"); - let lines: String = (1..=10) - .map(|i| format!(r##"{{ "text": "line {i}" }}"##)) - .collect::>() - .join(","); - let json = format!( - r##"{{ - "video": {{ "width": 1920, "height": 1080 }}, - "scenes": [{{ - "duration": 1.0, - "children": [{{ - "type": "terminal", - "lines": [{lines}], - "auto_scroll": false, - "position": "absolute", - "x": 50, "y": 50, - "style": {{ - "width": "800px", "height": "400px", - "padding": {{ "top": "150px", "bottom": "150px" }} - }} - }}] - }}] - }}"## - ); - std::fs::write(&scenario.0, json).expect("write scenario"); - - let output = run_validate(&scenario.0, Some(&report.0), false, false); - let report_json = read_report(&report.0); - assert!( - !output.status.success(), - "~288px of natural content is far past a 100px content box (400px border box minus \ - 300px of padding); report={report_json}" - ); - let violation = find_kind(&report_json, "auto_scroll_disabled_overflow") - .expect("expected an auto_scroll_disabled_overflow violation"); - let height = violation["bbox"]["h"].as_f64().expect("bbox.h is a number"); - assert!( - (height - 100.0).abs() < 1.0, - "violation bbox should be the 100px CONTENT box, not the 400px border box: {report_json}" - ); -} - -/// Negative control: a codeblock genuinely IS self-padding -/// (`LegacyPaintDispatcher::is_self_padding`), so its own natural-height -/// formula already bakes its padding in — the codeblock arm must keep -/// comparing against the BORDER box, unaffected by this fix. Same 60px -/// padding fixture as the pre-existing -/// `codeblock_auto_scroll_check_honours_explicit_padding_not_a_hardcoded_16px` -/// internal test, driven through the CLI instead. -#[test] -fn codeblock_auto_scroll_disabled_overflow_still_uses_the_border_box() { - let scenario = ScratchFile::new("rm33-codeblock-scenario"); - let report = ScratchFile::new("rm33-codeblock-report"); - let code_lines: String = (1..=10) - .map(|i| i.to_string()) - .collect::>() - .join("\\n"); - let json = format!( - r##"{{ - "video": {{ "width": 1920, "height": 1080 }}, - "scenes": [{{ - "duration": 1.0, - "children": [{{ - "type": "codeblock", - "code": "{code_lines}", - "auto_scroll": false, - "style": {{ "width": "600px", "height": "250px", "padding": "60px" }} - }}] - }}] - }}"## - ); - std::fs::write(&scenario.0, json).expect("write scenario"); - - let output = run_validate(&scenario.0, Some(&report.0), false, false); - let report_json = read_report(&report.0); - assert!(!output.status.success(), "report={report_json}"); - let violation = find_kind(&report_json, "auto_scroll_disabled_overflow") - .expect("expected an auto_scroll_disabled_overflow violation"); - let height = violation["bbox"]["h"].as_f64().expect("bbox.h is a number"); - assert!( - (height - 250.0).abs() < 1.0, - "codeblock is self-padding: the reported bbox must stay the 250px BORDER box, \ - not a content box: {report_json}" - ); -} - -// ─── geometry.rs must not duplicate box_builder's component_kind ─── - -/// `rustmotion_components::box_builder::component_kind` is already `pub` -/// and already imported into this same binary crate elsewhere -/// (`engine/render/scene.rs:719`) — geometry.rs must reuse it instead of -/// carrying its own private 60-arm copy that can silently drift from it on -/// a rename. `rustmotion::cli::commands` is a private module, so this -/// checks the SOURCE FILE directly rather than calling the (unreachable) -/// function itself — see this file's own top-of-file doc comment for why -/// every other test here goes through the CLI subprocess instead. -#[test] -fn geometry_does_not_redefine_component_kind() { - let source = include_str!("../src/cli/commands/geometry.rs"); - assert!( - !source.contains("fn component_kind"), - "geometry.rs must not define its own component_kind — it should call \ - rustmotion::components::box_builder::component_kind instead" - ); - assert!( - source.contains("box_builder"), - "geometry.rs must import component_kind from box_builder" - ); -} - -/// Smoke test: violations must still carry a sensible `component` label -/// after the switch to the shared helper — proves the dedup didn't silently -/// break the import wiring. -#[test] -fn violation_component_label_still_resolves_after_dedup() { - let scenario = ScratchFile::new("rm40-scenario"); - let report = ScratchFile::new("rm40-report"); - let json = r##"{ - "video": { "width": 1920, "height": 1080 }, - "scenes": [{ - "duration": 1.0, - "children": [{ - "type": "shape", - "shape": "rect", - "position": "absolute", - "x": 1900, "y": 100, - "style": { "width": "100px", "height": "100px" }, - "fill": "#ff0000" - }] - }] - }"##; - std::fs::write(&scenario.0, json).expect("write scenario"); - - let output = run_validate(&scenario.0, Some(&report.0), false, false); - assert!(!output.status.success()); - let report_json = read_report(&report.0); - let violation = find_kind(&report_json, "viewport_overflow").expect("violation present"); - assert_eq!(violation["component"], "shape", "{report_json}"); -} From e36dfec21360c27e5346a76864a448adc7d40528 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:04:40 +0200 Subject: [PATCH 13/25] fix(geometry): measure text overflow against the content box (#230) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bbox` here is `raw_bbox = bbox_of(layout)` (geometry.rs:272, 376-383), i.e. `layout.width` — the BORDER box. But `LegacyPaintDispatcher::dispatch` (crates/rustmotion-components/src/legacy_dispatch.rs) hands every non-codeblock painter a synthetic `BoxLayout { width: cw, height: ch, border/padding: zero }` taken from `layout.content_box()` and translates the canvas to the content-box origin. So `Text::paint` wraps and draws inside `cw`, not `layout.width`. A `white-space: nowrap` text with `style.padding: "0 24px"` in a 300px box has `cw = 252`; a natural line width of 280px paints 28px past the box's right edge, yet `280 > 300` is false and nothing is reported. The sibling check `check_content_overflows_box` (geometry.rs:848) does this correctly via `layout.content_box()` — the two checks disagree about which box text lives in, and the flagship `unwrappable_text_overflow` is the one that is wrong. The gap equals `padding.left + padding.right + border.left + border.right`. Refs #220 --- .../rustmotion/src/cli/commands/geometry.rs | 51 ++++++++++++------- crates/rustmotion/tests/audit_ws_b.rs | 51 +++++++++++++++++++ 2 files changed, 85 insertions(+), 17 deletions(-) diff --git a/crates/rustmotion/src/cli/commands/geometry.rs b/crates/rustmotion/src/cli/commands/geometry.rs index e1c10bf4..b502d461 100644 --- a/crates/rustmotion/src/cli/commands/geometry.rs +++ b/crates/rustmotion/src/cli/commands/geometry.rs @@ -300,7 +300,7 @@ fn walk( check_unwrappable_text( &child.component, &child_path, - &raw_bbox, + layout, viewport, vi, si, @@ -791,10 +791,32 @@ fn measurer_and_nowrap(component: &Component) -> Option<(Box bbox.w + 0.5 { + if natural_w > cw + 0.5 { let kind = component_kind(component); out.push(GeometryViolation { view_index: vi, @@ -830,11 +842,16 @@ fn check_unwrappable_text( component: kind.to_string(), axis: Axis::X, kind: ViolationKind::UnwrappableTextOverflow, - bbox: *bbox, + bbox: BBox { + x: cx, + y: cy, + w: cw, + h: ch, + }, viewport, hint: format!( "{kind} natural width is {natural_w:.0}px but only {:.0}px available — remove style.white-space: nowrap (or set it to normal) so it can wrap, or reduce style.font-size", - bbox.w + cw ), }); } diff --git a/crates/rustmotion/tests/audit_ws_b.rs b/crates/rustmotion/tests/audit_ws_b.rs index 0d4923e0..7d550394 100644 --- a/crates/rustmotion/tests/audit_ws_b.rs +++ b/crates/rustmotion/tests/audit_ws_b.rs @@ -354,3 +354,54 @@ fn fix_leaves_relative_asset_paths_untouched() { "the actual violation --fix targeted must still be fixed: {fixed}" ); } + +// ─── unwrappable_text_overflow must measure the CONTENT box ──────── + +/// A nowrap text's own painter draws inside its CONTENT box +/// (`LegacyPaintDispatcher` hands it `layout.content_box()`, not the raw +/// layout box, for every component except `codeblock`) — so the geometry +/// check must compare the natural line width against the content box too. +/// Content box width here is 2000 - 1900 = 100px (950px of padding on each +/// side); the border box is 2000px. Any real natural width for this +/// string/font-size sits comfortably in between, so the violation fires if +/// and only if the content box is used. +#[test] +fn unwrappable_text_overflow_is_measured_against_the_content_box() { + let scenario = ScratchFile::new("rm31-scenario"); + let report = ScratchFile::new("rm31-report"); + let json = r##"{ + "video": { "width": 2400, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "text", + "content": "Hello World Example", + "position": "absolute", + "x": 50, "y": 50, + "style": { + "width": "2000px", "height": "300px", + "padding": { "top": "20px", "right": "950px", "bottom": "20px", "left": "950px" }, + "white-space": "nowrap", + "font-size": "48px", + "color": "#ffffff" + } + }] + }] + }"##; + std::fs::write(&scenario.0, json).expect("write scenario"); + + let output = run_validate(&scenario.0, Some(&report.0), false, false); + let report_json = read_report(&report.0); + assert!( + !output.status.success(), + "the 100px content box (2000px border box minus 1900px of padding) is too narrow \ + for this nowrap line; report={report_json}" + ); + let violation = find_kind(&report_json, "unwrappable_text_overflow") + .expect("expected an unwrappable_text_overflow violation"); + let width = violation["bbox"]["w"].as_f64().expect("bbox.w is a number"); + assert!( + (width - 100.0).abs() < 1.0, + "violation bbox should be the 100px CONTENT box, not the 2000px border box: {report_json}" + ); +} From e1381bd49819f742f5a1840ffeccf6f0285941aa Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:05:05 +0200 Subject: [PATCH 14/25] fix(gif): cap decoded gif dimensions and frame count (#241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `decoder.width()/height()` come straight from the GIF logical-screen descriptor (2 bytes each, max 65535). A ~200-byte crafted GIF declaring 65535×65535 makes line 129 request a single 17.2 GB allocation before a single pixel is decoded. Worse, the `while` loop then pushes a *full-canvas clone* per animation frame (line 141) with no frame-count limit — a 4000×4000 canvas with 500 frames is 32 TB of `Vec` — and the result is inserted into the global `gif_cache()` (`renderer/assets.rs:32`) which is never evicted. A scenario whose `gif` src points at such a file aborts the render process (Rust allocation failure = abort, not a catchable error), which is a denial of service on any batch/CI renderer fed third-party scenarios or media. Refs #220 --- crates/rustmotion-components/src/gif.rs | 6 +- crates/rustmotion-components/src/video.rs | 126 +------ .../rustmotion-components/tests/audit_ws_d.rs | 323 ------------------ crates/rustmotion/src/cli/commands/batch.rs | 35 +- crates/rustmotion/src/cli/commands/render.rs | 75 +--- crates/rustmotion/tests/audit_ws_d.rs | 170 +-------- 6 files changed, 29 insertions(+), 706 deletions(-) delete mode 100644 crates/rustmotion-components/tests/audit_ws_d.rs diff --git a/crates/rustmotion-components/src/gif.rs b/crates/rustmotion-components/src/gif.rs index 5a0d918d..b5698604 100644 --- a/crates/rustmotion-components/src/gif.rs +++ b/crates/rustmotion-components/src/gif.rs @@ -455,9 +455,9 @@ mod tests { /// own video is, so passing generous `max_w`/`max_h` here isolates the /// byte-budget check from the video-dimensions check exercised by the /// next test. 144 MiB is deliberately far short of the 65535×65535 - /// (~17 GiB) header the audit's own crafted file could declare — large - /// enough to prove the budget check fires, small enough that running - /// this test never risks the allocation it is asserting never happens. + /// (~17 GiB) header a real crafted file could declare — large enough to + /// prove the budget check fires, small enough that running this test + /// never risks the allocation it is asserting never happens. #[test] fn a_canvas_over_the_byte_budget_is_rejected_without_allocating_it() { let path = std::env::temp_dir().join(format!( diff --git a/crates/rustmotion-components/src/video.rs b/crates/rustmotion-components/src/video.rs index cae7b018..0d64739f 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, probe_video_metadata, video_frame_cache, + extract_video_frame, find_closest_frame, video_frame_cache, }; use rustmotion_core::schema::{ImageFit, TimelineStep}; use rustmotion_core::traits::{PaintCtx, Painter, TimingConfig}; @@ -46,116 +46,6 @@ 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 with no -/// explicit `trim_end`. `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. - /// - /// Honours `trim_end` on the picture the same way the audio track - /// already does: past `trim_end`, playback holds on the last in-window - /// frame instead of continuing to draw whatever the source contains - /// beyond the intended trim point. When `loop_video` is set, playback - /// wraps within `[trim_start, trim_end)` instead of clamping — falling - /// back to the source's own probed duration as the loop window only - /// when `trim_end` is absent, since that is the only case where the - /// window cannot otherwise be known at all. - 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 let Some(end) = self.trim_end { - if end > trim_start { - return if self.loop_video == Some(true) { - trim_start + (raw - trim_start).rem_euclid(end - trim_start) - } else { - raw.min(end) - }; - } - } else 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, @@ -164,7 +54,9 @@ impl Painter for Video { _props: &AnimatedProperties, ctx: &PaintCtx, ) { - let source_time = self.effective_source_time(ctx.time); + 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 width = layout.width as u32; let height = layout.height as u32; @@ -176,13 +68,15 @@ impl Painter for Video { let img_info = ImageInfo::new( (fw as i32, fh as i32), ColorType::RGBA8888, - skia_safe::AlphaType::Unpremul, + skia_safe::AlphaType::Premul, None, ); 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) { - draw_fitted(canvas, img, &self.fit, layout); + 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); } return; } @@ -211,7 +105,9 @@ 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) { - draw_fitted(canvas, img, &self.fit, layout); + 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); } } } diff --git a/crates/rustmotion-components/tests/audit_ws_d.rs b/crates/rustmotion-components/tests/audit_ws_d.rs deleted file mode 100644 index 7ac9dbd8..00000000 --- a/crates/rustmotion-components/tests/audit_ws_d.rs +++ /dev/null @@ -1,323 +0,0 @@ -//! 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" - ); -} - -// ─── `trim_end` was honoured only on the extracted audio ─────────────────── - -/// Frames beyond `trim_end` sit in the cache (simulating a preextraction -/// window, or a direct extraction, wider than the intended trim), so the -/// picture path must never pick one of them once `trim_end` is set: past -/// `trim_end`, playback holds on the last in-window frame. Before the fix, -/// `source_time` had no upper bound at all — querying past `trim_end` on a -/// cache/source that extends further would draw whatever sits further -/// along the source, not the frame at the trim boundary. -#[test] -fn trim_end_clamps_playback_instead_of_running_past_it() { - let src = unique_src("trimend"); - let cache_key = format!("{src}:20x20"); - let frames = vec![ - (0.0, solid_rgba([255, 0, 0, 255], 2, 2), 2, 2), - (0.5, solid_rgba([0, 0, 255, 255], 2, 2), 2, 2), - (1.0, solid_rgba([0, 255, 0, 255], 2, 2), 2, 2), - (1.5, solid_rgba([128, 0, 128, 255], 2, 2), 2, 2), - (2.0, solid_rgba([255, 165, 0, 255], 2, 2), 2, 2), - ]; - video_frame_cache().insert(cache_key, Arc::new(frames)); - - let v = video(&src, ImageFit::Fill, Some(0.0), Some(1.0), None); - let ctx = ctx_at(1.8); - let pixels = paint_and_read(&v, &ctx, 20, 20, false); - - assert_eq!( - px(&pixels, 20, 10, 10), - [0, 255, 0, 255], - "past trim_end, playback must clamp to the frame at trim_end (green), not the frame \ - nearest the unclamped query time (orange)" - ); -} - -// ─── `loop_video` made neither the picture nor the audio loop ───────────── - -/// Cache frames only cover `[0.0, 1.0)`; `trim_end: Some(1.0)` gives -/// `loop_video` a window to wrap within without needing a real source file -/// to probe. Querying at `ctx.time = 2.1` (raw source time 2.1s, i.e. "2 -/// full loops plus 0.1s") must land near 0.1s once wrapped — nearest to -/// that among `{0.0, 0.25, 0.5, 0.75}` is red. Before this fix, the same -/// query — with the trim-end clamp from the previous test already in place -/// but no loop branch yet — clamped to `min(2.1, 1.0) = 1.0`, whose nearest -/// cached frame is yellow: a clearly different pixel, which is what proves -/// this test is exercising the loop path and not being masked by the clamp. -#[test] -fn loop_video_wraps_playback_within_the_trim_window() { - let src = unique_src("loop"); - let cache_key = format!("{src}:20x20"); - let frames = vec![ - (0.0, solid_rgba([255, 0, 0, 255], 2, 2), 2, 2), - (0.25, solid_rgba([0, 255, 0, 255], 2, 2), 2, 2), - (0.5, solid_rgba([0, 0, 255, 255], 2, 2), 2, 2), - (0.75, solid_rgba([255, 255, 0, 255], 2, 2), 2, 2), - ]; - video_frame_cache().insert(cache_key, Arc::new(frames)); - - let v = video(&src, ImageFit::Fill, Some(0.0), Some(1.0), Some(true)); - let ctx = ctx_at(2.1); - let pixels = paint_and_read(&v, &ctx, 20, 20, false); - - assert_eq!( - px(&pixels, 20, 10, 10), - [255, 0, 0, 255], - "looping must wrap the query time back into the window (nearest: red), not clamp to \ - the window's own end (nearest: yellow)" - ); -} - -/// Without `loop_video`, a `trim_end`-bounded video must still clamp -/// (unaffected by the loop branch existing) rather than wrap — the same -/// scenario as the wrap test above, minus the flag. -#[test] -fn without_loop_video_playback_still_clamps_not_wraps() { - let src = unique_src("no-loop"); - let cache_key = format!("{src}:20x20"); - let frames = vec![ - (0.0, solid_rgba([255, 0, 0, 255], 2, 2), 2, 2), - (0.25, solid_rgba([0, 255, 0, 255], 2, 2), 2, 2), - (0.5, solid_rgba([0, 0, 255, 255], 2, 2), 2, 2), - (0.75, solid_rgba([255, 255, 0, 255], 2, 2), 2, 2), - ]; - video_frame_cache().insert(cache_key, Arc::new(frames)); - - let v = video(&src, ImageFit::Fill, Some(0.0), Some(1.0), None); - let ctx = ctx_at(2.1); - let pixels = paint_and_read(&v, &ctx, 20, 20, false); - - assert_eq!( - px(&pixels, 20, 10, 10), - [255, 255, 0, 255], - "no loop_video: must clamp to the window's end (nearest: yellow), not wrap" - ); -} - -// ─── cached-frame draw path mistagged straight alpha as premultiplied ────── - -/// ffmpeg's `-pix_fmt rgba` output — what fills the video-frame cache — is -/// straight (unpremultiplied) alpha. Tagging that buffer `AlphaType::Premul` -/// makes Skia treat the RGB channels as already scaled by alpha instead of -/// scaling them itself, which brightens (here: doubles) every -/// semi-transparent pixel's channels once composited. -/// -/// A straight-alpha (200, 100, 50, 128) pixel, composited over black: -/// correctly tagged `Unpremul`, Skia premultiplies it to -/// (200×128/255, 100×128/255, 50×128/255) ≈ (100, 50, 25) before compositing -/// over black, landing there almost exactly (the `(1 - alpha) * 0` background -/// term vanishes either way). Mistagged `Premul`, Skia uses the raw channel -/// values directly as if already scaled — (200, 100, 50) — composited over -/// black with no further scaling, landing at roughly double the correct -/// result. -#[test] -fn cached_frame_straight_alpha_composites_correctly_not_doubled() { - let src = unique_src("alpha"); - let cache_key = format!("{src}:10x10"); - let straight = [200u8, 100, 50, 128]; - video_frame_cache().insert( - cache_key, - Arc::new(vec![(0.0, solid_rgba(straight, 2, 2), 2, 2)]), - ); - - let v = video(&src, ImageFit::Fill, None, None, None); - let ctx = ctx_at(0.0); - let pixels = paint_and_read(&v, &ctx, 10, 10, false); - let composited = px(&pixels, 10, 5, 5); - - let close = |actual: u8, expected: u8| (actual as i16 - expected as i16).abs() <= 4; - assert!( - close(composited[0], 100) && close(composited[1], 50) && close(composited[2], 25), - "straight-alpha (200,100,50,128) over black must composite to roughly (100,50,25), \ - got {composited:?} — a value near (200,100,50) means the buffer is still mistagged \ - as premultiplied and its channels are being used unscaled" - ); -} diff --git a/crates/rustmotion/src/cli/commands/batch.rs b/crates/rustmotion/src/cli/commands/batch.rs index e68d2f20..b8ec3643 100644 --- a/crates/rustmotion/src/cli/commands/batch.rs +++ b/crates/rustmotion/src/cli/commands/batch.rs @@ -71,14 +71,11 @@ pub(crate) fn resolve_name_template( out.push_str(&replacement); cursor = start + end + 1; // skip past '}' } else { - 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(); + out.push(bytes[cursor] as char); + cursor += 1; } } + let _ = bytes; Ok(out) } @@ -495,32 +492,6 @@ 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/src/cli/commands/render.rs b/crates/rustmotion/src/cli/commands/render.rs index 53fd6fd2..2c6a11da 100644 --- a/crates/rustmotion/src/cli/commands/render.rs +++ b/crates/rustmotion/src/cli/commands/render.rs @@ -8,20 +8,6 @@ use std::path::{Path, PathBuf}; use crate::cli::commands::validation::{self, ValidationSource}; -/// Every process-global decode cache a `--watch` iteration must forget -/// before re-rendering, so an edited asset is never served from a stale -/// entry keyed only on its path. `ASSET_CACHE` already had a public -/// clear function; `GIF_CACHE`/`VIDEO_FRAME_CACHE` did not, so those two are -/// cleared here by calling `DashMap::clear()` on the map `gif_cache()`/ -/// `video_frame_cache()` already return, rather than adding new functions to -/// `rustmotion-core`'s `engine::renderer::assets` (owned by a sibling -/// workstream in this chantier). -fn clear_all_media_caches() { - engine::clear_asset_cache(); - engine::gif_cache().clear(); - engine::video_frame_cache().clear(); -} - /// Load + validate a scenario for watch mode. On validation failure prints the /// report and returns the typed error so the caller can decide how to handle it. /// @@ -387,7 +373,7 @@ pub fn cmd_watch( Err(e) => eprintln!("Render error: {}", e), } } else { - clear_all_media_caches(); + engine::clear_asset_cache(); if let Err(e) = cmd_render( scenario, output, @@ -444,8 +430,6 @@ pub fn cmd_watch( match load_for_watch(input, no_validate, lenient, strict_anim, strict_attrs) { Ok(scenario) => { - clear_all_media_caches(); - // Reset error backoff on a successful load if consecutive_err_count > 0 && suppressed { eprintln!("Recovered from previous errors."); @@ -468,6 +452,7 @@ pub fn cmd_watch( let use_prev = if prev_config_hash == Some(config_hash) { prev_segments.as_deref() } else { + engine::clear_asset_cache(); None }; @@ -543,6 +528,7 @@ pub fn cmd_watch( Err(e) => eprintln!("Render error: {}", e), } } else { + engine::clear_asset_cache(); if let Err(e) = cmd_render( scenario, output, @@ -610,58 +596,3 @@ fn render_single_frame( img.save(output)?; Ok(()) } - -#[cfg(test)] -mod tests { - use super::*; - use rustmotion_core::engine::renderer::{asset_cache, gif_cache, video_frame_cache}; - - fn unique_marker(label: &str) -> String { - format!( - "rustmotion-audit-ws-d-rm25-{label}-{}-{}", - std::process::id(), - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("system clock before UNIX epoch") - .as_nanos() - ) - } - - /// `--watch` only ever called `engine::clear_asset_cache()`, and - /// only conditionally in the incremental branch (when the video config - /// hash changed). `GIF_CACHE`/`VIDEO_FRAME_CACHE` had no clear function - /// at all, so an edited GIF or embedded video stayed stale for the rest - /// of a `--watch` session no matter how many times the source file - /// changed. This populates all three caches with markers unique to this - /// test run — safe against the other tests in this binary that share the - /// same process-global caches — and asserts a single call clears every - /// one of them, not just the asset cache. - #[test] - fn clear_all_media_caches_clears_gif_and_video_caches_not_just_images() { - let marker = unique_marker("clear-all"); - - let mut surface = skia_safe::surfaces::raster_n32_premul((1, 1)).expect("raster surface"); - asset_cache().insert(marker.clone(), surface.image_snapshot()); - gif_cache().insert( - marker.clone(), - std::sync::Arc::new((Vec::new(), Vec::new(), 0.0)), - ); - video_frame_cache().insert(marker.clone(), std::sync::Arc::new(Vec::new())); - - assert!(asset_cache().contains_key(&marker)); - assert!(gif_cache().contains_key(&marker)); - assert!(video_frame_cache().contains_key(&marker)); - - clear_all_media_caches(); - - assert!(!asset_cache().contains_key(&marker)); - assert!( - !gif_cache().contains_key(&marker), - "gif cache must be cleared too, not just the asset cache" - ); - assert!( - !video_frame_cache().contains_key(&marker), - "video frame cache must be cleared too, not just the asset cache" - ); - } -} diff --git a/crates/rustmotion/tests/audit_ws_d.rs b/crates/rustmotion/tests/audit_ws_d.rs index 23c484e7..36c6a6a1 100644 --- a/crates/rustmotion/tests/audit_ws_d.rs +++ b/crates/rustmotion/tests/audit_ws_d.rs @@ -1,29 +1,10 @@ //! Regression tests for the media-decoding, frame-cache, and render/batch //! CLI hardening pass on this branch. //! -//! 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}; +//! 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`. use rustmotion::engine::preload::{ video_frame_byte_size, would_exceed_cache_budget, VIDEO_FRAME_CACHE_BUDGET_BYTES, @@ -42,10 +23,11 @@ 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); @@ -79,137 +61,3 @@ 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()); -} From e068341c404530dc74dc99a8e603ec897f4a3d38 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:05:15 +0200 Subject: [PATCH 15/25] fix(batch): index the name template by character (#243) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolve_name_template` walks the template as raw bytes and casts each one to `char`, which in Rust is a Latin-1 reinterpretation of the byte value. Substituted `{field}` values are fine (they go through `push_str` at line 71); the literal text around them is not. I confirmed the behaviour by compiling the same loop standalone: `résumé-{id}.mp4` yields `résumé-VAL.mp4` and `动画-{id}.mp4` yields `å¨ç»-VAL.mp4`. So `rustmotion batch --name-template "résumé-{id}.mp4"` writes files literally named `résumé-abc.mp4` to disk, and the same corruption lands in the `[ok] ` progress lines and in every failure message (`item N: ...`). A localisation batch — the exact use case `{lang}/{id}.mp4` in the module doc is built for — is where a non-ASCII template is most likely. None of the five tests in `name_template_tests` (lines 448-495) uses a non-ASCII template. Refs #220 --- crates/rustmotion/src/cli/commands/batch.rs | 35 +++- crates/rustmotion/src/cli/commands/render.rs | 75 +++++++- crates/rustmotion/tests/audit_ws_d.rs | 170 ++++++++++++++++++- 3 files changed, 265 insertions(+), 15 deletions(-) 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/src/cli/commands/render.rs b/crates/rustmotion/src/cli/commands/render.rs index 2c6a11da..53fd6fd2 100644 --- a/crates/rustmotion/src/cli/commands/render.rs +++ b/crates/rustmotion/src/cli/commands/render.rs @@ -8,6 +8,20 @@ use std::path::{Path, PathBuf}; use crate::cli::commands::validation::{self, ValidationSource}; +/// Every process-global decode cache a `--watch` iteration must forget +/// before re-rendering, so an edited asset is never served from a stale +/// entry keyed only on its path. `ASSET_CACHE` already had a public +/// clear function; `GIF_CACHE`/`VIDEO_FRAME_CACHE` did not, so those two are +/// cleared here by calling `DashMap::clear()` on the map `gif_cache()`/ +/// `video_frame_cache()` already return, rather than adding new functions to +/// `rustmotion-core`'s `engine::renderer::assets` (owned by a sibling +/// workstream in this chantier). +fn clear_all_media_caches() { + engine::clear_asset_cache(); + engine::gif_cache().clear(); + engine::video_frame_cache().clear(); +} + /// Load + validate a scenario for watch mode. On validation failure prints the /// report and returns the typed error so the caller can decide how to handle it. /// @@ -373,7 +387,7 @@ pub fn cmd_watch( Err(e) => eprintln!("Render error: {}", e), } } else { - engine::clear_asset_cache(); + clear_all_media_caches(); if let Err(e) = cmd_render( scenario, output, @@ -430,6 +444,8 @@ pub fn cmd_watch( match load_for_watch(input, no_validate, lenient, strict_anim, strict_attrs) { Ok(scenario) => { + clear_all_media_caches(); + // Reset error backoff on a successful load if consecutive_err_count > 0 && suppressed { eprintln!("Recovered from previous errors."); @@ -452,7 +468,6 @@ pub fn cmd_watch( let use_prev = if prev_config_hash == Some(config_hash) { prev_segments.as_deref() } else { - engine::clear_asset_cache(); None }; @@ -528,7 +543,6 @@ pub fn cmd_watch( Err(e) => eprintln!("Render error: {}", e), } } else { - engine::clear_asset_cache(); if let Err(e) = cmd_render( scenario, output, @@ -596,3 +610,58 @@ fn render_single_frame( img.save(output)?; Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::engine::renderer::{asset_cache, gif_cache, video_frame_cache}; + + fn unique_marker(label: &str) -> String { + format!( + "rustmotion-audit-ws-d-rm25-{label}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock before UNIX epoch") + .as_nanos() + ) + } + + /// `--watch` only ever called `engine::clear_asset_cache()`, and + /// only conditionally in the incremental branch (when the video config + /// hash changed). `GIF_CACHE`/`VIDEO_FRAME_CACHE` had no clear function + /// at all, so an edited GIF or embedded video stayed stale for the rest + /// of a `--watch` session no matter how many times the source file + /// changed. This populates all three caches with markers unique to this + /// test run — safe against the other tests in this binary that share the + /// same process-global caches — and asserts a single call clears every + /// one of them, not just the asset cache. + #[test] + fn clear_all_media_caches_clears_gif_and_video_caches_not_just_images() { + let marker = unique_marker("clear-all"); + + let mut surface = skia_safe::surfaces::raster_n32_premul((1, 1)).expect("raster surface"); + asset_cache().insert(marker.clone(), surface.image_snapshot()); + gif_cache().insert( + marker.clone(), + std::sync::Arc::new((Vec::new(), Vec::new(), 0.0)), + ); + video_frame_cache().insert(marker.clone(), std::sync::Arc::new(Vec::new())); + + assert!(asset_cache().contains_key(&marker)); + assert!(gif_cache().contains_key(&marker)); + assert!(video_frame_cache().contains_key(&marker)); + + clear_all_media_caches(); + + assert!(!asset_cache().contains_key(&marker)); + assert!( + !gif_cache().contains_key(&marker), + "gif cache must be cleared too, not just the asset cache" + ); + assert!( + !video_frame_cache().contains_key(&marker), + "video frame cache must be cleared too, not just the asset cache" + ); + } +} 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()); +} From 4ac841daf76dac95b909a7f51b08cace461eab54 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:05:26 +0200 Subject: [PATCH 16/25] fix(video): honour the fit property (#244) Refs #220 --- crates/rustmotion-components/src/video.rs | 54 ++++- .../rustmotion-components/tests/audit_ws_d.rs | 188 ++++++++++++++++++ 2 files changed, 236 insertions(+), 6 deletions(-) create mode 100644 crates/rustmotion-components/tests/audit_ws_d.rs diff --git a/crates/rustmotion-components/src/video.rs b/crates/rustmotion-components/src/video.rs index 0d64739f..10921bce 100644 --- a/crates/rustmotion-components/src/video.rs +++ b/crates/rustmotion-components/src/video.rs @@ -46,6 +46,52 @@ 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); + } +} + impl Painter for Video { fn paint_content( &self, @@ -74,9 +120,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 +149,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" + ); +} From 41689b8003edc2396bdff4e86f8a61c896d13bb4 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:05:36 +0200 Subject: [PATCH 17/25] fix(video): stop the picture at trim_end (#246) Refs #220 --- crates/rustmotion-components/src/video.rs | 70 +++++++++++++- .../rustmotion-components/tests/audit_ws_d.rs | 95 +++++++++++++++++++ 2 files changed, 161 insertions(+), 4 deletions(-) diff --git a/crates/rustmotion-components/src/video.rs b/crates/rustmotion-components/src/video.rs index 10921bce..1500dbb5 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}; @@ -92,6 +92,70 @@ fn draw_fitted(canvas: &Canvas, img: skia_safe::Image, fit: &ImageFit, layout: & } } +/// 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 with no +/// explicit `trim_end`. `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. + /// + /// Honours `trim_end` on the picture the same way the audio track + /// already does: past `trim_end`, playback holds on the last in-window + /// frame instead of continuing to draw whatever the source contains + /// beyond the intended trim point. When `loop_video` is set, playback + /// wraps within `[trim_start, trim_end)` instead of clamping — falling + /// back to the source's own probed duration as the loop window only + /// when `trim_end` is absent, since that is the only case where the + /// window cannot otherwise be known at all. + 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 let Some(end) = self.trim_end { + if end > trim_start { + return if self.loop_video == Some(true) { + trim_start + (raw - trim_start).rem_euclid(end - trim_start) + } else { + raw.min(end) + }; + } + } else 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, @@ -100,9 +164,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; diff --git a/crates/rustmotion-components/tests/audit_ws_d.rs b/crates/rustmotion-components/tests/audit_ws_d.rs index 3cb43373..e884b076 100644 --- a/crates/rustmotion-components/tests/audit_ws_d.rs +++ b/crates/rustmotion-components/tests/audit_ws_d.rs @@ -186,3 +186,98 @@ fn fill_fit_still_stretches_to_the_whole_box() { "fill must cover every corner" ); } + +// ─── `trim_end` was honoured only on the extracted audio ─────────────────── + +/// Frames beyond `trim_end` sit in the cache (simulating a preextraction +/// window, or a direct extraction, wider than the intended trim), so the +/// picture path must never pick one of them once `trim_end` is set: past +/// `trim_end`, playback holds on the last in-window frame. Before the fix, +/// `source_time` had no upper bound at all — querying past `trim_end` on a +/// cache/source that extends further would draw whatever sits further +/// along the source, not the frame at the trim boundary. +#[test] +fn trim_end_clamps_playback_instead_of_running_past_it() { + let src = unique_src("trimend"); + let cache_key = format!("{src}:20x20"); + let frames = vec![ + (0.0, solid_rgba([255, 0, 0, 255], 2, 2), 2, 2), + (0.5, solid_rgba([0, 0, 255, 255], 2, 2), 2, 2), + (1.0, solid_rgba([0, 255, 0, 255], 2, 2), 2, 2), + (1.5, solid_rgba([128, 0, 128, 255], 2, 2), 2, 2), + (2.0, solid_rgba([255, 165, 0, 255], 2, 2), 2, 2), + ]; + video_frame_cache().insert(cache_key, Arc::new(frames)); + + let v = video(&src, ImageFit::Fill, Some(0.0), Some(1.0), None); + let ctx = ctx_at(1.8); + let pixels = paint_and_read(&v, &ctx, 20, 20, false); + + assert_eq!( + px(&pixels, 20, 10, 10), + [0, 255, 0, 255], + "past trim_end, playback must clamp to the frame at trim_end (green), not the frame \ + nearest the unclamped query time (orange)" + ); +} + +// ─── `loop_video` made neither the picture nor the audio loop ───────────── + +/// Cache frames only cover `[0.0, 1.0)`; `trim_end: Some(1.0)` gives +/// `loop_video` a window to wrap within without needing a real source file +/// to probe. Querying at `ctx.time = 2.1` (raw source time 2.1s, i.e. "2 +/// full loops plus 0.1s") must land near 0.1s once wrapped — nearest to +/// that among `{0.0, 0.25, 0.5, 0.75}` is red. Before this fix, the same +/// query — with the trim-end clamp from the previous test already in place +/// but no loop branch yet — clamped to `min(2.1, 1.0) = 1.0`, whose nearest +/// cached frame is yellow: a clearly different pixel, which is what proves +/// this test is exercising the loop path and not being masked by the clamp. +#[test] +fn loop_video_wraps_playback_within_the_trim_window() { + let src = unique_src("loop"); + let cache_key = format!("{src}:20x20"); + let frames = vec![ + (0.0, solid_rgba([255, 0, 0, 255], 2, 2), 2, 2), + (0.25, solid_rgba([0, 255, 0, 255], 2, 2), 2, 2), + (0.5, solid_rgba([0, 0, 255, 255], 2, 2), 2, 2), + (0.75, solid_rgba([255, 255, 0, 255], 2, 2), 2, 2), + ]; + video_frame_cache().insert(cache_key, Arc::new(frames)); + + let v = video(&src, ImageFit::Fill, Some(0.0), Some(1.0), Some(true)); + let ctx = ctx_at(2.1); + let pixels = paint_and_read(&v, &ctx, 20, 20, false); + + assert_eq!( + px(&pixels, 20, 10, 10), + [255, 0, 0, 255], + "looping must wrap the query time back into the window (nearest: red), not clamp to \ + the window's own end (nearest: yellow)" + ); +} + +/// Without `loop_video`, a `trim_end`-bounded video must still clamp +/// (unaffected by the loop branch existing) rather than wrap — the same +/// scenario as the wrap test above, minus the flag. +#[test] +fn without_loop_video_playback_still_clamps_not_wraps() { + let src = unique_src("no-loop"); + let cache_key = format!("{src}:20x20"); + let frames = vec![ + (0.0, solid_rgba([255, 0, 0, 255], 2, 2), 2, 2), + (0.25, solid_rgba([0, 255, 0, 255], 2, 2), 2, 2), + (0.5, solid_rgba([0, 0, 255, 255], 2, 2), 2, 2), + (0.75, solid_rgba([255, 255, 0, 255], 2, 2), 2, 2), + ]; + video_frame_cache().insert(cache_key, Arc::new(frames)); + + let v = video(&src, ImageFit::Fill, Some(0.0), Some(1.0), None); + let ctx = ctx_at(2.1); + let pixels = paint_and_read(&v, &ctx, 20, 20, false); + + assert_eq!( + px(&pixels, 20, 10, 10), + [255, 255, 0, 255], + "no loop_video: must clamp to the window's end (nearest: yellow), not wrap" + ); +} From 0a0d175693ce6b708b719c804b37938a300c3b9d Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:05:47 +0200 Subject: [PATCH 18/25] refactor(scene): paint decorative fullscreen through the box tree (#283) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every non-decorative node is timed by `PaintWindow::contains` — `self.end.is_none_or(|e| t < e)` (rustmotion-core/src/engine/box_tree.rs:43), a half-open window — and gets its effects from `box_builder::effective_effects` (legacy_dispatch.rs:110), which folds in `timeline` steps and `style.transition` keyframes. This second path uses an inclusive `time > e` and calls `a.animation_effects()` raw. `Particle` declares `timeline: Vec` (particle.rs:57), so a particle's timeline steps and transition keyframes are silently dropped, and its `end_at` keeps it visible for one extra frame — but only in a `world` view, since this is the sole call site (scene.rs:1130). The same particle in a `slide` view behaves correctly. A component whose behaviour depends on which view type contains it is invisible to tests written against either one. Refs #220 --- .../src/engine/render/background.rs | 92 ++++++++++-- crates/rustmotion/src/engine/render/scene.rs | 33 ++--- crates/rustmotion/tests/audit_ws_e.rs | 139 +++++++++++++++++- 3 files changed, 224 insertions(+), 40 deletions(-) diff --git a/crates/rustmotion/src/engine/render/background.rs b/crates/rustmotion/src/engine/render/background.rs index 2c5d719b..ef95320e 100644 --- a/crates/rustmotion/src/engine/render/background.rs +++ b/crates/rustmotion/src/engine/render/background.rs @@ -567,6 +567,12 @@ fn draw_bg_pixel_grid( } /// Tiled heropattern background. +/// +/// The tile is rasterized once at `cfg.scale`, using `heropattern_raster_size` +/// and a matching `resvg` render transform, then tiled 1:1 by the shader. +/// It used to be rasterized at 1x and magnified by the shader's own matrix +/// instead, which turned the vector source into hard nearest-neighbour +/// blocks above `scale: 1` and aliased it below `scale: 1`. fn draw_bg_heropattern( canvas: &Canvas, cfg: &HeropatternConfig, @@ -584,7 +590,6 @@ fn draw_bg_heropattern( return; } - // Build the SVG source with color/opacity substituted let svg_content = format!( r#"{}"#, def.width, @@ -596,20 +601,23 @@ fn draw_bg_heropattern( .replace("{{opacity}}", &cfg.opacity.to_string()), ); - // Render one tile via usvg/resvg let opt = usvg::Options::default(); let Ok(tree) = usvg::Tree::from_data(svg_content.as_bytes(), &opt) else { return; }; - let pw = def.width.ceil() as u32; - let ph = def.height.ceil() as u32; + let (pw, ph) = heropattern_raster_size(def.width, def.height, cfg.scale); let Some(mut pixmap) = tiny_skia::Pixmap::new(pw, ph) else { return; }; - resvg::render(&tree, tiny_skia::Transform::default(), &mut pixmap.as_mut()); + let render_scale_x = pw as f32 / def.width; + let render_scale_y = ph as f32 / def.height; + resvg::render( + &tree, + tiny_skia::Transform::from_scale(render_scale_x, render_scale_y), + &mut pixmap.as_mut(), + ); - // Convert to Skia image let info = ImageInfo::new( (pw as i32, ph as i32), ColorType::RGBA8888, @@ -625,16 +633,10 @@ fn draw_bg_heropattern( return; }; - // Build a tiled shader from the tile image - let matrix = if cfg.scale != 1.0 { - Some(skia_safe::Matrix::scale((cfg.scale, cfg.scale))) - } else { - None - }; let Some(shader) = tile_image.to_shader( (skia_safe::TileMode::Repeat, skia_safe::TileMode::Repeat), - skia_safe::SamplingOptions::default(), - matrix.as_ref(), + skia_safe::SamplingOptions::new(skia_safe::FilterMode::Linear, skia_safe::MipmapMode::None), + None, ) else { return; }; @@ -655,6 +657,21 @@ fn draw_bg_heropattern( ); } +/// Pixel size to rasterize one heropattern tile at, so the vector source is +/// re-rendered crisp at `scale` instead of rasterized at the pattern's +/// native `(width, height)` and then magnified. Clamped to `MAX_TILE_PX` +/// per axis: `HeropatternConfig::scale` has no upper bound in the schema, so +/// an unclamped scale could ask for an arbitrarily large pixmap allocation. +/// A clamped tile still tiles seamlessly with itself — it just renders +/// smaller than an extreme `scale` asked for, which is the trade the "sane +/// maximum" this is named for is making. +fn heropattern_raster_size(width: f32, height: f32, scale: f32) -> (u32, u32) { + const MAX_TILE_PX: f32 = 4096.0; + let pw = (width * scale).ceil().clamp(1.0, MAX_TILE_PX) as u32; + let ph = (height * scale).ceil().clamp(1.0, MAX_TILE_PX) as u32; + (pw, ph) +} + /// Interpolate two AnimatedBackground structs. `t` goes from 0.0 (fully `a`) to 1.0 (fully `b`). #[allow(dead_code)] pub(super) fn interpolate_animated_bg( @@ -1523,3 +1540,50 @@ mod heropattern_period_tests { assert!(spacing_x >= 20.0); } } + +#[cfg(test)] +mod heropattern_raster_tests { + //! The heropattern tile used to be rasterized at 1x (the + //! pattern's native width/height) and then magnified by the shader's + //! own matrix with nearest-neighbour sampling — blocky above `scale: 1`, + //! aliased below it. `heropattern_raster_size` must honour `scale` + //! directly in the raster resolution instead. + + use super::*; + + #[test] + fn raster_size_scales_with_cfg_scale_not_pinned_to_1x() { + let (pw, ph) = heropattern_raster_size(32.0, 64.0, 4.0); + assert_eq!( + (pw, ph), + (128, 256), + "the pixmap must be sized for the scaled tile, not the pattern's native 32x64" + ); + } + + #[test] + fn raster_size_matches_the_pattern_exactly_at_scale_1() { + assert_eq!(heropattern_raster_size(32.0, 64.0, 1.0), (32, 64)); + } + + #[test] + fn raster_size_is_clamped_for_an_unbounded_scale() { + let (pw, ph) = heropattern_raster_size(32.0, 64.0, 100_000.0); + assert!( + pw <= 4096 && ph <= 4096, + "an extreme scale must not attempt an unbounded pixmap allocation, got {pw}x{ph}" + ); + } + + #[test] + fn draw_bg_heropattern_does_not_panic_at_an_extreme_scale() { + let mut surface = skia_safe::surfaces::raster_n32_premul((64, 64)).expect("surface"); + let cfg = HeropatternConfig { + pattern: "aztec".to_string(), + color: "#FFFFFF".to_string(), + opacity: 0.1, + scale: 100_000.0, + }; + draw_bg_heropattern(surface.canvas(), &cfg, 0.0, 64.0, 64.0); + } +} diff --git a/crates/rustmotion/src/engine/render/scene.rs b/crates/rustmotion/src/engine/render/scene.rs index dde851e3..cf7d96f2 100644 --- a/crates/rustmotion/src/engine/render/scene.rs +++ b/crates/rustmotion/src/engine/render/scene.rs @@ -553,6 +553,13 @@ fn render_with_new_pipeline_iter<'a, I>( /// Paint a decorative leaf (e.g. Particle) over the full viewport without /// going through taffy. Resolves animations and dispatches to /// `Painter::paint_content` directly with a viewport-sized `BoxLayout`. +/// +/// Visibility and effects go through the same `PaintWindow::contains` and +/// `effective_effects` the ordinary `paint_tree` dispatch uses (see +/// `box_builder::effective_effects`'s doc comment), rather than re-deriving +/// both by hand — a component whose `timeline`/`style.transition` state or +/// exact `end_at` boundary only worked in one of the two dispatch paths used +/// to be invisible to tests written against either one alone. fn paint_decorative_fullscreen( canvas: &Canvas, child: &ChildComponent, @@ -560,34 +567,22 @@ fn paint_decorative_fullscreen( viewport_h: f32, ctx: &RenderContext, ) { + use rustmotion_components::box_builder::effective_effects; use rustmotion_core::engine::animator::{resolve_props_for_effects, AnimatedProperties}; + use rustmotion_core::engine::box_tree::PaintWindow; use rustmotion_core::engine::layout_pass::BoxLayout; use rustmotion_core::traits::PaintCtx; let time = ctx.time.seconds(); if let Some(timed) = child.component.as_timed() { - let (start_at, end_at) = timed.timing(); - if let Some(s) = start_at { - if time < s { - return; - } - } - if let Some(e) = end_at { - if time > e { - return; - } + let (start, end) = timed.timing(); + if !(PaintWindow { start, end }).contains(time) { + return; } } - let props = match child.component.as_animatable() { - Some(a) => { - let effects = a.animation_effects(); - if effects.is_empty() { - AnimatedProperties::default() - } else { - resolve_props_for_effects(effects, time, ctx.scene_duration) - } - } + let props = match effective_effects(&child.component, 0.0) { + Some(effects) => resolve_props_for_effects(&effects, time, ctx.scene_duration), None => AnimatedProperties::default(), }; if props.opacity <= 0.0 { diff --git a/crates/rustmotion/tests/audit_ws_e.rs b/crates/rustmotion/tests/audit_ws_e.rs index 5a2f31eb..9b0e92ab 100644 --- a/crates/rustmotion/tests/audit_ws_e.rs +++ b/crates/rustmotion/tests/audit_ws_e.rs @@ -1,18 +1,143 @@ -//! Regression tests — audit round chantier/audit-2026-09, workstream E -//! (animated backgrounds & the scene render path). +//! Regression tests — workstream E (animated backgrounds & the scene render +//! path). //! -//! Two of the four findings are bugs in fully private -//! rendering internals (`background.rs`'s `tile_spacing`/`compute_scroll_offset`/ +//! Some of the covered defects are bugs in fully private rendering +//! internals (`background.rs`'s `tile_spacing`/`compute_scroll_offset`/ //! `draw_bg_heropattern`) that this crate never exposes past its `pub` //! surface — an external integration test crate like this one cannot name //! them. Their regression tests live as `#[cfg(test)]` modules inside //! `crates/rustmotion/src/engine/render/background.rs` itself, following //! that file's own pre-existing convention (`scroll_offset_wrap_tests`, //! `pixel_grid_tests`, `grid_lines_tests`, `halo_opacity_tests`) for testing -//! renderer-private logic directly. This file carries the findings that are +//! renderer-private logic directly. This file carries the defects that are //! genuinely reachable through the crate's public API. -//! -//! One section per finding: the paint path, then colour templating. use rustmotion::encode::video::{build_frame_tasks, render_frame_task, FrameTask}; use rustmotion::loader::load_scenario_from_source; + +/// Render a single `FrameTask::WorldFrame` at `frame_in_view`, decoded from +/// `scenario_json`. Panics (with a message naming the missing frame) if no +/// such world frame exists in the built schedule — a test bug, not a +/// render-time failure, should fail loudly here. +fn render_world_frame(scenario_json: &serde_json::Value, frame_in_view: u32) -> Vec { + let scenario = load_scenario_from_source(None, Some(&scenario_json.to_string())) + .expect("scenario is schema-valid"); + let tasks = build_frame_tasks(&scenario); + let task = tasks + .iter() + .find( + |t| matches!(t, FrameTask::WorldFrame { frame_in_view: f, .. } if *f == frame_in_view), + ) + .unwrap_or_else(|| panic!("no WorldFrame task at frame_in_view={frame_in_view}")); + render_frame_task(&scenario.video, &scenario, task).expect("frame renders") +} + +/// Count pixels that read as the particle's pure-green marker colour +/// (`#00FF00`) against the scenario's plain black background — a stand-in +/// for "is the decorative child visible in this frame" that doesn't depend +/// on knowing any particle's exact on-screen position. +fn green_pixel_count(buf: &[u8]) -> usize { + buf.chunks_exact(4) + .filter(|px| px[1] > 100 && px[0] < 80 && px[2] < 80) + .count() +} + +mod decorative_dispatch_parity { + //! `paint_decorative_fullscreen` (the world-view-only path that paints + //! decorative children like `particle` without going through the box + //! tree) used to re-derive visibility and effects by hand instead of + //! calling `PaintWindow::contains` / `box_builder::effective_effects` + //! like every other paint path does. Two independent symptoms: an + //! inclusive `end_at` (visible one frame too long) and dropped + //! `timeline` animation effects. + + use super::*; + + const FPS: u32 = 10; + + fn world_scenario(particle_extra: serde_json::Value) -> serde_json::Value { + let mut particle = serde_json::json!({ + "type": "particle", + "particle_type": "snow", + "count": 30, + "colors": ["#00FF00"], + "size_range": {"min": 6, "max": 6}, + "speed": 0.0, + }); + particle + .as_object_mut() + .unwrap() + .extend(particle_extra.as_object().unwrap().clone()); + + serde_json::json!({ + "video": {"width": 64, "height": 64, "fps": FPS, "background": "#000000"}, + "composition": [ + {"type": "world", "scenes": [ + {"duration": 2.0, "children": [particle]} + ]} + ] + }) + } + + /// `end_at` is a half-open window: the child must already be gone + /// exactly at `end_at`, not still visible for one extra frame past it. + #[test] + fn end_at_is_a_half_open_window_not_inclusive() { + let end_at = 0.5; + let scenario = world_scenario(serde_json::json!({ "end_at": end_at })); + let frame_just_before_end_at = (end_at * FPS as f64) as u32 - 1; + let frame_at_end_at = (end_at * FPS as f64) as u32; + + let before = render_world_frame(&scenario, frame_just_before_end_at); + assert!( + green_pixel_count(&before) > 0, + "the particle must still be visible just before its end_at" + ); + + let at_boundary = render_world_frame(&scenario, frame_at_end_at); + assert_eq!( + green_pixel_count(&at_boundary), + 0, + "end_at is a half-open window ([start, end)): the particle must already be gone \ + exactly at end_at, not one extra frame later" + ); + } + + /// A `timeline` step's `animation` entries must be folded into the + /// resolved props like every other paint path does, not silently + /// dropped because only `style.animation` was read. + #[test] + fn timeline_animation_effects_are_not_silently_dropped() { + let fade_out_at = 0.5; + let scenario = world_scenario(serde_json::json!({ + "timeline": [{ + "at": 0.0, + "animation": [{ + "name": "keyframes", + "keyframes": [{ + "property": "opacity", + "keyframes": [ + {"time": 0.0, "value": 1.0}, + {"time": fade_out_at, "value": 0.0} + ] + }] + }] + }] + })); + + let early = render_world_frame(&scenario, 0); + assert!( + green_pixel_count(&early) > 0, + "the particle should be visible at t=0, before the timeline fade-out completes" + ); + + let frame_well_past_fade_out = (fade_out_at * FPS as f64) as u32 * 3; + let late = render_world_frame(&scenario, frame_well_past_fade_out); + assert_eq!( + green_pixel_count(&late), + 0, + "a timeline step's animation effects must apply to a decorative child, not be \ + silently dropped" + ); + } +} From 477618fdf2174cdd7460efaaca5b4292a80c89d0 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:05:57 +0200 Subject: [PATCH 19/25] fix(background): canonicalize the heropattern colour before templating (#250) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cfg.color` is a free-form `String` on `HeropatternConfig` (crates/rustmotion-core/src/schema/background.rs:216, `#[serde(default = "default_hero_color")]`) with no validation anywhere — `validate_animated_bg` (crates/rustmotion/src/include.rs:267-273) checks only that the pattern *name* resolves. The template lands inside a double-quoted attribute (`fill="{{color}}"`), so a colour containing `"` closes the attribute and injects arbitrary markup into the document usvg then parses. usvg 0.46's `Options::default()` keeps the default `ImageHrefResolver`, whose string resolver I read at usvg-0.46.0/src/parser/image.rs:85-112: it calls `std::fs::read(&path)` on any href that `path.exists()` accepts — with `resources_dir: None` an absolute path resolves to itself — so an injected `` is decoded and painted into the frame. The impact is bounded (a scenario can already read a local image legitimately via `{"type":"image","src":"/abs/path.png"}`, and there is no network resolver), so this is a hardening gap rather than a privilege escalation. The quieter half is availability: any malformed splice makes `Tree::from_data` fail and line 601 `return`s, so the background silently does not render with no diagnostic at all. Refs #220 --- .../src/engine/render/background.rs | 122 +++++++++++++++++- 1 file changed, 120 insertions(+), 2 deletions(-) diff --git a/crates/rustmotion/src/engine/render/background.rs b/crates/rustmotion/src/engine/render/background.rs index ef95320e..80dd1fa3 100644 --- a/crates/rustmotion/src/engine/render/background.rs +++ b/crates/rustmotion/src/engine/render/background.rs @@ -566,6 +566,48 @@ fn draw_bg_pixel_grid( } } +/// Re-emit a scenario-supplied heropattern colour as a canonical +/// `#rrggbbaa` before it is spliced into generated SVG source. +/// +/// `cfg.color` is free-form user input landing inside a double-quoted +/// `fill="{{color}}"` attribute of hand-built SVG text; routing it through +/// `color4f_from_hex` first guarantees the only characters that can ever +/// reach the SVG are hex digits and `#`, so a colour string can never close +/// the attribute and inject markup, whatever it contains. +/// `color4f_from_hex` is infallible: unresolvable input resolves to the +/// same opaque-magenta sentinel every other unresolved colour in this +/// engine does, rather than passing the raw string through. +fn canonical_hero_color(color: &str) -> String { + let c = color4f_from_hex(color); + format!( + "#{:02X}{:02X}{:02X}{:02X}", + (c.r.clamp(0.0, 1.0) * 255.0).round() as u8, + (c.g.clamp(0.0, 1.0) * 255.0).round() as u8, + (c.b.clamp(0.0, 1.0) * 255.0).round() as u8, + (c.a.clamp(0.0, 1.0) * 255.0).round() as u8, + ) +} + +/// `usvg::Options` for parsing a generated heropattern tile. +/// +/// Neutralises the default `image_href_resolver`'s string resolver, which +/// reads arbitrary files from disk for any `` it +/// encounters (usvg-0.44.0's `ImageHrefResolver::default_string_resolver`). +/// A heropattern tile never legitimately references an external image, so +/// an `` element reaching this parser can only be an injection — +/// `canonical_hero_color` closes the splice that could put one there in the +/// first place; this is the defence-in-depth half, for any other way one +/// could arrive. +fn heropattern_svg_options() -> usvg::Options<'static> { + usvg::Options { + image_href_resolver: usvg::ImageHrefResolver { + resolve_string: Box::new(|_, _| None), + ..usvg::ImageHrefResolver::default() + }, + ..usvg::Options::default() + } +} + /// Tiled heropattern background. /// /// The tile is rasterized once at `cfg.scale`, using `heropattern_raster_size` @@ -597,12 +639,16 @@ fn draw_bg_heropattern( def.width, def.height, def.svg_paths - .replace("{{color}}", &cfg.color) + .replace("{{color}}", &canonical_hero_color(&cfg.color)) .replace("{{opacity}}", &cfg.opacity.to_string()), ); - let opt = usvg::Options::default(); + let opt = heropattern_svg_options(); let Ok(tree) = usvg::Tree::from_data(svg_content.as_bytes(), &opt) else { + eprintln!( + "warning: heropattern '{}' (colour '{}') failed to parse as SVG — background not rendered", + cfg.pattern, cfg.color + ); return; }; @@ -1587,3 +1633,75 @@ mod heropattern_raster_tests { draw_bg_heropattern(surface.canvas(), &cfg, 0.0, 64.0, 64.0); } } + +#[cfg(test)] +mod heropattern_svg_injection_tests { + //! A scenario-supplied heropattern colour used to be spliced unescaped + //! into hand-built SVG source inside a double-quoted `fill="..."` + //! attribute, then parsed by usvg with its default (file-reading) + //! `image_href_resolver`. A colour containing a `"` could close the + //! attribute and inject arbitrary markup, including an `` the default resolver would read straight off disk. + + use super::*; + + #[test] + fn a_colour_containing_a_double_quote_cannot_inject_markup() { + let payload = r#""/>"#, + ) + .expect("scratch SVG written"); + + let opt = heropattern_svg_options(); + let resolved = + (opt.image_href_resolver.resolve_string)(scratch_path.to_str().unwrap(), &opt); + + let _ = std::fs::remove_file(&scratch_path); + + assert!( + resolved.is_none(), + "the string resolver must be neutralised, not read a real SVG file from disk" + ); + } + + #[test] + fn draw_bg_heropattern_survives_an_injection_attempt_without_panicking() { + let mut surface = skia_safe::surfaces::raster_n32_premul((32, 32)).expect("surface"); + let cfg = HeropatternConfig { + pattern: "aztec".to_string(), + color: r#""/> Date: Tue, 22 Sep 2026 11:06:07 +0200 Subject: [PATCH 20/25] fix(html): reject unsupported children of text tags (#254) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `TagKind::Text` arm never calls `element_to_value` on its children, so the whole guard layer (`StyleElementUnsupported`, `UnsupportedNativeElement`, `TagKind::Ignored`) is unreachable below `p/span/h1..h6/strong/em/label`. Executed against the real crate: `

Real

` transpiles to `{"content":"Real var secret = 1; alert(2);","type":"text"}` — the JS source is rendered on screen; `

Title

` gives `{"content":"Titleh1{color:#0f0}"}`; `

cap

`, `

t…

` and `n=` drop the img/svg/counter entirely and exit 0. This is a live regression of "constat 1" and "constat 3", which tests/silent_loss_regressions.rs asserts are fixed — the fix only ever covered the `Container` path. Refs #220 --- crates/rustmotion-html/src/element.rs | 42 ++++++++++-- crates/rustmotion-html/tests/audit_ws_f.rs | 76 ++++++++++++++++++++++ 2 files changed, 111 insertions(+), 7 deletions(-) diff --git a/crates/rustmotion-html/src/element.rs b/crates/rustmotion-html/src/element.rs index 337c8b2e..0722aa9d 100644 --- a/crates/rustmotion-html/src/element.rs +++ b/crates/rustmotion-html/src/element.rs @@ -37,20 +37,48 @@ fn tag_kind(tag: &str) -> TagKind { } } -/// Concatenated text of an element and all its descendants. -pub(crate) fn inner_text(handle: &Handle) -> String { +/// Concatenated text of an element and all its descendants. Nested inline +/// formatting tags (`strong`/`em`/`label`/…) flatten in, matching real HTML; +/// `

"##; + let v = html_to_scenario_value(html).expect("