From 523f9958d4962b5833ed302b5f16e68b71b54bea Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:00:04 +0200 Subject: [PATCH] fix(render): clear asset caches between watch renders 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" + ); + } +}