From 0fa17f0eede8293a09fa258dc422ac705b64a544 Mon Sep 17 00:00:00 2001 From: Matthew Gregan Date: Wed, 10 Jun 2026 22:28:49 +1200 Subject: [PATCH] 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. --- mp4parse/src/lib.rs | 58 +++++++++++------- mp4parse/tests/public.rs | 62 ++++++++++++++++++++ mp4parse/tests/video_colr_nclx_and_icc.mp4 | Bin 0 -> 1685 bytes mp4parse/tests/video_colr_nclx_two_colr.mp4 | Bin 0 -> 1685 bytes 4 files changed, 98 insertions(+), 22 deletions(-) create mode 100644 mp4parse/tests/video_colr_nclx_and_icc.mp4 create mode 100644 mp4parse/tests/video_colr_nclx_two_colr.mp4 diff --git a/mp4parse/src/lib.rs b/mp4parse/src/lib.rs index 6a404556..b1bae2dd 100644 --- a/mp4parse/src/lib.rs +++ b/mp4parse/src/lib.rs @@ -467,8 +467,8 @@ impl From 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( 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( src: &mut BMFFBox, + colour_type: [u8; 4], strictness: ParseStrictness, ) -> Result { - let colour_type = be_u32(src)?.to_be_bytes(); - match &colour_type { b"nclx" => { const NUM_RESERVED_BITS: u8 = 7; @@ -5652,6 +5655,7 @@ fn read_video_sample_entry( let mut codec_specific = None; let mut pixel_aspect_ratio = None; let mut colour_info = None; + let mut colr_types_seen = TryVec::::new(); let mut hdr_mastering_display = None; let mut hdr_content_light_level = None; let mut protection_info = TryVec::new(); @@ -5771,22 +5775,32 @@ fn read_video_sample_entry( 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 => { diff --git a/mp4parse/tests/public.rs b/mp4parse/tests/public.rs index 98b4678d..632deabe 100644 --- a/mp4parse/tests/public.rs +++ b/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, + 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"); diff --git a/mp4parse/tests/video_colr_nclx_and_icc.mp4 b/mp4parse/tests/video_colr_nclx_and_icc.mp4 new file mode 100644 index 0000000000000000000000000000000000000000..90b033af32c94a24f459869b6d224adf27238817 GIT binary patch literal 1685 zcmZuy&5ImG6tCG$mKcJHu8_kDWsD$_neFab$K8d7tZQ%u1uy0hgr>T>db^qasI98q zoykcdhxHH8g9q`dh=*J}8CQrPcotmnVni`0dJDOj@%MUWvg4A+RK4H(tdIAqhcU)e zH`KYUOUC9Hag0FhNx#%9K4a`mYLtTQw=xk9{L6*YqtWL-U%C7B??+o-eRSz3e)Rm` z{|tiO3U6~+8^wJ<)AhYo-u0Egmaf6D)q%tETW?=^y}iOWu5Q9JQ4)sDS`Q1AIv#jl zw;gyv7s|{zy}rD>zrWwv%M(=>qUzLUdzn`0WUegWQ)`#km0jmj#6pHXH!2OgJW+8` z%YNvud+Qz-l_-W*h2EgM?hSmu%S#nzYQXJo4D<@uc8E~Skx4?o;~@wWFY|#)BFg9^ zA~K@dR-xbHGBb54B82+f8C4Xy1zH`fCei`Ooig;W1(95@D-{O*CEw?%urAWJpKJ0M zC)m+Zou*c~upMxh8AMQ`qOSWQgAg6-Y>`XsWvcLaWtuf)UJ6J1$SbEzA>agkTXPr?9LBtp|E;wa9ArA2b1nse=|d^>Y7 zY_(QZv|Vdh$4ZURz8WHT7`*IFU{vN6L~f<3l-zaUibvZtg09pkn<3oD=;3k6Ze%D; z%F7t{O2>i_2HlRwcjz#o*XaRjO5AWU2zzT#Sf_Nj!gGxxp&?L1Wz;t6t8~s;E-_Zla1lo(e3vcn?ES)dJnpy+{+U18JhnC z!hp3H{f_B>0@Nqu%g3INLUVXfH*qh07~ggMX3hB-$7!~~jBX64mZ{-{e3|sf-VDYr zs>E4HORBJV#0^md@}8RPdG4h$$pv)0OirFXfO>(wbGwOaXGviigo&Q#6cfKn&vi>w zNukK6^&<+F0xa&8P14zIUQEWer376;!634q+cgCr+G~3>ovz>Uy?_^aOrMgmv6;`0M&N7k+H584?C2M^vNT>db*kZsI98q zoykcdhxHH8g9q`dh=*J}8CQrPcotmnVni`0dXrqt_UkPRqyvc>*KxZVT|$A zjdX77lCcFw93v3>(l7Oz&lr0oHA=zuOPL4<{`u0`@%YmpuipFe*W;ZpKDhh?KYs4d zzlK3?jkme1jp9C_>H6L}@A}H$NH<{E>A+$2jW@5p)?VXV*S6uAC<()Mtw)7Q9S=OO z+YY>-3uWe<-dtThI5_C+=ZUHdQFUt5U!_$#nJWwU)Y|2BWjDDLv5=w9jY`8VPgGpg zauE8P-loSzC5n+%p*QSqdPCpu@=}GF8gjcA1HH_(9U&BRWRlSDcnHG8%Y3Mkh%)+! zh>WQED)f6?W~MGhgixP5qlzN8K8t?X$h*Z@;Fd-um!j@yb^ZjtI9t+8Ui3-+7m@#ly0$_n|Aw{VV~Wqs8AK z3|WiO?}Yv%Kz&NSa^m@aXaNuE7Vc#LHuHt?7<>a>+pUD18~?;s`rw}(M~8<; z*KfXe=3{gRJvLE2p@r2j z(S~U(qt~a^xP6x~8tj5~O{D-EA9MqikuBo_sDebZ_!s1fFLe#{sAj{Lro4$=L98SN zXcjy5XB@U06H_zOdfxz@pAM`m>#3b$J&brSaRU5$sTxk^XXm>Don(a8RJe!K&L^$+ a{3ouagwNpbgpyEFhVrB+kKaRiyng{fL3|eg literal 0 HcmV?d00001