Make duplicate colr detection per colour_type in video sample entries

Follow-up to #450. ISOBMFF permits multiple colr boxes in a
VisualSampleEntry, at most one for a given colour_type, so a valid
nclx + ICC pairing should not trigger ColrBadQuantityBMFF. Track the
colour types seen and flag only a repeated colour_type, matching the
HEIF item-property handling of ColrBadQuantity.

Also reword the ColrBadQuantityBMFF description, which still claimed a
sample entry shall have at most one colr box, contradicting the spec
text cited at the check site, and add regression tests pinning the
Strict-only rejection of repeated colour types.

Author: kinetiknz

mp4parse/src/lib.rs

@@ -467,8 +467,8 @@ impl From<Status> for &str {
                  per HEIF (ISO/IEC DIS 23008-12) § 6.5.5.1"
             }
             Status::ColrBadQuantityBMFF => {
-                "Each sample entry shall have at most one ColourInformationBox (colr) \
-                 per ISOBMFF (ISO 14496-12:2020) § 12.1.5"
+                "Each sample entry should have at most one ColourInformationBox (colr) \
+                 for a given value of colour_type per ISOBMFF (ISO 14496-12:2020) § 12.1.5"
             }
             Status::ColrBadSize => {
                 "Unexpected size for colr box"
@@ -3636,13 +3636,16 @@ fn read_ipco<T: Read>(
         let property = match b.head.name {
             BoxType::AuxiliaryTypeProperty => ItemProperty::AuxiliaryType(read_auxc(&mut b)?),
             BoxType::AV1CodecConfigurationBox => ItemProperty::AV1Config(read_av1c(&mut b)?),
-            BoxType::ColourInformationBox => match read_colr(&mut b, strictness)? {
-                ParsedColourInformation::Supported(colr) => ItemProperty::Colour(colr),
-                ParsedColourInformation::Unsupported(colour_type) => {
-                    error!("read_colr colour_type: {colour_type:?}");
-                    return Status::ColrBadType.into();
+            BoxType::ColourInformationBox => {
+                let colour_type = be_u32(&mut b)?.to_be_bytes();
+                match read_colr(&mut b, colour_type, strictness)? {
+                    ParsedColourInformation::Supported(colr) => ItemProperty::Colour(colr),
+                    ParsedColourInformation::Unsupported(colour_type) => {
+                        error!("read_colr colour_type: {colour_type:?}");
+                        return Status::ColrBadType.into();
+                    }
                 }
-            },
+            }
             BoxType::ImageMirror => ItemProperty::Mirroring(read_imir(&mut b)?),
             BoxType::ImageRotation => ItemProperty::Rotation(read_irot(&mut b)?),
             BoxType::ImageSpatialExtentsProperty => {
@@ -3837,12 +3840,12 @@ enum ParsedColourInformation {
 
 /// Parse colour information
 /// See ISOBMFF (ISO 14496-12:2020) § 12.1.5
+/// The caller is expected to have already read the colour_type field.
 fn read_colr<T: Read>(
     src: &mut BMFFBox<T>,
+    colour_type: [u8; 4],
     strictness: ParseStrictness,
 ) -> Result<ParsedColourInformation> {
-    let colour_type = be_u32(src)?.to_be_bytes();
-
     match &colour_type {
         b"nclx" => {
             const NUM_RESERVED_BITS: u8 = 7;
@@ -5683,6 +5686,7 @@ fn read_video_sample_entry<T: Read>(
     let mut codec_specific = None;
     let mut pixel_aspect_ratio = None;
     let mut colour_info = None;
+    let mut colr_types_seen = TryVec::<FourCC>::new();
     let mut hdr_mastering_display = None;
     let mut hdr_content_light_level = None;
     let mut protection_info = TryVec::new();
@@ -5802,22 +5806,32 @@ fn read_video_sample_entry<T: Read>(
                 debug!("Parsed pasp box: {pasp:?}, PAR {pixel_aspect_ratio:?}");
             }
             BoxType::ColourInformationBox => {
-                if colour_info.is_some() {
-                    // ISO/IEC 14496-12:2015 § 12.1.5.1 permits one or more colr boxes
-                    // in a VisualSampleEntry and assigns them no normative behaviour;
-                    // a reader may keep the first (most accurate) and ignore the rest.
-                    // Only reject under Strict.
-                    warn!("Multiple colr boxes in video sample entry, keeping first");
+                // ISO/IEC 14496-12:2015 § 12.1.5.1 permits one or more colr boxes
+                // in a VisualSampleEntry (at most one for a given colour_type) and
+                // assigns them no normative behaviour; a reader may keep the first
+                // (most accurate) and ignore the rest. Only reject a repeated
+                // colour_type, and only under Strict.
+                let colour_type = FourCC::from(be_u32(&mut b)?.to_be_bytes());
+                if colr_types_seen.contains(&colour_type) {
+                    warn!(
+                        "Multiple {colour_type:?} colr boxes in video sample entry, keeping first"
+                    );
                     fail_with_status_if(
                         strictness == ParseStrictness::Strict,
                         Status::ColrBadQuantityBMFF,
                     )?;
-                    skip_box_content(&mut b)?;
-                } else if let ParsedColourInformation::Supported(colr) =
-                    read_colr(&mut b, strictness)?
-                {
-                    debug!("Parsed colr box: {colr:?}");
-                    colour_info = Some(colr);
+                    skip_box_remain(&mut b)?;
+                } else {
+                    colr_types_seen.push(colour_type.clone())?;
+                    if colour_info.is_some() {
+                        debug!("Already have colour info, skipping {colour_type:?} colr box");
+                        skip_box_remain(&mut b)?;
+                    } else if let ParsedColourInformation::Supported(colr) =
+                        read_colr(&mut b, colour_type.value, strictness)?
+                    {
+                        debug!("Parsed colr box: {colr:?}");
+                        colour_info = Some(colr);
+                    }
                 }
             }
             BoxType::MasteringDisplayColourVolumeBox => {

mp4parse/tests/public.rs

@@ -237,6 +237,15 @@ static VIDEO_MP4V_MP4: &str = "tests/bbb_sunflower_QCIF_30fps_mp4v_noaudio_1f.mp
 // ffmpeg -f lavfi -i color=c=white:s=640x480 -c:v libx264 -frames:v 1 -pix_fmt yuv420p -vf "setsar=16/9" h264_white_frame_sar_16_9.mp4
 static VIDEO_H264_PASP_MP4: &str = "tests/h264_white_frame_sar_16_9.mp4";
 
+// An h264 video sample entry with two nclx colr boxes (HLG transfer=18 followed
+// by a BT.2020 transfer=14 fallback), the backward-compatible HLG signaling
+// convention. Copy of mp4parse_capi/tests/video_colr_nclx_two_colr.mp4.
+static VIDEO_TWO_NCLX_COLR_MP4: &str = "tests/video_colr_nclx_two_colr.mp4";
+// As above, but with the second colr box's colour_type patched from nclx to
+// rICC, giving one colr box of each colour_type as intended by
+// ISOBMFF (ISO 14496-12:2020) § 12.1.5.
+static VIDEO_NCLX_AND_ICC_COLR_MP4: &str = "tests/video_colr_nclx_and_icc.mp4";
+
 // Adapted from https://github.com/GuillaumeGomez/audio-video-metadata/blob/9dff40f565af71d5502e03a2e78ae63df95cfd40/src/metadata.rs#L53
 #[test]
 fn public_api() {
@@ -890,6 +899,59 @@ fn public_mp4_ctts_overflow() {
     );
 }
 
+fn assert_video_nclx_colour_info(
+    result: mp4::Result<mp4::MediaContext>,
+    expected_transfer_characteristics: u8,
+) {
+    let context = result.expect("read_mp4 failed");
+    let track = context.tracks.first().expect("expected a track");
+    let stsd = track.stsd.as_ref().expect("expected an stsd");
+    let v = match stsd.descriptions.first().expect("expected a SampleEntry") {
+        mp4::SampleEntry::Video(v) => v,
+        _ => panic!("expected a VideoSampleEntry"),
+    };
+    match &v.colour_info {
+        Some(mp4::ColourInformation::Nclx(nclx)) => {
+            assert_eq!(
+                nclx.transfer_characteristics,
+                expected_transfer_characteristics
+            );
+        }
+        other => panic!("expected nclx colour info, got {:?}", other),
+    }
+}
+
+/// Two colr boxes with the same colour_type in a video sample entry are
+/// accepted (keeping the first) except under Strict parsing, which yields
+/// ColrBadQuantityBMFF; see ISOBMFF (ISO 14496-12:2020) § 12.1.5.
+#[test]
+fn public_video_two_nclx_colr_boxes() {
+    for strictness in [ParseStrictness::Permissive, ParseStrictness::Normal] {
+        let input = &mut File::open(VIDEO_TWO_NCLX_COLR_MP4).expect("Unknown file");
+        assert_video_nclx_colour_info(mp4::read_mp4(input, strictness), 18);
+    }
+    let input = &mut File::open(VIDEO_TWO_NCLX_COLR_MP4).expect("Unknown file");
+    assert_eq!(
+        Status::from(mp4::read_mp4(input, ParseStrictness::Strict)),
+        Status::ColrBadQuantityBMFF
+    );
+}
+
+/// One colr box of each colour_type (nclx and rICC) in a video sample entry
+/// is valid per ISOBMFF (ISO 14496-12:2020) § 12.1.5 and accepted at all
+/// strictness levels, surfacing the first box.
+#[test]
+fn public_video_nclx_and_icc_colr_boxes() {
+    for strictness in [
+        ParseStrictness::Permissive,
+        ParseStrictness::Normal,
+        ParseStrictness::Strict,
+    ] {
+        let input = &mut File::open(VIDEO_NCLX_AND_ICC_COLR_MP4).expect("Unknown file");
+        assert_video_nclx_colour_info(mp4::read_mp4(input, strictness), 18);
+    }
+}
+
 #[test]
 fn public_avif_primary_item() {
     let input = &mut File::open(IMAGE_AVIF).expect("Unknown file");