From 992b69db4a6b3c0ea21c499a6baa9bc2da8e605d Mon Sep 17 00:00:00 2001 From: Vignesh Venkatasubramanian Date: Mon, 5 Feb 2024 11:44:37 -0800 Subject: [PATCH] Revert "Simpler itemID finding for alpha. (#1967)" This reverts commit 4378794207f7a244bc36730c63dcc2bc188b6b3c. Validating the ipma requirement of increasing item_ids inside avifMetaFindOrCreateItem is incorrect since that function is called when parsing several other boxes where new items could be added (for e.g. iloc). A sample avif file that is broken by this change: https://g-issues.chromium.org/issues/323735410#comment4 --- src/read.c | 68 +++++++++++++++++++++++++----------------------------- 1 file changed, 31 insertions(+), 37 deletions(-) diff --git a/src/read.c b/src/read.c index 4bc25479ac..6d91511073 100644 --- a/src/read.c +++ b/src/read.c @@ -787,7 +787,7 @@ static avifResult avifCheckItemID(const char * boxFourcc, uint32_t itemID, avifD return AVIF_RESULT_OK; } -static avifResult avifMetaFindOrCreateItem(avifMeta * meta, uint32_t itemID, avifDecoderItem ** item, avifDiagnostics * diag) +static avifResult avifMetaFindOrCreateItem(avifMeta * meta, uint32_t itemID, avifDecoderItem ** item) { *item = NULL; AVIF_ASSERT_OR_RETURN(itemID != 0); @@ -799,18 +799,6 @@ static avifResult avifMetaFindOrCreateItem(avifMeta * meta, uint32_t itemID, avi } } - if (meta->items.count != 0) { - // ISO/IEC 23008-12, First edition, 2017-12, Section 9.3.1: - // Each ItemPropertyAssociation box shall be ordered by increasing item_ID, and there shall - // be at most one association box for each item_ID, in any ItemPropertyAssociation box. - const uint32_t lastID = meta->items.item[meta->items.count - 1]->id; - if (itemID <= lastID) { - avifBreakOnError(); - avifDiagnosticsPrintf(diag, "The added itemID [%u] does not preserve the itemID order", itemID); - return AVIF_RESULT_BMFF_PARSE_FAILED; - } - } - avifDecoderItem ** itemPtr = (avifDecoderItem **)avifArrayPush(&meta->items); AVIF_CHECKERR(itemPtr != NULL, AVIF_RESULT_OUT_OF_MEMORY); *item = (avifDecoderItem *)avifAlloc(sizeof(avifDecoderItem)); @@ -1777,7 +1765,7 @@ static avifResult avifParseItemLocationBox(avifMeta * meta, const uint8_t * raw, #endif // AVIF_ENABLE_EXPERIMENTAL_AVIR avifDecoderItem * item; - AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, itemID, &item, diag)); + AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, itemID, &item)); if (item->extents.count > 0) { // This item has already been given extents via this iloc box. This is invalid. avifDiagnosticsPrintf(diag, "Item ID [%u] contains duplicate sets of extents", itemID); @@ -2391,7 +2379,7 @@ static avifResult avifParseItemPropertyAssociation(avifMeta * meta, const uint8_ prevItemID = itemID; avifDecoderItem * item; - AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, itemID, &item, diag)); + AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, itemID, &item)); if (item->ipmaSeen) { avifDiagnosticsPrintf(diag, "Duplicate Box[ipma] for item ID [%u]", itemID); return AVIF_RESULT_BMFF_PARSE_FAILED; @@ -2667,7 +2655,7 @@ static avifResult avifParseItemInfoEntry(avifMeta * meta, const uint8_t * raw, s } #endif // AVIF_ENABLE_EXPERIMENTAL_AVIR avifDecoderItem * item; - AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, itemID, &item, diag)); + AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, itemID, &item)); memcpy(item->type, itemType, sizeof(itemType)); item->contentType = contentType; @@ -2736,7 +2724,7 @@ static avifResult avifParseItemReferenceBox(avifMeta * meta, const uint8_t * raw AVIF_CHECKRES(avifCheckItemID("iref", fromID, diag)); avifDecoderItem * item; - AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, fromID, &item, diag)); + AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, fromID, &item)); if (!memcmp(irefHeader.type, "dimg", 4)) { if (item->hasDimgFrom) { // ISO/IEC 23008-12 (HEIF) 6.6.1: The number of SingleItemTypeReferenceBoxes with the box type 'dimg' @@ -2774,7 +2762,7 @@ static avifResult avifParseItemReferenceBox(avifMeta * meta, const uint8_t * raw } else if (!memcmp(irefHeader.type, "dimg", 4)) { // derived images refer in the opposite direction avifDecoderItem * dimg; - AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, toID, &dimg, diag)); + AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, toID, &dimg)); dimg->dimgForID = fromID; dimg->dimgIdx = refIndex; @@ -3574,7 +3562,7 @@ static avifResult avifParseCondensedImageBox(avifMeta * meta, uint64_t rawOffset meta->primaryItemID = 1; avifDecoderItem * colorItem; - AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, meta->primaryItemID, &colorItem, diag)); + AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, meta->primaryItemID, &colorItem)); memcpy(colorItem->type, "av01", 4); colorItem->width = width; colorItem->height = height; @@ -3583,7 +3571,7 @@ static avifResult avifParseCondensedImageBox(avifMeta * meta, uint64_t rawOffset avifDecoderItem * alphaItem = NULL; if (hasAlpha) { - AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, /*itemID=*/2, &alphaItem, diag)); + AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, /*itemID=*/2, &alphaItem)); memcpy(alphaItem->type, "av01", 4); alphaItem->width = width; alphaItem->height = height; @@ -3688,7 +3676,7 @@ static avifResult avifParseCondensedImageBox(avifMeta * meta, uint64_t rawOffset if (hasExif) { avifDecoderItem * exifItem; - AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, /*itemID=*/3, &exifItem, diag)); + AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, /*itemID=*/3, &exifItem)); memcpy(exifItem->type, "Exif", 4); exifItem->descForID = colorItem->id; colorItem->premByID = alphaIsPremultiplied; @@ -3703,7 +3691,7 @@ static avifResult avifParseCondensedImageBox(avifMeta * meta, uint64_t rawOffset if (hasXMP) { avifDecoderItem * xmpItem; - AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, /*itemID=*/4, &xmpItem, diag)); + AVIF_CHECKRES(avifMetaFindOrCreateItem(meta, /*itemID=*/4, &xmpItem)); memcpy(xmpItem->type, "mime", 4); memcpy(xmpItem->contentType.contentType, xmpContentType, xmpContentTypeSize); xmpItem->descForID = colorItem->id; @@ -4046,7 +4034,7 @@ avifResult avifDecoderNthImageMaxExtent(const avifDecoder * decoder, uint32_t fr // The data comes from an item. Let avifDecoderItemMaxExtent() do the heavy lifting. avifDecoderItem * item; - AVIF_CHECKRES(avifMetaFindOrCreateItem(decoder->data->meta, sample->itemID, &item, decoder->data->diag)); + AVIF_CHECKRES(avifMetaFindOrCreateItem(decoder->data->meta, sample->itemID, &item)); avifResult maxExtentResult = avifDecoderItemMaxExtent(item, sample, &sampleExtent); if (maxExtentResult != AVIF_RESULT_OK) { return maxExtentResult; @@ -4085,7 +4073,7 @@ static avifResult avifDecoderPrepareSample(avifDecoder * decoder, avifDecodeSamp // The data comes from an item. Let avifDecoderItemRead() do the heavy lifting. avifDecoderItem * item; - AVIF_CHECKRES(avifMetaFindOrCreateItem(decoder->data->meta, sample->itemID, &item, decoder->data->diag)); + AVIF_CHECKRES(avifMetaFindOrCreateItem(decoder->data->meta, sample->itemID, &item)); avifROData itemContents; if (sample->offset > SIZE_MAX) { return AVIF_RESULT_BMFF_PARSE_FAILED; @@ -4352,8 +4340,7 @@ static avifResult avifMetaFindAlphaItem(avifMeta * meta, const avifTileInfo * colorInfo, avifDecoderItem ** alphaItem, avifTileInfo * alphaInfo, - avifBool * isAlphaItemInInput, - avifDiagnostics * diag) + avifBool * isAlphaItemInInput) { for (uint32_t itemIndex = 0; itemIndex < meta->items.count; ++itemIndex) { avifDecoderItem * item = meta->items.item[itemIndex]; @@ -4411,17 +4398,25 @@ static avifResult avifMetaFindAlphaItem(avifMeta * meta, } } AVIF_ASSERT_OR_RETURN(alphaItemCount == colorItemCount); - // Figure out the last used itemID. + // Find an unused ID. avifResult result; - const uint32_t lastID = meta->items.item[meta->items.count - 1]->id; - if (lastID == UINT32_MAX) { - // In the improbable case where the last ID is the maximum one, ids cannot be kept ordered. - avifDiagnosticsPrintf(diag, - "Cannot set an itemID for alpha that fits the increasing " - "order as the maximum possible ID is in use."); + if (meta->items.count >= UINT32_MAX - 1) { + // In the improbable case where all IDs are used. result = AVIF_RESULT_DECODE_ALPHA_FAILED; } else { - result = avifMetaFindOrCreateItem(meta, lastID + 1, alphaItem, diag); // Create new empty item. + uint32_t newItemID = 0; + avifBool isUsed; + do { + ++newItemID; + isUsed = AVIF_FALSE; + for (uint32_t i = 0; i < meta->items.count; ++i) { + if (meta->items.item[i]->id == newItemID) { + isUsed = AVIF_TRUE; + break; + } + } + } while (isUsed && newItemID != 0); + result = avifMetaFindOrCreateItem(meta, newItemID, alphaItem); // Create new empty item. } if (result != AVIF_RESULT_OK) { avifFree(alphaItemIndices); @@ -4565,7 +4560,7 @@ static avifResult avifDecoderFindGainMapItem(const avifDecoder * decoder, AVIF_ASSERT_OR_RETURN(gainMapItemID != 0); avifDecoderItem * gainMapItemTmp; - AVIF_CHECKRES(avifMetaFindOrCreateItem(data->meta, gainMapItemID, &gainMapItemTmp, data->diag)); + AVIF_CHECKRES(avifMetaFindOrCreateItem(data->meta, gainMapItemID, &gainMapItemTmp)); if (avifDecoderItemShouldBeSkipped(gainMapItemTmp)) { avifDiagnosticsPrintf(data->diag, "Box[tmap] gain map item %d is not a supported image type", gainMapItemID); return AVIF_RESULT_INVALID_TONE_MAPPED_IMAGE; @@ -4891,8 +4886,7 @@ avifResult avifDecoderReset(avifDecoder * decoder) &data->tileInfos[AVIF_ITEM_COLOR], &mainItems[AVIF_ITEM_ALPHA], &data->tileInfos[AVIF_ITEM_ALPHA], - &isAlphaItemInInput, - data->diag)); + &isAlphaItemInInput)); if (mainItems[AVIF_ITEM_ALPHA]) { AVIF_CHECKRES(avifDecoderItemReadAndParse(decoder, mainItems[AVIF_ITEM_ALPHA],