New PicParamSetId to perform proper bounds check

Using a single ParamSetId had the problem that Ids for pic_parameter_set
and seq_parameter_set have different maximum allowed values.  This change
removes that original type and replaces it with seperate PicParamSetId and
SeqParamSetId types.

SeqParamSetId enforces a limit of 31 as before, but PicParamSetId enforces a
limit of 255.

Fixes #56

CHANGELOG.md

@@ -2,6 +2,11 @@
 
 ## Unreleased
 
+### Changed
+
+*   BREAKING CHANGE: The `ParamSetId` type has been removed and replaced with separate `PicParamSetId` and
+    `SeqParamSetId` types, since the allowed range of values needs to be different in these two usages.
+
 ## 0.7.0 - 2023-05-30
 
 ### Changed

src/avcc.rs

@@ -195,7 +195,8 @@ impl<'buf> Iterator for ParamSetIter<'buf> {
 #[cfg(test)]
 mod test {
     use super::*;
-    use crate::nal::pps::ParamSetId;
+    use crate::nal::pps::PicParamSetId;
+    use crate::nal::sps::SeqParamSetId;
     use hex_literal::*;
 
     #[test]
@@ -214,13 +215,16 @@ mod test {
         assert!(!flags.flag5());
         let ctx = avcc.create_context().unwrap();
         let sps = ctx
-            .sps_by_id(ParamSetId::from_u32(0).unwrap())
+            .sps_by_id(SeqParamSetId::from_u32(0).unwrap())
             .expect("missing sps");
         assert_eq!(avcc.avc_level_indication(), sps.level());
         assert_eq!(avcc.avc_profile_indication(), sps.profile_idc);
-        assert_eq!(ParamSetId::from_u32(0).unwrap(), sps.seq_parameter_set_id);
+        assert_eq!(
+            SeqParamSetId::from_u32(0).unwrap(),
+            sps.seq_parameter_set_id
+        );
         let _pps = ctx
-            .pps_by_id(ParamSetId::from_u32(0).unwrap())
+            .pps_by_id(PicParamSetId::from_u32(0).unwrap())
             .expect("missing pps");
     }
     #[test]
@@ -235,7 +239,7 @@ mod test {
         let _sps_data = avcc.sequence_parameter_sets().next().unwrap().unwrap();
         let ctx = avcc.create_context().unwrap();
         let _sps = ctx
-            .sps_by_id(ParamSetId::from_u32(0).unwrap())
+            .sps_by_id(SeqParamSetId::from_u32(0).unwrap())
             .expect("missing sps");
     }
 }

src/lib.rs

@@ -24,7 +24,7 @@ impl Context {
         Default::default()
     }
     #[inline]
-    pub fn sps_by_id(&self, id: nal::pps::ParamSetId) -> Option<&nal::sps::SeqParameterSet> {
+    pub fn sps_by_id(&self, id: nal::sps::SeqParamSetId) -> Option<&nal::sps::SeqParameterSet> {
         self.seq_param_sets.get(usize::from(id.id()))
     }
     #[inline]
@@ -37,7 +37,7 @@ impl Context {
         self.seq_param_sets.put(i, sps);
     }
     #[inline]
-    pub fn pps_by_id(&self, id: nal::pps::ParamSetId) -> Option<&nal::pps::PicParameterSet> {
+    pub fn pps_by_id(&self, id: nal::pps::PicParamSetId) -> Option<&nal::pps::PicParameterSet> {
         self.pic_param_sets.get(usize::from(id.id()))
     }
     #[inline]
@@ -90,7 +90,7 @@ mod tests {
     #[test]
     fn map() {
         let mut s = super::ParamSetMap::default();
-        assert_eq!(s.iter().copied().collect::<Vec<_>>(), &[]);
+        assert!(s.iter().copied().collect::<Vec<_>>().is_empty());
         s.put(0, 0);
         assert_eq!(s.iter().copied().collect::<Vec<_>>(), &[0]);
         s.put(2, 2);

src/nal/mod.rs

@@ -444,7 +444,7 @@ mod test {
         r.consume(1);
         assert_eq!(r.fill_buf().unwrap(), &[1, 2, 3, 4]);
         r.consume(4);
-        assert_eq!(r.fill_buf().unwrap(), &[]);
+        assert!(r.fill_buf().unwrap().is_empty());
     }
 
     #[test]

src/nal/pps.rs

@@ -1,4 +1,5 @@
 use super::sps;
+use crate::nal::sps::{SeqParamSetId, SeqParamSetIdError};
 use crate::rbsp::BitRead;
 use crate::{rbsp, Context};
 
@@ -9,9 +10,9 @@ pub enum PpsError {
     InvalidNumSliceGroupsMinus1(u32),
     InvalidNumRefIdx(&'static str, u32),
     InvalidSliceGroupChangeType(u32),
-    UnknownSeqParamSetId(ParamSetId),
-    BadPicParamSetId(ParamSetIdError),
-    BadSeqParamSetId(ParamSetIdError),
+    UnknownSeqParamSetId(SeqParamSetId),
+    BadPicParamSetId(PicParamSetIdError),
+    BadSeqParamSetId(SeqParamSetIdError),
     ScalingMatrix(sps::ScalingMatrixError),
 }
 
@@ -207,18 +208,18 @@ impl PicParameterSetExtra {
 }
 
 #[derive(Debug, PartialEq)]
-pub enum ParamSetIdError {
+pub enum PicParamSetIdError {
     IdTooLarge(u32),
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
-pub struct ParamSetId(u8);
-impl ParamSetId {
-    pub fn from_u32(id: u32) -> Result<ParamSetId, ParamSetIdError> {
-        if id > 31 {
-            Err(ParamSetIdError::IdTooLarge(id))
+pub struct PicParamSetId(u8);
+impl PicParamSetId {
+    pub fn from_u32(id: u32) -> Result<PicParamSetId, PicParamSetIdError> {
+        if id > 255 {
+            Err(PicParamSetIdError::IdTooLarge(id))
         } else {
-            Ok(ParamSetId(id as u8))
+            Ok(PicParamSetId(id as u8))
         }
     }
     pub fn id(self) -> u8 {
@@ -228,8 +229,8 @@ impl ParamSetId {
 
 #[derive(Clone, Debug)]
 pub struct PicParameterSet {
-    pub pic_parameter_set_id: ParamSetId,
-    pub seq_parameter_set_id: ParamSetId,
+    pub pic_parameter_set_id: PicParamSetId,
+    pub seq_parameter_set_id: SeqParamSetId,
     pub entropy_coding_mode_flag: bool,
     pub bottom_field_pic_order_in_frame_present_flag: bool,
     pub slice_groups: Option<SliceGroup>,
@@ -247,9 +248,9 @@ pub struct PicParameterSet {
 }
 impl PicParameterSet {
     pub fn from_bits<R: BitRead>(ctx: &Context, mut r: R) -> Result<PicParameterSet, PpsError> {
-        let pic_parameter_set_id = ParamSetId::from_u32(r.read_ue("pic_parameter_set_id")?)
+        let pic_parameter_set_id = PicParamSetId::from_u32(r.read_ue("pic_parameter_set_id")?)
             .map_err(PpsError::BadPicParamSetId)?;
-        let seq_parameter_set_id = ParamSetId::from_u32(r.read_ue("seq_parameter_set_id")?)
+        let seq_parameter_set_id = SeqParamSetId::from_u32(r.read_ue("seq_parameter_set_id")?)
             .map_err(PpsError::BadSeqParamSetId)?;
         let seq_parameter_set = ctx
             .sps_by_id(seq_parameter_set_id)
@@ -364,4 +365,20 @@ mod test {
             })
         ));
     }
+
+    // Earlier versions of h264-reader incorrectly limited pic_parameter_set_id to at most 32,
+    // while the spec allows up to 255.  Test that a value over 32 is accepted.
+    #[test]
+    fn pps_id_greater32() {
+        // test SPS/PPS values courtesy of @astraw
+        let sps = hex!("42c01643235010020b3cf00f08846a");
+        let pps = hex!("0448e3c8");
+        let sps = sps::SeqParameterSet::from_bits(rbsp::BitReader::new(&sps[..])).unwrap();
+        let mut ctx = Context::default();
+        ctx.put_seq_param_set(sps);
+
+        let pps = PicParameterSet::from_bits(&ctx, rbsp::BitReader::new(&pps[..])).unwrap();
+
+        assert_eq!(pps.pic_parameter_set_id, PicParamSetId(33));
+    }
 }

src/nal/sei/buffering_period.rs

@@ -1,23 +1,23 @@
 use super::SeiMessage;
 use crate::nal::sei::HeaderType;
-use crate::nal::{pps, sps};
+use crate::nal::sps;
 use crate::rbsp::BitRead;
 use crate::rbsp::BitReaderError;
 use crate::Context;
 
 #[derive(Debug)]
 pub enum BufferingPeriodError {
     ReaderError(BitReaderError),
-    UndefinedSeqParamSetId(pps::ParamSetId),
-    InvalidSeqParamSetId(pps::ParamSetIdError),
+    UndefinedSeqParamSetId(sps::SeqParamSetId),
+    InvalidSeqParamSetId(sps::SeqParamSetIdError),
 }
 impl From<BitReaderError> for BufferingPeriodError {
     fn from(e: BitReaderError) -> Self {
         BufferingPeriodError::ReaderError(e)
     }
 }
-impl From<pps::ParamSetIdError> for BufferingPeriodError {
-    fn from(e: pps::ParamSetIdError) -> Self {
+impl From<sps::SeqParamSetIdError> for BufferingPeriodError {
+    fn from(e: sps::SeqParamSetIdError) -> Self {
         BufferingPeriodError::InvalidSeqParamSetId(e)
     }
 }
@@ -56,7 +56,8 @@ impl BufferingPeriod {
     ) -> Result<BufferingPeriod, BufferingPeriodError> {
         assert_eq!(msg.payload_type, HeaderType::BufferingPeriod);
         let mut r = crate::rbsp::BitReader::new(msg.payload);
-        let seq_parameter_set_id = pps::ParamSetId::from_u32(r.read_ue("seq_parameter_set_id")?)?;
+        let seq_parameter_set_id =
+            sps::SeqParamSetId::from_u32(r.read_ue("seq_parameter_set_id")?)?;
         let sps = ctx
             .sps_by_id(seq_parameter_set_id)
             .ok_or_else(|| BufferingPeriodError::UndefinedSeqParamSetId(seq_parameter_set_id))?;

src/nal/slice/mod.rs

@@ -1,5 +1,5 @@
 use crate::nal::pps;
-use crate::nal::pps::{ParamSetId, PicParameterSet};
+use crate::nal::pps::{PicParamSetId, PicParameterSet};
 use crate::nal::sps;
 use crate::nal::sps::SeqParameterSet;
 use crate::nal::NalHeader;
@@ -79,9 +79,9 @@ impl SliceType {
 pub enum SliceHeaderError {
     RbspError(BitReaderError),
     InvalidSliceType(u32),
-    InvalidSeqParamSetId(pps::ParamSetIdError),
-    UndefinedPicParamSetId(pps::ParamSetId),
-    UndefinedSeqParamSetId(pps::ParamSetId),
+    InvalidSeqParamSetId(pps::PicParamSetIdError),
+    UndefinedPicParamSetId(pps::PicParamSetId),
+    UndefinedSeqParamSetId(sps::SeqParamSetId),
     ColourPlaneError(ColourPlaneError),
     InvalidModificationOfPicNumIdc(u32),
     InvalidMemoryManagementControlOperation(u32),
@@ -101,8 +101,8 @@ impl From<BitReaderError> for SliceHeaderError {
         SliceHeaderError::RbspError(e)
     }
 }
-impl From<pps::ParamSetIdError> for SliceHeaderError {
-    fn from(e: pps::ParamSetIdError) -> Self {
+impl From<pps::PicParamSetIdError> for SliceHeaderError {
+    fn from(e: pps::PicParamSetIdError) -> Self {
         SliceHeaderError::InvalidSeqParamSetId(e)
     }
 }
@@ -443,7 +443,7 @@ impl SliceHeader {
     ) -> Result<(SliceHeader, &'a SeqParameterSet, &'a PicParameterSet), SliceHeaderError> {
         let first_mb_in_slice = r.read_ue("first_mb_in_slice")?;
         let slice_type = SliceType::from_id(r.read_ue("slice_type")?)?;
-        let pic_parameter_set_id = ParamSetId::from_u32(r.read_ue("pic_parameter_set_id")?)?;
+        let pic_parameter_set_id = PicParamSetId::from_u32(r.read_ue("pic_parameter_set_id")?)?;
         let pps =
             ctx.pps_by_id(pic_parameter_set_id)
                 .ok_or(SliceHeaderError::UndefinedPicParamSetId(