From 1fe638bcf83d5e9bbad149ea173de4c1ce92072c Mon Sep 17 00:00:00 2001 From: Nikita Rakitin Date: Tue, 13 Oct 2026 20:55:12 +0300 Subject: [PATCH] AV1: reject duplicate film-grain scaling points; a failed job still publishes (#394) The film-grain header accepted scaling points whose values didn't strictly increase, and building the scaling lookup then divided by zero. In threaded decoding the panic left the frame's reference and picture slots unset, so the output and dependent frames waited forever. - The header rejects Y/Cb/Cr point values that don't strictly increase (spec 6.8.20). - The scaling lookup moves into scaling_lut, which skips a non-increasing pair and uses checked indexing (defence in depth). - run_job decodes inside guarded (catch_unwind), so a panic becomes an error published to the picture and reference slots. Co-authored-by: Cursor --- crates/av1/src/decoder.rs | 39 ++++++++++++++++++++++- crates/av1/src/grain.rs | 67 +++++++++++++++++++++++++++++---------- crates/av1/src/header.rs | 23 ++++++++++++++ 3 files changed, 111 insertions(+), 18 deletions(-) diff --git a/crates/av1/src/decoder.rs b/crates/av1/src/decoder.rs index 66c96698..e310feb9 100644 --- a/crates/av1/src/decoder.rs +++ b/crates/av1/src/decoder.rs @@ -633,7 +633,9 @@ impl Decoder { /// Decode a frame job and publish its results. fn run_job(job: FrameJob, pool: &Pool, stats: &Mutex) { let mut st = DecodeStats::default(); - match decode_frame(&job, pool, &mut st) { + // A panic while decoding must still publish an error: the output and every frame that + // references this one wait on these slots. + match guarded(|| decode_frame(&job, pool, &mut st)) { Ok((rf, pic)) => { if let Some(ps) = &job.picture { ps.set(pic.map(Arc::new).ok_or(Error::Invalid("shown frame without picture"))); @@ -652,6 +654,12 @@ fn run_job(job: FrameJob, pool: &Pool, stats: &Mutex) { } } +/// Run `f`, turning a panic into an error (the decoder's own code never panics on purpose; this +/// keeps a bug from stranding the job's waiters). +fn guarded(f: impl FnOnce() -> Result) -> Result { + std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)).unwrap_or(Err(Error::Invalid("frame decode panicked"))) +} + /// The whole decoding process of one frame: setup, tiles, post-filters, motion vector storage /// and the output picture. fn decode_frame(job: &FrameJob, pool: &Pool, stats: &mut DecodeStats) -> Result<(Arc, Option)> { @@ -947,3 +955,32 @@ fn project(fs: &mut FrameState, src: usize, dst_sign: i32) -> bool { } true } + +#[cfg(test)] +mod guard_tests { + use super::*; + + #[test] + fn a_panicking_job_still_publishes_an_error_to_its_waiters() { + let slot: Arc> = Slot::new(); + let picture: Arc> = Slot::new(); + // Waiters: the output and a dependent frame, on their own threads. + let (tx, rx) = std::sync::mpsc::channel(); + for s in [slot.clone(), picture.clone()] { + let tx = tx.clone(); + std::thread::spawn(move || { + let _ = tx.send(s.wait().is_err()); + }); + } + let (s, p) = (slot.clone(), picture.clone()); + std::thread::spawn(move || { + let r: Result = guarded(|| panic!("hostile frame")); + p.set(r.clone()); + s.set(r); + }); + for _ in 0..2 { + assert_eq!(rx.recv_timeout(std::time::Duration::from_secs(10)), Ok(true)); + } + assert_eq!(guarded(|| Ok(5)).ok(), Some(5)); + } +} diff --git a/crates/av1/src/grain.rs b/crates/av1/src/grain.rs index d4baad62..4222c672 100644 --- a/crates/av1/src/grain.rs +++ b/crates/av1/src/grain.rs @@ -5,6 +5,39 @@ use crate::header::{FilmGrainParams, SequenceHeader}; use crate::spec_tables::{GAUSSIAN_SEQUENCE, MC_IDENTITY}; #[inline] +/// One plane's scaling lookup (7.18.3.4) from its `n` points. The header rejects points whose +/// values don't strictly increase; a pair that still doesn't (defence in depth) is skipped +/// instead of dividing by zero. No points: all zero. +fn scaling_lut(n: usize, xs: &[u8], ys: &[u8]) -> [i32; 256] { + let mut l = [0i32; 256]; + let n = n.min(xs.len()).min(ys.len()); + let (Some(&x0), Some(&y0)) = (xs.first(), ys.first()) else { return l }; + if n == 0 { + return l; + } + for v in l.iter_mut().take(x0 as usize) { + *v = y0 as i32; + } + for i in 0..n - 1 { + let (xa, xb, ya, yb) = (xs[i] as i32, xs[i + 1] as i32, ys[i] as i32, ys[i + 1] as i32); + let dx = xb - xa; + if dx <= 0 { + continue; + } + let delta = (yb - ya) * ((65536 + (dx >> 1)) / dx); + for x in 0..dx { + if let Some(v) = l.get_mut((xa + x) as usize) { + *v = ya + ((x * delta + 32768) >> 16); + } + } + } + let (xl, yl) = (xs[n - 1], ys[n - 1]); + for v in l.iter_mut().skip(xl as usize) { + *v = yl as i32; + } + l +} + fn round2(x: i32, n: u32) -> i32 { if n == 0 { x } else { (x + (1 << (n - 1))) >> n } } @@ -124,23 +157,7 @@ pub(crate) fn apply(pic: &mut Picture, g: &FilmGrainParams, seq: &SequenceHeader } else { (g.num_cr_points, &g.point_cr_value, &g.point_cr_scaling) }; - if n == 0 { - continue; - } - for v in l.iter_mut().take(xs[0] as usize) { - *v = ys[0] as i32; - } - for i in 0..n - 1 { - let dy = ys[i + 1] as i32 - ys[i] as i32; - let dx = xs[i + 1] as i32 - xs[i] as i32; - let delta = dy * ((65536 + (dx >> 1)) / dx); - for x in 0..dx { - l[xs[i] as usize + x as usize] = ys[i] as i32 + ((x * delta + 32768) >> 16); - } - } - for v in l.iter_mut().skip(xs[n - 1] as usize) { - *v = ys[n - 1] as i32; - } + *l = scaling_lut(n, xs, ys); } let scale_lut = |plane: usize, index: i32| -> i32 { let sh = bd - 8; @@ -274,3 +291,19 @@ pub(crate) fn apply(pic: &mut Picture, g: &FilmGrainParams, seq: &SequenceHeader } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn scaling_lut_interpolates_and_tolerates_duplicate_points() { + let l = scaling_lut(2, &[0, 255], &[0, 255]); + assert_eq!((l[0], l[128], l[255]), (0, 128, 255)); + // Duplicate and descending x values don't divide by zero. + let l = scaling_lut(4, &[10, 10, 5, 200], &[20, 40, 60, 80]); + assert_eq!(l[0], 20); + assert_eq!(l[255], 80); + assert_eq!(scaling_lut(0, &[], &[]), [0; 256]); + } +} diff --git a/crates/av1/src/header.rs b/crates/av1/src/header.rs index fa9db0a6..16cccd9e 100644 --- a/crates/av1/src/header.rs +++ b/crates/av1/src/header.rs @@ -1256,6 +1256,7 @@ impl FrameHeader { g.point_y_value[i] = r.f(8)? as u8; g.point_y_scaling[i] = r.f(8)? as u8; } + check_increasing(&g.point_y_value[..g.num_y_points], "point_y_value")?; g.chroma_scaling_from_luma = if seq.color.mono_chrome { false } else { r.flag()? }; if seq.color.mono_chrome || g.chroma_scaling_from_luma @@ -1272,6 +1273,7 @@ impl FrameHeader { g.point_cb_value[i] = r.f(8)? as u8; g.point_cb_scaling[i] = r.f(8)? as u8; } + check_increasing(&g.point_cb_value[..g.num_cb_points], "point_cb_value")?; g.num_cr_points = r.f(4)? as usize; if g.num_cr_points > 10 { return Err(Error::Invalid("num_cr_points")); @@ -1280,6 +1282,7 @@ impl FrameHeader { g.point_cr_value[i] = r.f(8)? as u8; g.point_cr_scaling[i] = r.f(8)? as u8; } + check_increasing(&g.point_cr_value[..g.num_cr_points], "point_cr_value")?; } g.grain_scaling_minus_8 = r.f(2)?; g.ar_coeff_lag = r.f(2)?; @@ -1367,3 +1370,23 @@ fn decode_subexp(r: &mut BitReader, num_syms: i32) -> Result { } } } + +/// Film-grain scaling point values must strictly increase (AV1 spec 6.8.20); a duplicate would +/// divide by zero when the scaling lookup is built. +fn check_increasing(values: &[u8], what: &'static str) -> Result<()> { + if values.windows(2).all(|w| w[0] < w[1]) { Ok(()) } else { Err(Error::Invalid(what)) } +} + +#[cfg(test)] +mod grain_point_tests { + use super::*; + + #[test] + fn scaling_points_must_strictly_increase() { + assert!(check_increasing(&[], "p").is_ok()); + assert!(check_increasing(&[7], "p").is_ok()); + assert!(check_increasing(&[0, 10, 255], "p").is_ok()); + assert!(matches!(check_increasing(&[0, 10, 10], "point_y_value"), Err(Error::Invalid("point_y_value")))); + assert!(check_increasing(&[20, 10], "p").is_err()); + } +}