Skip to content

Commit 7298fd8

Browse files
authored
fix(video): treat decoded frames as unpremultiplied (#247)
Refs #220
1 parent 9890365 commit 7298fd8

3 files changed

Lines changed: 161 additions & 12 deletions

File tree

‎crates/rustmotion-components/src/gif.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -455,9 +455,9 @@ mod tests {
455455
/// own video is, so passing generous `max_w`/`max_h` here isolates the
456456
/// byte-budget check from the video-dimensions check exercised by the
457457
/// next test. 144 MiB is deliberately far short of the 65535×65535
458-
/// (~17 GiB) header a real crafted file could declare — large enough to
459-
/// prove the budget check fires, small enough that running this test
460-
/// never risks the allocation it is asserting never happens.
458+
/// (~17 GiB) header the audit's own crafted file could declare — large
459+
/// enough to prove the budget check fires, small enough that running
460+
/// this test never risks the allocation it is asserting never happens.
461461
#[test]
462462
fn a_canvas_over_the_byte_budget_is_rejected_without_allocating_it() {
463463
let path = std::env::temp_dir().join(format!(

‎crates/rustmotion-components/src/video.rs‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -95,10 +95,10 @@ fn draw_fitted(canvas: &Canvas, img: skia_safe::Image, fit: &ImageFit, layout: &
9595
/// The source clip's own duration, probed via `ffprobe` and memoized per
9696
/// `src` for the life of the process — `effective_source_time` below is
9797
/// called once per painted frame, and re-probing on every one of them would
98-
/// mean one subprocess spawn per frame for any looping video. `None` on a
99-
/// probe failure (no ffprobe on `PATH`, or the source can't be read) is
100-
/// memoized too, so a broken source fails fast on every subsequent frame
101-
/// instead of retrying the same failing probe.
98+
/// mean one subprocess spawn per frame for any looping video with no
99+
/// explicit `trim_end`. `None` on a probe failure (no ffprobe on `PATH`, or
100+
/// the source can't be read) is memoized too, so a broken source fails fast
101+
/// on every subsequent frame instead of retrying the same failing probe.
102102
fn video_duration_secs(src: &str) -> Option<f64> {
103103
static CACHE: std::sync::OnceLock<
104104
std::sync::Mutex<std::collections::HashMap<String, Option<f64>>>,
@@ -122,15 +122,29 @@ fn video_duration_secs(src: &str) -> Option<f64> {
122122

123123
impl Video {
124124
/// The timestamp to sample from the source clip for a given scene time.
125-
/// When `loop_video` is set, playback wraps within the source's own
126-
/// probed duration instead of running past it and holding on
127-
/// whatever the last extractable frame happens to be.
125+
///
126+
/// Honours `trim_end` on the picture the same way the audio track
127+
/// already does: past `trim_end`, playback holds on the last in-window
128+
/// frame instead of continuing to draw whatever the source contains
129+
/// beyond the intended trim point. When `loop_video` is set, playback
130+
/// wraps within `[trim_start, trim_end)` instead of clamping — falling
131+
/// back to the source's own probed duration as the loop window only
132+
/// when `trim_end` is absent, since that is the only case where the
133+
/// window cannot otherwise be known at all.
128134
fn effective_source_time(&self, ctx_time: f64) -> f64 {
129135
let rate = self.playback_rate.unwrap_or(1.0);
130136
let trim_start = self.trim_start.unwrap_or(0.0);
131137
let raw = trim_start + ctx_time * rate;
132138

133-
if self.loop_video == Some(true) {
139+
if let Some(end) = self.trim_end {
140+
if end > trim_start {
141+
return if self.loop_video == Some(true) {
142+
trim_start + (raw - trim_start).rem_euclid(end - trim_start)
143+
} else {
144+
raw.min(end)
145+
};
146+
}
147+
} else if self.loop_video == Some(true) {
134148
if let Some(duration) = video_duration_secs(&self.src) {
135149
if duration > trim_start {
136150
return trim_start + (raw - trim_start).rem_euclid(duration - trim_start);
@@ -162,7 +176,7 @@ impl Painter for Video {
162176
let img_info = ImageInfo::new(
163177
(fw as i32, fh as i32),
164178
ColorType::RGBA8888,
165-
skia_safe::AlphaType::Premul,
179+
skia_safe::AlphaType::Unpremul,
166180
None,
167181
);
168182
let row_bytes = fw as usize * 4;

‎crates/rustmotion-components/tests/audit_ws_d.rs‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,3 +186,138 @@ fn fill_fit_still_stretches_to_the_whole_box() {
186186
"fill must cover every corner"
187187
);
188188
}
189+
190+
// ─── `trim_end` was honoured only on the extracted audio ───────────────────
191+
192+
/// Frames beyond `trim_end` sit in the cache (simulating a preextraction
193+
/// window, or a direct extraction, wider than the intended trim), so the
194+
/// picture path must never pick one of them once `trim_end` is set: past
195+
/// `trim_end`, playback holds on the last in-window frame. Before the fix,
196+
/// `source_time` had no upper bound at all — querying past `trim_end` on a
197+
/// cache/source that extends further would draw whatever sits further
198+
/// along the source, not the frame at the trim boundary.
199+
#[test]
200+
fn trim_end_clamps_playback_instead_of_running_past_it() {
201+
let src = unique_src("trimend");
202+
let cache_key = format!("{src}:20x20");
203+
let frames = vec![
204+
(0.0, solid_rgba([255, 0, 0, 255], 2, 2), 2, 2),
205+
(0.5, solid_rgba([0, 0, 255, 255], 2, 2), 2, 2),
206+
(1.0, solid_rgba([0, 255, 0, 255], 2, 2), 2, 2),
207+
(1.5, solid_rgba([128, 0, 128, 255], 2, 2), 2, 2),
208+
(2.0, solid_rgba([255, 165, 0, 255], 2, 2), 2, 2),
209+
];
210+
video_frame_cache().insert(cache_key, Arc::new(frames));
211+
212+
let v = video(&src, ImageFit::Fill, Some(0.0), Some(1.0), None);
213+
let ctx = ctx_at(1.8);
214+
let pixels = paint_and_read(&v, &ctx, 20, 20, false);
215+
216+
assert_eq!(
217+
px(&pixels, 20, 10, 10),
218+
[0, 255, 0, 255],
219+
"past trim_end, playback must clamp to the frame at trim_end (green), not the frame \
220+
nearest the unclamped query time (orange)"
221+
);
222+
}
223+
224+
// ─── `loop_video` made neither the picture nor the audio loop ─────────────
225+
226+
/// Cache frames only cover `[0.0, 1.0)`; `trim_end: Some(1.0)` gives
227+
/// `loop_video` a window to wrap within without needing a real source file
228+
/// to probe. Querying at `ctx.time = 2.1` (raw source time 2.1s, i.e. "2
229+
/// full loops plus 0.1s") must land near 0.1s once wrapped — nearest to
230+
/// that among `{0.0, 0.25, 0.5, 0.75}` is red. Before this fix, the same
231+
/// query — with the trim-end clamp from the previous test already in place
232+
/// but no loop branch yet — clamped to `min(2.1, 1.0) = 1.0`, whose nearest
233+
/// cached frame is yellow: a clearly different pixel, which is what proves
234+
/// this test is exercising the loop path and not being masked by the clamp.
235+
#[test]
236+
fn loop_video_wraps_playback_within_the_trim_window() {
237+
let src = unique_src("loop");
238+
let cache_key = format!("{src}:20x20");
239+
let frames = vec![
240+
(0.0, solid_rgba([255, 0, 0, 255], 2, 2), 2, 2),
241+
(0.25, solid_rgba([0, 255, 0, 255], 2, 2), 2, 2),
242+
(0.5, solid_rgba([0, 0, 255, 255], 2, 2), 2, 2),
243+
(0.75, solid_rgba([255, 255, 0, 255], 2, 2), 2, 2),
244+
];
245+
video_frame_cache().insert(cache_key, Arc::new(frames));
246+
247+
let v = video(&src, ImageFit::Fill, Some(0.0), Some(1.0), Some(true));
248+
let ctx = ctx_at(2.1);
249+
let pixels = paint_and_read(&v, &ctx, 20, 20, false);
250+
251+
assert_eq!(
252+
px(&pixels, 20, 10, 10),
253+
[255, 0, 0, 255],
254+
"looping must wrap the query time back into the window (nearest: red), not clamp to \
255+
the window's own end (nearest: yellow)"
256+
);
257+
}
258+
259+
/// Without `loop_video`, a `trim_end`-bounded video must still clamp
260+
/// (unaffected by the loop branch existing) rather than wrap — the same
261+
/// scenario as the wrap test above, minus the flag.
262+
#[test]
263+
fn without_loop_video_playback_still_clamps_not_wraps() {
264+
let src = unique_src("no-loop");
265+
let cache_key = format!("{src}:20x20");
266+
let frames = vec![
267+
(0.0, solid_rgba([255, 0, 0, 255], 2, 2), 2, 2),
268+
(0.25, solid_rgba([0, 255, 0, 255], 2, 2), 2, 2),
269+
(0.5, solid_rgba([0, 0, 255, 255], 2, 2), 2, 2),
270+
(0.75, solid_rgba([255, 255, 0, 255], 2, 2), 2, 2),
271+
];
272+
video_frame_cache().insert(cache_key, Arc::new(frames));
273+
274+
let v = video(&src, ImageFit::Fill, Some(0.0), Some(1.0), None);
275+
let ctx = ctx_at(2.1);
276+
let pixels = paint_and_read(&v, &ctx, 20, 20, false);
277+
278+
assert_eq!(
279+
px(&pixels, 20, 10, 10),
280+
[255, 255, 0, 255],
281+
"no loop_video: must clamp to the window's end (nearest: yellow), not wrap"
282+
);
283+
}
284+
285+
// ─── cached-frame draw path mistagged straight alpha as premultiplied ──────
286+
287+
/// ffmpeg's `-pix_fmt rgba` output — what fills the video-frame cache — is
288+
/// straight (unpremultiplied) alpha. Tagging that buffer `AlphaType::Premul`
289+
/// makes Skia treat the RGB channels as already scaled by alpha instead of
290+
/// scaling them itself, which brightens (here: doubles) every
291+
/// semi-transparent pixel's channels once composited.
292+
///
293+
/// A straight-alpha (200, 100, 50, 128) pixel, composited over black:
294+
/// correctly tagged `Unpremul`, Skia premultiplies it to
295+
/// (200×128/255, 100×128/255, 50×128/255) ≈ (100, 50, 25) before compositing
296+
/// over black, landing there almost exactly (the `(1 - alpha) * 0` background
297+
/// term vanishes either way). Mistagged `Premul`, Skia uses the raw channel
298+
/// values directly as if already scaled — (200, 100, 50) — composited over
299+
/// black with no further scaling, landing at roughly double the correct
300+
/// result.
301+
#[test]
302+
fn cached_frame_straight_alpha_composites_correctly_not_doubled() {
303+
let src = unique_src("alpha");
304+
let cache_key = format!("{src}:10x10");
305+
let straight = [200u8, 100, 50, 128];
306+
video_frame_cache().insert(
307+
cache_key,
308+
Arc::new(vec![(0.0, solid_rgba(straight, 2, 2), 2, 2)]),
309+
);
310+
311+
let v = video(&src, ImageFit::Fill, None, None, None);
312+
let ctx = ctx_at(0.0);
313+
let pixels = paint_and_read(&v, &ctx, 10, 10, false);
314+
let composited = px(&pixels, 10, 5, 5);
315+
316+
let close = |actual: u8, expected: u8| (actual as i16 - expected as i16).abs() <= 4;
317+
assert!(
318+
close(composited[0], 100) && close(composited[1], 50) && close(composited[2], 25),
319+
"straight-alpha (200,100,50,128) over black must composite to roughly (100,50,25), \
320+
got {composited:?} — a value near (200,100,50) means the buffer is still mistagged \
321+
as premultiplied and its channels are being used unscaled"
322+
);
323+
}

0 commit comments

Comments
 (0)