Merge pull request #336 from mozilla/permit-hdlr-nonzero-reserved

Allow `HandlerBox` (`hdlr`) non-zero values in `reserved` and `pre-defined` fields

Author: baumanj

mp4parse/src/lib.rs

@@ -4410,8 +4410,9 @@ fn read_alac<T: Read>(src: &mut BMFFBox<T>) -> Result<ALACSpecificBox> {
     Ok(ALACSpecificBox { version, data })
 }
 
-/// Parse a Handler Reference Box.
-/// See ISOBMFF (ISO 14496-12:2020) § 8.4.3
+/// Parse a Handler Reference Box.<br />
+/// See ISOBMFF (ISO 14496-12:2020) § 8.4.3<br />
+/// See [\[ISOBMFF\]: reserved (field = 0;) handling is ambiguous](https://github.com/MPEGGroup/FileFormat/issues/36)
 fn read_hdlr<T: Read>(src: &mut BMFFBox<T>, strictness: ParseStrictness) -> Result<HandlerBox> {
     if read_fullbox_version_no_flags(src)? != 0 {
         return Err(Error::Unsupported("hdlr version"));
@@ -4420,7 +4421,7 @@ fn read_hdlr<T: Read>(src: &mut BMFFBox<T>, strictness: ParseStrictness) -> Resu
     let pre_defined = be_u32(src)?;
     if pre_defined != 0 {
         fail_if(
-            strictness != ParseStrictness::Permissive,
+            strictness == ParseStrictness::Strict,
             "The HandlerBox 'pre_defined' field shall be 0 \
              per ISOBMFF (ISO 14496-12:2020) § 8.4.3.2",
         )?;
@@ -4432,7 +4433,7 @@ fn read_hdlr<T: Read>(src: &mut BMFFBox<T>, strictness: ParseStrictness) -> Resu
         let reserved = be_u32(src)?;
         if reserved != 0 {
             fail_if(
-                strictness != ParseStrictness::Permissive,
+                strictness == ParseStrictness::Strict,
                 "The HandlerBox 'reserved' fields shall be 0 \
                  per ISOBMFF (ISO 14496-12:2020) § 8.4.3.2",
             )?;

mp4parse/src/tests.rs

@@ -515,7 +515,7 @@ fn read_hdlr_invalid_pre_defined_field() {
     let mut stream = iter.next_box().unwrap().unwrap();
     assert_eq!(stream.head.name, BoxType::HandlerBox);
     assert_eq!(stream.head.size, 32);
-    match super::read_hdlr(&mut stream, ParseStrictness::Normal) {
+    match super::read_hdlr(&mut stream, ParseStrictness::Strict) {
         Err(Error::InvalidData(msg)) => assert_eq!(
             "The HandlerBox 'pre_defined' field shall be 0 \
              per ISOBMFF (ISO 14496-12:2020) § 8.4.3.2",
@@ -537,7 +537,7 @@ fn read_hdlr_invalid_reserved_field() {
     let mut stream = iter.next_box().unwrap().unwrap();
     assert_eq!(stream.head.name, BoxType::HandlerBox);
     assert_eq!(stream.head.size, 32);
-    match super::read_hdlr(&mut stream, ParseStrictness::Normal) {
+    match super::read_hdlr(&mut stream, ParseStrictness::Strict) {
         Err(Error::InvalidData(msg)) => assert_eq!(
             "The HandlerBox 'reserved' fields shall be 0 \
              per ISOBMFF (ISO 14496-12:2020) § 8.4.3.2",

mp4parse/tests/hdlr-nonzero-reserved.avif

@@ -47,6 +47,7 @@ static IMAGE_AVIF_IPMA_INVALID_PROPERTY_INDEX: &str =
 static IMAGE_AVIF_NO_HDLR: &str = "tests/corrupt/hdlr-not-first.avif";
 static IMAGE_AVIF_HDLR_NOT_FIRST: &str = "tests/corrupt/no-hdlr.avif";
 static IMAGE_AVIF_HDLR_NOT_PICT: &str = "tests/corrupt/hdlr-not-pict.avif";
+static IMAGE_AVIF_HDLR_NONZERO_RESERVED: &str = "tests/hdlr-nonzero-reserved.avif";
 static IMAGE_AVIF_NO_MIF1: &str = "tests/no-mif1.avif";
 static IMAGE_AVIF_NO_PIXI: &str = "tests/corrupt/no-pixi.avif";
 static IMAGE_AVIF_NO_AV1C: &str = "tests/corrupt/no-av1C.avif";
@@ -934,6 +935,15 @@ fn public_avif_hdlr_is_pict() {
     assert_avif_shall(IMAGE_AVIF_HDLR_NOT_PICT, expected_msg);
 }
 
+#[test]
+fn public_avif_hdlr_nonzero_reserved() {
+    let expected_msg = "The HandlerBox 'reserved' fields shall be 0 \
+                        per ISOBMFF (ISO 14496-12:2020) § 8.4.3.2";
+    // This is a "should" despite the spec indicating a (somewhat ambiguous)
+    // requirement that this field is set to zero.
+    assert_avif_should(IMAGE_AVIF_HDLR_NONZERO_RESERVED, expected_msg);
+}
+
 #[test]
 fn public_avif_no_mif1() {
     let expected_msg = "The FileTypeBox should contain 'mif1' in the compatible_brands list \