From 1e9f14e486793509b9be7a0107e517c2ef1d664c Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 11 Aug 2026 14:54:55 +0200 Subject: [PATCH] test(effects): record why per-node effects need paint_pass, with numbers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Investigating the "per-node visual effects" gap produced a negative result rather than a feature, and this commit ships the evidence so the next attempt does not have to rediscover it. `effects` is a field of `Scene` only, so an effect covers the whole frame. The obvious workaround — render normally, then crop the effect to a node's rect from `render_scene_hits` — does not merely cost too much, it is wrong. A background node with `pixelate` applied that way contaminates **5904 of 10000 pixels** inside an overlapping sibling's own box; a sample pixel goes from pure blue to `[155, 0, 99]`. `buffer_crop_strategy_leaks_effect_onto_ overlapping_sibling` pins that: it passes, and what it documents is the rejection. The same approach also costs x1.47 on a real scenario (mega-showcase scene 0, 1920x1080, release: 6.0ms/frame becomes 8.8ms), but the leak is the disqualifying part, not the cost. The cost benchmark is kept `#[ignore]`d because its other measurement is the one that matters for whoever implements this properly: bounding the CPU effect pass to a node's real box instead of the full frame is **x14.57** faster at a 13.9x area ratio. That is the same argument PR #154 already made for the `save_layer` it bounded (42-60s down to 0.5s over 60 frames), and it says any real implementation must bound its layer to the node. No production file is touched. The feature is blocked on three things that sit outside one file: a node's device-space rect exists only inside `paint_node` (animation presets resolve into `CssStyle.transform`, so a rect computed from `run_layout` alone is silently wrong for most animated content); `post_effects.rs` lives in the crate that depends on the one holding `paint_pass.rs`, not the reverse; and `CssStyle` is the only per-node channel that reaches `BoxNode` without teaching `box_builder` a new field. --- crates/rustmotion/tests/node_effects_cost.rs | 187 ++++++++++++++++++ .../tests/node_effects_leak_proof.rs | 126 ++++++++++++ 2 files changed, 313 insertions(+) create mode 100644 crates/rustmotion/tests/node_effects_cost.rs create mode 100644 crates/rustmotion/tests/node_effects_leak_proof.rs diff --git a/crates/rustmotion/tests/node_effects_cost.rs b/crates/rustmotion/tests/node_effects_cost.rs new file mode 100644 index 0000000..4804dcf --- /dev/null +++ b/crates/rustmotion/tests/node_effects_cost.rs @@ -0,0 +1,187 @@ +//! Cost measurements for the "node-level effects" investigation +//! (see chantier brief: node-effects). +//! +//! These are NOT correctness tests — they print wall-clock numbers used to +//! justify two decisions in the accompanying report: +//! +//! 1. Any per-node effect layer MUST be bounded to the node's own box, never +//! the viewport — `bench_pixel_effects_full_frame_vs_node_box` reproduces, +//! for the CPU pixel-effect pass (`post_effects.rs`), the same argument PR +//! #154 already made for the Skia `save_layer` allocation. +//! 2. Obtaining a node's on-screen box via a second call to +//! `render_scene_hits` (a full duplicate paint into a throwaway surface) +//! is NOT an acceptable substitute for touching `paint_pass.rs` — +//! `bench_duplicate_paint_via_render_scene_hits` quantifies the multiplier +//! that shortcut would add to every frame of a scene using node effects. +//! +//! Run explicitly (they are `#[ignore]`d so `cargo test --workspace` stays +//! fast and deterministic): +//! +//! ```text +//! CARGO_TARGET_DIR=/tmp/rm-fx cargo test -p rustmotion --test node_effects_cost -- --ignored --nocapture +//! ``` + +use std::path::PathBuf; +use std::time::Instant; + +use rustmotion::engine::render::post_effects::apply_post_effects; +use rustmotion::engine::render::{render_scene_frame_scaled, render_scene_hits}; +use rustmotion::loader::load_scenario; +use rustmotion::schema::{BlurDirection, PostEffect, ResolvedScenario}; + +/// A realistic, already-shipped-and-validated scenario: 9 scenes, 31 +/// top-level children, 1920x1080@30fps. `Scene` isn't `Clone`, so this +/// returns the owned `ResolvedScenario` — callers index `all_scenes_vec()[0]` +/// (7 children, 4.5s = 135 frames — the densest early scene) for a `&Scene`. +fn load_mega_showcase() -> ResolvedScenario { + let path = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("..") + .join("..") + .join("examples") + .join("mega-showcase.json"); + load_scenario(&path).expect("load examples/mega-showcase.json") +} + +fn realistic_effects() -> Vec { + vec![ + PostEffect::Grain { + intensity: 0.15, + seed: 42, + animated: true, + }, + PostEffect::Vignette { + intensity: 0.5, + radius: 0.75, + }, + PostEffect::Pixelate { size: 8 }, + PostEffect::ProgressiveBlur { + direction: BlurDirection::Bottom, + start: 0.5, + max_radius: 12.0, + }, + ] +} + +/// Benchmark 1: bound-to-box vs full-frame cost of the *existing* CPU pixel +/// effect pass. Mirrors PR #154's Skia `save_layer` finding, but for +/// `post_effects.rs`'s pixel loops instead of the layer allocation. +/// +/// The "node box" size is not invented: it is the largest on-screen +/// component rect actually produced by `render_scene_hits` for mega-showcase +/// scene 0, frame 0 — i.e. the box a real per-node effect would be bounded +/// to if one were attached to that node. +#[test] +#[ignore = "prints timing numbers for the node-effects cost report; not a correctness check"] +fn bench_pixel_effects_full_frame_vs_node_box() { + let scenario = load_mega_showcase(); + let config = &scenario.video; + let scene = scenario.all_scenes_vec()[0]; + let full_w = config.width; + let full_h = config.height; + + let hits = render_scene_hits(config, scene, 0); + let viewport_area = (full_w * full_h) as f32; + // Exclude full-bleed layers (backgrounds/base cards that intentionally + // cover the whole frame) — a per-node effect on one of those would + // legitimately need the full viewport, so they are not the case this + // benchmark is arguing about. The largest of what remains is the + // biggest *bounded* component in this scene (e.g. a hero card), the + // realistic "worst case" box a node effect would actually be sized to. + let mut sizes: Vec<(f32, f32)> = hits.iter().map(|h| (h.rect.w, h.rect.h)).collect(); + sizes.sort_by(|a, b| (a.0 * a.1).partial_cmp(&(b.0 * b.1)).unwrap()); + let node = hits + .iter() + .filter(|h| h.rect.w * h.rect.h < 0.9 * viewport_area) + .max_by(|a, b| { + (a.rect.w * a.rect.h) + .partial_cmp(&(b.rect.w * b.rect.h)) + .unwrap() + }) + .expect("at least one non-full-bleed component in scene 0"); + let box_w = node.rect.w.round().max(1.0) as u32; + let box_h = node.rect.h.round().max(1.0) as u32; + eprintln!( + "[node-effects cost] all {} component hit-rect sizes in scene 0 (w x h): {:?}", + sizes.len(), + sizes + .iter() + .map(|(w, h)| format!("{}x{}", w.round(), h.round())) + .collect::>() + ); + + let effects = realistic_effects(); + const FRAMES: u32 = 30; + + // Full-frame buffer, effects applied unbounded (as if the layer were + // sized to the viewport instead of the node box). + let mut full_buf = vec![0u8; (full_w * full_h * 4) as usize]; + let t0 = Instant::now(); + for f in 0..FRAMES { + apply_post_effects(&mut full_buf, full_w, full_h, &effects, f); + } + let full_elapsed = t0.elapsed(); + + // Node-box buffer, same effects, same frame count. + let mut box_buf = vec![0u8; (box_w * box_h * 4) as usize]; + let t0 = Instant::now(); + for f in 0..FRAMES { + apply_post_effects(&mut box_buf, box_w, box_h, &effects, f); + } + let box_elapsed = t0.elapsed(); + + let area_ratio = (full_w * full_h) as f64 / (box_w * box_h) as f64; + let time_ratio = full_elapsed.as_secs_f64() / box_elapsed.as_secs_f64().max(1e-9); + + eprintln!( + "\n[node-effects cost] full-frame {full_w}x{full_h} vs node-box {box_w}x{box_h} \ + (largest non-full-bleed component in mega-showcase scene 0), {FRAMES} frames, 4 effects (grain+vignette+pixelate+progressive_blur):\n\ + \x20 full-frame : {full_elapsed:?} ({:.3} ms/frame)\n\ + \x20 node-box : {box_elapsed:?} ({:.3} ms/frame)\n\ + \x20 area ratio : {area_ratio:.2}x time ratio: {time_ratio:.2}x\n", + full_elapsed.as_secs_f64() * 1000.0 / FRAMES as f64, + box_elapsed.as_secs_f64() * 1000.0 / FRAMES as f64, + ); +} + +/// Benchmark 2: the cost of the rejected "use `render_scene_hits` to find +/// node boxes for every frame" shortcut, relative to the render that must +/// happen anyway. `render_scene_hits`'s own doc comment says it "paints to a +/// throwaway surface purely to collect the on-screen bounding box of each +/// component. Used by the studio overlay; not part of the video encode +/// path." — this benchmark quantifies what putting it INTO the encode path +/// would cost. +#[test] +#[ignore = "prints timing numbers for the node-effects cost report; not a correctness check"] +fn bench_duplicate_paint_via_render_scene_hits() { + let scenario = load_mega_showcase(); + let config = &scenario.video; + let scene = scenario.all_scenes_vec()[0]; + const FRAMES: u32 = 30; + + let t0 = Instant::now(); + for f in 0..FRAMES { + let _ = render_scene_frame_scaled(config, scene, f, FRAMES, 1.0).expect("render"); + } + let normal_elapsed = t0.elapsed(); + + let t0 = Instant::now(); + for f in 0..FRAMES { + let _ = render_scene_hits(config, scene, f); + } + let hits_elapsed = t0.elapsed(); + + let combined = normal_elapsed + hits_elapsed; + let overhead_ratio = combined.as_secs_f64() / normal_elapsed.as_secs_f64(); + + eprintln!( + "\n[node-effects cost] mega-showcase scene 0 ({}x{}@{}fps), {FRAMES} frames:\n\ + \x20 normal render (paid today) : {normal_elapsed:?} ({:.3} ms/frame)\n\ + \x20 + render_scene_hits (rejected path) : {hits_elapsed:?} ({:.3} ms/frame)\n\ + \x20 combined vs normal-only : {overhead_ratio:.2}x\n", + config.width, + config.height, + config.fps, + normal_elapsed.as_secs_f64() * 1000.0 / FRAMES as f64, + hits_elapsed.as_secs_f64() * 1000.0 / FRAMES as f64, + ); +} diff --git a/crates/rustmotion/tests/node_effects_leak_proof.rs b/crates/rustmotion/tests/node_effects_leak_proof.rs new file mode 100644 index 0000000..53b0caa --- /dev/null +++ b/crates/rustmotion/tests/node_effects_leak_proof.rs @@ -0,0 +1,126 @@ +//! Proof that the "crop the already-composited frame buffer to a node's +//! `render_scene_hits` rect" implementation strategy considered for +//! node-level effects — and rejected — leaks onto overlapping siblings. +//! +//! This is the concrete, quantified reason the node-effects report gives for +//! not shipping that strategy: it is the ONLY per-node box-location mechanism +//! reachable without touching `paint_pass.rs` (which is frozen for this +//! chantier), but because it operates on already-flattened pixels, an effect +//! "on" a background node also mutates any sibling painted on top of it +//! within the same screen rectangle — exactly the brief's own motivating +//! example ("un grain sur une image de fond mais pas sur le texte par-dessus") +//! backfires under this strategy. + +use rustmotion::engine::render::post_effects::apply_post_effects; +use rustmotion::engine::render::{render_scene_frame_scaled, render_scene_hits}; +use rustmotion::schema::{PostEffect, Scene, VideoConfig}; + +/// A background rect (red, 800x800 at the origin) with a foreground rect +/// (blue, 100x100) painted on top of it, fully inside the background's box — +/// the "text over a background image" shape from the brief's own example, +/// reduced to two solid-colour shapes so the proof needs no font rendering. +fn background_and_overlapping_foreground() -> (VideoConfig, Scene) { + let config: VideoConfig = + serde_json::from_value(serde_json::json!({ "width": 800, "height": 800, "fps": 30 })) + .expect("config"); + let scene: Scene = serde_json::from_value(serde_json::json!({ + "duration": 1.0, + "children": [ + { + "type": "shape", "shape": "rect", "fill": "#ff0000", + "position": "absolute", "x": 0, "y": 0, + "style": { "width": "800px", "height": "800px" } + }, + { + "type": "shape", "shape": "rect", "fill": "#0000ff", + "position": "absolute", "x": 300, "y": 300, + "style": { "width": "100px", "height": "100px" } + } + ] + })) + .expect("scene"); + (config, scene) +} + +fn pixel_at(buf: &[u8], w: u32, x: u32, y: u32) -> [u8; 4] { + let base = ((y * w + x) * 4) as usize; + [buf[base], buf[base + 1], buf[base + 2], buf[base + 3]] +} + +#[test] +fn buffer_crop_strategy_leaks_effect_onto_overlapping_sibling() { + let (config, scene) = background_and_overlapping_foreground(); + + // The real, already-rendered frame: red background, blue square on top. + let rendered = render_scene_frame_scaled(&config, &scene, 0, 30, 1.0).expect("render"); + + // Sanity: the foreground square really is visible (opaque blue), not + // occluded or blended away — otherwise the "leak" below would be trivial. + let before = pixel_at(&rendered, config.width, 350, 350); + assert_eq!( + before, + [0, 0, 255, 255], + "foreground square must render as pure blue before any effect" + ); + + // The naive strategy: find the background node's on-screen box via + // `render_scene_hits` (the only per-node box available without touching + // paint_pass.rs), then crop-apply the CPU pixel effect to that + // rectangle of the FINAL, already-composited buffer. + let hits = render_scene_hits(&config, &scene, 0); + let background_hit = hits + .first() + .expect("background shape is the first painted component"); + assert!( + (background_hit.rect.w - 800.0).abs() < 1.0 && (background_hit.rect.h - 800.0).abs() < 1.0, + "background hit rect should cover the full 800x800 canvas, got {:?}", + background_hit.rect + ); + + let mut buf = rendered.clone(); + let effects = vec![PostEffect::Pixelate { size: 32 }]; + apply_post_effects(&mut buf, config.width, config.height, &effects, 0); + + // The foreground square sits entirely inside the background's hit rect, + // so the crop-and-apply strategy touches its pixels too, even though it + // is a later sibling painted on top and was never meant to be affected. + // A 32px pixelate block straddling the red/blue boundary mixes the two; + // a block that lands fully inside the blue square stays pure blue by + // coincidence, so the proof is the fraction of the sibling's own box + // that changed, not any single sample point. + let mut changed = 0u32; + let mut worst: Option<(u32, u32, [u8; 4], [u8; 4])> = None; + for y in 300..400 { + for x in 300..400 { + let a = pixel_at(&rendered, config.width, x, y); + let b = pixel_at(&buf, config.width, x, y); + if a != b { + changed += 1; + worst.get_or_insert((x, y, a, b)); + } + } + } + let (sx, sy, sample_before, sample_after) = + worst.expect("at least one changed pixel to report"); + + eprintln!( + "\n[node-effects leak proof] pixelate(size=32) applied to the background node's \ + render_scene_hits rect (0,0,800,800) via buffer-crop: {changed}/10000 pixels inside the \ + *foreground* sibling's own 100x100 box changed (sample at ({sx},{sy}): {sample_before:?} \ + -> {sample_after:?}; centre pixel (350,350) unaffected by coincidence: {before:?})\n" + ); + + assert!( + changed > 0, + "buffer-crop strategy must NOT change any of the foreground sibling's pixels — but it \ + changes {changed}/10000 of them, which is exactly why it was rejected in favour of an \ + isolated Skia layer inside paint_pass.rs (frozen for this chantier)" + ); + // The strategy is not just imperfect at the edges — it corrupts the + // majority of the sibling's own box, since a 32px pixelate block is + // larger than most of the sibling's 100px extent near its border. + assert!( + changed > 5_000, + "expected the leak to affect the majority of the sibling's box, got {changed}/10000" + ); +}