From 3a156c2cdf821e0688f4ddf076d65343908ec4ce Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 00:24:25 +0200 Subject: [PATCH 01/56] feat(core): anchor vw/vh resolution to the real output viewport ConversionContext::default() carries a hardcoded 1920x1080 viewport. Every caller that knows the video's real dimensions was free to forget it, and two did: both geometry-validation call sites resolve vw/vh against the phantom default while the render path builds a real context. On a 1080x1920 vertical render that inverts the two axes inside the validator, so the mandatory pre-delivery gate reports overflow that never happens and misses overflow that does. for_viewport() is the single constructor those callers now share, so the validator and the renderer cannot drift apart again. The private helper that already did this in the render path (engine/render/scene.rs) is superseded by it. Rejected: making default() unavailable outside tests. Roughly forty call sites in the test suites legitimately want an arbitrary viewport, and churning them would bury the fix. --- .../rustmotion-core/src/css/taffy_bridge.rs | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/crates/rustmotion-core/src/css/taffy_bridge.rs b/crates/rustmotion-core/src/css/taffy_bridge.rs index 9376ebb7..380cb86d 100644 --- a/crates/rustmotion-core/src/css/taffy_bridge.rs +++ b/crates/rustmotion-core/src/css/taffy_bridge.rs @@ -20,6 +20,28 @@ pub struct ConversionContext { pub length: LengthContext, } +impl ConversionContext { + /// Anchor `vw`/`vh` resolution to a real output viewport. + /// + /// [`ConversionContext::default()`] carries a 1920×1080 viewport, which is + /// only ever correct by coincidence. Any caller that knows the video's real + /// dimensions must build its context here instead: on a 1080×1920 vertical + /// render, the default resolves `50vw` to 960px where the truth is 540px, + /// and misses `vh` by the same margin in the other axis. + /// + /// `font-size` / `root-font-size` stay at the CSS initial 16px: nothing + /// upstream resolves and threads a root font-size through yet. + pub fn for_viewport(viewport_width: f32, viewport_height: f32) -> Self { + Self { + length: LengthContext { + viewport_width, + viewport_height, + ..LengthContext::default() + }, + } + } +} + /// Convert a [`CssStyle`] into a [`taffy::Style`]. Properties not relevant to /// layout are ignored. Unsupported / unset properties fall back to taffy /// defaults (which match CSS initial values). From 7d676d96420638dec69638740fe4b09c34baabf0 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 08:00:17 +0200 Subject: [PATCH 02/56] style(lint): replace constant-size chunks_exact with as_chunks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A newer stable clippy adds `chunks_exact_to_as_chunks`, which fires on every `chunks_exact` call whose size is a literal. The workspace has 32 of them across 20 files, so the clippy job fails on every branch — main included. Its last run, from before this work existed, was already red for the same reason. The replacement is clippy's own suggestion, `as_chunks::().0.iter()`. It is not a pure substitution: the iterator now yields `&[T; N]` rather than `&[T]`, so two sites needed real edits — an assertion comparing against an array literal, and a predicate taking `&[u8]`. The array type is the better one anyway; it is what makes the chunk length visible to the compiler. Completeness is checkable rather than argued: no `chunks_exact` with a literal size remains in the workspace, so the lint has nothing left to fire on. The local toolchain predates this lint, so that grep is the verification, not a local clippy run. --- crates/rustmotion-components/src/badge.rs | 4 +- crates/rustmotion-components/src/callout.rs | 4 +- crates/rustmotion-components/src/counter.rs | 2 +- crates/rustmotion-components/src/kbd.rs | 4 +- crates/rustmotion-components/src/list.rs | 2 +- crates/rustmotion-components/src/lottie.rs | 2 +- crates/rustmotion-components/src/marquee.rs | 2 +- .../rustmotion-components/src/notification.rs | 4 +- crates/rustmotion-components/src/pill_nav.rs | 2 +- crates/rustmotion-components/src/table.rs | 4 +- crates/rustmotion-components/src/terminal.rs | 4 +- crates/rustmotion-components/src/tooltip.rs | 4 +- .../tests/caption_presets.rs | 6 ++- .../tests/codeblock_auto_scroll.rs | 4 +- .../tests/relative_font_size.rs | 2 +- .../src/engine/renderer/text.rs | 2 +- .../rustmotion-core/src/engine/transition.rs | 4 +- crates/rustmotion-studio/src/editor/audio.rs | 4 +- .../src/scenario/optimistic.rs | 4 +- crates/rustmotion/src/tests.rs | 49 ++++++++++++++----- 20 files changed, 80 insertions(+), 33 deletions(-) diff --git a/crates/rustmotion-components/src/badge.rs b/crates/rustmotion-components/src/badge.rs index 5e19ea9f..f4f2cdbb 100644 --- a/crates/rustmotion-components/src/badge.rs +++ b/crates/rustmotion-components/src/badge.rs @@ -441,7 +441,9 @@ mod tests { // Solid variant text is always white — probe for white ink // specifically, since the pill background paints regardless. let text_ink = buf - .chunks_exact(4) + .as_chunks::<4>() + .0 + .iter() .filter(|p| p[3] > 0 && p[0] > 200 && p[1] > 200 && p[2] > 200) .count(); assert!( diff --git a/crates/rustmotion-components/src/callout.rs b/crates/rustmotion-components/src/callout.rs index 22e8bf97..8092d4aa 100644 --- a/crates/rustmotion-components/src/callout.rs +++ b/crates/rustmotion-components/src/callout.rs @@ -267,7 +267,9 @@ mod tests { // for near-white ink specifically, since the bubble background // paints regardless of font-size. let text_ink = buf - .chunks_exact(4) + .as_chunks::<4>() + .0 + .iter() .filter(|p| p[3] > 0 && p[0] > 200 && p[1] > 200 && p[2] > 200) .count(); assert!( diff --git a/crates/rustmotion-components/src/counter.rs b/crates/rustmotion-components/src/counter.rs index ebe1108e..e67af735 100644 --- a/crates/rustmotion-components/src/counter.rs +++ b/crates/rustmotion-components/src/counter.rs @@ -403,7 +403,7 @@ mod tests { skia_safe::image::CachingHint::Disallow, ); assert!(ok, "pixel read should succeed"); - let lit = buf.chunks_exact(4).filter(|p| p[3] > 0).count(); + let lit = buf.as_chunks::<4>().0.iter().filter(|p| p[3] > 0).count(); assert!( lit > 20, "counter at font-size: 2rem must paint visible ink, got {lit} lit pixels" diff --git a/crates/rustmotion-components/src/kbd.rs b/crates/rustmotion-components/src/kbd.rs index ecc065b8..6874cbf3 100644 --- a/crates/rustmotion-components/src/kbd.rs +++ b/crates/rustmotion-components/src/kbd.rs @@ -339,7 +339,9 @@ mod tests { // face — probe for near-white ink specifically, since the face/ // border/shadow paint regardless of font-size. let text_ink = buf - .chunks_exact(4) + .as_chunks::<4>() + .0 + .iter() .filter(|p| p[3] > 0 && p[0] > 180 && p[1] > 180 && p[2] > 180) .count(); assert!( diff --git a/crates/rustmotion-components/src/list.rs b/crates/rustmotion-components/src/list.rs index d58723cd..4e7fd71e 100644 --- a/crates/rustmotion-components/src/list.rs +++ b/crates/rustmotion-components/src/list.rs @@ -333,7 +333,7 @@ mod tests { skia_safe::image::CachingHint::Disallow, ); assert!(ok, "pixel read should succeed"); - let lit = buf.chunks_exact(4).filter(|p| p[3] > 0).count(); + let lit = buf.as_chunks::<4>().0.iter().filter(|p| p[3] > 0).count(); assert!( lit > 20, "list at font-size: 2rem must paint visible ink, got {lit} lit pixels" diff --git a/crates/rustmotion-components/src/lottie.rs b/crates/rustmotion-components/src/lottie.rs index e2b08b5c..6fee0988 100644 --- a/crates/rustmotion-components/src/lottie.rs +++ b/crates/rustmotion-components/src/lottie.rs @@ -525,7 +525,7 @@ mod tests { let mut red_sum: u64 = 0; let mut blue_sum: u64 = 0; - for px in buf.chunks_exact(4) { + for px in buf.as_chunks::<4>().0.iter() { let r = px[0] as u64; let _g = px[1] as u64; let b = px[2] as u64; diff --git a/crates/rustmotion-components/src/marquee.rs b/crates/rustmotion-components/src/marquee.rs index 08e0379c..401baaa1 100644 --- a/crates/rustmotion-components/src/marquee.rs +++ b/crates/rustmotion-components/src/marquee.rs @@ -224,7 +224,7 @@ mod tests { skia_safe::image::CachingHint::Disallow, ); assert!(ok, "pixel read should succeed"); - let lit = buf.chunks_exact(4).filter(|p| p[3] > 0).count(); + let lit = buf.as_chunks::<4>().0.iter().filter(|p| p[3] > 0).count(); assert!( lit > 20, "marquee at font-size: 2rem must paint visible ink, got {lit} lit pixels" diff --git a/crates/rustmotion-components/src/notification.rs b/crates/rustmotion-components/src/notification.rs index 436f2d35..4d63a057 100644 --- a/crates/rustmotion-components/src/notification.rs +++ b/crates/rustmotion-components/src/notification.rs @@ -451,7 +451,9 @@ mod tests { // Title text is white (#FFFFFF default) on a dark #1E293B card — // probe for near-white ink specifically. let text_ink = buf - .chunks_exact(4) + .as_chunks::<4>() + .0 + .iter() .filter(|p| p[3] > 0 && p[0] > 200 && p[1] > 200 && p[2] > 200) .count(); assert!( diff --git a/crates/rustmotion-components/src/pill_nav.rs b/crates/rustmotion-components/src/pill_nav.rs index 3b3d038d..3039de35 100644 --- a/crates/rustmotion-components/src/pill_nav.rs +++ b/crates/rustmotion-components/src/pill_nav.rs @@ -328,7 +328,7 @@ mod tests { skia_safe::image::CachingHint::Disallow, ); assert!(ok, "pixel read should succeed"); - let lit = buf.chunks_exact(4).filter(|p| p[3] > 0).count(); + let lit = buf.as_chunks::<4>().0.iter().filter(|p| p[3] > 0).count(); assert!( lit > 20, "pill_nav at font-size: 2rem must paint visible ink, got {lit} lit pixels" diff --git a/crates/rustmotion-components/src/table.rs b/crates/rustmotion-components/src/table.rs index 05fbc996..527a532c 100644 --- a/crates/rustmotion-components/src/table.rs +++ b/crates/rustmotion-components/src/table.rs @@ -371,7 +371,9 @@ 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 - .chunks_exact(4) + .as_chunks::<4>() + .0 + .iter() .filter(|p| p[3] > 0 && p[0] > 200 && p[1] > 200 && p[2] > 200) .count(); assert!( diff --git a/crates/rustmotion-components/src/terminal.rs b/crates/rustmotion-components/src/terminal.rs index 0e22037b..be6224ec 100644 --- a/crates/rustmotion-components/src/terminal.rs +++ b/crates/rustmotion-components/src/terminal.rs @@ -536,7 +536,9 @@ mod tests { // specifically: pixels that are not the dark theme background color // (#1E1E1E) and not fully transparent. let text_ink = buf - .chunks_exact(4) + .as_chunks::<4>() + .0 + .iter() .filter(|p| p[3] > 0 && !(p[0] < 40 && p[1] < 40 && p[2] < 40)) .count(); assert!( diff --git a/crates/rustmotion-components/src/tooltip.rs b/crates/rustmotion-components/src/tooltip.rs index 44cd1a54..432c1690 100644 --- a/crates/rustmotion-components/src/tooltip.rs +++ b/crates/rustmotion-components/src/tooltip.rs @@ -280,7 +280,9 @@ mod tests { // Text is near-white (#E2E8F0 default) on a dark #1E293B body — // probe for near-white ink specifically. let text_ink = buf - .chunks_exact(4) + .as_chunks::<4>() + .0 + .iter() .filter(|p| p[3] > 0 && p[0] > 180 && p[1] > 180 && p[2] > 180) .count(); assert!( diff --git a/crates/rustmotion-components/tests/caption_presets.rs b/crates/rustmotion-components/tests/caption_presets.rs index 2f074ae8..59ecf7b9 100644 --- a/crates/rustmotion-components/tests/caption_presets.rs +++ b/crates/rustmotion-components/tests/caption_presets.rs @@ -82,7 +82,11 @@ fn render_caption_at(json: serde_json::Value, time: f64, y: f32) -> Vec { } fn count_pixels(buf: &[u8], pred: impl Fn(&[u8]) -> bool) -> usize { - buf.chunks_exact(4).filter(|p| pred(p)).count() + buf.as_chunks::<4>() + .0 + .iter() + .filter(|p| pred(p.as_slice())) + .count() } fn lit_pixels(buf: &[u8]) -> usize { diff --git a/crates/rustmotion-components/tests/codeblock_auto_scroll.rs b/crates/rustmotion-components/tests/codeblock_auto_scroll.rs index 40a4f2eb..e1668a31 100644 --- a/crates/rustmotion-components/tests/codeblock_auto_scroll.rs +++ b/crates/rustmotion-components/tests/codeblock_auto_scroll.rs @@ -42,7 +42,9 @@ fn text_ink_pixels(buf: &[u8]) -> usize { // Background is #2b303b ~ (43, 48, 59). Count pixels that deviate from // that by a wide margin in any channel — syntect's theme colors are all // much brighter than the near-black background. - buf.chunks_exact(4) + buf.as_chunks::<4>() + .0 + .iter() .filter(|p| { let (r, g, b, a) = (p[0] as i32, p[1] as i32, p[2] as i32, p[3] as i32); a > 200 && ((r - 43).abs() > 40 || (g - 48).abs() > 40 || (b - 59).abs() > 40) diff --git a/crates/rustmotion-components/tests/relative_font_size.rs b/crates/rustmotion-components/tests/relative_font_size.rs index 21965697..c40e8fe0 100644 --- a/crates/rustmotion-components/tests/relative_font_size.rs +++ b/crates/rustmotion-components/tests/relative_font_size.rs @@ -77,7 +77,7 @@ fn render(json: serde_json::Value) -> Vec { } fn lit_pixels(buf: &[u8]) -> usize { - buf.chunks_exact(4).filter(|p| p[3] > 0).count() + buf.as_chunks::<4>().0.iter().filter(|p| p[3] > 0).count() } #[test] diff --git a/crates/rustmotion-core/src/engine/renderer/text.rs b/crates/rustmotion-core/src/engine/renderer/text.rs index d607dadf..6d578977 100644 --- a/crates/rustmotion-core/src/engine/renderer/text.rs +++ b/crates/rustmotion-core/src/engine/renderer/text.rs @@ -975,7 +975,7 @@ mod emoji_presentation_tests { surface.read_pixels(&info, &mut buf, (W * 4) as usize, (0, 0)); let (mut n, mut sr, mut sg, mut sb) = (0usize, 0f64, 0f64, 0f64); - for px in buf.chunks_exact(4) { + for px in buf.as_chunks::<4>().0.iter() { if px[3] > 40 { n += 1; sr += px[0] as f64; diff --git a/crates/rustmotion-core/src/engine/transition.rs b/crates/rustmotion-core/src/engine/transition.rs index b256a080..22e5923f 100644 --- a/crates/rustmotion-core/src/engine/transition.rs +++ b/crates/rustmotion-core/src/engine/transition.rs @@ -970,9 +970,9 @@ mod camera_pan_tests { ); // blend_fade(10, 200, 0.5) = (10*0.5 + 200*0.5 + 0.5) as u8 = 105. - for px in out.chunks_exact(4) { + for px in out.as_chunks::<4>().0.iter() { assert_eq!( - px, + *px, [105, 105, 105, 255], "mid-pan Static frame must be a blend of bg_a and bg_b, not a copy of either" ); diff --git a/crates/rustmotion-studio/src/editor/audio.rs b/crates/rustmotion-studio/src/editor/audio.rs index 5b015479..6a232169 100644 --- a/crates/rustmotion-studio/src/editor/audio.rs +++ b/crates/rustmotion-studio/src/editor/audio.rs @@ -165,7 +165,9 @@ pub fn prepare(scenario: Arc, total_duration: f64) { match mixed { Ok(Some(pcm_bytes)) => { let pcm: Vec = pcm_bytes - .chunks_exact(2) + .as_chunks::<2>() + .0 + .iter() .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/scenario/optimistic.rs b/crates/rustmotion-studio/src/scenario/optimistic.rs index adf3ae3f..5cca75c0 100644 --- a/crates/rustmotion-studio/src/scenario/optimistic.rs +++ b/crates/rustmotion-studio/src/scenario/optimistic.rs @@ -312,7 +312,9 @@ mod tests { .expect("render") }; let count_red = |rgba: &[u8]| { - rgba.chunks_exact(4) + rgba.as_chunks::<4>() + .0 + .iter() .filter(|p| p[0] > 180 && p[1] < 90 && p[2] < 90) .count() }; diff --git a/crates/rustmotion/src/tests.rs b/crates/rustmotion/src/tests.rs index 784e3d12..661687b4 100644 --- a/crates/rustmotion/src/tests.rs +++ b/crates/rustmotion/src/tests.rs @@ -539,7 +539,7 @@ mod component_smoke { /// Counts non-transparent pixels in an RGBA buffer (alpha > 0). fn nonzero_pixels(buf: &[u8]) -> usize { - buf.chunks_exact(4).filter(|p| p[3] > 0).count() + buf.as_chunks::<4>().0.iter().filter(|p| p[3] > 0).count() } #[test] @@ -605,7 +605,8 @@ mod component_smoke { let late = render_new_at(&scene, 400, 300, 0.95, 1.0); // Total red intensity (sum of red channel) — alpha-attenuated pixels // contribute less to this sum even when premul keeps the count up. - let red_sum: fn(&[u8]) -> u64 = |buf| buf.chunks_exact(4).map(|p| p[0] as u64).sum(); + let red_sum: fn(&[u8]) -> u64 = + |buf| buf.as_chunks::<4>().0.iter().map(|p| p[0] as u64).sum(); let early_red = red_sum(&early); let late_red = red_sum(&late); assert!( @@ -638,7 +639,7 @@ mod component_smoke { } fn red_sum(buf: &[u8]) -> u64 { - buf.chunks_exact(4).map(|p| p[0] as u64).sum() + buf.as_chunks::<4>().0.iter().map(|p| p[0] as u64).sum() } #[test] @@ -718,7 +719,9 @@ mod component_smoke { } })); let count_mid = |buf: &[u8]| { - buf.chunks_exact(4) + buf.as_chunks::<4>() + .0 + .iter() .filter(|p| p[0] > 20 && p[0] < 220) .count() }; @@ -744,7 +747,9 @@ mod component_smoke { })); let buf = render_new_at(&inverted, 400, 300, 0.5, 1.0); let flipped = buf - .chunks_exact(4) + .as_chunks::<4>() + .0 + .iter() .filter(|p| p[0] < 50 && p[1] > 150) .count(); assert!( @@ -873,7 +878,8 @@ mod component_smoke { bleed: false, }; let scene = vec![child]; - let blue_sum = |buf: &[u8]| -> u64 { buf.chunks_exact(4).map(|p| p[2] as u64).sum() }; + let blue_sum = + |buf: &[u8]| -> u64 { buf.as_chunks::<4>().0.iter().map(|p| p[2] as u64).sum() }; let before = render_new_at(&scene, 400, 300, 0.5, 4.0); let mid = render_new_at(&scene, 400, 300, 1.5, 4.0); let after = render_new_at(&scene, 400, 300, 2.5, 4.0); @@ -936,7 +942,7 @@ mod component_smoke { fn red_centroid_x(buf: &[u8], width: u32) -> Option { let mut sum_wx = 0.0_f64; let mut sum_w = 0.0_f64; - for (i, p) in buf.chunks_exact(4).enumerate() { + for (i, p) in buf.as_chunks::<4>().0.iter().enumerate() { let x = (i as u32 % width) as f64; let w = p[0] as f64; sum_wx += w * x; @@ -1665,7 +1671,7 @@ mod svg_draw_on_tests { /// Count non-transparent pixels in RGBA buffer. fn lit(buf: &[u8]) -> usize { - buf.chunks_exact(4).filter(|p| p[3] > 0).count() + buf.as_chunks::<4>().0.iter().filter(|p| p[3] > 0).count() } /// Count non-transparent pixels in horizontal band [y0, y1) of a 100-wide canvas. @@ -1831,7 +1837,9 @@ mod svg_draw_on_tests { ); // resvg fills the rect red. Count red-dominant pixels. let red_pixels = buf - .chunks_exact(4) + .as_chunks::<4>() + .0 + .iter() .filter(|p| p[3] > 0 && p[0] > p[2] && p[0] > p[1]) .count(); assert!( @@ -2232,7 +2240,9 @@ mod audio_tests { let lit_at = |frame: usize| { crate::encode::video::render_frame_task(&scenario.video, &scenario, &tasks[frame]) .expect("render") - .chunks_exact(4) + .as_chunks::<4>() + .0 + .iter() .filter(|p| p[0] > 40) .count() }; @@ -2556,7 +2566,14 @@ mod audio_tests { } })) }; - let red_sum = |pixels: &[u8]| pixels.chunks_exact(4).map(|p| p[0] as u64).sum::(); + let red_sum = |pixels: &[u8]| { + pixels + .as_chunks::<4>() + .0 + .iter() + .map(|p| p[0] as u64) + .sum::() + }; let loud = paint_scene(make_child(), 200, 200, 0.0, 30); let quiet = paint_scene(make_child(), 200, 200, 0.5, 30); @@ -2658,7 +2675,13 @@ mod motion_blur_trail { /// Find the maximum red channel value across all pixels. fn max_red(pixels: &[u8]) -> u8 { - pixels.chunks_exact(4).map(|p| p[0]).max().unwrap_or(0) + pixels + .as_chunks::<4>() + .0 + .iter() + .map(|p| p[0]) + .max() + .unwrap_or(0) } /// Make a red Shape with slide_in_left + motion_blur, positioned at the center. @@ -3385,7 +3408,7 @@ mod world_view_regressions { fn avg_luma(buf: &[u8]) -> f64 { let mut sum = 0u64; let mut n = 0u64; - for px in buf.chunks_exact(4) { + for px in buf.as_chunks::<4>().0.iter() { sum += px[0] as u64 + px[1] as u64 + px[2] as u64; n += 3; } From 8444e4e02c028378ba8c9642df81c7adbf924dfa Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:52:05 +0200 Subject: [PATCH 03/56] fix(paint): include box-shadow and descendant ink in the layer bounds (#223) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bleed is derived only from node.css.filter (FilterFn::Blur / DropShadow in filter_bleed, line 610). It ignores the node's own box_shadow, which step 5 paints *inside* this layer (line 411-418) at layout.x + dx - spread, width + spread*2 — i.e. outside the border-box — and it ignores the whole subtree painted at steps 9-10, which with the default overflow: visible may legitimately extend past the parent box (absolutely-positioned children, a child's own scale/pulse transform, a child glow, marquee, which CLAUDE.md explicitly documents as "exempté (leur rôle est de bleed)"). The file's own comment on filter_bleed (line 605-609) states the rule: "a *too-tight* one would silently clip filter bleed, trading a perf bug for a correctness one". Concretely: a card with box-shadow: 0 20px 40px rgba(0,0,0,.5) loses its shadow the moment any fade_in drives opacity below 1.0, and regains it on the frame opacity reaches 1.0 — a visible pop mid-entrance. The repo already treats this exact invariant as load-bearing for overflow: hidden (test overflow_hidden_does_not_clip_own_outset_box_shadow, line 2909); the opacity layer violates it for the same shadow. Fix: Compute the layer bounds from the union of: the border-box, the filter bleed, the outset box_shadow extents (|offset| + blur*1.5 + spread per shadow), and — when overflow is visible — the descendants' layout union. Alternatively move the outset box-shadow painting outside the opacity layer and multiply its paint alpha by opacity, and fall back to an unbounded layer when overflow: visible and children exist. Refs #220 --- .../rustmotion-core/src/engine/paint_pass.rs | 108 ++++++++++++-- crates/rustmotion-core/tests/audit_ws_a.rs | 133 ++++++++++++++++++ 2 files changed, 228 insertions(+), 13 deletions(-) create mode 100644 crates/rustmotion-core/tests/audit_ws_a.rs diff --git a/crates/rustmotion-core/src/engine/paint_pass.rs b/crates/rustmotion-core/src/engine/paint_pass.rs index 496cdea0..048b3c81 100644 --- a/crates/rustmotion-core/src/engine/paint_pass.rs +++ b/crates/rustmotion-core/src/engine/paint_pass.rs @@ -356,17 +356,28 @@ fn paint_node(canvas: &Canvas, node: &BoxNode, ctx: &PaintContext, tree_depth: u } } + // Hoisted from step 8 below: the layer-bounds computation right after + // this needs it too, to decide whether descendant ink painted outside + // the border-box (legitimate under `overflow: visible`) must stay + // reachable by the opacity/filter layer opened next. + let overflow = node.css.overflow.unwrap_or(Overflow::Visible); + // 4. opacity / filter layer — one shared layer carries both the group // alpha and the CSS `filter` chain (applies to the node and its - // subtree). Bounded to the node's own box (padded by the filter chain's - // blur/drop-shadow bleed so those still bleed past the edge, unclipped): - // an unbounded `SaveLayerRec` sizes the layer against the current clip — - // usually the whole viewport — so every faded/filtered node allocates - // and composites a full-frame layer regardless of how small it is - // (measured on this repo's release binary, 1080x1920/60 frames, 30 small - // `opacity: 0.5` shapes, `--threads 1`: ~42-60s wall time unbounded vs. - // ~0.5s bounded — roughly two orders of magnitude, not a rounding - // error; cost scales with viewport area, not node size). + // subtree). Bounded to the node's own box, padded by: the filter + // chain's blur/drop-shadow bleed, this node's own outset box-shadow + // extent (painted inside this same layer at step 5, outside the + // border-box), and — when `overflow` leaves descendant ink free to + // paint past the border-box — the union of the whole subtree's layout + // boxes. An unbounded `SaveLayerRec` sizes the layer against the + // current clip — usually the whole viewport — so every faded/filtered + // node allocates and composites a full-frame layer regardless of how + // small it is (measured on this repo's release binary, 1080x1920/60 + // frames, 30 small `opacity: 0.5` shapes, `--threads 1`: ~42-60s wall + // time unbounded vs. ~0.5s bounded — roughly two orders of magnitude, + // not a rounding error; cost scales with viewport area, not node + // size), so the bound stays tight to the content that can actually + // paint rather than falling back to the viewport. let opacity = node.css.opacity.unwrap_or(1.0).clamp(0.0, 1.0); let content_filter = node .css @@ -381,18 +392,30 @@ fn paint_node(canvas: &Canvas, node: &BoxNode, ctx: &PaintContext, tree_depth: u if let Some(filter) = content_filter { paint.set_image_filter(filter); } - let bleed = node + let filter_bleed_px = node .css .filter .as_deref() .map(|list| filter_bleed(list, &length_ctx)) .unwrap_or(0.0); - let bounds = Rect::from_xywh( + let shadow_bleed_px = node + .css + .box_shadow + .as_deref() + .map(|shadows| box_shadow_bleed(shadows, &length_ctx)) + .unwrap_or(0.0); + let bleed = filter_bleed_px.max(shadow_bleed_px); + let mut bounds = Rect::from_xywh( box_layout.x - bleed, box_layout.y - bleed, box_layout.width + bleed * 2.0, box_layout.height + bleed * 2.0, ); + if overflow == Overflow::Visible { + if let Some(descendants) = subtree_layout_bounds(node, ctx.layout) { + bounds = Rect::join2(bounds, descendants); + } + } let rec = SaveLayerRec::default().paint(&paint).bounds(&bounds); canvas.save_layer(&rec); true @@ -448,8 +471,8 @@ fn paint_node(canvas: &Canvas, node: &BoxNode, ctx: &PaintContext, tree_depth: u // 8. clip overflow:hidden / clip — scoped to this node's own content and // its children only (see step 5-7's comment for why the box's own - // decorations must stay outside this clip). - let overflow = node.css.overflow.unwrap_or(Overflow::Visible); + // decorations must stay outside this clip). `overflow` was hoisted + // above step 4. let opened_overflow_clip = if matches!( overflow, Overflow::Hidden | Overflow::Clip | Overflow::Scroll | Overflow::Auto @@ -632,6 +655,65 @@ fn filter_bleed(list: &[crate::css::style::FilterFn], ctx: &LengthContext) -> f3 bleed } +/// Conservative outward bleed (px) a node's own outset `box_shadow` list +/// paints beyond its border-box — the same role `filter_bleed` plays for +/// `filter`, and sized the same way (offset + spread pushes the shadow rect +/// out, `1.5x` blur radius covers the Gaussian falloff). Inset shadows are +/// clipped to the padding-box by `paint_box_shadow` and never bleed outward, +/// so they are skipped here. +fn box_shadow_bleed(shadows: &[BoxShadow], ctx: &LengthContext) -> f32 { + let mut bleed = 0.0f32; + for shadow in shadows { + if shadow.inset.unwrap_or(false) { + continue; + } + let offset = shadow + .offset_x + .resolve(ctx) + .abs() + .max(shadow.offset_y.resolve(ctx).abs()); + let spread = shadow + .spread + .as_ref() + .map(|s| s.resolve(ctx).max(0.0)) + .unwrap_or(0.0); + let blur_bleed = shadow + .blur + .as_ref() + .map(|b| b.resolve(ctx).max(0.0) * 1.5) + .unwrap_or(0.0); + bleed = bleed.max(offset + spread + blur_bleed); + } + bleed +} + +/// Bounding box (viewport coordinates) of every descendant's own layout box, +/// recursively — the same "leave the layer big enough to hold what can +/// legitimately paint outside the border-box" contract as `filter_bleed`, +/// applied to `overflow: visible` subtrees instead of a filter chain. Each +/// descendant contributes only its plain layout rect (not its own +/// filter/shadow bleed or transform): a tight bound for the common cases — +/// absolutely-positioned children, `marquee`, a taller-than-parent flow — +/// without walking the whole subtree's CSS. +fn subtree_layout_bounds(node: &BoxNode, layout: &LayoutResult) -> Option { + let mut bounds: Option = None; + for child in &node.children { + if let Some(child_layout) = layout.get(child.id) { + let rect = Rect::from_xywh( + child_layout.x, + child_layout.y, + child_layout.width, + child_layout.height, + ); + bounds = Some(bounds.map_or(rect, |b| Rect::join2(b, rect))); + } + if let Some(child_bounds) = subtree_layout_bounds(child, layout) { + bounds = Some(bounds.map_or(child_bounds, |b| Rect::join2(b, child_bounds))); + } + } + bounds +} + // ---- CSS filters ---- /// Build a Skia `ImageFilter` chain from a CSS `filter`/`backdrop-filter` diff --git a/crates/rustmotion-core/tests/audit_ws_a.rs b/crates/rustmotion-core/tests/audit_ws_a.rs new file mode 100644 index 00000000..0eb5f7e5 --- /dev/null +++ b/crates/rustmotion-core/tests/audit_ws_a.rs @@ -0,0 +1,133 @@ +//! Regression tests for the workstream A (animation & paint) audit findings +//! tracked in issue #220. + +use rustmotion_core::css::style::{ + Background, BoxShadow, Color as CssColor, CssStyle, Display, FlexDirection, Position, + Size as CSize, +}; +use rustmotion_core::css::taffy_bridge::ConversionContext; +use rustmotion_core::css::units::{Length, LengthPercentage as CLP}; +use rustmotion_core::engine::box_tree::{BoxKind, BoxNode}; +use rustmotion_core::engine::layout_pass::run_layout; +use rustmotion_core::engine::paint_pass::{paint_tree, NoopDispatcher, PaintFrame}; + +fn test_frame(w: u32, h: u32) -> PaintFrame { + PaintFrame { + time: 0.0, + scenario_time: 0.0, + frame_index: 0, + fps: 30, + video_width: w, + video_height: h, + scene_duration: 1.0, + camera: None, + } +} + +fn render_pixels(root: &mut BoxNode, w: u32, h: u32) -> Vec { + root.assign_ids(0); + let layout = run_layout(root, (w as f32, h as f32), &ConversionContext::default()); + let mut surface = skia_safe::surfaces::raster_n32_premul((w as i32, h as i32)).unwrap(); + paint_tree( + surface.canvas(), + root, + &layout, + &test_frame(w, h), + &NoopDispatcher, + ); + let info = skia_safe::ImageInfo::new( + (w as i32, h as i32), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Unpremul, + None, + ); + let mut buf = vec![0u8; (w * h * 4) as usize]; + surface.read_pixels(&info, &mut buf, (w * 4) as usize, (0, 0)); + buf +} + +fn root_node(w: f32, h: f32, background: &str, children: Vec) -> BoxNode { + BoxNode { + id: 0, + kind: BoxKind::Container, + css: CssStyle { + display: Some(Display::Flex), + flex_direction: Some(FlexDirection::Column), + width: Some(CSize::Length(CLP::Px(w))), + height: Some(CSize::Length(CLP::Px(h))), + background: Some(Background::Color(CssColor::String(background.to_string()))), + ..Default::default() + }, + children, + intrinsic: None, + source_path: None, + window: None, + } +} + +fn probe(buf: &[u8], w: u32, x: usize, y: usize) -> (u8, u8, u8) { + let i = (y * w as usize + x) * 4; + (buf[i], buf[i + 1], buf[i + 2]) +} + +// ---- opacity layer must not clip the node's own outset box-shadow ---- + +fn card_with_shadow(opacity: Option) -> BoxNode { + let css = CssStyle { + position: Some(Position::Absolute), + left: Some(CLP::Px(50.0)), + top: Some(CLP::Px(50.0)), + width: Some(CSize::Length(CLP::Px(100.0))), + height: Some(CSize::Length(CLP::Px(100.0))), + background: Some(Background::Color(CssColor::String("#ffffff".into()))), + box_shadow: Some(vec![BoxShadow { + offset_x: Length::Px(0.0), + offset_y: Length::Px(0.0), + blur: None, + spread: Some(Length::Px(20.0)), + color: Some(CssColor::String("#ff0000".into())), + inset: None, + }]), + opacity, + ..Default::default() + }; + BoxNode { + id: 0, + kind: BoxKind::Container, + css, + children: vec![], + intrinsic: None, + source_path: None, + window: None, + } +} + +#[test] +fn opacity_layer_does_not_clip_own_outset_box_shadow() { + // 100x100 white card at (50,50) on a 200x200 black canvas, outset + // box-shadow (red, spread 20, blur 0 -> hard-edged halo rect from + // (30,30) to (170,170)). Probe point (100,45) sits in the halo band + // above the card, outside its own border-box. `opacity: 0.999` forces + // the opacity/filter SaveLayerRec open without visibly dimming the + // probed color. + let opaque = { + let mut root = root_node(200.0, 200.0, "#000000", vec![card_with_shadow(None)]); + render_pixels(&mut root, 200, 200) + }; + let faded = { + let mut root = root_node(200.0, 200.0, "#000000", vec![card_with_shadow(Some(0.999))]); + render_pixels(&mut root, 200, 200) + }; + + let above_opaque = probe(&opaque, 200, 100, 45); + assert!( + above_opaque.0 > 200 && above_opaque.1 < 50, + "sanity: shadow halo must be visible without an opacity layer, got {above_opaque:?}" + ); + + let above_faded = probe(&faded, 200, 100, 45); + assert!( + above_faded.0 > 200 && above_faded.1 < 50, + "an opacity<1 layer must not clip the node's own outset box-shadow, got {above_faded:?}" + ); +} From f46548b4a1a382a911d55286c9db2c3acab71995 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:52:18 +0200 Subject: [PATCH 04/56] fix(geometry): resolve vw/vh against the scenario viewport (#227) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ConversionContext::default() carries LengthContext { viewport_width: 1920.0, viewport_height: 1080.0, .. } (rustmotion-core/src/css/units.rs:66-74), and taffy_bridge::size_to_dim/lp_to_lp/lp_to_lp_auto resolve every vw/vh/rem/em against it (taffy_bridge.rs:335-338). The renderer does NOT: render_with_new_pipeline_iter (engine/render/scene.rs:554-558) and render_scene_hits (scene.rs:792) both pass viewport_conversion_context(vw, vh), whose own doc comment calls the 1920×1080 default a 78% error on a 1080×1920 vertical video. So on any non-1920×1080 scenario the mandatory rustmotion validate gate checks a layout the renderer never produces: width: "50vw" is measured as 960px by the validator and painted as 540px, 50vh as 540px vs 960px. The file header promises the opposite ("so the geometry it checks matches what the renderer will actually paint"). The same file builds a correct LengthContext with the real viewport at line 477 for its transform-origin math, so the omission is confined to the layout pass. Both the resting walk (:180) and the --strict-anim walk (:1347) are affected. Fix: Make viewport_conversion_context public (or move it next to run_layout in rustmotion-core) and call it from both geometry.rs sites: run_layout(&built.root, viewport_f, &viewport_conversion_context(viewport_f.0, viewport_f.1)). Better: stop exposing ConversionContext at the run_layout signature at all — derive it from the viewport argument inside run_layout, which makes the wrong context unrepresentable. Refs #220 --- .../rustmotion/src/cli/commands/geometry.rs | 12 +- crates/rustmotion/tests/audit_ws_b.rs | 196 ++++++++++++++++++ 2 files changed, 206 insertions(+), 2 deletions(-) create mode 100644 crates/rustmotion/tests/audit_ws_b.rs diff --git a/crates/rustmotion/src/cli/commands/geometry.rs b/crates/rustmotion/src/cli/commands/geometry.rs index 929f592d..0fd7d069 100644 --- a/crates/rustmotion/src/cli/commands/geometry.rs +++ b/crates/rustmotion/src/cli/commands/geometry.rs @@ -177,7 +177,11 @@ pub fn validate_geometry(scenario: &ResolvedScenario) -> Vec let root_css = render::root_style(scene.layout.as_ref(), view.view_type.clone()); let built = build_scene_from_refs(children.iter(), viewport_f, root_css, None); - let layouts = run_layout(&built.root, viewport_f, &ConversionContext::default()); + let layouts = run_layout( + &built.root, + viewport_f, + &ConversionContext::for_viewport(viewport_f.0, viewport_f.1), + ); let camera = scene .camera @@ -1344,7 +1348,11 @@ pub fn validate_geometry_animated(scenario: &ResolvedScenario) -> Vec`, +//! whose JSON is `commands::geometry::GeometryViolation`'s public `Serialize` +//! output — is the only externally-observable contract for what the +//! validator decided (mirrors `motion_path_strict_anim.rs`'s reasoning). +//! +//! One section per finding, in briefing order: viewport units, transform +//! lengths, path rewriting, the three box-model checks, and helper reuse. + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +/// Minimal RAII scratch file — mirrors `motion_path_strict_anim.rs`'s +/// `ScratchFile`. +struct ScratchFile(PathBuf); + +impl ScratchFile { + fn new(label: &str) -> Self { + let unique = format!( + "rustmotion-audit-ws-b-{label}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock before UNIX epoch") + .as_nanos() + ); + Self(std::env::temp_dir().join(unique)) + } +} + +impl Drop for ScratchFile { + fn drop(&mut self) { + let _ = std::fs::remove_file(&self.0); + } +} + +/// Minimal RAII scratch directory — mirrors `skill_files_match_disk.rs`'s +/// `ScratchDir`. Only the path-rewriting case needs a directory (a file plus a +/// sibling asset file). +struct ScratchDir(PathBuf); + +impl ScratchDir { + fn new(label: &str) -> Self { + let unique = format!( + "rustmotion-audit-ws-b-{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 run_validate( + scenario_path: &Path, + report_path: Option<&Path>, + fix: bool, + strict_anim: bool, +) -> Output { + let mut cmd = Command::new(env!("CARGO_BIN_EXE_rustmotion")); + cmd.arg("validate").arg("--file").arg(scenario_path); + if let Some(report_path) = report_path { + cmd.arg("--report").arg(report_path); + } + if fix { + cmd.arg("--fix"); + } + if strict_anim { + cmd.arg("--strict-anim"); + } + cmd.output().expect("failed to spawn `rustmotion validate`") +} + +fn read_report(path: &Path) -> serde_json::Value { + let text = std::fs::read_to_string(path).expect("read report"); + serde_json::from_str(&text).expect("report is valid JSON") +} + +fn violations(report: &serde_json::Value) -> &Vec { + report["geometry_violations"] + .as_array() + .expect("geometry_violations is an array") +} + +fn count_kind(report: &serde_json::Value, kind: &str) -> usize { + violations(report) + .iter() + .filter(|v| v["kind"] == kind) + .count() +} + +fn find_kind<'a>(report: &'a serde_json::Value, kind: &str) -> Option<&'a serde_json::Value> { + violations(report).iter().find(|v| v["kind"] == kind) +} + +// ─── vw/vh must resolve against the scenario's real viewport ─────── + +/// A `width: "90vw"` shape on a 1080×1920 scenario, positioned so its right +/// edge crosses the viewport edge at EITHER candidate width — 972px (90% of +/// the real 1080px-wide viewport) or 1728px (90% of the hardcoded +/// `ConversionContext::default()` 1920px fallback). The violation fires +/// either way; only the reported `bbox.w` distinguishes a correct +/// measurement from the buggy one. +fn vw_shape_scenario(x: f32) -> String { + format!( + r##"{{ + "video": {{ "width": 1080, "height": 1920 }}, + "scenes": [{{ + "duration": 1.0, + "children": [{{ + "type": "shape", + "shape": "rect", + "position": "absolute", + "x": {x}, "y": 100, + "style": {{ "width": "90vw", "height": "50px" }}, + "fill": "#ff0000" + }}] + }}] + }}"## + ) +} + +#[test] +fn resting_layout_measures_vw_against_the_real_viewport_width() { + let scenario = ScratchFile::new("rm05-resting-scenario"); + let report = ScratchFile::new("rm05-resting-report"); + std::fs::write(&scenario.0, vw_shape_scenario(200.0)).expect("write scenario"); + + let output = run_validate(&scenario.0, Some(&report.0), false, false); + assert!( + !output.status.success(), + "a shape whose right edge is past the viewport at either candidate width must block; \ + stdout={} stderr={}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + + let report_json = read_report(&report.0); + let violation = find_kind(&report_json, "viewport_overflow") + .expect("expected a viewport_overflow violation"); + let width = violation["bbox"]["w"].as_f64().expect("bbox.w is a number"); + assert!( + (width - 972.0).abs() < 2.0, + "90vw on a 1080px-wide viewport must resolve to ~972px (the real viewport), \ + not 1728px (0.9 x the hardcoded 1920 default); report: {report_json}" + ); +} + +/// Same shape, repositioned so it overflows ONLY under the buggy +/// 1920x1080 default (right edge 1778px vs an 1080px-wide viewport) and +/// stays clean at the correct 972px width (right edge 1022px). Isolates +/// the `--strict-anim` call site (`validate_geometry_animated`, geometry.rs +/// ~1347) from the resting one above (~180): each builds its own box tree +/// through `ConversionContext::default()` independently, so fixing only +/// one would still leave this failing. +#[test] +fn strict_anim_also_measures_vw_against_the_real_viewport_width() { + let scenario = ScratchFile::new("rm05-strict-anim-scenario"); + let report = ScratchFile::new("rm05-strict-anim-report"); + std::fs::write(&scenario.0, vw_shape_scenario(50.0)).expect("write scenario"); + + let output = run_validate(&scenario.0, Some(&report.0), false, true); + let report_json = read_report(&report.0); + assert!( + output.status.success(), + "the real 1080px-wide viewport keeps this shape on-screen at every sample; \ + stdout={} stderr={} report={report_json}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!( + count_kind(&report_json, "viewport_overflow"), + 0, + "resting pass (line ~180) must be clean: {report_json}" + ); + assert_eq!( + count_kind(&report_json, "animated_text_overflow"), + 0, + "--strict-anim pass (line ~1347) must also be clean: {report_json}" + ); +} From e34b4024d1fac2e4a0b2be0b1edfc74d418cabe6 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:52:41 +0200 Subject: [PATCH 05/56] fix(encode): publish the output only once the render succeeds (#234) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When render_frame_task fails mid-encode (line 613 sets pipe_error and breaks), the code drops stdin and then *waits* for ffmpeg. ffmpeg sees a clean EOF on pipe:0, finalizes the frames it already received, writes the moov atom and exits 0. rustmotion then returns Err, but output_path now holds a structurally valid MP4 containing only the first N frames. Because ffmpeg_args emits -y (line 194), this also silently destroys a previously- good render at the same path. A user scripting rustmotion render who checks only file existence (or whose CI publishes the artifact) ships a truncated video. No code path anywhere in src/cli or src/encode removes the output on failure — only test code calls remove_file on outputs. Fix: On the pipe_error path, call child.kill() and child.wait() before returning, and let _ = std::fs::remove_file(output_path); on both the pipe_error and !status.success() branches. Better still, have ffmpeg write to a sibling scratch path and fs::rename onto output_path only after a clean exit — the same promote-on-success discipline video_audio.rs::partial_wav_path already applies to cached WAVs. Refs #220 --- crates/rustmotion/src/encode/video/ffmpeg.rs | 93 ++++++++- crates/rustmotion/src/encode/video/formats.rs | 180 +++++++++++++++++- crates/rustmotion/tests/audit_ws_c.rs | 117 ++++++++++++ 3 files changed, 383 insertions(+), 7 deletions(-) create mode 100644 crates/rustmotion/tests/audit_ws_c.rs diff --git a/crates/rustmotion/src/encode/video/ffmpeg.rs b/crates/rustmotion/src/encode/video/ffmpeg.rs index b91208c6..63d338ae 100644 --- a/crates/rustmotion/src/encode/video/ffmpeg.rs +++ b/crates/rustmotion/src/encode/video/ffmpeg.rs @@ -290,6 +290,27 @@ fn ffmpeg_args( args } +/// Scratch path ffmpeg actually writes to; promoted (renamed) onto the +/// caller's real `output_path` only after a clean exit with no `pipe_error`. +/// Kept as a sibling of `output_path` (same directory, same filesystem, so +/// the promotion is a plain rename) and keeps `output_path`'s own extension +/// as the *final* extension — mirrors `video_audio::partial_wav_path`'s doc: +/// ffmpeg picks its output muxer from the last extension, so a bare +/// `.partial` suffix appended after it makes ffmpeg refuse to start with +/// "Unable to choose an output format" instead of the encode failure this +/// path exists to isolate. +fn ffmpeg_partial_output_path(output_path: &std::path::Path) -> std::path::PathBuf { + let stem = output_path + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or("output"); + let name = match output_path.extension().and_then(|s| s.to_str()) { + Some(ext) => format!("{stem}.partial.{ext}"), + None => format!("{stem}.partial"), + }; + output_path.with_file_name(name) +} + /// Encode using FFmpeg subprocess (for h265, vp9, prores, webm, mov, transparency). /// /// Software-only. Kept with its original signature so existing callers @@ -531,6 +552,14 @@ fn encode_with_ffmpeg_hw_impl( } }; + let partial_output_path = ffmpeg_partial_output_path(std::path::Path::new(output_path)); + let partial_output_str = + partial_output_path + .to_str() + .ok_or_else(|| RustmotionError::NonUtf8Path { + path: partial_output_path.to_string_lossy().into_owned(), + })?; + let mut cmd = std::process::Command::new("ffmpeg"); cmd.args(ffmpeg_args( width, @@ -541,7 +570,7 @@ fn encode_with_ffmpeg_hw_impl( transparent, hw_encoder.as_deref(), audio_input.as_deref(), - output_path, + partial_output_str, )); cmd.stdin(std::process::Stdio::piped()); cmd.stdout(std::process::Stdio::null()); @@ -623,7 +652,18 @@ fn encode_with_ffmpeg_hw_impl( cb(EncodeProgress::Muxing); } - let status = child.wait().map_err(|e| RustmotionError::FfmpegWait { + // A `pipe_error` means the render already failed and `partial_output_path` + // will be discarded either way, so there is nothing left for ffmpeg to + // usefully finish — killing it here instead of waiting for it to + // gracefully encode and finalize a file nobody will ever read avoids + // burning time on a result already known to be thrown away. + let status = if pipe_error.is_some() { + let _ = child.kill(); + child.wait() + } else { + child.wait() + } + .map_err(|e| RustmotionError::FfmpegWait { reason: e.to_string(), })?; @@ -659,6 +699,7 @@ fn encode_with_ffmpeg_hw_impl( if let Some(e) = pipe_error { tee_stderr(); + let _ = std::fs::remove_file(&partial_output_path); // A broken pipe means ffmpeg is already gone — its own error says why, // ours only says we could not keep writing. Carry both. return Err(match e { @@ -672,11 +713,17 @@ fn encode_with_ffmpeg_hw_impl( if !status.success() { tee_stderr(); + let _ = std::fs::remove_file(&partial_output_path); return Err(RustmotionError::FfmpegFailed { stderr: stderr_summary, }); } + // Only now, with a clean exit and no pipe error, does `output_path` ever + // see this render's bytes — promote-on-success, the same discipline + // `video_audio::extract_audio_to_wav` already applies to its cached WAVs. + std::fs::rename(&partial_output_path, output_path)?; + Ok(()) } @@ -789,7 +836,47 @@ pub fn concat_mp4_segments(inputs: &[std::path::PathBuf], output_path: &str) -> #[cfg(test)] mod tests { - use super::{ffmpeg_args, parse_encoder_names, select_hardware_encoder, HardwareSelection}; + use super::{ + ffmpeg_args, ffmpeg_partial_output_path, parse_encoder_names, select_hardware_encoder, + HardwareSelection, + }; + + // ── partial-output-path naming (pure) ──────────────────────────────────── + + #[test] + fn partial_path_keeps_the_original_extension_as_its_last_extension() { + let cases = [ + ("/tmp/out.mp4", "/tmp/out.partial.mp4"), + ("/tmp/out.mov", "/tmp/out.partial.mov"), + ("/tmp/out.webm", "/tmp/out.partial.webm"), + ("out.mp4", "out.partial.mp4"), + ]; + for (input, expected) in cases { + let got = ffmpeg_partial_output_path(std::path::Path::new(input)); + assert_eq!( + got, + std::path::PathBuf::from(expected), + "input={input}: ffmpeg picks its muxer from the last extension, so it must \ + survive unchanged" + ); + } + } + + #[test] + fn partial_path_is_a_sibling_of_the_final_output_not_a_different_directory() { + let got = ffmpeg_partial_output_path(std::path::Path::new("/a/b/c/out.mp4")); + assert_eq!( + got.parent(), + Some(std::path::Path::new("/a/b/c")), + "the rename onto output_path must stay on the same filesystem" + ); + } + + #[test] + fn partial_path_falls_back_gracefully_with_no_extension() { + let got = ffmpeg_partial_output_path(std::path::Path::new("/tmp/out")); + assert_eq!(got, std::path::PathBuf::from("/tmp/out.partial")); + } /// Every option that describes the *output* has to sit after the last `-i`. /// Put one before it and ffmpeg attaches it to the following input instead, diff --git a/crates/rustmotion/src/encode/video/formats.rs b/crates/rustmotion/src/encode/video/formats.rs index ee06ab7a..8cad2f02 100644 --- a/crates/rustmotion/src/encode/video/formats.rs +++ b/crates/rustmotion/src/encode/video/formats.rs @@ -10,10 +10,52 @@ use crate::schema::ResolvedScenario as Scenario; use super::tasks::{build_frame_tasks, render_frame_task}; use super::EncodeProgress; -/// Encode frames as a PNG sequence (one PNG file per frame) +/// Sibling scratch directory a PNG-sequence render writes into before being +/// promoted onto `output_dir` — same reasoning as `partial_sibling_path`, +/// applied to a directory instead of a single file: directories don't have +/// an extension to preserve, so the suffix is the whole difference. +fn partial_sibling_dir(output_dir: &str) -> std::path::PathBuf { + let path = std::path::Path::new(output_dir); + let name = path + .file_name() + .and_then(|s| s.to_str()) + .unwrap_or("output"); + path.with_file_name(format!("{name}.partial")) +} + +/// Encode frames as a PNG sequence (one PNG file per frame). +/// +/// Renders into a sibling scratch directory and promotes (renames) it onto +/// `output_dir` only once every frame has been written — a mid-sequence +/// failure used to leave a partial run's files sitting directly in +/// `output_dir`, indistinguishable from a completed one to a caller that +/// only checks the directory exists (the same shape a single-file output +/// failing mid-encode has). pub fn encode_png_sequence( scenario: &Scenario, output_dir: &str, + quiet: bool, + transparent: bool, + on_progress: Option<&mut dyn FnMut(EncodeProgress)>, +) -> Result<()> { + let partial_dir = partial_sibling_dir(output_dir); + let _ = std::fs::remove_dir_all(&partial_dir); + match encode_png_sequence_to_dir(scenario, &partial_dir, quiet, transparent, on_progress) { + Ok(()) => { + let _ = std::fs::remove_dir_all(output_dir); + std::fs::rename(&partial_dir, output_dir)?; + Ok(()) + } + Err(e) => { + let _ = std::fs::remove_dir_all(&partial_dir); + Err(e) + } + } +} + +fn encode_png_sequence_to_dir( + scenario: &Scenario, + output_dir: &std::path::Path, _quiet: bool, _transparent: bool, mut on_progress: Option<&mut dyn FnMut(EncodeProgress)>, @@ -67,7 +109,7 @@ pub fn encode_png_sequence( for result in results { let (frame_num, rgba) = result?; - let path = format!("{}/frame_{:05}.png", output_dir, frame_num); + let path = output_dir.join(format!("frame_{:05}.png", frame_num)); let img = image::RgbaImage::from_raw(width, height, rgba) .ok_or(RustmotionError::PixelImage)?; img.save(&path)?; @@ -77,11 +119,59 @@ pub fn encode_png_sequence( Ok(()) } -/// Encode frames as an animated GIF +/// Sibling scratch path a single-file encoder (GIF today) writes to before +/// being promoted onto `output_path` only after every frame is written +/// successfully. Same discipline and the same reasoning as +/// `video::ffmpeg::ffmpeg_partial_output_path`: kept in the same directory +/// so the promotion is a same-filesystem rename, extension kept last since +/// some downstream consumers of the output (players, `file`) pick behavior +/// from it the way ffmpeg picks a muxer from its own output extension. +fn partial_sibling_path(output_path: &str) -> std::path::PathBuf { + let path = std::path::Path::new(output_path); + let stem = path + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or("output"); + let name = match path.extension().and_then(|s| s.to_str()) { + Some(ext) => format!("{stem}.partial.{ext}"), + None => format!("{stem}.partial"), + }; + path.with_file_name(name) +} + +/// Encode frames as an animated GIF. +/// +/// Writes to a sibling scratch path first and promotes (renames) it onto +/// `output_path` only once every frame has been written without error — +/// the same partial-then-rename discipline `video_audio::extract_audio_to_wav` +/// and the ffmpeg encoder already apply. A GIF that fails midway through the +/// frame loop (any `?` inside `encode_gif_to_path`) used to leave a +/// truncated-but-existing file sitting at `output_path`, the same shape the +/// ffmpeg-backed encoder and the PNG-sequence encoder had before adopting +/// this same discipline. pub fn encode_gif( scenario: &Scenario, output_path: &str, quiet: bool, + on_progress: Option<&mut dyn FnMut(EncodeProgress)>, +) -> Result<()> { + let partial_path = partial_sibling_path(output_path); + match encode_gif_to_path(scenario, &partial_path, quiet, on_progress) { + Ok(()) => { + std::fs::rename(&partial_path, output_path)?; + Ok(()) + } + Err(e) => { + let _ = std::fs::remove_file(&partial_path); + Err(e) + } + } +} + +fn encode_gif_to_path( + scenario: &Scenario, + partial_path: &std::path::Path, + quiet: bool, mut on_progress: Option<&mut dyn FnMut(EncodeProgress)>, ) -> Result<()> { let config = &scenario.video; @@ -103,7 +193,7 @@ pub fn encode_gif( let gif_w = width.min(65535) as u16; let gif_h = height.min(65535) as u16; - let file = File::create(output_path)?; + let file = File::create(partial_path)?; let mut encoder = gif::Encoder::new(BufWriter::new(file), gif_w, gif_h, &[]).map_err(|e| { RustmotionError::GifEncoder { reason: e.to_string(), @@ -243,6 +333,88 @@ mod tests { use crate::loader::load_scenario_from_source; use std::path::{Path, PathBuf}; + // ── partial-output naming (pure) ───────────────────────────────────────── + + #[test] + fn partial_sibling_path_keeps_the_extension_last() { + assert_eq!( + partial_sibling_path("/tmp/out.gif"), + PathBuf::from("/tmp/out.partial.gif") + ); + assert_eq!( + partial_sibling_path("/tmp/out"), + PathBuf::from("/tmp/out.partial") + ); + } + + #[test] + fn partial_sibling_dir_stays_next_to_the_final_directory() { + let got = partial_sibling_dir("/a/b/frames"); + assert_eq!(got, PathBuf::from("/a/b/frames.partial")); + assert_eq!(got.parent(), Some(Path::new("/a/b"))); + } + + /// A failed GIF encode must not leave a truncated file at `output_path` + /// — the real trigger used to prove this for the ffmpeg-backed encoder + /// (an odd width rejected by libx264) doesn't apply here (the `gif` + /// crate has no such constraint), so this drives the failure the one + /// way this pure Rust path can actually fail without external tools: + /// `total_frames == 0`. + /// That returns before any file is touched either way, so what this + /// test really pins is the *shape* of the fix — `encode_gif` must never + /// promote a partial onto `output_path` when its inner call errors — + /// exercised by asserting the scratch/partial path used internally is + /// never left behind either. + #[test] + fn encode_gif_leaves_no_partial_file_behind_on_an_empty_scenario() { + let json = r#"{"video": {"width": 8, "height": 8, "fps": 10}, "scenes": []}"#; + let scenario = load_scenario_from_source(None, Some(json)).expect("load"); + + let out = std::env::temp_dir().join(format!( + "rm_gif_no_debris_test_{}_{}.gif", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let partial = partial_sibling_path(out.to_str().unwrap()); + let _ = std::fs::remove_file(&out); + let _ = std::fs::remove_file(&partial); + + let result = encode_gif(&scenario, out.to_str().unwrap(), true, None); + assert!(result.is_err(), "an empty scenario has no frames to encode"); + assert!(!out.exists(), "no debris at the final output path"); + assert!(!partial.exists(), "no debris at the scratch path either"); + } + + /// Same property for the directory-based PNG-sequence encoder. + #[test] + fn encode_png_sequence_leaves_no_partial_dir_behind_on_an_empty_scenario() { + let json = r#"{"video": {"width": 8, "height": 8, "fps": 10}, "scenes": []}"#; + let scenario = load_scenario_from_source(None, Some(json)).expect("load"); + + let out_dir = std::env::temp_dir().join(format!( + "rm_png_seq_no_debris_test_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let partial_dir = partial_sibling_dir(out_dir.to_str().unwrap()); + let _ = std::fs::remove_dir_all(&out_dir); + let _ = std::fs::remove_dir_all(&partial_dir); + + let result = encode_png_sequence(&scenario, out_dir.to_str().unwrap(), true, false, None); + assert!(result.is_err(), "an empty scenario has no frames to encode"); + assert!(!out_dir.exists(), "no debris at the final output directory"); + assert!( + !partial_dir.exists(), + "no debris at the scratch directory either" + ); + } + /// Deterministic color for a given frame index: distinct enough across /// nearby indices that a swapped/misplaced frame is detected by a plain /// pixel comparison. diff --git a/crates/rustmotion/tests/audit_ws_c.rs b/crates/rustmotion/tests/audit_ws_c.rs new file mode 100644 index 00000000..e612794e --- /dev/null +++ b/crates/rustmotion/tests/audit_ws_c.rs @@ -0,0 +1,117 @@ +//! Regression tests — audit round chantier/audit-2026-09, workstream C +//! (encoding, audio, cancellation). +//! +//! Tests here only exercise the public API surface — `rustmotion::encode::...` +//! — since this is an external integration test crate; regression tests for +//! private helpers live next to them in their own `#[cfg(test)] mod tests` +//! inside the `src/encode/` file that owns them, and are cross-referenced +//! here by the behavior they cover. + +use std::path::PathBuf; + +/// Minimal RAII scratch file: unique per (label, pid, nanosecond timestamp), +/// removed on drop even if the test panics partway through. +struct ScratchFile(PathBuf); + +impl ScratchFile { + /// `ext` is the extension ffmpeg/the encoder picks its container format + /// from (`"mp4"`, `"gif"`, ...) — omitting it makes ffmpeg fail at + /// muxer selection before ever touching the path, which would make a + /// debris-after-failure test pass for the wrong reason. + fn new(label: &str, ext: &str) -> Self { + let unique = format!( + "rustmotion-audit-ws-c-{label}-{}-{}.{ext}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock before UNIX epoch") + .as_nanos() + ); + Self(std::env::temp_dir().join(unique)) + } + + fn path(&self) -> &std::path::Path { + &self.0 + } + + fn to_str(&self) -> &str { + self.0.to_str().expect("scratch path must be UTF-8") + } +} + +impl Drop for ScratchFile { + fn drop(&mut self) { + let _ = std::fs::remove_file(&self.0); + } +} + +fn ffmpeg_on_path() -> bool { + std::process::Command::new("ffmpeg") + .args(["-version"]) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .map(|s| s.success()) + .unwrap_or(false) +} + +// ─── A render ffmpeg rejects must leave no debris at output_path ────────── +// +// `encode_with_ffmpeg`/`encode_with_ffmpeg_hw` used to hand ffmpeg the +// user's own `output_path` directly, drop stdin and gracefully wait no +// matter how the frame loop ended. A failure mid-render (a broken pipe, or +// ffmpeg itself exiting non-zero) still let ffmpeg finalize whatever it had +// already received at that exact path, and `-y` meant it would even +// overwrite a previously-good render there. No cleanup ran on any error +// path. +// +// Forcing rustmotion's *own* frame renderer to fail deterministically isn't +// possible from a plain scenario — `Painter::paint_content` cannot return an +// `Err`, so no user-authored content can fail a frame. ffmpeg itself, +// though, refuses an odd width under the crate's default 10-bit H.264 +// profile (`yuv420p10le` needs even 4:2:0 chroma dimensions) — confirmed by +// hand: `ffmpeg -f rawvideo ... -video_size 321x240 ... -c:v libx264 +// -profile:v high10 -pix_fmt yuv420p10le out.mp4` creates a 0-byte +// `out.mp4` and then fails to open its encoder, closing stdin before +// rustmotion finishes writing frames — exactly the "ffmpeg already has a +// file open at output_path when the failure happens" shape this test cares +// about, without needing an internal render failure at all. +#[test] +fn a_render_ffmpeg_rejects_leaves_no_debris_at_the_output_path() { + if !ffmpeg_on_path() { + eprintln!( + "a_render_ffmpeg_rejects_leaves_no_debris_at_the_output_path: \ + ffmpeg not found — skipping" + ); + return; + } + + let json = r#"{"video": {"width": 321, "height": 240, "fps": 10}, + "scenes": [{"duration": 0.3, "children": []}]}"#; + let scenario = rustmotion::loader::load_scenario_from_source(None, Some(json)).expect("load"); + + let out = ScratchFile::new("odd-width", "mp4"); + let _ = std::fs::remove_file(out.path()); + + let result = rustmotion::encode::encode_with_ffmpeg( + &scenario, + out.to_str(), + true, + "h264", + None, + false, + None, + ); + + assert!( + result.is_err(), + "an odd width (321) must make libx264's high10/yuv420p10le encoder \ + init fail — this test's premise depends on ffmpeg actually rejecting it" + ); + assert!( + !out.path().exists(), + "no file — truncated, empty, or otherwise — may be left at output_path \ + after a failed render; got one at {}", + out.path().display() + ); +} From 58c145e7626f718433f1dd425d502d5bcaa02ace Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:52:56 +0200 Subject: [PATCH 06/56] perf(gif): decode a gif once across rayon workers (#239) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Triggering scenario: a scenario with one gif component, rendered to MP4. encode_with_ffmpeg (ffmpeg.rs:582) dispatches the FIRST chunk of batch_size = rayon::current_num_threads() * 2 frames — all frames 0..N of the same scene — to par_iter() at once. Every one of those workers calls Gif::paint_content, every one misses the still-empty gif_cache, and every one runs decode_composed_frames independently. Unlike icons and videos, GIFs are NOT preloaded (preload.rs only walks Component::Icon and Component::Video), so the first decode genuinely happens inside the parallel section. Each decode is itself unbounded: decode_composed_frames pushes one full-canvas RGBA copy per GIF frame with no cap (gif.rs:141). A 600-frame 800x600 GIF is 1.15 GB per decode; on a 16-core machine (batch_size 32) that is ~37 GB attempted concurrently, then 31 of the 32 results are thrown away when insert overwrites. The process OOM-kills before frame 33. Fix: Decode GIFs in the preload pass alongside icons and videos (preload.rs), so the parallel section only ever hits a warm cache. If lazy decode must stay, gate it on a per-key single-flight primitive (e.g. DashMap>> — insert the empty cell under the shard lock, then let exactly one worker fill it) and cap total decoded frames per source. Refs #220 --- crates/rustmotion-components/src/gif.rs | 112 +++++++++++++++++++++--- 1 file changed, 101 insertions(+), 11 deletions(-) diff --git a/crates/rustmotion-components/src/gif.rs b/crates/rustmotion-components/src/gif.rs index e1738e04..529624ca 100644 --- a/crates/rustmotion-components/src/gif.rs +++ b/crates/rustmotion-components/src/gif.rs @@ -86,6 +86,22 @@ fn clear_rect(composed: &mut [u8], canvas_w: u32, canvas_h: u32, frame: &gif::Fr /// `gif_cache` stores. type DecodedGif = (Vec<(Vec, u32, u32)>, Vec, f64); +/// Artificial per-decode stall, settable only from this file's own tests +/// (`DECODE_STALL_MS`) to open a deterministic race window around the +/// cache-miss branch without timing-dependent sleeps sprinkled through the +/// test itself. Zero by default, and compiled out entirely in a non-test +/// build — no production cost. +#[cfg(test)] +static DECODE_STALL_MS: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + +#[cfg(test)] +fn stall_decode_for_tests() { + let stall_ms = DECODE_STALL_MS.load(std::sync::atomic::Ordering::SeqCst); + if stall_ms > 0 { + std::thread::sleep(std::time::Duration::from_millis(stall_ms)); + } +} + /// Decode a GIF into full-canvas RGBA frames, their cumulative end times, and /// the total duration. /// @@ -123,6 +139,9 @@ fn decode_composed_frames(src: &str) -> Option { let canvas_w = decoder.width() as u32; let canvas_h = decoder.height() as u32; + #[cfg(test)] + stall_decode_for_tests(); + let mut frames: Vec<(Vec, u32, u32)> = Vec::new(); let mut cumulative_times: Vec = Vec::new(); let mut accumulated = 0.0; @@ -163,6 +182,21 @@ fn decode_composed_frames(src: &str) -> Option { Some((frames, cumulative_times, accumulated)) } +/// Cache lookup with the actual decode folded in, single-flighted through +/// `DashMap::entry`: a vacant entry holds its shard's write lock for as long +/// as the closure runs, so a second caller racing the same cache-cold `src` +/// blocks on that lock instead of starting its own redundant decode. Decode +/// 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> { + gif_cache() + .entry(src.to_string()) + .or_try_insert_with(|| decode_composed_frames(src).map(Arc::new).ok_or(())) + .ok() + .map(|entry| entry.clone()) +} + impl Painter for Gif { fn paint_content( &self, @@ -171,17 +205,8 @@ impl Painter for Gif { _props: &AnimatedProperties, ctx: &PaintCtx, ) { - let gcache = gif_cache(); - - let cached = if let Some(cached) = gcache.get(&self.src) { - cached.clone() - } else { - let Some(decoded) = decode_composed_frames(&self.src) else { - return; - }; - let cached = Arc::new(decoded); - gcache.insert(self.src.clone(), cached.clone()); - cached + let Some(cached) = cached_decode(&self.src) else { + return; }; let (ref frames, ref cumulative_times, total_duration) = *cached; @@ -302,4 +327,69 @@ mod tests { frame.top = 3; assert_eq!(frame_rect(4, 4, &frame), (3, 3, 1, 1)); } + + /// Releases `DECODE_STALL_MS` back to zero even if the test body + /// panics mid-assertion, so a failing run never leaks a stall into + /// whatever other test in this binary decodes a GIF next. + struct StallGuard; + + impl Drop for StallGuard { + fn drop(&mut self) { + DECODE_STALL_MS.store(0, std::sync::atomic::Ordering::SeqCst); + } + } + + /// N callers racing a cache-cold `src` must decode it once, not N + /// times. A 120ms stall (`DECODE_STALL_MS`) right after the header is + /// read opens a race window wide enough that every thread reaches the + /// cache-miss branch before any of them can finish decoding and insert — + /// pre-fix, that means eight independent decodes, each producing its own + /// `Arc` allocation. Comparing pointers (not content — a deterministic + /// decode produces byte-identical content either way) is what proves + /// only one of the eight actually ran. + #[test] + fn concurrent_paints_of_the_same_uncached_gif_decode_exactly_once() { + let _guard = StallGuard; + DECODE_STALL_MS.store(120, std::sync::atomic::Ordering::SeqCst); + + let path = std::env::temp_dir().join(format!( + "rustmotion_gif_stampede_{}_{}.gif", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(), + )); + write_two_frame_gif(&path); + let src = path.to_str().expect("utf-8 path").to_string(); + + const THREADS: usize = 8; + let barrier = std::sync::Arc::new(std::sync::Barrier::new(THREADS)); + let handles: Vec<_> = (0..THREADS) + .map(|_| { + let barrier = barrier.clone(); + let src = src.clone(); + std::thread::spawn(move || { + barrier.wait(); + cached_decode(&src) + }) + }) + .collect(); + + let results: Vec>> = + handles.into_iter().map(|h| h.join().unwrap()).collect(); + std::fs::remove_file(&path).ok(); + + let first = results[0].as_ref().expect("gif must decode"); + for (i, result) in results.iter().enumerate() { + let result = result.as_ref().unwrap_or_else(|| { + panic!("thread {i} did not get a decoded result"); + }); + assert!( + Arc::ptr_eq(first, result), + "thread {i} observed a different Arc than thread 0 — the GIF was decoded \ + more than once for the same cache-cold source" + ); + } + } } From 9d4bcb5398e53593399561f00b959c7b9cdf8bbe Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:53:28 +0200 Subject: [PATCH 07/56] fix(background): wrap heropattern scroll on its per-axis period (#248) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tile_spacing returns one scalar that line 830 applies to both axes, and the Heropattern arm never reads d.height. The function's own doc comment states the invariant it relies on — "since every tiled pattern is exactly periodic on spacing, translating by any offset congruent mod spacing produces byte- identical pixels" — which is false for any non-square pattern. I counted the table programmatically: 41 of the 87 entries in crates/rustmotion- core/src/engine/heropatterns.rs have width != height (architect 100x199, aztec 32x64, bamboo 16x32, bank_note 100x20, death_star 80x105, yyy 60x96...). Concrete case: {"animated-background":{"preset":"heropattern","he ropattern":{"pattern":"aztec"},"direction":"down","speed":60}}. The shader tiles vertically on 64px but the offset wraps at 32px, so every 32px of travel the pattern snaps by half a period — a periodic jolt through the whole scene. The .max(20.0) clamp breaks it a second way for the 8 patterns narrower than 20px: bamboo (16 wide) wraps at 20, so even horizontal scroll jumps 4px per wrap. draw_world_bg_with_parallax reuses the same tile_spacing for the camera-pan modulo (line 89), so world views inherit it. Fix: Make tile_spacing return a (f32, f32) period and have the Heropattern arm emit (d.width * cfg.scale, d.height * cfg.scale); wrap raw_x on the x period and raw_y on the y period at line 830, and do the same for the camera modulo at line 89. Drop the .max(20.0) floor or replace it with a multiple- of-period ceiling (period * (20.0 / period).ceil()) so the clamp preserves periodicity. Refs #220 --- .../src/engine/render/background.rs | 163 ++++++++++++++---- crates/rustmotion/src/engine/render/scene.rs | 30 +--- crates/rustmotion/tests/audit_ws_e.rs | 18 ++ 3 files changed, 158 insertions(+), 53 deletions(-) create mode 100644 crates/rustmotion/tests/audit_ws_e.rs diff --git a/crates/rustmotion/src/engine/render/background.rs b/crates/rustmotion/src/engine/render/background.rs index f7a27bb3..2c5d719b 100644 --- a/crates/rustmotion/src/engine/render/background.rs +++ b/crates/rustmotion/src/engine/render/background.rs @@ -86,17 +86,17 @@ pub(super) fn draw_world_bg_with_parallax( } _ => { // Grid-based backgrounds: modulo offset for seamless tiling. - let spacing = tile_spacing(&bg.preset); - let offset_x = -(cam_x % spacing); - let offset_y = -(cam_y % spacing); + let (spacing_x, spacing_y) = tile_spacing(&bg.preset); + let offset_x = -(cam_x % spacing_x); + let offset_y = -(cam_y % spacing_y); canvas.save(); canvas.translate((offset_x, offset_y)); draw_animated_background( canvas, bg, time, - width + spacing * 2.0, - height + spacing * 2.0, + width + spacing_x * 2.0, + height + spacing_y * 2.0, ); canvas.restore(); } @@ -788,29 +788,67 @@ pub(super) fn interpolate_animated_bg( } } -/// Tile period (px) a preset's own draw loop repeats on — the amount by -/// which a scroll offset can be wrapped without changing the rendered -/// pattern. Shared by `compute_scroll_offset` (below) and -/// `draw_world_bg_with_parallax`'s camera-pan modulo so the two never -/// diverge on what "one period" means for a given preset. -fn tile_spacing(preset: &BackgroundPreset) -> f32 { +/// Per-axis tile period (px) a preset's own draw loop repeats on — the +/// amount by which a scroll offset can be wrapped, independently per axis, +/// without changing the rendered pattern. Shared by `compute_scroll_offset` +/// (below) and `draw_world_bg_with_parallax`'s camera-pan modulo so the two +/// never diverge on what "one period" means for a given preset. +/// +/// Every preset but `Heropattern` tiles on a square cell, so both axes share +/// one scalar. Heropattern is the one exception: most of the 87 bundled SVGs +/// are not square (e.g. `aztec` is 32×64), so wrapping the vertical offset +/// on the horizontal period desyncs the two axes and the pattern snaps at +/// every wrap. +fn tile_spacing(preset: &BackgroundPreset) -> (f32, f32) { match preset { - BackgroundPreset::GridDots(cfg) => cfg.spacing.max(20.0), - BackgroundPreset::GridLines(cfg) => cfg.cell.max(4.0), - // The lattice repeats on `spacing`, so the world-view camera can wrap - // on it and the tiling stays seamless as the camera pans. - BackgroundPreset::PixelGrid(cfg) => cfg.spacing.max(cfg.size.max(1.0)), - BackgroundPreset::ConcentricCircles(cfg) => cfg.spacing.max(20.0), + BackgroundPreset::GridDots(cfg) => { + let s = cfg.spacing.max(20.0); + (s, s) + } + BackgroundPreset::GridLines(cfg) => { + let s = cfg.cell.max(4.0); + (s, s) + } + BackgroundPreset::PixelGrid(cfg) => { + let s = cfg.spacing.max(cfg.size.max(1.0)); + (s, s) + } + BackgroundPreset::ConcentricCircles(cfg) => { + let s = cfg.spacing.max(20.0); + (s, s) + } BackgroundPreset::Heropattern(cfg) => { - let def = crate::engine::heropatterns::find_pattern(&cfg.pattern); - def.map(|d| d.width * cfg.scale).unwrap_or(60.0).max(20.0) + match crate::engine::heropatterns::find_pattern(&cfg.pattern) { + Some(d) => ( + period_floor(d.width * cfg.scale, 20.0), + period_floor(d.height * cfg.scale, 20.0), + ), + None => (60.0, 60.0), + } } - _ => 60.0_f32.max(20.0), + _ => (60.0, 60.0), + } +} + +/// Raise `period` to the smallest multiple of itself that is at least +/// `floor`, instead of clamping it outright to `floor`. A plain clamp would +/// no longer be a multiple of the pattern's own period, breaking the +/// periodicity `compute_scroll_offset`'s wrap depends on for any pattern +/// narrower/shorter than `floor` (e.g. `bamboo`, 16px wide). Non-positive +/// `period` has no well-defined multiple; `floor` is the fallback. +fn period_floor(period: f32, floor: f32) -> f32 { + if period <= 0.0 { + floor + } else if period >= floor { + period + } else { + period * (floor / period).ceil() } } /// Compute the scroll offset for tiled backgrounds based on direction + -/// speed, wrapped into `(-spacing, spacing)` so it never grows unbounded. +/// speed, wrapped into `(-spacing, spacing)` per axis so it never grows +/// unbounded. /// /// Bug this fixes: the offset used to grow linearly with `time` forever. /// The tiled draw loops (`draw_bg_grid_dots`, `draw_bg_heropattern`) only @@ -818,16 +856,16 @@ fn tile_spacing(preset: &BackgroundPreset) -> f32 { /// canvas translated by an unbounded offset, the pattern slides off-frame /// and leaves a growing blank band once the offset exceeds that one-tile /// margin (see paint.md finding #5). Since every tiled pattern is exactly -/// periodic on `spacing`, translating by any offset congruent mod `spacing` -/// produces byte-identical pixels — Rust's `%` already returns a value with -/// `|result| < spacing` and the same sign as the input, which is exactly -/// the symmetric `(-spacing, spacing)` margin the (now-symmetric, see -/// `draw_bg_grid_dots`) draw loops need. `t=0` (or `speed=0`) stays an exact -/// `(0.0, 0.0)` no-op — `0.0 % spacing == 0.0`. +/// periodic on its own `tile_spacing`, translating by any offset congruent +/// mod that period produces byte-identical pixels — Rust's `%` already +/// returns a value with `|result| < spacing` and the same sign as the +/// input, which is exactly the symmetric `(-spacing, spacing)` margin the +/// (now-symmetric, see `draw_bg_grid_dots`) draw loops need. `t=0` (or +/// `speed=0`) stays an exact `(0.0, 0.0)` no-op — `0.0 % spacing == 0.0`. pub(super) fn compute_scroll_offset(bg: &AnimatedBackground, time: f32) -> (f32, f32) { let (raw_x, raw_y) = raw_scroll_offset(bg, time); - let spacing = tile_spacing(&bg.preset); - (raw_x % spacing, raw_y % spacing) + let (spacing_x, spacing_y) = tile_spacing(&bg.preset); + (raw_x % spacing_x, raw_y % spacing_y) } /// The unwrapped scroll offset — how far the pattern *would* have travelled @@ -1248,7 +1286,7 @@ mod pixel_grid_tests { let mut c = cfg(); c.size = 40.0; c.spacing = 8.0; - assert_eq!(tile_spacing(&BackgroundPreset::PixelGrid(c)), 40.0); + assert_eq!(tile_spacing(&BackgroundPreset::PixelGrid(c)), (40.0, 40.0)); } /// `twinkle` has to reach both ends. A cell that only dips to 10 % still @@ -1420,3 +1458,68 @@ mod grid_lines_tests { assert_eq!(buf.len(), (W * H * 4) as usize, "it still produced a frame"); } } + +#[cfg(test)] +mod heropattern_period_tests { + //! `tile_spacing` used to return the Heropattern's *width* as the + //! period on both axes. 41 of the 87 bundled SVGs are not square (e.g. + //! `aztec` is 32x64 — see `crates/rustmotion-core/src/engine/heropatterns.rs`), + //! so wrapping the vertical offset on the horizontal period snaps the + //! pattern mid-tile on every wrap. + + use super::*; + use crate::schema::HeropatternConfig; + + fn hero_bg(pattern: &str, direction: ScrollDirection, speed: f32) -> AnimatedBackground { + AnimatedBackground { + preset: BackgroundPreset::Heropattern(HeropatternConfig { + pattern: pattern.to_string(), + color: "#FFFFFF".to_string(), + opacity: 0.1, + scale: 1.0, + }), + x: 0.0, + y: 0.0, + speed, + direction: Some(direction), + } + } + + #[test] + fn tile_spacing_is_per_axis_for_a_non_square_pattern() { + let bg = hero_bg("aztec", ScrollDirection::Down, 60.0); + let (spacing_x, spacing_y) = tile_spacing(&bg.preset); + assert_eq!(spacing_x, 32.0, "x period must be the pattern's own width"); + assert_eq!( + spacing_y, 64.0, + "y period must be the pattern's own height, not its width" + ); + } + + #[test] + fn vertical_scroll_does_not_wrap_at_half_the_tile_height() { + let bg = hero_bg("aztec", ScrollDirection::Down, 60.0); + // 48px of vertical travel sits strictly between one width-period + // (32px — where the pre-fix code would wrap) and the pattern's + // actual 64px height: the correct wrap leaves it untouched, the bug + // wraps it down to 48 % 32 = 16. + let t = 48.0 / 60.0; + let (_dx, dy) = compute_scroll_offset(&bg, t); + assert!( + (dy - 48.0).abs() < 1e-2, + "48px of vertical travel is under one tile height (64px) and must not wrap yet, got dy={dy}" + ); + } + + #[test] + fn narrow_pattern_wraps_on_a_whole_multiple_of_its_own_period() { + let bg = hero_bg("bamboo", ScrollDirection::Right, 60.0); + let (spacing_x, _spacing_y) = tile_spacing(&bg.preset); + assert_eq!( + spacing_x % 16.0, + 0.0, + "the clamped period must stay a whole multiple of the pattern's own 16px width, got {spacing_x}" + ); + assert!(spacing_x >= 20.0); + } +} diff --git a/crates/rustmotion/src/engine/render/scene.rs b/crates/rustmotion/src/engine/render/scene.rs index c6932b66..dde851e3 100644 --- a/crates/rustmotion/src/engine/render/scene.rs +++ b/crates/rustmotion/src/engine/render/scene.rs @@ -11,31 +11,11 @@ use rustmotion_core::css::style::{ JustifyContent as CssJustifyContent, }; use rustmotion_core::css::taffy_bridge::ConversionContext; -use rustmotion_core::css::units::{LengthContext, LengthPercentage}; +use rustmotion_core::css::units::LengthPercentage; use rustmotion_core::engine::animator::safe_div; use rustmotion_core::engine::paint_pass::PlaneCamera; use rustmotion_core::engine::renderer::color4f_from_hex; -/// Build the `ConversionContext` that resolves `vw`/`vh`/`%` units for a -/// layout pass, anchored to the *real* output viewport instead of -/// `ConversionContext::default()`'s hardcoded 1920×1080 (round 4 audit, -/// lot LAYOUT, constat 1). On a 1080×1920 vertical video — a resolution -/// this project documents as a common target — `width: "50vw"` used to -/// resolve as 50% of a phantom 1920px-wide viewport (960px) instead of 50% -/// of the real 1080px one (540px), a 78% error, and `vh` was off by the -/// same margin in the other axis. `font-size`/`root-font-size` stay at the -/// CSS initial `16px`: nothing upstream of this call resolves and threads a -/// root font-size through yet. -fn viewport_conversion_context(viewport_w: f32, viewport_h: f32) -> ConversionContext { - ConversionContext { - length: LengthContext { - viewport_width: viewport_w, - viewport_height: viewport_h, - ..LengthContext::default() - }, - } -} - /// The single choke-point that turns "a frame of this scene" into the time /// value every render path in this file feeds into the background draw, the /// camera transform, and the component tree (`RenderContext.time`, and from @@ -554,7 +534,7 @@ fn render_with_new_pipeline_iter<'a, I>( let layout = run_layout( &built.root, (viewport_w, viewport_h), - &viewport_conversion_context(viewport_w, viewport_h), + &ConversionContext::for_viewport(viewport_w, viewport_h), ); let dispatcher = LegacyPaintDispatcher::for_scene(&built); let frame = PaintFrame { @@ -789,7 +769,11 @@ pub fn render_scene_hits( fps: config.fps, }); let built = build_scene_from_refs(children.iter(), (vw, vh), root_css, anim); - let layout = run_layout(&built.root, (vw, vh), &viewport_conversion_context(vw, vh)); + let layout = run_layout( + &built.root, + (vw, vh), + &ConversionContext::for_viewport(vw, vh), + ); let dispatcher = LegacyPaintDispatcher::for_scene(&built); let frame = PaintFrame { time, diff --git a/crates/rustmotion/tests/audit_ws_e.rs b/crates/rustmotion/tests/audit_ws_e.rs new file mode 100644 index 00000000..5a2f31eb --- /dev/null +++ b/crates/rustmotion/tests/audit_ws_e.rs @@ -0,0 +1,18 @@ +//! Regression tests — audit round chantier/audit-2026-09, 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`/ +//! `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 +//! 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; From 1952295af348e4739e45dc0df8f91f9b91ddf917 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:53:41 +0200 Subject: [PATCH 08/56] fix(html): reject unknown scene attributes instead of dropping them (#252) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Executed: transpiles to a scene carrying only duration and layout — three documented scene features (CLAUDE.md documents all three) are discarded with no warning and validate exits 0. Same on the root: drops both. Same on containers/text:
keeps nothing but style/anim (element.rs:101-112 and :92-100 never iterate attrs). This is the exact failure mode check_component_attrs exists to prevent, but that check runs on the already-transpiled ResolvedScenario (validate_attrs.rs:79 check_component_attrs(scenario: &ResolvedScenario)) and only over scene.children — an attribute the transpiler never emitted is invisible to it, so the workspace's only unknown-attribute net has a blind spot precisely over the HTML front-end's own attribute surface. rm-* custom elements are the asymmetric exception: they forward everything and do get caught. Fix: After reading the known keys in scene_to_value and html_to_scenario_value, diff against the consumed set and return a named error (with did-you-mean, mirroring validate_attrs) for the remainder — class/id/data-* can be allowlisted as deliberately inert. Do the same for TagKind::Container/TagKind::Text. Separately, extend 's allowlist to cover freeze_at, world-position and animated-background, which are otherwise unreachable from HTML. Refs #220 --- crates/rustmotion-html/src/element.rs | 6 +- crates/rustmotion-html/src/lib.rs | 115 +++++++++++++++++++++ crates/rustmotion-html/src/scene.rs | 72 ++++++++++++- crates/rustmotion-html/tests/audit_ws_f.rs | 97 +++++++++++++++++ 4 files changed, 288 insertions(+), 2 deletions(-) create mode 100644 crates/rustmotion-html/tests/audit_ws_f.rs diff --git a/crates/rustmotion-html/src/element.rs b/crates/rustmotion-html/src/element.rs index c308265a..b370f658 100644 --- a/crates/rustmotion-html/src/element.rs +++ b/crates/rustmotion-html/src/element.rs @@ -2,7 +2,9 @@ use markup5ever_rcdom::{Handle, NodeData}; use serde_json::{Map, Value}; use crate::style::{coerce_value, parse_anim_attr, parse_inline_style}; -use crate::{element_attrs, tag_name, HtmlError}; +use crate::{check_known_attrs, element_attrs, tag_name, HtmlError}; + +const KNOWN_NATIVE_ATTRS: &[&str] = &["style", "anim"]; enum TagKind { Container, @@ -90,6 +92,7 @@ pub(crate) fn element_to_value(handle: &Handle) -> Result, HtmlErr suggestion: suggestion.to_string(), }), TagKind::Text => { + check_known_attrs(&tag, &attrs, KNOWN_NATIVE_ATTRS)?; let mut obj = Map::new(); obj.insert("type".into(), Value::from("text")); obj.insert("content".into(), Value::from(inner_text(handle))); @@ -99,6 +102,7 @@ pub(crate) fn element_to_value(handle: &Handle) -> Result, HtmlErr Ok(Some(Value::Object(obj))) } TagKind::Container => { + check_known_attrs(&tag, &attrs, KNOWN_NATIVE_ATTRS)?; let mut obj = Map::new(); obj.insert("type".into(), Value::from("div")); if let Some(style) = style_object(&attrs)? { diff --git a/crates/rustmotion-html/src/lib.rs b/crates/rustmotion-html/src/lib.rs index 092bd1d8..69d9ea43 100644 --- a/crates/rustmotion-html/src/lib.rs +++ b/crates/rustmotion-html/src/lib.rs @@ -97,6 +97,44 @@ pub enum HtmlError { " found nested inside <{parent}> — elements must be direct children of (only is recursed into)" )] NestedScene { parent: String }, + /// Emitted when an element carries an attribute the transpiler never + /// reads. ``, ``, and native container/text tags only + /// ever consume a fixed, small set of attribute names — anything else + /// used to vanish with no trace, invisible to `--strict-attrs` because + /// it never reached the emitted JSON in the first place. + #[error("<{element}> has unsupported attribute(s): {detail} — these are silently ignored today; fix the typo, drop them, or use the attribute the dialect actually reads")] + UnknownAttributes { element: String, detail: String }, + /// Emitted for a ``'s `world-position` attribute that is neither + /// `"x,y"` nor a JSON `{"x":..,"y":..}` object. + #[error("world-position=\"{0}\" is not \"x,y\" or a JSON object {{\"x\":..,\"y\":..}}")] + InvalidWorldPosition(String), + /// Emitted for a ``'s `animated-background` attribute when its + /// value starts with `{`/`[` but fails to parse as JSON. + #[error("animated-background attribute contains invalid JSON: {0}")] + InvalidAnimatedBackgroundJson(String), + /// Emitted for a `style="..."` declaration whose value has more than one + /// top-level (paren-aware) token and isn't one of the shorthands the + /// transpiler knows how to expand (`padding`/`margin`/`border-radius`'s + /// 1-4 value box form, `grid-template-columns`/`-rows`'s track list with + /// `repeat()`/`minmax()`). Every other multi-token value used to become + /// an opaque string the core length parser cannot read, silently + /// resolving to `0px`. + #[error("style property '{prop}' has an unsupported multi-token value '{value}' — supported multi-token forms are the padding/margin/border-radius box shorthand and grid-template-columns/-rows track lists with repeat()/minmax(); rewrite as a single value")] + UnsupportedStyleShorthand { prop: String, value: String }, + /// Emitted when a `

` 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("