Parse a1lx and expose item extents for layered AVIF images - #456
Conversation
A layered (progressive) AVIF image item holds up to four AV1 frames concatenated in its payload, each a better rendering of the same picture. `a1lx` gives the byte sizes of the first three, and is the only signal saying where one layer ends and the next begins. It was previously recorded as an unsupported feature and skipped without reading its payload, so a caller had no way to find a layer boundary and could only decode the item whole. Parse it, and give callers what they need to slice the payload themselves: - `ItemProperty::LayeredImageIndexing` carries the layer sizes, and `Feature::A1lx` becomes supported. - `AvifItem` retains its `iloc` extents and construction method, reached through `primary_item_extents`, `alpha_item_extents` and the `*_is_file_construction` predicates. A caller decoding an item incrementally needs to know which of its bytes have arrived, which the existing item-data copy cannot tell it -- and for a multi-extent item that copy would have captured the whole payload anyway. - `primary_item_a1lx`, `alpha_item_a1lx` and `primary_item_lsel` report the properties that decide whether an item is progressively renderable. An alpha auxiliary item can be layered too, with its own `a1lx`. A malformed `a1lx` -- truncated, overlong, or with non-zero reserved bits -- is deliberately not fatal. It is recorded as present with no layer sizes, which reads as "not a layered image". Such a file decoded fine when the property was skipped, and failing it now would be a regression. The C API gains the matching `Mp4parseAvifInfo` fields, plus `Mp4parseItemExtents` as a borrowed slice of `ItemExtent`, valid for the lifetime of the parser. Because `a1lx` is no longer an unsupported feature, the files whose only unsupported property it was now parse with an empty `unsupported_features` set, so they come off `AVIF_UNSUPPORTED_IMAGES`. `animals_00_multilayer_grid_a1lx` and `quebec_3layer_op2` stay, since `grid` and `a1op` are still unsupported.
| pub struct ItemExtent { | ||
| pub offset: u64, | ||
| pub len: u64, | ||
| pub to_end: bool, |
There was a problem hiding this comment.
Why put the repr-C-friendly form of this in the pure-Rust part of things? The advantage to the non-repr-C-friendly Extent enum is that it makes the representation of illegal states impossible as well as the interpretation of the data clear from the code rather than comments. Is there an essential reason why the transformation can't be constrained to the part of the code concerned with FFI?
There was a problem hiding this comment.
Fair. I've moved it. Again, I tested it with the Firefox patch.
There was a problem hiding this comment.
I don't have time to review all of this closely enough to give a general approval, but with this revision, I don't see anything objectionable from a cursory perusal. Thanks for taking the time to add this cool feature.
There was a problem hiding this comment.
Hi Jon hope you're well! And thanks for giving it a look. Myself or @kinetiknz will look more closely.
There was a problem hiding this comment.
Hi Paul! I miss y'all. Feel free to ping me sometime; it would be lovely to catch up.
| // so a reader is entitled to ignore it entirely and still | ||
| // render the item -- it only ever documents where the layers | ||
| // are, never how to decode them. | ||
| let a1lx = read_a1lx(&mut b).unwrap_or_default(); |
There was a problem hiding this comment.
This approach swallows I/O errors silently. I'd prefer read_a1lx to handle malformed entries internally.
| .item_properties | ||
| .get(item.id, BoxType::AV1LayeredImageIndexingProperty) | ||
| { | ||
| Ok(Some(ItemProperty::LayeredImageIndexing(a1lx))) => Some(a1lx), |
There was a problem hiding this comment.
Before returning this index, should we check that the sum of its sizes is smaller than self.item_as_slice(item).len()? Otherwise the documented layers can consume or exceed the entire payload, leaving no final layer. This needs checking per associated item, since the same property can be shared by items with different payload lengths.
`read_a1lx(&mut b).unwrap_or_default()` treated every error the same, so a failure of the underlying reader was quietly turned into an empty index along with the malformed input it was meant to tolerate. Check the bytes are there before reading them instead, so the function itself decides what is malformed and a read error still propagates. An `a1lx` which is empty, truncated, overlong or has non-zero reserved bits is warned about and reported with no layer sizes, which reads as "not a layered image". Validate the zero terminator while we're here: "a value of zero means that all the layers except the last one have been documented and following values shall be 0", so a non-zero size after a zero one leaves no way to tell which entries are layer boundaries. `[0, 10, 0]` was previously returned unchanged.
The layer sizes an `a1lx` documents have to leave room for the last, undocumented layer, since "the size of the last layer can be determined by subtracting the sum of the sizes of all layers indicated in this property from the entire item size". Sizes which consume or exceed the whole payload describe nothing, so report no index for that item. The check belongs per associated item rather than in `read_a1lx`: one property can be associated with several items, whose payloads differ in length. Document what `Some`, and a non-null `Mp4parseAvifInfo::*_item_a1lx`, now means: the property is present and usable for this item. It may still carry all-zero sizes -- both a spec-legal single-layer `a1lx` and a malformed one look like that -- so presence alone doesn't establish a layer boundary.
7697e53 to
3320b0f
Compare
|
@kinetiknz those issues should be fixed now, and I've tested it against the Firefox patch. |
|
That's great, thanks for the patch! |
4 participants
This is part of https://bugzilla.mozilla.org/show_bug.cgi?id=2072679 - supporting progressive AVIF rendering in Firefox. This change is needed to expose the progressive layers.
Forgive me, for this was largely vibe coded, although I've manually tested it integrated with Firefox as part of https://phabricator.services.mozilla.com/D326564.
Here's some slop explaining what's going on here:
A layered (progressive) AVIF image item holds up to four AV1 frames concatenated in its payload, each a better rendering of the same picture.
a1lxgives the byte sizes of the first three, and is the only signal saying where one layer ends and the next begins. It was previously recorded as an unsupported feature and skipped without reading its payload, so a caller had no way to find a layer boundary and could only decode the item whole.Parse it, and give callers what they need to slice the payload themselves:
ItemProperty::LayeredImageIndexingcarries the layer sizes, andFeature::A1lxbecomes supported.AvifItemretains itsilocextents and construction method, reached throughprimary_item_extents,alpha_item_extentsand the*_is_file_constructionpredicates. A caller decoding an item incrementally needs to know which of its bytes have arrived, which the existing item-data copy cannot tell it -- and for a multi-extent item that copy would have captured the whole payload anyway.primary_item_a1lx,alpha_item_a1lxandprimary_item_lselreport the properties that decide whether an item is progressively renderable. An alpha auxiliary item can be layered too, with its owna1lx.A malformed
a1lx-- truncated, overlong, or with non-zero reserved bits -- is deliberately not fatal. It is recorded as present with no layer sizes, which reads as "not a layered image". Such a file decoded fine when the property was skipped, and failing it now would be a regression.The C API gains the matching
Mp4parseAvifInfofields, plusMp4parseItemExtentsas a borrowed slice ofItemExtent, valid for the lifetime of the parser.Because
a1lxis no longer an unsupported feature, the files whose only unsupported property it was now parse with an emptyunsupported_featuresset, so they come offAVIF_UNSUPPORTED_IMAGES.animals_00_multilayer_grid_a1lxandquebec_3layer_op2stay, sincegridanda1opare still unsupported.