From 78ab8f062629894793d9270d7687deb212f89613 Mon Sep 17 00:00:00 2001 From: SusieeTheFemboy <96988330+SusieeTheLinuxUser@users.noreply.github.com> Date: Sat, 24 Oct 2026 00:09:16 +0200 Subject: [PATCH 1/3] H.264: temporal direct mvs are scaled in i32 and clipped to the spec range MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The temporal direct derivation truncated (mvCol · DistScaleFactor + 128) >> 8 with `as i16`, wrapping mvL0 past the spec's motion-vector range on crafted streams, and mvL0 − mvCol then panicked a debug build with "attempt to subtract with overflow". The scale and the subtraction now run in i32 (dsf is already clamped to ±1024, so they cannot overflow) and only the results are clipped to −2^15..2^15−1, as ITU-T H.264 8.4.1.2.4 reads — mvL1 scaled from the unclamped mvL0, the shape review settled for the same decoder code in SoundCraft (soundcraft#994): dsf 512, mvCol 32767 keeps mvL1 at 32767. Extends the grouped robustness audit's codec-overflow list (#1183), which covers the POC/MMCO/latency sites but not this one. Co-Authored-By: GLM-5.3 --- crates/h264/src/slicedec.rs | 52 ++++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 7 deletions(-) diff --git a/crates/h264/src/slicedec.rs b/crates/h264/src/slicedec.rs index 10132d68..fee675a2 100644 --- a/crates/h264/src/slicedec.rs +++ b/crates/h264/src/slicedec.rs @@ -566,6 +566,19 @@ impl<'a> SliceDecoder<'a> { (refs, mvs, false) } + /// Temporal direct mvs (8.4.1.2.4) from the co-located mv and `DistScaleFactor` (clamped + /// to ±1024 in `SliceDecoder`): mvL0 = clip_mv((mvCol · dsf + 128) >> 8), mvL1 = + /// clip_mv(mvL0 − mvCol), both clipped to the spec's motion-vector range −2^15..2^15−1. + /// The i32 maths cannot overflow; a truncating `as i16` used to wrap mvL0 past the range, + /// after which the mvL1 subtraction overflowed a debug build. mvL1 is scaled from the + /// unclamped mvL0 with only the results clipped, as the sibling SoundCraft decoder's + /// review established (soundcraft#994): dsf 512, mvCol 32767 keeps mvL1 at 32767. + fn temporal_direct_mvs(dsf: i32, mv_col: [i16; 2]) -> ([i16; 2], [i16; 2]) { + let clamp = |v: i32| v.clamp(-(1 << 15), (1 << 15) - 1) as i16; + let m0 = mv_col.map(|c| (dsf * i32::from(c) + 128) >> 8); + (m0.map(clamp), [clamp(m0[0] - i32::from(mv_col[0])), clamp(m0[1] - i32::from(mv_col[1]))]) + } + /// Derive direct-mode motion for the 8x8 blocks in `b8_mask` of the current MB. fn derive_direct(&mut self, b8_mask: u8) -> Result<()> { let spatial = self.sh.direct_spatial_mv_pred; @@ -600,13 +613,7 @@ impl<'a> SliceDecoder<'a> { } }; let (dsf, ltr) = self.tdirect.get(ref0 as usize).copied().unwrap_or((256, true)); - let (mv0, mv1) = if ltr { - (mv_col, [0, 0]) - } else { - let s = |c: i16| ((dsf * c as i32 + 128) >> 8) as i16; - let m0 = [s(mv_col[0]), s(mv_col[1])]; - (m0, [m0[0] - mv_col[0], m0[1] - mv_col[1]]) - }; + let (mv0, mv1) = if ltr { (mv_col, [0, 0]) } else { Self::temporal_direct_mvs(dsf, mv_col) }; ([ref0, 0], [mv0, mv1]) }; self.set_motion(x, y, unit, unit, refs, mvs); @@ -1467,3 +1474,34 @@ impl<'a> SliceDecoder<'a> { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::SliceDecoder; + + /// Temporal direct mvs stay inside the spec's motion-vector range −2^15..2^15−1 with a + /// large `DistScaleFactor` and a co-located mv at an i16 extreme: the scale used to + /// truncate past the range with `as i16`, and mvL0 − mvCol then overflowed a debug build + /// ("attempt to subtract with overflow"). `dsf` 256 is the identity scale. + #[test] + fn temporal_direct_mvs_at_i16_extremes_stay_in_the_spec_range() { + for dsf in [-1024i32, -1, 256, 1023] { + for col in [[i16::MIN, i16::MIN], [i16::MAX, i16::MAX], [i16::MIN, i16::MAX]] { + let (mv0, mv1) = SliceDecoder::temporal_direct_mvs(dsf, col); + for mv in [mv0, mv1] { + for c in mv { + assert!((-(1 << 15)..(1 << 15)).contains(&i32::from(c)), "dsf {dsf}, col {col:?}: {mv:?}"); + } + } + if dsf == 256 { + assert_eq!((mv0, mv1), (col, [0, 0]), "dsf 256 is the identity scale"); + } + // soundcraft#994: mvL1 is scaled from the unclamped mvL0 with only the results + // clipped — deriving it from the clipped mvL0 gave [0, 0] here. + if dsf == 512 && col == [i16::MAX, 0] { + assert_eq!((mv0, mv1), ([i16::MAX, 0], [i16::MAX, 0]), "mvL1 keeps mvL0's off-range part"); + } + } + } + } +} From 48c5ff67bf42ce58bf2f6cd9436bb02050b5f8b7 Mon Sep 17 00:00:00 2001 From: SusieeTheFemboy <96988330+SusieeTheLinuxUser@users.noreply.github.com> Date: Sat, 24 Oct 2026 00:40:21 +0200 Subject: [PATCH 2/3] H.264: the temporal-direct test runs the dsf 512 case The dsf 512 / mvCol [32767, 0] check sat inside loops that never produce that pair, so it never ran. It is now a direct assertion after the sweep. Co-Authored-By: Claude Opus 5.5 --- crates/h264/src/slicedec.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/crates/h264/src/slicedec.rs b/crates/h264/src/slicedec.rs index fee675a2..2ada5159 100644 --- a/crates/h264/src/slicedec.rs +++ b/crates/h264/src/slicedec.rs @@ -1496,12 +1496,11 @@ mod tests { if dsf == 256 { assert_eq!((mv0, mv1), (col, [0, 0]), "dsf 256 is the identity scale"); } - // soundcraft#994: mvL1 is scaled from the unclamped mvL0 with only the results - // clipped — deriving it from the clipped mvL0 gave [0, 0] here. - if dsf == 512 && col == [i16::MAX, 0] { - assert_eq!((mv0, mv1), ([i16::MAX, 0], [i16::MAX, 0]), "mvL1 keeps mvL0's off-range part"); - } } } + // soundcraft#994: mvL1 is scaled from the unclamped mvL0 with only the results clipped; + // deriving it from the clipped mvL0 gave [0, 0] here. + let (mv0, mv1) = SliceDecoder::temporal_direct_mvs(512, [i16::MAX, 0]); + assert_eq!((mv0, mv1), ([i16::MAX, 0], [i16::MAX, 0]), "mvL1 keeps mvL0's off-range part"); } } From 14cc73101b37a7c2305ff3763a29eaf28bd72eaa Mon Sep 17 00:00:00 2001 From: SusieeTheFemboy <96988330+SusieeTheLinuxUser@users.noreply.github.com> Date: Sun, 1 Nov 2026 16:46:05 +0100 Subject: [PATCH 3/3] H.264: cite temporal direct as 8.4.1.2.3 Co-Authored-By: Claude Opus 5.5 --- crates/h264/src/slicedec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/h264/src/slicedec.rs b/crates/h264/src/slicedec.rs index 2ada5159..557a94d4 100644 --- a/crates/h264/src/slicedec.rs +++ b/crates/h264/src/slicedec.rs @@ -566,7 +566,7 @@ impl<'a> SliceDecoder<'a> { (refs, mvs, false) } - /// Temporal direct mvs (8.4.1.2.4) from the co-located mv and `DistScaleFactor` (clamped + /// Temporal direct mvs (8.4.1.2.3) from the co-located mv and `DistScaleFactor` (clamped /// to ±1024 in `SliceDecoder`): mvL0 = clip_mv((mvCol · dsf + 128) >> 8), mvL1 = /// clip_mv(mvL0 − mvCol), both clipped to the spec's motion-vector range −2^15..2^15−1. /// The i32 maths cannot overflow; a truncating `as i16` used to wrap mvL0 past the range,