From 90e9bfaa53e4802eb3743d3db77d6566b3fcb028 Mon Sep 17 00:00:00 2001 From: Yannis Guyon Date: Tue, 23 Jul 2024 16:23:08 +0200 Subject: [PATCH] Fix out-of-order dimg grid associations --- CHANGELOG.md | 2 + src/read.c | 104 ++++++++++++++++++++++++------------ tests/CMakeLists.txt | 9 ++-- tests/gtest/avifdimgtest.cc | 80 +++++++++++++++++++++++++++ 4 files changed, 155 insertions(+), 40 deletions(-) create mode 100644 tests/gtest/avifdimgtest.cc diff --git a/CHANGELOG.md b/CHANGELOG.md index 90fed690a8..df21e894f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ The changes are relative to the previous release, unless the baseline is specifi * Fix CMake config shared library leaks https://github.com/AOMediaCodec/libavif/issues/2264. * Fix clang-cl compilation. +* Fix out-of-order 'dimg' grid associations + https://github.com/AOMediaCodec/libavif/issues/2311. ## [1.1.0] - 2024-07-11 diff --git a/src/read.c b/src/read.c index a942841a61..56ef55f8fc 100644 --- a/src/read.c +++ b/src/read.c @@ -1420,17 +1420,42 @@ static avifCodecType avifDecoderItemGetGridCodecType(const avifDecoderItem * gri return AVIF_CODEC_TYPE_UNKNOWN; } -static avifResult avifDecoderGenerateImageGridTiles(avifDecoder * decoder, avifImageGrid * grid, avifDecoderItem * gridItem, avifItemCategory itemCategory) +// Fills the dimgIdxToItemIdx array with a mapping from each 0-based tile index in the 'dimg' reference +// to its corresponding 0-based index in the avifMeta::items array. +static avifResult avifFillDimgIdxToItemIdxArray(uint32_t * dimgIdxToItemIdx, avifImageGrid * grid, avifDecoderItem * gridItem) { - // Count number of dimg for this item, bail out if it doesn't match perfectly - unsigned int tilesAvailable = 0; - avifDecoderItem * firstTileItem = NULL; - avifBool progressive = AVIF_TRUE; + const uint32_t numTiles = grid->rows * grid->columns; + const uint32_t itemIndexNotSet = UINT32_MAX; + for (uint32_t dimgIdx = 0; dimgIdx < numTiles; ++dimgIdx) { + dimgIdxToItemIdx[dimgIdx] = itemIndexNotSet; + } for (uint32_t i = 0; i < gridItem->meta->items.count; ++i) { - avifDecoderItem * item = gridItem->meta->items.item[i]; - if (item->dimgForID != gridItem->id) { - continue; + if (gridItem->meta->items.item[i]->dimgForID == gridItem->id) { + avifDecoderItem * tileItem = gridItem->meta->items.item[i]; + AVIF_CHECKERR(tileItem->dimgIdx < numTiles, AVIF_RESULT_BMFF_PARSE_FAILED); + AVIF_CHECKERR(dimgIdxToItemIdx[tileItem->dimgIdx] == itemIndexNotSet, AVIF_RESULT_BMFF_PARSE_FAILED); + dimgIdxToItemIdx[tileItem->dimgIdx] = i; } + } + for (uint32_t dimgIdx = 0; dimgIdx < numTiles; ++dimgIdx) { + AVIF_CHECKERR(dimgIdxToItemIdx[dimgIdx] != itemIndexNotSet, AVIF_RESULT_BMFF_PARSE_FAILED); + } + return AVIF_RESULT_OK; +} + +// Creates the tiles and associate them to the items in the order of the 'dimg' association. +static avifResult avifDecoderGenerateImageGridTiles(avifDecoder * decoder, + avifImageGrid * grid, + avifDecoderItem * gridItem, + avifItemCategory itemCategory, + const uint32_t * dimgIdxToItemIdx) +{ + avifDecoderItem * firstTileItem = NULL; + avifBool progressive = AVIF_TRUE; + for (uint32_t dimgIdx = 0; dimgIdx < grid->rows * grid->columns; ++dimgIdx) { + const uint32_t itemIdx = dimgIdxToItemIdx[dimgIdx]; + AVIF_ASSERT_OR_RETURN(itemIdx < gridItem->meta->items.count); + avifDecoderItem * item = gridItem->meta->items.item[itemIdx]; // According to HEIF (ISO 14496-12), Section 6.6.2.3.1, the SingleItemTypeReferenceBox of type 'dimg' // identifies the input images of the derived image item of type 'grid'. Since the reference_count @@ -1505,22 +1530,11 @@ static avifResult avifDecoderGenerateImageGridTiles(avifDecoder * decoder, avifI if (!item->progressive) { progressive = AVIF_FALSE; } - ++tilesAvailable; } if (itemCategory == AVIF_ITEM_COLOR && progressive) { // If all the items that make up the grid are progressive, then propagate that status to the top-level grid item. gridItem->progressive = AVIF_TRUE; } - - if (tilesAvailable != grid->rows * grid->columns) { - avifDiagnosticsPrintf(&decoder->diag, - "Grid image of dimensions %ux%u requires %u tiles, but %u were found", - grid->columns, - grid->rows, - grid->rows * grid->columns, - tilesAvailable); - return AVIF_RESULT_INVALID_IMAGE_GRID; - } return AVIF_RESULT_OK; } @@ -4650,15 +4664,19 @@ static avifResult avifMetaFindAlphaItem(avifMeta * meta, return AVIF_RESULT_OK; } // If color item is a grid, check if there is an alpha channel which is represented as an auxl item to each color tile item. - uint32_t colorItemCount = colorInfo->grid.rows * colorInfo->grid.columns; - if (colorItemCount == 0) { + const uint32_t tileCount = colorInfo->grid.rows * colorInfo->grid.columns; + if (tileCount == 0) { *alphaItem = NULL; *isAlphaItemInInput = AVIF_FALSE; return AVIF_RESULT_OK; } - uint32_t * alphaItemIndices = (uint32_t *)avifAlloc(colorItemCount * sizeof(uint32_t)); - AVIF_CHECKERR(alphaItemIndices, AVIF_RESULT_OUT_OF_MEMORY); - uint32_t alphaItemCount = 0; + // Keep the same 'dimg' order as it defines where each tile is located in the reconstructed image. + uint32_t * dimgIdxToAlphaItemIdx = (uint32_t *)avifAlloc(tileCount * sizeof(uint32_t)); + AVIF_CHECKERR(dimgIdxToAlphaItemIdx != NULL, AVIF_RESULT_OUT_OF_MEMORY); + const uint32_t itemIndexNotSet = UINT32_MAX; + for (uint32_t dimgIdx = 0; dimgIdx < tileCount; ++dimgIdx) { + dimgIdxToAlphaItemIdx[dimgIdx] = itemIndexNotSet; + } for (uint32_t i = 0; i < meta->items.count; ++i) { const avifDecoderItem * const item = meta->items.item[i]; if (item->dimgForID == colorItem->id) { @@ -4670,25 +4688,29 @@ static avifResult avifMetaFindAlphaItem(avifMeta * meta, // One of the following invalid cases: // * Multiple items are claiming to be the alpha auxiliary of the current item. // * Alpha auxiliary is dimg for another item. - avifFree(alphaItemIndices); + avifFree(dimgIdxToAlphaItemIdx); *alphaItem = NULL; *isAlphaItemInInput = AVIF_FALSE; return AVIF_RESULT_INVALID_IMAGE_GRID; } - alphaItemIndices[alphaItemCount++] = j; + if (item->dimgIdx >= tileCount || dimgIdxToAlphaItemIdx[item->dimgIdx] != itemIndexNotSet) { + avifFree(dimgIdxToAlphaItemIdx); + *isAlphaItemInInput = AVIF_FALSE; + return AVIF_RESULT_BMFF_PARSE_FAILED; + } + dimgIdxToAlphaItemIdx[item->dimgIdx] = j; seenAlphaForCurrentItem = AVIF_TRUE; } } if (!seenAlphaForCurrentItem) { // No alpha auxiliary item was found for the current item. Treat this as an image without alpha. - avifFree(alphaItemIndices); + avifFree(dimgIdxToAlphaItemIdx); *alphaItem = NULL; *isAlphaItemInInput = AVIF_FALSE; return AVIF_RESULT_OK; } } } - AVIF_ASSERT_OR_RETURN(alphaItemCount == colorItemCount); // Find an unused ID. avifResult result; if (meta->items.count >= UINT32_MAX - 1) { @@ -4710,18 +4732,24 @@ static avifResult avifMetaFindAlphaItem(avifMeta * meta, result = avifMetaFindOrCreateItem(meta, newItemID, alphaItem); // Create new empty item. } if (result != AVIF_RESULT_OK) { - avifFree(alphaItemIndices); + avifFree(dimgIdxToAlphaItemIdx); *isAlphaItemInInput = AVIF_FALSE; return result; } memcpy((*alphaItem)->type, "grid", 4); // Make it a grid and register alpha items as its tiles. (*alphaItem)->width = colorItem->width; (*alphaItem)->height = colorItem->height; - for (uint32_t i = 0; i < alphaItemCount; ++i) { - avifDecoderItem * item = meta->items.item[alphaItemIndices[i]]; - item->dimgForID = (*alphaItem)->id; + for (uint32_t dimgIdx = 0; dimgIdx < tileCount; ++dimgIdx) { + if (dimgIdxToAlphaItemIdx[dimgIdx] == itemIndexNotSet) { + avifFree(dimgIdxToAlphaItemIdx); + *isAlphaItemInInput = AVIF_FALSE; + return AVIF_RESULT_BMFF_PARSE_FAILED; + } + avifDecoderItem * alphaTileItem = meta->items.item[dimgIdxToAlphaItemIdx[dimgIdx]]; + alphaTileItem->dimgForID = (*alphaItem)->id; + alphaTileItem->dimgIdx = dimgIdx; } - avifFree(alphaItemIndices); + avifFree(dimgIdxToAlphaItemIdx); *isAlphaItemInInput = AVIF_FALSE; alphaInfo->grid = colorInfo->grid; return AVIF_RESULT_OK; @@ -5017,7 +5045,15 @@ static avifResult avifDecoderGenerateImageTiles(avifDecoder * decoder, avifTileI { const uint32_t previousTileCount = decoder->data->tiles.count; if ((info->grid.rows > 0) && (info->grid.columns > 0)) { - AVIF_CHECKRES(avifDecoderGenerateImageGridTiles(decoder, &info->grid, item, itemCategory)); + // The number of tiles was verified in avifDecoderItemReadAndParse(). + uint32_t * dimgIdxToItemIdx = (uint32_t *)avifAlloc((size_t)info->grid.rows * info->grid.columns * sizeof(uint32_t)); + AVIF_CHECKERR(dimgIdxToItemIdx != NULL, AVIF_RESULT_OUT_OF_MEMORY); + avifResult result = avifFillDimgIdxToItemIdxArray(dimgIdxToItemIdx, &info->grid, item); + if (result == AVIF_RESULT_OK) { + result = avifDecoderGenerateImageGridTiles(decoder, &info->grid, item, itemCategory, dimgIdxToItemIdx); + } + avifFree(dimgIdxToItemIdx); + AVIF_CHECKRES(result); } else { AVIF_CHECKERR(item->size != 0, AVIF_RESULT_MISSING_IMAGE_ITEM); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 1d8f8b3b39..b932fb4b86 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -110,6 +110,7 @@ if(AVIF_ENABLE_GTEST) add_avif_internal_gtest_with_data(avifcolrconverttest) add_avif_internal_gtest(avifcolrtest) add_avif_gtest_with_data(avifdecodetest) + add_avif_gtest_with_data(avifdimgtest avifincrtest_helpers) add_avif_gtest_with_data(avifencodetest) if(AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP) @@ -123,12 +124,7 @@ if(AVIF_ENABLE_GTEST) add_avif_gtest(avifgridapitest) add_avif_gtest_with_data(avifilocextenttest) add_avif_gtest(avifimagetest) - - add_executable(avifincrtest gtest/avifincrtest.cc) - target_link_libraries(avifincrtest aviftest_helpers avifincrtest_helpers) - add_test(NAME avifincrtest COMMAND avifincrtest ${CMAKE_CURRENT_SOURCE_DIR}/data/) - register_test_for_coverage(avifincrtest ${CMAKE_CURRENT_SOURCE_DIR}/data/) - + add_avif_gtest_with_data(avifincrtest avifincrtest_helpers) add_avif_gtest_with_data(avifiostatstest) add_avif_gtest_with_data(avifkeyframetest) add_avif_gtest_with_data(aviflosslesstest) @@ -343,6 +339,7 @@ if(AVIF_CODEC_AVM_ENABLED) avifchangesettingtest avifcllitest avifcolrconverttest + avifdimgtest avifencodetest avifgridapitest avifincrtest diff --git a/tests/gtest/avifdimgtest.cc b/tests/gtest/avifdimgtest.cc new file mode 100644 index 0000000000..9617ffbc63 --- /dev/null +++ b/tests/gtest/avifdimgtest.cc @@ -0,0 +1,80 @@ +// Copyright 2024 Google LLC +// SPDX-License-Identifier: BSD-2-Clause + +#include +#include +#include + +#include "avif/avif.h" +#include "avifincrtest_helpers.h" +#include "aviftest_helpers.h" +#include "gtest/gtest.h" + +namespace avif { +namespace { + +// Used to pass the data folder path to the GoogleTest suites. +const char* data_path = nullptr; + +//------------------------------------------------------------------------------ + +TEST(IncrementalTest, Dimg) { + testutil::AvifRwData avif = + testutil::ReadFile(std::string(data_path) + "sofa_grid1x5_420.avif"); + ASSERT_NE(avif.size, 0u); + ImagePtr reference(avifImageCreateEmpty()); + ASSERT_NE(reference, nullptr); + DecoderPtr decoder(avifDecoderCreate()); + ASSERT_NE(decoder, nullptr); + ASSERT_EQ(avifDecoderReadMemory(decoder.get(), reference.get(), avif.data, + avif.size), + AVIF_RESULT_OK); + + // Change the order of the 'dimg' associations. + const uint8_t* kMeta = reinterpret_cast("dimg"); + uint8_t* dimg_position = + std::search(avif.data, avif.data + avif.size, kMeta, kMeta + 4); + ASSERT_NE(dimg_position, avif.data + avif.size); + uint8_t* to_item_id = dimg_position + /*"dimg"*/ 4 + /*from_item_ID*/ 2 + + /*reference_count*/ 2; + for (uint8_t i = 0; i < 5; ++i) { + const size_t offset = + i * sizeof(uint16_t) + /*most significant byte of uint16_t*/ 1; + EXPECT_EQ(to_item_id[offset], /*first tile item ID*/ 2 + i); + to_item_id[offset] = /*first tile item ID*/ 2 + 4 - i; + } + + // Verify that libavif detects the 'dimg' modification. + ImagePtr reference_dimg_swapped(avifImageCreateEmpty()); + ASSERT_NE(reference_dimg_swapped, nullptr); + ASSERT_EQ(avifDecoderReadMemory(decoder.get(), reference.get(), avif.data, + avif.size), + AVIF_RESULT_OK); + EXPECT_FALSE(testutil::AreImagesEqual(*reference, *reference_dimg_swapped)); + + // Verify that it works incrementally. + // enable_fine_incremental_check is false because the tiles are out-of-order. + ASSERT_EQ( + testutil::DecodeIncrementally( + avif, decoder.get(), /*is_persistent=*/true, /*give_size_hint=*/true, + /*use_nth_image_api=*/false, *reference_dimg_swapped, + /*cell_height=*/154, /*enable_fine_incremental_check=*/false), + AVIF_RESULT_OK); +} + +//------------------------------------------------------------------------------ + +} // namespace +} // namespace avif + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + if (argc < 2) { + std::cerr + << "The path to the test data folder must be provided as an argument" + << std::endl; + return 1; + } + avif::data_path = argv[1]; + return RUN_ALL_TESTS(); +}