From b0869b2f3502ecd377b70e6fbdeee52670bbce9b Mon Sep 17 00:00:00 2001 From: Maryla Date: Fri, 17 Jan 2025 18:04:18 +0100 Subject: [PATCH 1/3] Fix assert failure when the gainmap has a constant value. Add defensive clamps. Add more tests. --- src/gainmap.c | 15 ++++--- tests/gtest/avifgainmaptest.cc | 73 +++++++++++++++++++++++++++++++++- 2 files changed, 80 insertions(+), 8 deletions(-) diff --git a/src/gainmap.c b/src/gainmap.c index d7acc700a5..78dd2b937f 100644 --- a/src/gainmap.c +++ b/src/gainmap.c @@ -704,17 +704,20 @@ avifResult avifRGBImageComputeGainMap(const avifRGBImage * baseRgbImage, // Scale the gain map values to map [min, max] range to [0, 1]. for (int c = 0; c < numGainMapChannels; ++c) { - const float range = gainMapMaxLog2[c] - gainMapMinLog2[c]; - if (range <= 0.0f) { - continue; - } + const float range = AVIF_MAX(gainMapMaxLog2[c] - gainMapMinLog2[c], 0.0f); const float gainMapGamma = avifUnsignedFractionToFloat(gainMap->gainMapGamma[c]); for (int j = 0; j < height; ++j) { for (int i = 0; i < width; ++i) { // Remap [min; max] range to [0; 1] - const float v = AVIF_CLAMP(gainMapF[c][j * width + i], gainMapMinLog2[c], gainMapMaxLog2[c]); - gainMapF[c][j * width + i] = powf((v - gainMapMinLog2[c]) / range, gainMapGamma); + if (range == 0.0f) { + // If the range is 0, the gain map values will be multiplied by zero when tonemapping so the values + // don't matter, but we still need to make sure that gainMapF is in [0,1]. + gainMapF[c][j * width + i] = 0.0f; + } else { + const float v = AVIF_CLAMP(gainMapF[c][j * width + i], gainMapMinLog2[c], gainMapMaxLog2[c]); + gainMapF[c][j * width + i] = AVIF_CLAMP(powf((v - gainMapMinLog2[c]) / range, gainMapGamma), 0.0f, 1.0f); + } } } } diff --git a/tests/gtest/avifgainmaptest.cc b/tests/gtest/avifgainmaptest.cc index f9932fb665..bd827d5ff7 100644 --- a/tests/gtest/avifgainmaptest.cc +++ b/tests/gtest/avifgainmaptest.cc @@ -1292,8 +1292,7 @@ TEST_P(CreateGainMapTest, Create) { (uint32_t)std::round((float)image1->width / downscaling), 1u); const uint32_t gain_map_height = std::max( (uint32_t)std::round((float)image1->height / downscaling), 1u); - std::unique_ptr gain_map( - avifGainMapCreate(), avifGainMapDestroy); + GainMapPtr gain_map(avifGainMapCreate()); gain_map->image = avifImageCreate(gain_map_width, gain_map_height, gain_map_depth, gain_map_format); @@ -1441,10 +1440,32 @@ INSTANTIATE_TEST_SUITE_P( /*gain_map_format=*/AVIF_PIXEL_FORMAT_YUV444, /*min_psnr=*/55.0f, /*max_psnr=*/80.0f))); +TEST(GainMapTest, CreateGainMapConstantFactor) { + // Used only to initialize rgb images. + ImagePtr yuv(avifImageCreate(10, 10, 8, AVIF_PIXEL_FORMAT_YUV444)); + testutil::AvifRgbImage base_image(yuv.get(), 8, AVIF_RGB_FORMAT_RGB); + testutil::AvifRgbImage alt_image(yuv.get(), 8, AVIF_RGB_FORMAT_RGB); + for (uint32_t i = 0; i < base_image.width * base_image.height * 3; ++i) { + base_image.pixels[i] = 10; + alt_image.pixels[i] = 200; // 20x factor compared to the base image. + } + GainMapPtr gain_map(avifGainMapCreate()); + gain_map->image = avifImageCreate(5, 5, 8, AVIF_PIXEL_FORMAT_YUV444); + avifDiagnostics diag; + avifResult result = avifRGBImageComputeGainMap( + &base_image, AVIF_COLOR_PRIMARIES_SRGB, + AVIF_TRANSFER_CHARACTERISTICS_SRGB, &alt_image, AVIF_COLOR_PRIMARIES_SRGB, + AVIF_TRANSFER_CHARACTERISTICS_SRGB, gain_map.get(), &diag); + + ASSERT_EQ(result, AVIF_RESULT_OK) + << avifResultToString(result) << " " << diag.error; +} + TEST(FindMinMaxWithoutOutliers, AllSame) { constexpr int kNumValues = 10000; for (float v : {0.0f, 42.f, -12.f, 1.52f}) { + SCOPED_TRACE(v); std::vector values(kNumValues, v); float min, max; @@ -1456,10 +1477,58 @@ TEST(FindMinMaxWithoutOutliers, AllSame) { } } +TEST(FindMinMaxWithoutOutliers, AllSameExceptOne) { + constexpr int kNumValues = 10000; + + for (float v : {42.f, -12.f, 1.52f}) { + SCOPED_TRACE(v); + std::vector values(kNumValues, v); + values[42] = v * 2; + + float min, max; + ASSERT_EQ( + avifFindMinMaxWithoutOutliers(values.data(), kNumValues, &min, &max), + AVIF_RESULT_OK); + constexpr float kBucketSize = 0.01f; // Should match the value in gainmap.c + const float kEpsilon = 0.00001f; + if (v > 0) { + EXPECT_NEAR(min, v, kEpsilon); + EXPECT_NEAR(max, v + kBucketSize, kEpsilon); + } else { + EXPECT_NEAR(min, v - kBucketSize, kEpsilon); + EXPECT_NEAR(max, v, kEpsilon); + } + } +} + +TEST(FindMinMaxWithoutOutliers, AllSameExceptOneFewValues) { + constexpr int kNumValues = 100; // Not enough values to remove outliers. + + for (float v : {42.f, -12.f, 1.52f}) { + SCOPED_TRACE(v); + std::vector values(kNumValues, v); + values[42] = v * 2; + + float min, max; + ASSERT_EQ( + avifFindMinMaxWithoutOutliers(values.data(), kNumValues, &min, &max), + AVIF_RESULT_OK); + const float kEpsilon = 0.00001f; + if (v > 0) { + EXPECT_NEAR(min, v, kEpsilon); + EXPECT_NEAR(max, v * 2, kEpsilon); // Outlier is kept. + } else { + EXPECT_NEAR(min, v * 2, kEpsilon); // Outlier is kept. + EXPECT_NEAR(max, v, kEpsilon); + } + } +} + TEST(FindMinMaxWithoutOutliers, Test) { constexpr int kNumValues = 10000; for (const float value_shift : {0.0f, -20.0f, 20.0f}) { + SCOPED_TRACE(value_shift); SCOPED_TRACE("value_shift: " + std::to_string(value_shift)); std::vector values(kNumValues, value_shift + 2.0f); int k = 0; From 5e2c3e8e10d0a5f7f7f125ffb0ab0f1aa921d849 Mon Sep 17 00:00:00 2001 From: Maryla Date: Wed, 22 Jan 2025 14:38:17 +0100 Subject: [PATCH 2/3] Refactor. Move condition out of the loops. Move computation out of macro. --- src/gainmap.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/gainmap.c b/src/gainmap.c index 78dd2b937f..cd8650927a 100644 --- a/src/gainmap.c +++ b/src/gainmap.c @@ -707,16 +707,22 @@ avifResult avifRGBImageComputeGainMap(const avifRGBImage * baseRgbImage, const float range = AVIF_MAX(gainMapMaxLog2[c] - gainMapMinLog2[c], 0.0f); const float gainMapGamma = avifUnsignedFractionToFloat(gainMap->gainMapGamma[c]); - for (int j = 0; j < height; ++j) { - for (int i = 0; i < width; ++i) { - // Remap [min; max] range to [0; 1] - if (range == 0.0f) { + if (range == 0.0f) { + for (int j = 0; j < height; ++j) { + for (int i = 0; i < width; ++i) { // If the range is 0, the gain map values will be multiplied by zero when tonemapping so the values // don't matter, but we still need to make sure that gainMapF is in [0,1]. gainMapF[c][j * width + i] = 0.0f; - } else { - const float v = AVIF_CLAMP(gainMapF[c][j * width + i], gainMapMinLog2[c], gainMapMaxLog2[c]); - gainMapF[c][j * width + i] = AVIF_CLAMP(powf((v - gainMapMinLog2[c]) / range, gainMapGamma), 0.0f, 1.0f); + } + } + } else { + // Remap [min; max] range to [0; 1] + for (int j = 0; j < height; ++j) { + for (int i = 0; i < width; ++i) { + float v = gainMapF[c][j * width + i]; + v = AVIF_CLAMP(v, gainMapMinLog2[c], gainMapMaxLog2[c]); + v = powf((v - gainMapMinLog2[c]) / range, gainMapGamma); + gainMapF[c][j * width + i] = AVIF_CLAMP(v, 0.0f, 1.0f); } } } From 7fc2f80d5d00a2f4b97561ad1782f19b7614c655 Mon Sep 17 00:00:00 2001 From: Maryla Date: Wed, 22 Jan 2025 15:06:22 +0100 Subject: [PATCH 3/3] Fix review comment --- src/gainmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gainmap.c b/src/gainmap.c index cd8650927a..9b7a1d4c8e 100644 --- a/src/gainmap.c +++ b/src/gainmap.c @@ -705,7 +705,6 @@ avifResult avifRGBImageComputeGainMap(const avifRGBImage * baseRgbImage, // Scale the gain map values to map [min, max] range to [0, 1]. for (int c = 0; c < numGainMapChannels; ++c) { const float range = AVIF_MAX(gainMapMaxLog2[c] - gainMapMinLog2[c], 0.0f); - const float gainMapGamma = avifUnsignedFractionToFloat(gainMap->gainMapGamma[c]); if (range == 0.0f) { for (int j = 0; j < height; ++j) { @@ -717,6 +716,7 @@ avifResult avifRGBImageComputeGainMap(const avifRGBImage * baseRgbImage, } } else { // Remap [min; max] range to [0; 1] + const float gainMapGamma = avifUnsignedFractionToFloat(gainMap->gainMapGamma[c]); for (int j = 0; j < height; ++j) { for (int i = 0; i < width; ++i) { float v = gainMapF[c][j * width + i];