From e4bfd753ac194521e6c595c31eac9d4c71ca223f Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:04:58 +0200 Subject: [PATCH] fix(gif): cap decoded gif dimensions and frame count 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()); -}