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()); + } +}