Support grids with too many items for 16-bit item IDs"

[krishna28238-arch]

Problem
avifEncoderDataCreateItem() used a uint16_t lastItemID counter and silentlyoverflowed it when a file needed more than 65535 items: the new item IDs wrappedaround (the item ID 0 is invalid per ISO/IEC 14496-12 §8.11.1.1, and the lastcell item collided with an existing item), the 'iinf' entry_count and'iloc' item_count fields were truncated, and the 'dimg' reference_countof the grid item wrapped to 0 so that its 'dimg' box was omitted entirely.avifEncoderAddImageGrid() and avifEncoderFinish() still returnedAVIF_RESULT_OK, and only the decoder complained (AVIF_RESULT_BMFF_PARSE_FAILED)when reading the generated file.

Fix
All encoder item IDs are now uint32_t. Item IDs are unsigned int(32) inthe file format (ISO/IEC 14496-12 §8.11.6), so this matches the format andremoves the overflow.
When an item ID or item count does not fit on 16 bits, the encoder writes the32-bit box variants: 'iloc' version 2, 'iinf' version 1 with 'infe'version 3 entries, 'iref' version 1 and 'ipma' version 1 ('pitm'version 1 if ever needed). These variants are only used when required: outputfiles that fit within the 16-bit item ID space are byte-for-byte identical tobefore. The reading side already supports all these box versions, sosrc/read.c is unchanged.
However, a grid item cannot reference more than 65535 cells:ISO/IEC 14496-12 §8.11.12.1 requires all references from one item of a giventype to be collected into a single item type reference box whosereference_count is unsigned int(16) whatever the 'iref' version(§8.11.12.2), and ISO/IEC 23008-12 (HEIF) §6.6.1 forbids more than one'dimg' box with the same from_item_ID. avifEncoderAddImageGrid()therefore now rejects grids with more than 65535 cells (e.g. 256×256)upfront with AVIF_RESULT_INVALID_IMAGE_GRID and a diagnostic, instead ofgenerating a broken file. Since 65535 = 3×5×17×257 has no factor pair withboth factors ≤ 256, the largest single-plane grid is 256×255 cells; gridswith several planes (color+alpha, gain map) can still total more than 65535items, which is what the uint32_t rework enables.
The 'dimg' target items are listed before the item loop inavifEncoderFinish(), avoiding an O(items²) scan which took about 20 secondsfor a 65538-item file.
Tests
GridApiTest.ColorAlphaGridExceeding16BitItemIDs: encodes a 128×256-cellcolor grid with a 128×256-cell alpha grid (64×64 cells, the smallest allowedby [MIAF]; 65538 items in total) and checks that the generated file parsesback with all 65538 unique item IDs. The generated file was also verifiedbyte-per-byte against the ISO/IEC 14496-12 box layouts and decodes correctlywith libheif (with its default 1000-item security limit raised).
GridApiTest.CellsTooManyForDimgReferenceCount: checks that a 256×256-cellgrid is rejected with AVIF_RESULT_INVALID_IMAGE_GRID.

[y-guyon]

Thank you for your interest in libavif.

Item IDs are stored as uint16_t. avifEncoderAddImageGrid() accepts grids of up to 256x256 cells, but a 256x256 grid contains 65536 cells, so encoding one requires 65537 distinct item IDs (65536 cell items plus the grid item).

Is that an issue on the libavif side or on the file format side? Is such a file conformant to HEIF/AVIF? If so, the error should be NOT_IMPLEMENTED. Or even better, just use uint32_t so that it is supported in libavif.

avifEncoderDataCreateItem() silently overflows the uint16_t lastItemID counter in that case: item IDs wrap around, the first cell item ends up with the same ID as the grid item, and the resulting file is invalid (duplicate IDs in iinf/iloc, truncated item_count).

If I understand corectly, libavif returns an error as it should, but the wrong error, and somewhat late?

