From 9e5d7e1be7c2b531d3f39fedd78860c2cbf442b5 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:00:47 +0200 Subject: [PATCH] fix(video): treat decoded frames as unpremultiplied Refs #220 --- crates/rustmotion-components/src/gif.rs | 6 +- crates/rustmotion-components/src/video.rs | 32 +++-- .../rustmotion-components/tests/audit_ws_d.rs | 135 ++++++++++++++++++ 3 files changed, 161 insertions(+), 12 deletions(-) diff --git a/crates/rustmotion-components/src/gif.rs b/crates/rustmotion-components/src/gif.rs index b5698604..5a0d918d 100644 --- a/crates/rustmotion-components/src/gif.rs +++ b/crates/rustmotion-components/src/gif.rs @@ -455,9 +455,9 @@ mod tests { /// own video is, so passing generous `max_w`/`max_h` here isolates the /// byte-budget check from the video-dimensions check exercised by the /// next test. 144 MiB is deliberately far short of the 65535×65535 - /// (~17 GiB) header a real crafted file could declare — large enough to - /// prove the budget check fires, small enough that running this test - /// never risks the allocation it is asserting never happens. + /// (~17 GiB) header the audit's own crafted file could declare — large + /// enough to prove the budget check fires, small enough that running + /// this test never risks the allocation it is asserting never happens. #[test] fn a_canvas_over_the_byte_budget_is_rejected_without_allocating_it() { let path = std::env::temp_dir().join(format!( diff --git a/crates/rustmotion-components/src/video.rs b/crates/rustmotion-components/src/video.rs index d76cf0fc..cae7b018 100644 --- a/crates/rustmotion-components/src/video.rs +++ b/crates/rustmotion-components/src/video.rs @@ -95,10 +95,10 @@ fn draw_fitted(canvas: &Canvas, img: skia_safe::Image, fit: &ImageFit, layout: & /// The source clip's own duration, probed via `ffprobe` and memoized per /// `src` for the life of the process — `effective_source_time` below is /// called once per painted frame, and re-probing on every one of them would -/// mean one subprocess spawn per frame for any looping video. `None` on a -/// probe failure (no ffprobe on `PATH`, or the source can't be read) is -/// memoized too, so a broken source fails fast on every subsequent frame -/// instead of retrying the same failing probe. +/// mean one subprocess spawn per frame for any looping video with no +/// explicit `trim_end`. `None` on a probe failure (no ffprobe on `PATH`, or +/// the source can't be read) is memoized too, so a broken source fails fast +/// on every subsequent frame instead of retrying the same failing probe. fn video_duration_secs(src: &str) -> Option { static CACHE: std::sync::OnceLock< std::sync::Mutex>>, @@ -122,15 +122,29 @@ fn video_duration_secs(src: &str) -> Option { impl Video { /// The timestamp to sample from the source clip for a given scene time. - /// When `loop_video` is set, playback wraps within the source's own - /// probed duration instead of running past it and holding on - /// whatever the last extractable frame happens to be. + /// + /// Honours `trim_end` on the picture the same way the audio track + /// already does: past `trim_end`, playback holds on the last in-window + /// frame instead of continuing to draw whatever the source contains + /// beyond the intended trim point. When `loop_video` is set, playback + /// wraps within `[trim_start, trim_end)` instead of clamping — falling + /// back to the source's own probed duration as the loop window only + /// when `trim_end` is absent, since that is the only case where the + /// window cannot otherwise be known at all. fn effective_source_time(&self, ctx_time: f64) -> f64 { let rate = self.playback_rate.unwrap_or(1.0); let trim_start = self.trim_start.unwrap_or(0.0); let raw = trim_start + ctx_time * rate; - if self.loop_video == Some(true) { + if let Some(end) = self.trim_end { + if end > trim_start { + return if self.loop_video == Some(true) { + trim_start + (raw - trim_start).rem_euclid(end - trim_start) + } else { + raw.min(end) + }; + } + } else if self.loop_video == Some(true) { if let Some(duration) = video_duration_secs(&self.src) { if duration > trim_start { return trim_start + (raw - trim_start).rem_euclid(duration - trim_start); @@ -162,7 +176,7 @@ impl Painter for Video { let img_info = ImageInfo::new( (fw as i32, fh as i32), ColorType::RGBA8888, - skia_safe::AlphaType::Premul, + skia_safe::AlphaType::Unpremul, None, ); let row_bytes = fw as usize * 4; diff --git a/crates/rustmotion-components/tests/audit_ws_d.rs b/crates/rustmotion-components/tests/audit_ws_d.rs index 3cb43373..7ac9dbd8 100644 --- a/crates/rustmotion-components/tests/audit_ws_d.rs +++ b/crates/rustmotion-components/tests/audit_ws_d.rs @@ -186,3 +186,138 @@ fn fill_fit_still_stretches_to_the_whole_box() { "fill must cover every corner" ); } + +// ─── `trim_end` was honoured only on the extracted audio ─────────────────── + +/// Frames beyond `trim_end` sit in the cache (simulating a preextraction +/// window, or a direct extraction, wider than the intended trim), so the +/// picture path must never pick one of them once `trim_end` is set: past +/// `trim_end`, playback holds on the last in-window frame. Before the fix, +/// `source_time` had no upper bound at all — querying past `trim_end` on a +/// cache/source that extends further would draw whatever sits further +/// along the source, not the frame at the trim boundary. +#[test] +fn trim_end_clamps_playback_instead_of_running_past_it() { + let src = unique_src("trimend"); + let cache_key = format!("{src}:20x20"); + let frames = vec![ + (0.0, solid_rgba([255, 0, 0, 255], 2, 2), 2, 2), + (0.5, solid_rgba([0, 0, 255, 255], 2, 2), 2, 2), + (1.0, solid_rgba([0, 255, 0, 255], 2, 2), 2, 2), + (1.5, solid_rgba([128, 0, 128, 255], 2, 2), 2, 2), + (2.0, solid_rgba([255, 165, 0, 255], 2, 2), 2, 2), + ]; + video_frame_cache().insert(cache_key, Arc::new(frames)); + + let v = video(&src, ImageFit::Fill, Some(0.0), Some(1.0), None); + let ctx = ctx_at(1.8); + let pixels = paint_and_read(&v, &ctx, 20, 20, false); + + assert_eq!( + px(&pixels, 20, 10, 10), + [0, 255, 0, 255], + "past trim_end, playback must clamp to the frame at trim_end (green), not the frame \ + nearest the unclamped query time (orange)" + ); +} + +// ─── `loop_video` made neither the picture nor the audio loop ───────────── + +/// Cache frames only cover `[0.0, 1.0)`; `trim_end: Some(1.0)` gives +/// `loop_video` a window to wrap within without needing a real source file +/// to probe. Querying at `ctx.time = 2.1` (raw source time 2.1s, i.e. "2 +/// full loops plus 0.1s") must land near 0.1s once wrapped — nearest to +/// that among `{0.0, 0.25, 0.5, 0.75}` is red. Before this fix, the same +/// query — with the trim-end clamp from the previous test already in place +/// but no loop branch yet — clamped to `min(2.1, 1.0) = 1.0`, whose nearest +/// cached frame is yellow: a clearly different pixel, which is what proves +/// this test is exercising the loop path and not being masked by the clamp. +#[test] +fn loop_video_wraps_playback_within_the_trim_window() { + let src = unique_src("loop"); + let cache_key = format!("{src}:20x20"); + let frames = vec![ + (0.0, solid_rgba([255, 0, 0, 255], 2, 2), 2, 2), + (0.25, solid_rgba([0, 255, 0, 255], 2, 2), 2, 2), + (0.5, solid_rgba([0, 0, 255, 255], 2, 2), 2, 2), + (0.75, solid_rgba([255, 255, 0, 255], 2, 2), 2, 2), + ]; + video_frame_cache().insert(cache_key, Arc::new(frames)); + + let v = video(&src, ImageFit::Fill, Some(0.0), Some(1.0), Some(true)); + let ctx = ctx_at(2.1); + let pixels = paint_and_read(&v, &ctx, 20, 20, false); + + assert_eq!( + px(&pixels, 20, 10, 10), + [255, 0, 0, 255], + "looping must wrap the query time back into the window (nearest: red), not clamp to \ + the window's own end (nearest: yellow)" + ); +} + +/// Without `loop_video`, a `trim_end`-bounded video must still clamp +/// (unaffected by the loop branch existing) rather than wrap — the same +/// scenario as the wrap test above, minus the flag. +#[test] +fn without_loop_video_playback_still_clamps_not_wraps() { + let src = unique_src("no-loop"); + let cache_key = format!("{src}:20x20"); + let frames = vec![ + (0.0, solid_rgba([255, 0, 0, 255], 2, 2), 2, 2), + (0.25, solid_rgba([0, 255, 0, 255], 2, 2), 2, 2), + (0.5, solid_rgba([0, 0, 255, 255], 2, 2), 2, 2), + (0.75, solid_rgba([255, 255, 0, 255], 2, 2), 2, 2), + ]; + video_frame_cache().insert(cache_key, Arc::new(frames)); + + let v = video(&src, ImageFit::Fill, Some(0.0), Some(1.0), None); + let ctx = ctx_at(2.1); + let pixels = paint_and_read(&v, &ctx, 20, 20, false); + + assert_eq!( + px(&pixels, 20, 10, 10), + [255, 255, 0, 255], + "no loop_video: must clamp to the window's end (nearest: yellow), not wrap" + ); +} + +// ─── cached-frame draw path mistagged straight alpha as premultiplied ────── + +/// ffmpeg's `-pix_fmt rgba` output — what fills the video-frame cache — is +/// straight (unpremultiplied) alpha. Tagging that buffer `AlphaType::Premul` +/// makes Skia treat the RGB channels as already scaled by alpha instead of +/// scaling them itself, which brightens (here: doubles) every +/// semi-transparent pixel's channels once composited. +/// +/// A straight-alpha (200, 100, 50, 128) pixel, composited over black: +/// correctly tagged `Unpremul`, Skia premultiplies it to +/// (200×128/255, 100×128/255, 50×128/255) ≈ (100, 50, 25) before compositing +/// over black, landing there almost exactly (the `(1 - alpha) * 0` background +/// term vanishes either way). Mistagged `Premul`, Skia uses the raw channel +/// values directly as if already scaled — (200, 100, 50) — composited over +/// black with no further scaling, landing at roughly double the correct +/// result. +#[test] +fn cached_frame_straight_alpha_composites_correctly_not_doubled() { + let src = unique_src("alpha"); + let cache_key = format!("{src}:10x10"); + let straight = [200u8, 100, 50, 128]; + video_frame_cache().insert( + cache_key, + Arc::new(vec![(0.0, solid_rgba(straight, 2, 2), 2, 2)]), + ); + + let v = video(&src, ImageFit::Fill, None, None, None); + let ctx = ctx_at(0.0); + let pixels = paint_and_read(&v, &ctx, 10, 10, false); + let composited = px(&pixels, 10, 5, 5); + + let close = |actual: u8, expected: u8| (actual as i16 - expected as i16).abs() <= 4; + assert!( + close(composited[0], 100) && close(composited[1], 50) && close(composited[2], 25), + "straight-alpha (200,100,50,128) over black must composite to roughly (100,50,25), \ + got {composited:?} — a value near (200,100,50) means the buffer is still mistagged \ + as premultiplied and its channels are being used unscaled" + ); +}