search

AOMediaCodec / libavif

libavif - Library for encoding and decoding .avif files
Other
1.44k stars 187 forks source link

Bail out of avifDecoderParse() if an essential-required item property is not flagged as essential #541

Open joedrago opened 3 years ago

joedrago commented 3 years ago

Some item's properties (such as av1c), if used, must be flagged as essential when associating with the property, partially due to standards compliance / correctness, and partially for future compatibility. I propose we check that the essential flag is set in a property association on boxes we currently support with this requirement, and we'll add more boxes to this restriction as we add support for them. We already bail out if we see an essential property we don't support (future compatibility), so this is just validating in the other direction too.

NOTE: One important side effect of this is that AVIFs generated with avifenc prior to v0.7.2 (roughly May 2020) will no longer be recognized as valid (as they are actually invalid), so I plan to offer a simple standalone script to fix up such files in-place.

Boxes libavif supports at the time of writing this issue that must be flagged as essential:

@cconcolato Am I missing any?

baumanj commented 3 years ago

Here's a test case of an otherwise valid AVIF which is missing the essential flag for the av1C property.

joedrago commented 3 years ago

I've removed the compliance label from this as it sounds like the standard is going to be changed (from today's meeting).

baumanj commented 3 years ago

I see https://github.com/AOMediaCodec/av1-avif/pull/144 changing the requirement for av1C, but what what about clap, irot and imir? Are those required to me marked essential? I couldn't find that in any specification, but perhaps I missed it.

Also, per HEIF (ISO/IEC 23008-12:2017) § 6.5.11.1:

essential shall be equal to 1 for an 'lsel' item property.

So should the failure to mark that property essential be an error?

baumanj commented 3 years ago

Since this required a fair amount of spec reading to put together, I'm writing this down for future reference.

Per HEIF (ISO/IEC 23008-12:2017) § 6.5.1: Image properties, General

Descriptive properties are non-essential, unless stated otherwise in their specification.

and § 9.3.1: Item Properties Box (iprp) Definition

Each property association may be marked as either essential or non-essential. A reader shall not process an item that is associated with a property that is not recognized or not supported by the reader and that is marked as essential to the item. A reader may ignore an associated item property that is marked non-essential to the item.

and § 10.2.1.2: 'mif1' structural brand, Requirements on readers

Readers shall recognize the following item properties.

  • ispe image spatial extents
  • pasp pixel aspect ratio
  • colr colour information
  • pixi pixel information
  • rloc relative location
  • auxC image properties for auxiliary images
  • clap clean aperture
  • irot image rotation
  • imir image mirroring

All but the last 3 are descriptive (not transformative) properties, so together with § 6.5.1 and… MIAF (ISO/IEC 23000-22:2019) § 7.3.9: Transformations and derived items

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.

That's where we get that clap, irot and imir are required to have the essential bit set: they're required transformative properties. But they're also required for readers of the mif1 brand, so assuming a future spec change that the essential bit is only relevant for non-required items, there doesn't seem to be a need to reject any files that don't have the essential bit set for any of these.