• Without the fix, avifEncoderAddImageGrid() silently accepts the 256x256 grid and starts creating 65537 items and encoding 65536 cells; the new test does not complete (killed after a 25s timeout).

Does it ever finish? What happens then?

[krishna28238-arch]

Thank you for your interest in libavif.

Item IDs are stored as uint16_t. avifEncoderAddImageGrid() accepts grids of up to 256x256 cells, but a 256x256 grid contains 65536 cells, so encoding one requires 65537 distinct item IDs (65536 cell items plus the grid item).

Is that an issue on the libavif side or on the file format side? Is such a file conformant to HEIF/AVIF? If so, the error should be NOT_IMPLEMENTED. Or even better, just use uint32_t so that it is supported in libavif.

avifEncoderDataCreateItem() silently overflows the uint16_t lastItemID counter in that case: item IDs wrap around, the first cell item ends up with the same ID as the grid item, and the resulting file is invalid (duplicate IDs in iinf/iloc, truncated item_count).

If I understand corectly, libavif returns an error as it should, but the wrong error, and somewhat late?

• Without the fix, avifEncoderAddImageGrid() silently accepts the 256x256 grid and starts creating 65537 items and encoding 65536 cells; the new test does not complete (killed after a 25s timeout).

Does it ever finish? What happens then?

Thank you for the review! I dug into the spec and reworked the patchaccordingly (force-pushed). Answers to your questions:

1. libavif-side or file format side?

Both, at two different levels:

The 16-bit item ID truncation is purely a libavif-side bug. Item IDs areunsigned int(32) in the file format (ISO/IEC 14496-12 §8.11.6), so filescontaining more than 65535 items are conformant: they simply use the'infe' version 3, 'iloc' version 2, 'iref' version 1 and 'ipma'version 1 boxes — all of which libavif's reader already fully supports.A concrete conformant case that unpatched libavif silently corrupts: a colorgrid plus an alpha grid of 128×256 cells each (2 grid items + 2×32768 cellitems = 65538 items, each 'dimg' box holding 32768 ≤ 65535 references).Unpatched, uint16_t lastItemID wraps, the last alpha cells collide withexisting IDs, and the file is written broken.
However, the original 256×256-cell repro itself is not representableconformantly, for an independent reason: §8.11.12.1 of ISO/IEC 14496-12requires that "all the references for one item of a specific type arecollected into a single item type reference box", whose reference_countfield is unsigned int(16) in both 'iref' versions (§8.11.12.2 — onlyfrom_item_ID/to_item_ID widen to 32 bits in version 1), and ISO/IEC23008-12 (HEIF) §6.6.1 additionally states that "the number ofSingleItemTypeReferenceBoxes with the box type 'dimg' and with the same valueof from_item_ID shall not be greater than 1". So a grid item can referenceat most 65535 cells, and a 256×256-cell grid (65536 cells) cannot beencoded by any conformant encoder.
2. uint32_t (done) + upfront rejection of what the format cannot express.

I implemented your preferred option: all encoder item IDs are now uint32_t,and the 32-bit box variants are written only when an item ID or item countdoes not fit on 16 bits (files that fit in the 16-bit space are byte-for-byteunchanged — verified). On top of that, avifEncoderAddImageGrid() now rejectsgrids with more than 65535 cells (e.g. 256×256) upfront withAVIF_RESULT_INVALID_IMAGE_GRID and a diagnostic explaining the 'dimg'reference_count limit, instead of generating a broken file. Note that65535 = 3×5×17×257 admits no factor pair with both factors ≤ 256, so thelargest single-plane grid is 256×255 cells; grids of multiple planes(color+alpha, gain map) can still total more than 65535 items — up to 65535cells each — which is exactly what the uint32_t rework enables.

3. Does encoding finish, and what happens?

