Merge pull request #1492 from marcoarnulfo/fix/hevc-pps-trailing-zero

fix(codecs): in-band parameter sets compare without trailing zero bytes

crates/codecs/src/hw.rs

@@ -175,9 +175,10 @@ impl StreamInfo {
     /// with (parameter sets, a sequence header, a key frame of another format).
     pub fn parameters_changed(&self, sample: &[u8]) -> bool {
         match self {
-            Self::Nal(i) => {
-                i.nals(sample).into_iter().any(|n| n.first().is_some_and(|&h| i.is_parameter_set(i.nal_type(h))) && !i.parameter_sets.iter().any(|p| p == n))
-            }
+            Self::Nal(i) => i.nals(sample).into_iter().any(|n| {
+                n.first().is_some_and(|&h| i.is_parameter_set(i.nal_type(h)))
+                    && !i.parameter_sets.iter().any(|p| without_trailing_zeros(p) == without_trailing_zeros(n))
+            }),
             Self::Frame(i) => i.parameters_changed(sample),
         }
     }
@@ -199,6 +200,14 @@ impl StreamInfo {
     }
 }
 
+/// A NAL unit without the zero bytes after its last byte: a NAL unit never ends with 0x00
+/// (H.264 7.4.1, H.265 7.4.2), so they are trailing_zero_8bits some muxers keep (ffmpeg in `hvcC`), not
+/// part of the parameter set.
+fn without_trailing_zeros(nal: &[u8]) -> &[u8] {
+    let end = nal.iter().rposition(|&b| b != 0).map_or(0, |i| i + 1);
+    nal.get(..end).unwrap_or_default()
+}
+
 /// Length-prefixed parameter-set entries (`u16` length + bytes) starting at `pos`.
 fn read_sets(rec: &[u8], pos: &mut usize, count: usize, out: &mut Vec<Vec<u8>>) -> Result<()> {
     for _ in 0..count {
@@ -402,13 +411,12 @@ mod tests {
         }
     }
 
-    #[test]
-    fn nal_splitting_is_bounded() {
-        let info = NalStreamInfo {
+    fn hevc_info(parameter_sets: Vec<Vec<u8>>) -> NalStreamInfo {
+        NalStreamInfo {
             codec: NalCodec::Hevc,
             length_size: 4,
             highest_tid: None,
-            parameter_sets: Vec::new(),
+            parameter_sets,
             coded: (16, 16),
             crop: (0, 0, 16, 16),
             chroma_format_idc: 1,
@@ -419,11 +427,33 @@ mod tests {
             color: filmcraft_color::ColorInfo::REC709,
             par: (1, 1),
             reorder: 0,
-        };
+        }
+    }
+
+    #[test]
+    fn nal_splitting_is_bounded() {
+        let info = hevc_info(Vec::new());
         // CRA (21) then a length that runs past the end
         let s = [0, 0, 0, 2, 21 << 1, 1, 0xff, 0xff, 0xff, 0xff, 1];
         assert_eq!(info.nal_types(&s), vec![21]);
         assert!(info.is_irap(&s));
         assert!(!info.is_irap(&[0, 0, 0, 2, 1 << 1, 1]));
     }
+
+    #[test]
+    fn parameter_sets_compare_without_trailing_zero_bytes() {
+        // the hvcC PPS of an ffmpeg-muxed file and the in-band one of its first sample (#1483)
+        let hvcc_pps = vec![0x44, 0x01, 0xe0, 0xf7, 0xc0, 0x4c, 0x90, 0x00];
+        let info = StreamInfo::Nal(hevc_info(vec![hvcc_pps.clone()]));
+        let sample = |nal: &[u8]| {
+            let mut v = (nal.len() as u32).to_be_bytes().to_vec();
+            v.extend_from_slice(nal);
+            v
+        };
+        assert!(!info.parameters_changed(&sample(&hvcc_pps)), "the same PPS");
+        assert!(!info.parameters_changed(&sample(&hvcc_pps[..7])), "a trailing zero byte is not part of the NAL unit");
+        assert!(!info.parameters_changed(&sample(&[&hvcc_pps[..], &[0, 0]].concat())), "nor are several");
+        assert!(info.parameters_changed(&sample(&hvcc_pps[..6])), "a missing byte that is not zero is a change");
+        assert!(info.parameters_changed(&sample(&[0x44, 0x01, 0xe0, 0xf7, 0xc0, 0x4c, 0x91])), "another PPS is a change");
+    }
 }

crates/platform/tests/fallback.rs

@@ -97,9 +97,10 @@ fn changed_in_band_parameter_sets_switch_to_software() {
     let out = decode_all(&mut d, &same);
     assert!(d.is_hardware());
     assert_same("repeated SPS", &out, &decode_all(filmcraft_codecs::software_video_decoder(&s.entry).unwrap().as_mut(), &same));
-    // a different SPS (same values, trailing zero byte) at the second sync sample: software
+    // a different SPS (same values, another nal_ref_idc: a trailing zero byte is not a change) at
+    // the second sync sample: software
     let mut changed_sps = sps.clone();
-    changed_sps.push(0);
+    changed_sps[0] ^= 0x40;
     let k = (1..s.samples.len()).find(|&i| s.sync[i]).unwrap();
     let changed: Vec<_> = s.samples.iter().enumerate().map(|(i, (x, p))| (if i == k { with_sps(&changed_sps, x) } else { x.clone() }, *p)).collect();
     let mut d = hybrid(&s, usize::MAX);