gpac / ComplianceWarden

A pluggable compliance checker (ISOBMFF, HEIF/MIAF/AVIF, AV1 HDR10+)
https://gpac.github.io/ComplianceWarden-wasm/
Other
16 stars 7 forks source link

False negative: at most one `ColourInformationBox` (`colr`) per item #30

Open baumanj opened 2 years ago

baumanj commented 2 years ago

Per HEIF (ISO/IEC 23008-12:2017) § 6.5.5.1:

Quantity (per item): At most one

This is in the process of being amended in DIS 23008-12 to be

At most one for a given value of colour_type

but multiple associations of colr boxes of the same colour_type per item should be disallowed under either standard. However, these inputs generate no errors or warnings:

invalid-avif-colr-multiple.zip

rbouqueau commented 2 years ago

Thank you. The version of the spec is more recent than what's currently supported by the ComplianceWarden. Let's see how we can progress on an update, I have a TODO to list the request changes.

rbouqueau commented 2 years ago

If that's urgent let me know, I could implement this before update the rest of the specs.

baumanj commented 2 years ago

I'm not sure I'd call it urgent, but it would be very helpful as people are filing lots of webcompat bugs [1][2] to be able to have a simple independent test to show what the issue is

baumanj commented 2 years ago

You do understand that ICC operates or XYZ or LAB or RGB or CMYK, while nclx operates before that on RGB --> R'G'B' --> YCbCr. Both should be present. Techincally primaries of nclx are incompatible with ICC profile, sure, but still.

Both are only required if the primaries or transfer characteristics aren't representable with CICP values (possible, but unlikely)and the matrix coefficients aren't BT.601 (again, quite unlikely). So while there are important use cases which require both, the vast majority should only need one. See https://github.com/AOMediaCodec/libavif/wiki/CICP for more.

Eventually the spec will be updated (see https://github.com/AOMediaCodec/av1-avif/issues/164 and https://github.com/MPEGGroup/FileFormat/issues/39), but right now the published text is clear.

rbouqueau commented 1 year ago

Is https://github.com/AOMediaCodec/av1-avif/issues/164 or any of the discussion blocking the addition of this new test?

The additional checks would be:

  1. at most one ColourInformationBox (colr) per item
  2. multiple associations of colr boxes of the same colour_type per item should be disallowed (I see it in the update version of the standard)
rbouqueau commented 1 year ago

@cconcolato It seems that https://github.com/MPEGGroup/FileFormat/issues/39 would allow to close https://github.com/AOMediaCodec/av1-avif/issues/164 and unlocks here. Correct?

cconcolato commented 1 year ago

@rbouqueau what's important is that:

Regarding the color restrictions, in terms of spec and publication status, we have

  1. HEIF ISO/IEC 23008-12:2022 (published) (https://dms.mpeg.expert/doc_end_user/documents/140_Mainz/wg11/MDS22076_WG03_N00749.zip) says:

    Quantity (per item): At most one for a given value of colour_type

  2. HEIF ISO/IEC 23008-12:2022 AMD1 to be published at the end of the year (see https://github.com/AOMediaCodec/av1-avif/issues/164#issuecomment-1572516221) will say:

    Quantity (per item): Either at most one, or two with restriction described below In addition, following definitions specific to the use of colour information in image items also applies: — When two ColourInformationBoxes are associated with an image item, one shall have a colour_type value of 'rICC' or 'prof' (providing either restricted or unrestricted ICC profiles respectively) and the other one shall have a colour_type value of 'nclx' with colour_primaries equal to 2 and transfer_characteristics equal to 2 (2 indicating "unspecified", since these data are supplied by the ICC profile instead).

To close this issue, I think we should implement only 1 (in the master branch) and 2 (in a branch that should be merged only after the AMD is officially approved/published).