Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
baumanj opened this issue Sep 8, 2021 · 7 comments
Open

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

baumanj opened this issue Sep 8, 2021 · 7 comments

Comments

@baumanj
Copy link

baumanj commented Sep 8, 2021

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
Copy link
Member

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
Copy link
Member

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

@baumanj
Copy link
Author

baumanj commented Sep 15, 2021

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
Copy link
Author

baumanj commented Oct 5, 2021

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 AOMediaCodec/av1-avif#164 and MPEGGroup/FileFormat#39), but right now the published text is clear.

@rbouqueau
Copy link
Member

Is AOMediaCodec/av1-avif#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
Copy link
Member

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

@cconcolato
Copy link
Member

@rbouqueau what's important is that:

  • ComplianceWarden in the master branch only implements published specifications (other branches can implement in-progress specifications)
  • ComplianceWarden indicates correctly the specification it implements (in each branch)

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

  1. HEIF ISO/IEC 23008-12:2022 AMD1 to be published at the end of the year (see Add additional restriction to colr box from HEIF DAmd2 AOMediaCodec/av1-avif#164 (comment)) 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants
@cconcolato @rbouqueau @baumanj and others