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

Ignore descr props after transf props #2571

Merged
merged 3 commits into from
Jan 15, 2025
Merged

Conversation

y-guyon
Copy link
Collaborator

@y-guyon y-guyon commented Jan 14, 2025

Also reject non-essential transformative properties.

Fixes #2525.

@y-guyon y-guyon requested a review from wantehchang January 14, 2025 16:17
@wantehchang wantehchang requested a review from vigneshvg January 14, 2025 23:02
Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the texts quoted from the standards. Some of the section numbers are wrong. Perhaps you are using unpublished drafts of the new versions of the standards.

src/read.c Show resolved Hide resolved
src/read.c Outdated Show resolved Hide resolved
src/read.c Outdated Show resolved Hide resolved
src/read.c Outdated Show resolved Hide resolved
src/read.c Outdated Show resolved Hide resolved
// Under any brand, the primary item (or an alternative if alternative support is required)
// shall be processable by a reader implementing only the required features of that brand.
// Specifically, given that each brand has a set of properties that a reader is required to
// support: the item shall not have properties that are marked as essential and are outside
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[This should be fixed in HEIF.]

The colon (:) after "support" should be a comma (,).

src/read.c Outdated Show resolved Hide resolved
src/read.c Outdated Show resolved Hide resolved
Also reject non-essential transformative properties.
@y-guyon y-guyon merged commit 49729e4 into AOMediaCodec:main Jan 15, 2025
35 checks passed
@y-guyon y-guyon deleted the ignore_props branch January 15, 2025 12:11
@@ -35,6 +35,27 @@ License: [same as libavif](https://github.com/AOMediaCodec/libavif/blob/main/LIC
Source: `avifenc circle-trns-after-plte.png` with custom properties added in
`avifRWStreamWriteProperties()`: FullBox `1234`, Box `abcd` and Box `uuid`.

### File [clap_irot_imir_non_essential.avif](clap_irot_imir_non_essential.avif)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the Compliance Warden report. Does it meet your expectations?

The only issue I see is that Compliance Warden should use the Amendment 2 version of [miaf][Rule #25] Section 7.3.9.

Compliance Warden, version v35-master-rev3-g1500613
+--------------------------------------+
| avif validation |
+--------------------------------------+

Specification description: AVIF v1.0.0, 19 February 2019
https://aomediacodec.github.io/av1-avif/

========================================
[avif] No errors.
========================================

+--------------------------------------+
| miaf validation |
+--------------------------------------+

Specification description: MIAF (Multi-Image Application Format)
MPEG-A part 22 - ISO/IEC 23000-22 - w18260 FDIS - Jan 2019

[miaf][Rule #25] Error: Property "clap" shall be marked as essential (item_ID=1)
[miaf][Rule #25] Error: Property "irot" shall be marked as essential (item_ID=1)
[miaf][Rule #25] Error: Property "imir" shall be marked as essential (item_ID=1)

========================================
[miaf] 3 error(s), 0 warning(s).
========================================

===== Involved rules descriptions:

[miaf][Rule #25] Section 7.3.9
All transformative properties associated with coded and derived images required
or conditionally required by this document shall be marked as essential, and
shall be from the set that are permitted by this document or the applicable
profile. No other essential transformative property shall be associated with
such images.

+--------------------------------------+
| heif validation |
+--------------------------------------+

Specification description: HEIF - ISO/IEC 23008-12 - 2nd Edition N18310

========================================
[heif] No errors.
========================================

+--------------------------------------+
| isobmff validation |
+--------------------------------------+

Specification description: ISO Base Media File Format
MPEG-4 part 12 - ISO/IEC 14496-12 - m17277 (6th+FDAM1+FDAM2+COR1-R4)

========================================
[isobmff] No errors.
========================================

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the Compliance Warden report. Does it meet your expectations?

Yes. Thank you for checking.

The only issue I see is that Compliance Warden should use the Amendment 2 version of [miaf][Rule #25] Section 7.3.9.

Section 7.3.9 is part of w18260 "Revised text of ISO/IEC FDIS 23000-22 Multi-Image Application Format". Are you saying Amd2 was published since then and there is no need to refer to an FDIS output document?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Section 7.3.9 is in both ISO/IEC 23000-22:2019 and ISO/IEC 23000-22:2019/Amd. 2:2021.

In ISO/IEC 23000-22:2019, the paragraph reads:

All transformative properties associated with coded and derived images required or conditionally required by this document shall be marked as essential, and shall be from the set that are permitted by this document or the applicable profile. No other essential transformative property shall be associated with such images.

In ISO/IEC 23000-22:2019/Amd. 2:2021, the paragraph is modified and reads:

All transformative properties associated with coded and derived images shall be marked as essential,
and shall be from the set defined in 7.3.6.7 or the applicable MIAF profile. No other essential
transformative property shall be associated with such images.

So Compliance Warden quotes the older version of the paragraph in the error message. It is better to quote the newer version of the paragraph in the error message.

In general Compliance Warden should not refer to an FDIS output document.

tests/data/README.md Show resolved Hide resolved
@@ -55,6 +55,8 @@ The changes are relative to the previous release, unless the baseline is specifi
* Deprecate avifCropRectConvertCleanApertureBox() and
avifCleanApertureBoxConvertCropRect(). Replace them with
avifCropRectFromCleanApertureBox() and avifCleanApertureBoxFromCropRect().
* Ignore descriptive properties associated after transformative ones.
* Reject non-essential transformative properties.
Copy link
Collaborator

@wantehchang wantehchang Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@farindk FYI.

We should audit the AVIF encoders in libavif and libheif and make sure they generate an ItemPropertyAssociationBox that meets the requirements checked in this pull request:

  • All transformative properties (clap, irot, imir) associated with coded and derived images shall be marked as essential.
  • If property_index is 0, essential shall also be 0.
  • Should descriptive properties be listed before transformative properties in the ItemPropertyAssociationBox? Otherwise they will be ignored by readers.

Copy link

@farindk farindk Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All transformative properties (clap, irot, imir) associated with coded and derived images shall be marked as essential.

This should always be the case in libheif.

If property_index is 0, essential shall also be 0.

libheif never generates property_index=0. Is there any potential use case for this that I have missed?

Should descriptive properties be listed before transformative properties in the ItemPropertyAssociationBox? Otherwise they will be ignored by readers.

I will have to check this: strukturag/libheif#1443
BTW: what is the rationale for this rule? I cannot think about any case where it would be important to have the descriptive items before the transformative items.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dirk: Thanks for the quick reply!

If property_index is 0, essential shall also be 0.

libheif never generates property_index=0. Is there any potential use case for this that I have missed?

I also asked this question. Please see Yannis's reply in #2571 (comment).

Should descriptive properties be listed before transformative properties in the ItemPropertyAssociationBox? Otherwise they will be ignored by readers.

I will have to check this: strukturag/libheif#1443 BTW: what is the rationale for this rule? I cannot think about any case where it would be important to have the descriptive items before the transformative items.

This comes from the following in ISO/IEC 23008-12:2022 Section 6.5.1:

Readers shall allow and ignore descriptive properties following the first transformative or unrecognized property, whichever is earlier, in the sequence associating properties with an item.

Note that it says "allow and ignore", so it isn't a hard rule. I don't know what is the rationale is.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Readers shall allow and ignore descriptive properties following the first transformative or unrecognized property, whichever is earlier, in the sequence associating properties with an item.

Note that it says "allow and ignore", so it isn't a hard rule. I don't know what is the rationale is.

But if a reader may ignore properties that are in the "wrong order", it effectively forces the writer to use the "correct order".

I also think that "Readers shall allow and ignore descriptive properties following the first transformative or unrecognized property" does not work well in practice because when encoders add new descriptive properties that are unknown to old readers, readers may ignore all descriptive properties following that new property, even if the new property was not essential (but a successive old property may be essential). I.e. images may be decoded differently by adding a non-essential property.

I thought that this is what the essential flag is for. When the reader sees an essential property that it does not understand, it knows that the decoded output might be incorrect. Non-essential properties can be ignored.
The separation of transformative and descriptive properties on-the-other hand makes little sense to me. Even more so since that means that the set of transformative properties may never be extended in future standards as old decoders have no way to classify them as transformative.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Readers shall allow and ignore descriptive properties following the first transformative or unrecognized property, whichever is earlier, in the sequence associating properties with an item.

Note that it says "allow and ignore", so it isn't a hard rule. I don't know what is the rationale is.

But if a reader may ignore properties that are in the "wrong order", it effectively forces the writer to use the "correct order".

It is not "may" but "shall ignore". To me it is a hard rule.

I.e. images may be decoded differently by adding a non-essential property.

I disagree: MIAF* forbids non-essential transformative properties, and descriptive properties should not impact decoding.

*and maybe HEIF? that is unclear to me because it depends on profiles if I remember correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

colr should not be a descriptive property. It probably should be marked as essential. Since colr is widely supported, whether it is marked as essential or not doesn't really matter in practice.

Copy link

@farindk farindk Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this also seems to be non-consistent. HEIF says in 6.5.1 that "Descriptive properties are non-essential, unless stated otherwise in their specification".

According to HEIF, colr is not essential, but in all files I looked at, it was marked as essential.

In fact, many of the descriptive properties affect how the decoded image should look like. Also boxes like pasp. Thus, we might need some clarification what essential really means.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dirk: Do you have the access to view MPEGGroup/FileFormat#113? The following problematic requirment in ISO/IEC 23008-12:2022 (HEIF) Clause 6.5.1 will be removed in Amd2 of HEIF:

Readers shall allow and ignore descriptive properties following the first transformative or unrecognized property, whichever is earlier, in the sequence associating properties with an item.

Copy link

@farindk farindk Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I know MPEGGroup/FileFormat#113, but the point of my last comment is a bit different:

if all properties that influence how a decoded image will look like, should be marked essential, then more properties should be essential. Not just colr (which is usually marked essential despite the standard telling otherwise), but also, for example pasp, which might indicate a stretching at the decoder for the image to be displayed in the correct aspect ratio.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding:

  • Transformative properties may impact the decoded output buffer (the pixel values). That is why MIAF mandates them as essential. Maybe HEIF should do that too but currently it explicitly mentions that they can be optionally applied by a player.
  • Descriptive properties may affect how the decoded output buffer is rendered, but not change the pixel values within the buffer.
  • Non-impacting, informational metadata is better located in Exif/XMP.

Yes, this also seems to be non-consistent. HEIF says in 6.5.1 that "Descriptive properties are non-essential, unless stated otherwise in their specification".

There is also a note:

"NOTE Each property descriptor definition below specifies whether the property is mandatory, i.e. whether the property is required to be present in the file. Note that when a property is mandatory, it might or might not be essential."

My understanding:

  • essential=1 means the player has to understand and apply the property.
  • A mandatory property for files means the whole file is invalid if the property is not in it. It may or may not be essential, that is orthogonal.
  • A mandatory property for players means the player has to understand it (which makes essential only mean apply the property).

So it is somewhat clear to me. I agree it could be constrained further to remove the possibility of non-essential colr, which makes rendering non-deterministic depending on the player's choice.
This is aligned with the permissive spirit of HEIF in my opinion. I suggest filing an issue to have all render-impacting descriptive properties be essential in MIAF, much like transformative properties.

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

Successfully merging this pull request may close these issues.

Ignore descriptive properties associated after non-descriptive ones
3 participants