Yes: on unpatched main, both avifEncoderAddImageGrid() andavifEncoderFinish() return AVIF_RESULT_OK (≈42 s and a 3.9 MB file for the256×256 repro — no hang, no error), and only decoding fails withAVIF_RESULT_BMFF_PARSE_FAILED ("Box[iref] has an invalid item ID [0]"):the failure is completely silent on the producer side. With the patch,conformant grids (≤65535 cells per plane) encode and round-trip correctly, andnon-representable grids fail immediately with the diagnostic above.

About the earlier CI failures on this branch: my first push contained twospec misreadings on my side — I wrote/parsed the 'iref' v1 reference_countas 32-bit and placed 'iloc''s construction_method before item_ID — whichbroke the idat tests, plus clang-format violations. Both box layouts are nowper spec (reference_count stays 16-bit; item_ID precedesconstruction_method), which is also why the reader needs no changes at all.The 128×256 color+alpha grid test file was additionally validated byte-per-byteagainst §8.11.3/§8.11.12/§8.11.14 layouts and decodes correctly with libheif1.19.8 (with its item-count security limit raised). Full test suite passeslocally (62/62).

The regression tests cover:

GridApiTest.ColorAlphaGridExceeding16BitItemIDs: encodes the 128×256color+alpha grid described above (64×64 cells, smallest allowed by MIAF) andchecks the file parses back with all 65538 unique item IDs;
GridApiTest.CellsTooManyForDimgReferenceCount: checks a 256×256-cell gridis rejected with AVIF_RESULT_INVALID_IMAGE_GRID.
Happy to adjust anything — in particular, if you prefer a different error codethan AVIF_RESULT_INVALID_IMAGE_GRID for the >65535-cell rejection (e.g.AVIF_RESULT_NOT_IMPLEMENTED), or a mention of the 65535-cell limit in theavifEncoderAddImageGrid() documentation in avif.h, I can do that.

[fallenmi]

Reviewed exact head 34803bfcf910c3cdaf2f159043d77d98f2afb615 against exact PR base b6fb1860837541d6e0c94386f2d09ba7c2341770 and current main eb673097950e0b0ec77f696958091ef0244a45a9. The current branch merges cleanly; I tested merge tree ec910aaad50434f75fe7291ad91efe459125d958 because GitHub's merge ref is still based on the previous main commit.

The 32-bit item-ID transition is internally consistent: iloc, iinf/infe, iref, and ipma switch versions together only when the consecutive ID space exceeds 16 bits, while ordinary files retain the existing encodings. Rejecting grids above 65,535 cells before item creation also correctly respects the 16-bit dimg reference count, and the small target pre-index removes the previous quadratic scan without changing reference order.

I generated and independently parsed the 128x256 color+alpha case. The 3,867,276-byte artifact contains exactly 65,538 unique item IDs across iloc v2, iinf v1 / infe v3, and ipma v1; iref v1 contains two ordered 32,768-target dimg references plus the expected auxl. Exact head and the current merge produced the same SHA-256 (a4d3eb01f00fbe4ee0c040224b18b8607248d49b962323de03b8c1e94212e3). A normal 768x512 encode is byte-identical across base, head, and current merge.

The grid suite passes 5/5 on base and 7/7 on both head and current merge. An additional 153 writer-adjacent tests pass on each of head and current merge, git diff --check is clean, and all 25 GitHub check-runs / 11 workflow runs are successful. I found no blocking issue.

AI disclosure: I used OpenAI Codex to inspect the exact revisions and repository policy, build the current-main merge oracle, run the focused and adjacent test suites, generate and independently parse the large-grid artifact, compare deterministic outputs, and draft this review. I verified the evidence and conclusion.

[y-guyon]

Thanks for the update. There is too much text and this PR is too big for me to review easily.

Please send a separate tiny PR for each of the following changes:

• Do not call avifEncoderDataCreateItem() if there are too many items already
• Use uint32_t to store item IDs where relevant
• Refuse encoding too many cells

There should be a test in each PR that fails at head and passes with the change.

For the other changes contained in this PR, let's discuss them again once the above are merged.