Skip to content

Commit 1fea957

Browse files
alastor0325kinetiknz
authored andcommitted
Address review feedback on colr box parsing for video tracks
- Add ColrBadQuantityBMFF status (distinct from the HEIF-specific ColrBadQuantity) and enforce it via fail_with_status_if when strictness != Permissive for duplicate colr boxes in video tracks. - Introduce ParsedColourInformation enum so read_colr returns Supported/Unsupported instead of always erroring on unknown colour_type values; the AVIF call site converts Unsupported to ColrBadType, while the video call site warns and continues. - Document that VideoSampleEntry::colour_info only surfaces Nclx through the C API.
1 parent 9e456d8 commit 1fea957

1 file changed

Lines changed: 44 additions & 18 deletions

File tree

mp4parse/src/lib.rs

Lines changed: 44 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,7 @@ pub enum Status {
185185
BoxBadWideSize,
186186
CheckParserStateErr,
187187
ColrBadQuantity,
188+
ColrBadQuantityBMFF,
188189
ColrBadSize,
189190
ColrBadType,
190191
ColrReservedNonzero,
@@ -465,6 +466,10 @@ impl From<Status> for &str {
465466
ColourInformationBox (colr) for a given value of colour_type \
466467
per HEIF (ISO/IEC DIS 23008-12) § 6.5.5.1"
467468
}
469+
Status::ColrBadQuantityBMFF => {
470+
"Each sample entry shall have at most one ColourInformationBox (colr) \
471+
per ISOBMFF (ISO 14496-12:2020) § 12.1.5"
472+
}
468473
Status::ColrBadSize => {
469474
"Unexpected size for colr box"
470475
}
@@ -1175,6 +1180,8 @@ pub struct VideoSampleEntry {
11751180
pub codec_specific: VideoCodecSpecific,
11761181
pub protection_info: TryVec<ProtectionSchemeInfoBox>,
11771182
pub pixel_aspect_ratio: Option<f32>,
1183+
/// Only `ColourInformation::Nclx` is currently surfaced through the C API;
1184+
/// `ColourInformation::Icc` is stored but not exposed to C consumers.
11781185
pub colour_info: Option<ColourInformation>,
11791186
}
11801187

@@ -3601,7 +3608,13 @@ fn read_ipco<T: Read>(
36013608
let property = match b.head.name {
36023609
BoxType::AuxiliaryTypeProperty => ItemProperty::AuxiliaryType(read_auxc(&mut b)?),
36033610
BoxType::AV1CodecConfigurationBox => ItemProperty::AV1Config(read_av1c(&mut b)?),
3604-
BoxType::ColourInformationBox => ItemProperty::Colour(read_colr(&mut b, strictness)?),
3611+
BoxType::ColourInformationBox => match read_colr(&mut b, strictness)? {
3612+
ParsedColourInformation::Supported(colr) => ItemProperty::Colour(colr),
3613+
ParsedColourInformation::Unsupported(colour_type) => {
3614+
error!("read_colr colour_type: {colour_type:?}");
3615+
return Status::ColrBadType.into();
3616+
}
3617+
},
36053618
BoxType::ImageMirror => ItemProperty::Mirroring(read_imir(&mut b)?),
36063619
BoxType::ImageRotation => ItemProperty::Rotation(read_irot(&mut b)?),
36073620
BoxType::ImageSpatialExtentsProperty => {
@@ -3759,12 +3772,17 @@ impl ColourInformation {
37593772
}
37603773
}
37613774

3775+
enum ParsedColourInformation {
3776+
Supported(ColourInformation),
3777+
Unsupported(FourCC),
3778+
}
3779+
37623780
/// Parse colour information
37633781
/// See ISOBMFF (ISO 14496-12:2020) § 12.1.5
37643782
fn read_colr<T: Read>(
37653783
src: &mut BMFFBox<T>,
37663784
strictness: ParseStrictness,
3767-
) -> Result<ColourInformation> {
3785+
) -> Result<ParsedColourInformation> {
37683786
let colour_type = be_u32(src)?.to_be_bytes();
37693787

37703788
match &colour_type {
@@ -3791,22 +3809,26 @@ fn read_colr<T: Read>(
37913809
)?;
37923810
}
37933811

3794-
Ok(ColourInformation::Nclx(NclxColourInformation {
3795-
colour_primaries,
3796-
transfer_characteristics,
3797-
matrix_coefficients,
3798-
full_range_flag,
3799-
}))
3812+
Ok(ParsedColourInformation::Supported(ColourInformation::Nclx(
3813+
NclxColourInformation {
3814+
colour_primaries,
3815+
transfer_characteristics,
3816+
matrix_coefficients,
3817+
full_range_flag,
3818+
},
3819+
)))
38003820
}
3801-
b"rICC" | b"prof" => Ok(ColourInformation::Icc(
3821+
b"rICC" | b"prof" => Ok(ParsedColourInformation::Supported(ColourInformation::Icc(
38023822
IccColourInformation {
38033823
bytes: src.read_into_try_vec()?,
38043824
},
38053825
FourCC::from(colour_type),
3806-
)),
3826+
))),
38073827
_ => {
3808-
error!("read_colr colour_type: {colour_type:?}");
3809-
Status::ColrBadType.into()
3828+
let four_cc = FourCC::from(colour_type);
3829+
warn!("read_colr: unsupported colour_type {four_cc:?}, skipping");
3830+
skip_box_remain(src)?;
3831+
Ok(ParsedColourInformation::Unsupported(four_cc))
38103832
}
38113833
}
38123834
}
@@ -5690,15 +5712,19 @@ fn read_video_sample_entry<T: Read>(
56905712
}
56915713
BoxType::ColourInformationBox => {
56925714
if colour_info.is_some() {
5693-
// ISO 14496-12:2020 §12.1.5 requires at most one ColourInformationBox
5694-
// per sample entry. Duplicates are a spec violation, but we tolerate
5695-
// them to avoid breaking playback of malformed content.
56965715
warn!("Multiple colr boxes in video sample entry, keeping first");
5716+
fail_with_status_if(
5717+
strictness != ParseStrictness::Permissive,
5718+
Status::ColrBadQuantityBMFF,
5719+
)?;
56975720
skip_box_content(&mut b)?;
56985721
} else {
5699-
let colr = read_colr(&mut b, strictness)?;
5700-
debug!("Parsed colr box: {colr:?}");
5701-
colour_info = Some(colr);
5722+
if let ParsedColourInformation::Supported(colr) =
5723+
read_colr(&mut b, strictness)?
5724+
{
5725+
debug!("Parsed colr box: {colr:?}");
5726+
colour_info = Some(colr);
5727+
}
57025728
}
57035729
}
57045730
_ => {

0 commit comments

Comments
 (0)