Merge PR #481 from NikitkaRa: AV1: reject duplicate film-grain scaling points; a failed job still publishes (#394)

# Conflicts:
#	crates/av1/src/decoder.rs

crates/av1/src/decoder.rs

@@ -634,7 +634,9 @@ impl Decoder {
 /// Decode a frame job and publish its results.
 fn run_job(job: FrameJob, pool: &Pool, stats: &Mutex<DecodeStats>) {
     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")));
@@ -653,6 +655,12 @@ fn run_job(job: FrameJob, pool: &Pool, stats: &Mutex<DecodeStats>) {
     }
 }
 
+/// 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<R>(f: impl FnOnce() -> Result<R>) -> Result<R> {
+    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<RefFrame>, Option<Picture>)> {
@@ -971,3 +979,32 @@ mod tests {
         assert!(check_tile_group_start(2, 3).is_err());
     }
 }
+
+#[cfg(test)]
+mod guard_tests {
+    use super::*;
+
+    #[test]
+    fn a_panicking_job_still_publishes_an_error_to_its_waiters() {
+        let slot: Arc<Slot<u32>> = Slot::new();
+        let picture: Arc<Slot<u32>> = 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<u32> = 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));
+    }
+}

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

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