From 7280dbe2be83fe5914ff5748cbd7d7df190bfff1 Mon Sep 17 00:00:00 2001 From: Maryla Date: Wed, 16 Oct 2024 17:31:35 +0200 Subject: [PATCH 1/3] Make sure the gain map metadata is towards the start of the file. The 'tmap' item data containing the gain map metadata was mistakenly being put at the end of the file. --- src/write.c | 7 ++-- tests/gtest/avifgainmaptest.cc | 8 +++-- tests/gtest/avifincrtest_helpers.cc | 53 +++++++++++++++++++++-------- tests/gtest/aviftest_helpers.cc | 8 ++--- tests/gtest/aviftest_helpers.h | 4 ++- 5 files changed, 54 insertions(+), 26 deletions(-) diff --git a/src/write.c b/src/write.c index 7e482804f6..7b1afaf8c9 100644 --- a/src/write.c +++ b/src/write.c @@ -2195,8 +2195,8 @@ static avifResult avifEncoderWriteMediaDataBox(avifEncoder * encoder, const size_t mdatStartOffset = avifRWStreamOffset(s); for (uint32_t itemPasses = 0; itemPasses < 3; ++itemPasses) { // Use multiple passes to pack in the following order: - // * Pass 0: metadata (Exif/XMP) - // * Pass 1: alpha, gain map (AV1) + // * Pass 0: metadata (Exif/XMP/gain map metadata) + // * Pass 1: alpha, gain map image (AV1) // * Pass 2: all other item data (AV1 color) // // See here for the discussion on alpha coming before color: @@ -2215,7 +2215,8 @@ static avifResult avifEncoderWriteMediaDataBox(avifEncoder * encoder, // this item has nothing for the mdat box continue; } - const avifBool isMetadata = !memcmp(item->type, "mime", 4) || !memcmp(item->type, "Exif", 4); + const avifBool isMetadata = !memcmp(item->type, "mime", 4) || !memcmp(item->type, "Exif", 4) || + !memcmp(item->type, "tmap", 4); if (metadataPass != isMetadata) { // only process metadata (XMP/Exif) payloads when metadataPass is true continue; diff --git a/tests/gtest/avifgainmaptest.cc b/tests/gtest/avifgainmaptest.cc index 1dadf9605d..aaf76c781a 100644 --- a/tests/gtest/avifgainmaptest.cc +++ b/tests/gtest/avifgainmaptest.cc @@ -344,17 +344,18 @@ TEST(GainMapTest, EncodeDecodeGrid) { constexpr int kCellHeight = 200; for (int i = 0; i < kGridCols * kGridRows; ++i) { + const int gradient_offset = i * 10; ImagePtr image = testutil::CreateImage(kCellWidth, kCellHeight, /*depth=*/10, AVIF_PIXEL_FORMAT_YUV444, AVIF_PLANES_ALL); ASSERT_NE(image, nullptr); image->transferCharacteristics = AVIF_TRANSFER_CHARACTERISTICS_PQ; - testutil::FillImageGradient(image.get()); + testutil::FillImageGradient(image.get(), gradient_offset); ImagePtr gain_map = testutil::CreateImage(kCellWidth / 2, kCellHeight / 2, /*depth=*/8, AVIF_PIXEL_FORMAT_YUV420, AVIF_PLANES_YUV); ASSERT_NE(gain_map, nullptr); - testutil::FillImageGradient(gain_map.get()); + testutil::FillImageGradient(gain_map.get(), gradient_offset); // 'image' now owns the gain map. image->gainMap = avifGainMapCreate(); ASSERT_NE(image->gainMap, nullptr); @@ -422,7 +423,8 @@ TEST(GainMapTest, EncodeDecodeGrid) { /*is_persistent=*/true, /*give_size_hint=*/true, /*use_nth_image_api=*/false, kCellHeight, /*enable_fine_incremental_check=*/true), - AVIF_RESULT_OK); + AVIF_RESULT_OK) + << decoder->diag.error; // Uncomment the following to save the encoded image as an AVIF file. // std::ofstream("/tmp/avifgainmaptest_grid.avif", std::ios::binary) diff --git a/tests/gtest/avifincrtest_helpers.cc b/tests/gtest/avifincrtest_helpers.cc index 51b232c53b..10a083ab36 100644 --- a/tests/gtest/avifincrtest_helpers.cc +++ b/tests/gtest/avifincrtest_helpers.cc @@ -80,8 +80,8 @@ void ComparePartialYuva(const avifImage& image1, const avifImage& image2, // byte_count were given to the decoder, for an image of height rows, split into // cells of cell_height rows. uint32_t GetMinDecodedRowCount(uint32_t height, uint32_t cell_height, - bool has_alpha, size_t available_byte_count, - size_t byte_count, + bool has_alpha, bool has_gain_map, + size_t available_byte_count, size_t byte_count, bool enable_fine_incremental_check) { // The whole image should be available when the full input is. if (available_byte_count >= byte_count) { @@ -104,15 +104,14 @@ uint32_t GetMinDecodedRowCount(uint32_t height, uint32_t cell_height, } available_byte_count -= 500; byte_count -= 500; - // Alpha, if any, is assumed to be located before the other planes and to - // represent at most 50% of the payload. - if (has_alpha) { - if (available_byte_count <= (byte_count / 2)) { - return 0; - } - available_byte_count -= byte_count / 2; - byte_count -= byte_count / 2; + // Extra planes (alpha, gain map), if any, are assumed to be located before + // the other planes and to represent at most 50% of the payload. + const int num_extra_planes = (has_alpha ? 1 : 0) + (has_gain_map ? 1 : 0); + if (available_byte_count <= ((byte_count / 2) * num_extra_planes)) { + return 0; } + available_byte_count -= (byte_count / 2) * num_extra_planes; + byte_count -= (byte_count / 2) * num_extra_planes; // Linearly map the input availability ratio to the decoded row ratio. const uint32_t min_decoded_cell_row_count = static_cast( (height / cell_height) * available_byte_count / byte_count); @@ -332,6 +331,13 @@ avifResult DecodeIncrementally(const avifRWData& encoded_avif, data.available.size = std::min(data.available.size + step, data.full_size); parse_result = avifDecoderParse(decoder); } + if (data.available.size == data.full_size) { + // Can happen if the data is in 'idat', or if some metadata is at the end of + // the file. But ideally this should be avoided. + printf( + "WARNING: had to provide the whole file for avifDecoderParse() to " + "succeed\n"); + } AVIF_CHECKRES(parse_result); // Decoding is incremental. @@ -347,13 +353,30 @@ avifResult DecodeIncrementally(const avifRWData& encoded_avif, return AVIF_RESULT_INVALID_ARGUMENT; } const uint32_t decoded_row_count = avifDecoderDecodedRowCount(decoder); - AVIF_CHECKERR(decoded_row_count >= previously_decoded_row_count, - AVIF_RESULT_INVALID_ARGUMENT); + if (decoded_row_count < previously_decoded_row_count) { + printf("ERROR: decoded row count decreased from %d to %d\n", + previously_decoded_row_count, decoded_row_count); + return AVIF_RESULT_INVALID_ARGUMENT; + } + bool has_gain_map = false; +#if defined(AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP) + has_gain_map = (reference.gainMap != nullptr); +#endif const uint32_t min_decoded_row_count = GetMinDecodedRowCount( reference.height, cell_height, reference.alphaPlane != nullptr, - data.available.size, data.full_size, enable_fine_incremental_check); - AVIF_CHECKERR(decoded_row_count >= min_decoded_row_count, - AVIF_RESULT_INVALID_ARGUMENT); + has_gain_map, data.available.size, data.full_size, + enable_fine_incremental_check); + if (decoded_row_count != previously_decoded_row_count) { + printf("bytes %zu decoded %d lines (expected min %d)\n", + data.available.size, decoded_row_count, min_decoded_row_count); + } + if (decoded_row_count < min_decoded_row_count) { + printf( + "ERROR: expected to have decoded at least %d rows with %zu available " + "bytes, but only %d were decoded\n", + min_decoded_row_count, data.available.size, decoded_row_count); + return AVIF_RESULT_INVALID_ARGUMENT; + } ComparePartialYuva(reference, *decoder->image, decoded_row_count); previously_decoded_row_count = decoded_row_count; diff --git a/tests/gtest/aviftest_helpers.cc b/tests/gtest/aviftest_helpers.cc index 3192972dd7..e1badd0833 100644 --- a/tests/gtest/aviftest_helpers.cc +++ b/tests/gtest/aviftest_helpers.cc @@ -145,7 +145,7 @@ void FillImagePlain(avifImage* image, const uint32_t yuva[4]) { } } -void FillImageGradient(avifImage* image) { +void FillImageGradient(avifImage* image, int offset) { for (avifChannelIndex c : {AVIF_CHAN_Y, AVIF_CHAN_U, AVIF_CHAN_V, AVIF_CHAN_A}) { const uint32_t limitedRangeMin = @@ -160,13 +160,13 @@ void FillImageGradient(avifImage* image) { const uint32_t row_bytes = avifImagePlaneRowBytes(image, c); for (uint32_t y = 0; y < plane_height; ++y) { for (uint32_t x = 0; x < plane_width; ++x) { - uint32_t value; + uint32_t value = (x + y + offset) % (plane_width + plane_height); if (image->yuvRange == AVIF_RANGE_FULL || c == AVIF_CHAN_A) { - value = (x + y) * ((1u << image->depth) - 1u) / + value = value * ((1u << image->depth) - 1u) / std::max(1u, plane_width + plane_height - 2); } else { value = limitedRangeMin + - (x + y) * (limitedRangeMax - limitedRangeMin) / + value * (limitedRangeMax - limitedRangeMin) / std::max(1u, plane_width + plane_height - 2); } if (avifImageUsesU16(image)) { diff --git a/tests/gtest/aviftest_helpers.h b/tests/gtest/aviftest_helpers.h index f464fd030c..4adfb59a18 100644 --- a/tests/gtest/aviftest_helpers.h +++ b/tests/gtest/aviftest_helpers.h @@ -115,7 +115,9 @@ ImagePtr CreateImage(int width, int height, int depth, // Set all pixels of each plane of an image. void FillImagePlain(avifImage* image, const uint32_t yuva[4]); -void FillImageGradient(avifImage* image); +// 'offset' is a value to spatially offset the gradient, useful to create +// distinct images. +void FillImageGradient(avifImage* image, int offset = 0); void FillImageChannel(avifRGBImage* image, uint32_t channel_offset, uint32_t value); From c50dfa702d53a869a22d83d5b4e18bb47c13a31f Mon Sep 17 00:00:00 2001 From: Maryla Date: Thu, 17 Oct 2024 14:13:21 +0200 Subject: [PATCH 2/3] Address review comments --- tests/gtest/avif_fuzztest_dec_incr.cc | 5 ++- tests/gtest/avif_fuzztest_enc_dec_incr.cc | 4 +- ...avif_fuzztest_enc_dec_incr_experimental.cc | 3 +- tests/gtest/avifincrtest_helpers.cc | 41 ++++++++++--------- tests/gtest/avifincrtest_helpers.h | 20 +++++---- 5 files changed, 43 insertions(+), 30 deletions(-) diff --git a/tests/gtest/avif_fuzztest_dec_incr.cc b/tests/gtest/avif_fuzztest_dec_incr.cc index 5d7814f01a..cc0e310d17 100644 --- a/tests/gtest/avif_fuzztest_dec_incr.cc +++ b/tests/gtest/avif_fuzztest_dec_incr.cc @@ -101,7 +101,10 @@ void DecodeIncr(const std::string& arbitrary_bytes, bool is_persistent, const uint32_t max_cell_height = reference->height; const avifResult result = DecodeIncrementally( encoded_data, decoder.get(), is_persistent, give_size_hint, - use_nth_image_api, *reference, max_cell_height); + use_nth_image_api, *reference, max_cell_height, + /*enable_fine_incremental_check=*/false, + /*expect_whole_file_read=*/true, + /*expect_parse_success_from_partial_file=*/false); // The result does not matter, as long as we do not crash. (void)result; } diff --git a/tests/gtest/avif_fuzztest_enc_dec_incr.cc b/tests/gtest/avif_fuzztest_enc_dec_incr.cc index 14917f3ceb..288fd1021c 100644 --- a/tests/gtest/avif_fuzztest_enc_dec_incr.cc +++ b/tests/gtest/avif_fuzztest_enc_dec_incr.cc @@ -61,7 +61,9 @@ void EncodeDecodeGridValid(ImagePtr image, EncoderPtr encoder, const avifResult decode_result = DecodeNonIncrementallyAndIncrementally( encoded_data, decoder.get(), is_encoded_data_persistent, - give_size_hint_to_decoder, /*use_nth_image_api=*/true, cell_height); + give_size_hint_to_decoder, /*use_nth_image_api=*/true, cell_height, + /*enable_fine_incremental_check=*/false, /*expect_whole_file_read=*/true, + /*expect_parse_success_from_partial_file=*/false); ASSERT_EQ(decode_result, AVIF_RESULT_OK) << avifResultToString(decode_result); } diff --git a/tests/gtest/avif_fuzztest_enc_dec_incr_experimental.cc b/tests/gtest/avif_fuzztest_enc_dec_incr_experimental.cc index a7b410cee0..26c181294e 100644 --- a/tests/gtest/avif_fuzztest_enc_dec_incr_experimental.cc +++ b/tests/gtest/avif_fuzztest_enc_dec_incr_experimental.cc @@ -93,7 +93,8 @@ void EncodeDecodeGridValid(ImagePtr image, EncoderPtr encoder, const avifResult decode_result = DecodeNonIncrementallyAndIncrementally( encoded_data, decoder.get(), is_encoded_data_persistent, give_size_hint_to_decoder, /*use_nth_image_api=*/true, cell_height, - /*enable_fine_incremental_check=*/false, expect_whole_file_read); + /*enable_fine_incremental_check=*/false, expect_whole_file_read, + /*expect_parse_success_from_partial_file=*/false); ASSERT_EQ(decode_result, AVIF_RESULT_OK) << avifResultToString(decode_result); } diff --git a/tests/gtest/avifincrtest_helpers.cc b/tests/gtest/avifincrtest_helpers.cc index 10a083ab36..6c0a8b83e5 100644 --- a/tests/gtest/avifincrtest_helpers.cc +++ b/tests/gtest/avifincrtest_helpers.cc @@ -105,13 +105,16 @@ uint32_t GetMinDecodedRowCount(uint32_t height, uint32_t cell_height, available_byte_count -= 500; byte_count -= 500; // Extra planes (alpha, gain map), if any, are assumed to be located before - // the other planes and to represent at most 50% of the payload. + // the color planes. It's assumed that each extra planes is at most + // total_size / (1 + num_extra_planes). const int num_extra_planes = (has_alpha ? 1 : 0) + (has_gain_map ? 1 : 0); - if (available_byte_count <= ((byte_count / 2) * num_extra_planes)) { + const size_t max_size_of_extra_planes = static_cast( + (byte_count / (num_extra_planes + 1)) * num_extra_planes); + if (available_byte_count <= max_size_of_extra_planes) { return 0; } - available_byte_count -= (byte_count / 2) * num_extra_planes; - byte_count -= (byte_count / 2) * num_extra_planes; + available_byte_count -= max_size_of_extra_planes; + byte_count -= max_size_of_extra_planes; // Linearly map the input availability ratio to the decoded row ratio. const uint32_t min_decoded_cell_row_count = static_cast( (height / cell_height) * available_byte_count / byte_count); @@ -294,7 +297,8 @@ avifResult DecodeIncrementally(const avifRWData& encoded_avif, bool give_size_hint, bool use_nth_image_api, const avifImage& reference, uint32_t cell_height, bool enable_fine_incremental_check, - bool expect_whole_file_read) { + bool expect_whole_file_read, + bool expect_parse_success_from_partial_file) { // AVIF cells are at least 64 pixels tall. if (cell_height != reference.height) { AVIF_CHECKERR(cell_height >= 64u, AVIF_RESULT_INVALID_ARGUMENT); @@ -331,12 +335,14 @@ avifResult DecodeIncrementally(const avifRWData& encoded_avif, data.available.size = std::min(data.available.size + step, data.full_size); parse_result = avifDecoderParse(decoder); } - if (data.available.size == data.full_size) { + if (data.available.size == data.full_size && + expect_parse_success_from_partial_file) { // Can happen if the data is in 'idat', or if some metadata is at the end of // the file. But ideally this should be avoided. printf( - "WARNING: had to provide the whole file for avifDecoderParse() to " + "ERROR: had to provide the whole file for avifDecoderParse() to " "succeed\n"); + AVIF_CHECKERR(false, AVIF_RESULT_INVALID_ARGUMENT); } AVIF_CHECKRES(parse_result); @@ -350,13 +356,13 @@ avifResult DecodeIncrementally(const avifRWData& encoded_avif, std::cerr << (use_nth_image_api ? "avifDecoderNthImage(0)" : "avifDecoderNextImage()") << " returned WAITING_ON_IO instead of OK"; - return AVIF_RESULT_INVALID_ARGUMENT; + AVIF_CHECKERR(false, AVIF_RESULT_INVALID_ARGUMENT); } const uint32_t decoded_row_count = avifDecoderDecodedRowCount(decoder); if (decoded_row_count < previously_decoded_row_count) { printf("ERROR: decoded row count decreased from %d to %d\n", previously_decoded_row_count, decoded_row_count); - return AVIF_RESULT_INVALID_ARGUMENT; + AVIF_CHECKERR(false, AVIF_RESULT_INVALID_ARGUMENT); } bool has_gain_map = false; #if defined(AVIF_ENABLE_EXPERIMENTAL_GAIN_MAP) @@ -366,16 +372,12 @@ avifResult DecodeIncrementally(const avifRWData& encoded_avif, reference.height, cell_height, reference.alphaPlane != nullptr, has_gain_map, data.available.size, data.full_size, enable_fine_incremental_check); - if (decoded_row_count != previously_decoded_row_count) { - printf("bytes %zu decoded %d lines (expected min %d)\n", - data.available.size, decoded_row_count, min_decoded_row_count); - } if (decoded_row_count < min_decoded_row_count) { printf( "ERROR: expected to have decoded at least %d rows with %zu available " "bytes, but only %d were decoded\n", min_decoded_row_count, data.available.size, decoded_row_count); - return AVIF_RESULT_INVALID_ARGUMENT; + AVIF_CHECKERR(false, AVIF_RESULT_INVALID_ARGUMENT); } ComparePartialYuva(reference, *decoder->image, decoded_row_count); @@ -399,19 +401,18 @@ avifResult DecodeIncrementally(const avifRWData& encoded_avif, avifResult DecodeNonIncrementallyAndIncrementally( const avifRWData& encoded_avif, avifDecoder* decoder, bool is_persistent, bool give_size_hint, bool use_nth_image_api, uint32_t cell_height, - bool enable_fine_incremental_check, bool expect_whole_file_read) { + bool enable_fine_incremental_check, bool expect_whole_file_read, + bool expect_parse_success_from_partial_file) { ImagePtr reference(avifImageCreateEmpty()); if (reference == nullptr) return AVIF_RESULT_INVALID_ARGUMENT; decoder->allowIncremental = AVIF_FALSE; - if (avifDecoderReadMemory(decoder, reference.get(), encoded_avif.data, - encoded_avif.size) != AVIF_RESULT_OK) { - return AVIF_RESULT_INVALID_ARGUMENT; - } + AVIF_CHECKRES(avifDecoderReadMemory(decoder, reference.get(), + encoded_avif.data, encoded_avif.size)); const avifResult result = DecodeIncrementally( encoded_avif, decoder, is_persistent, give_size_hint, use_nth_image_api, *reference, cell_height, enable_fine_incremental_check, - expect_whole_file_read); + expect_whole_file_read, expect_parse_success_from_partial_file); return result; } diff --git a/tests/gtest/avifincrtest_helpers.h b/tests/gtest/avifincrtest_helpers.h index 7dd6c94190..147a1fc08a 100644 --- a/tests/gtest/avifincrtest_helpers.h +++ b/tests/gtest/avifincrtest_helpers.h @@ -52,12 +52,17 @@ void EncodeRectAsIncremental(const avifImage& image, uint32_t width, // incremental granularity. enable_fine_incremental_check checks that sample // rows are gradually output when feeding more and more input bytes to the // decoder. -avifResult DecodeIncrementally(const avifRWData& encoded_avif, - avifDecoder* decoder, bool is_persistent, - bool give_size_hint, bool use_nth_image_api, - const avifImage& reference, uint32_t cell_height, - bool enable_fine_incremental_check = false, - bool expect_whole_file_read = true); +// If 'expect_parse_success_from_partial_file' is true, avifDecoderParse +// should succeed before the whole file is available. Returns an error +// if avifDecoderParse fails until all the bytes are available. +// `expect_parse_success_from_partial_file` should be set to false if the file +// may be using 'idat' or may have some metadata at the end of the file. +avifResult DecodeIncrementally( + const avifRWData& encoded_avif, avifDecoder* decoder, bool is_persistent, + bool give_size_hint, bool use_nth_image_api, const avifImage& reference, + uint32_t cell_height, bool enable_fine_incremental_check = false, + bool expect_whole_file_read = true, + bool expect_parse_success_from_partial_file = true); // Calls DecodeIncrementally() with the reference being a regular decoding of // encoded_avif. @@ -65,7 +70,8 @@ avifResult DecodeNonIncrementallyAndIncrementally( const avifRWData& encoded_avif, avifDecoder* decoder, bool is_persistent, bool give_size_hint, bool use_nth_image_api, uint32_t cell_height, bool enable_fine_incremental_check = false, - bool expect_whole_file_read = true); + bool expect_whole_file_read = true, + bool expect_parse_success_from_partial_file = true); } // namespace testutil } // namespace avif From 014e44eab8bb8528285aa570a8449ce8c6033823 Mon Sep 17 00:00:00 2001 From: Maryla Date: Thu, 17 Oct 2024 14:58:26 +0200 Subject: [PATCH 3/3] Fix gradient value too large --- tests/gtest/aviftest_helpers.cc | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/gtest/aviftest_helpers.cc b/tests/gtest/aviftest_helpers.cc index e1badd0833..7a3a360e84 100644 --- a/tests/gtest/aviftest_helpers.cc +++ b/tests/gtest/aviftest_helpers.cc @@ -158,16 +158,17 @@ void FillImageGradient(avifImage* image, int offset) { const uint32_t plane_height = avifImagePlaneHeight(image, c); uint8_t* row = avifImagePlane(image, c); const uint32_t row_bytes = avifImagePlaneRowBytes(image, c); + const uint32_t max_xy_sum = plane_width + plane_height - 2; for (uint32_t y = 0; y < plane_height; ++y) { for (uint32_t x = 0; x < plane_width; ++x) { - uint32_t value = (x + y + offset) % (plane_width + plane_height); + uint32_t value = (x + y + offset) % (max_xy_sum + 1); if (image->yuvRange == AVIF_RANGE_FULL || c == AVIF_CHAN_A) { - value = value * ((1u << image->depth) - 1u) / - std::max(1u, plane_width + plane_height - 2); + value = + value * ((1u << image->depth) - 1u) / std::max(1u, max_xy_sum); } else { - value = limitedRangeMin + - value * (limitedRangeMax - limitedRangeMin) / - std::max(1u, plane_width + plane_height - 2); + value = limitedRangeMin + value * + (limitedRangeMax - limitedRangeMin) / + std::max(1u, max_xy_sum); } if (avifImageUsesU16(image)) { reinterpret_cast(row)[x] = static_cast(value);