decoder: keep 'dimg' inputs as an ordered list on the derived item - #885
Open
Nexory wants to merge 1 commit into
Open
decoder: keep 'dimg' inputs as an ordered list on the derived item#885Nexory wants to merge 1 commit into
Nexory wants to merge 1 commit into
Conversation
Item::source_item_ids already holds the input item ids in 'dimg' order, but it was reconstructed rather than parsed: populate_source_item_ids() walked every item, kept the ones whose single-valued dimg_for_id matched, and sorted by dimg_index to recover the order the box had already given. Record the ids on the derived item while the 'iref' box is read instead. The walk, the sort and both fields go away, and an item can be an input of several derived items, which the single-valued mapping could not express and rejected with NotImplemented. The rule from section 8.11.12.1 that a value occurs at most once within one array is kept, and its diagnostic is unchanged. color_grid_alpha_grid_tile_shared_in_dimg.avif now reaches the codec configuration check instead: its item 10 is a cell of both the color grid and the alpha grid, and those av1C differ. The new test file points both 'dimg' lists at the same four cells so the tiles agree and the decode completes; it is derived from that same file by editing five bytes.
y-guyon
requested changes
Sep 3, 2026
y-guyon
left a comment
y-guyon
left a comment
Collaborator
There was a problem hiding this comment.
Thank you for this change.
|
|
||
| let mut source_item_ids: Vec<u32> = Vec::new(); | ||
| // The input items are recorded on the derived item in 'dimg' order while the | ||
| // 'iref' box is parsed, so nothing has to be collected or reordered here. |
Collaborator
There was a problem hiding this comment.
Delete the second part of the sentence.
| let mut source_item_ids: Vec<u32> = Vec::new(); | ||
| // The input items are recorded on the derived item in 'dimg' order while the | ||
| // 'iref' box is parsed, so nothing has to be collected or reordered here. | ||
| let source_item_ids = self.items.get(&item_id).unwrap().source_item_ids.clone(); |
Comment on lines
+745
to
+749
| if reference.reference_type == "dimg" { | ||
| // Derived images refer in the opposite direction, so make sure the input | ||
| // item exists before the derived item is borrowed below. | ||
| insert_item_if_not_exists(reference.to_item_id, &mut items); | ||
| } |
Collaborator
There was a problem hiding this comment.
Please move that check back where it was. Repeat the row 751 after the check if necessary.
Comment on lines
+746
to
+747
| // Derived images refer in the opposite direction, so make sure the input | ||
| // item exists before the derived item is borrowed below. |
Collaborator
There was a problem hiding this comment.
That comment is inaccurate. Revert to the original comment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requested in AOMediaCodec/libavif#3342, which made the
same change on the libavif side.
Item::source_item_idsalready holds the input item ids indimgorder, but itwas reconstructed rather than parsed:
populate_source_item_ids()walked everyitem, kept the ones whose single-valued
dimg_for_idmatched, and sorted bydimg_indexto recover the order the box had already given. This records the idson the derived item while the
irefbox is read instead, so the walk, the sortand both fields go away.
It also lets an item be an input of several derived items, which the
single-valued mapping could not express and rejected with
NotImplemented. Thatrestriction came from #248, mirroring AOMediaCodec/libavif#2315; libavif has now
dropped it. The rule from section 8.11.12.1 that a value occurs at most once
within one array is kept, and its diagnostic is unchanged.
dimg_sharedmoves fromNotImplementedtoBmffParseFailed: item 10 ofcolor_grid_alpha_grid_tile_shared_in_dimg.avifis a cell of both the color gridand the alpha grid, so with the restriction gone the file reaches the codec
configuration check and stops there.
color_grid_alpha_grid_shared_tiles.avifisadded for the case that now decodes, with both
dimglists pointing at the samefour cells; it is derived from that same file by editing five bytes.
Tested with
cargo test,cargo clippy --all-targets -- -D warningsandcargo +nightly fmt --all -- --check.