Skip to content

Commit

Permalink
Address code review comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
@maryla-uc
maryla-uc committed Sep 26, 2024
1 parent 527b80f commit 1448be6
Show file tree
Hide file tree
Showing 7 changed files with 113 additions and 144 deletions.
9 changes: 5 additions & 4 deletions apps/avifgainmaputil/swapbase_command.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,12 @@ avifResult ChangeBase(const avifImage& image, int depth,
swapped->depth = depth;
swapped->yuvFormat = yuvFormat;

if (image.gainMap->alternateHdrHeadroom.d == 0) {
return AVIF_RESULT_INVALID_ARGUMENT;
}
const float headroom =
image.gainMap->alternateHdrHeadroom.d == 0
? 0.0f
: static_cast<float>(image.gainMap->alternateHdrHeadroom.n) /
image.gainMap->alternateHdrHeadroom.d;
static_cast<float>(image.gainMap->alternateHdrHeadroom.n) /
image.gainMap->alternateHdrHeadroom.d;
const bool tone_mapping_to_sdr = (headroom == 0.0f);

swapped->colorPrimaries = image.gainMap->altColorPrimaries;
Expand Down
16 changes: 8 additions & 8 deletions apps/avifgainmaputil/tonemap_command.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,17 +85,17 @@ avifResult TonemapCommand::Run() {
<< " does not contain a gain map\n";
return AVIF_RESULT_INVALID_ARGUMENT;
}
if (image->gainMap->baseHdrHeadroom.d == 0 ||
image->gainMap->alternateHdrHeadroom.d == 0) {
return AVIF_RESULT_INVALID_ARGUMENT;
}

const float base_hdr_hreadroom =
image->gainMap->baseHdrHeadroom.d == 0
? 0.0f
: (float)image->gainMap->baseHdrHeadroom.n /
image->gainMap->baseHdrHeadroom.d;
static_cast<float>(image->gainMap->baseHdrHeadroom.n) /
image->gainMap->baseHdrHeadroom.d;
const float alternate_hdr_hreadroom =
image->gainMap->alternateHdrHeadroom.d == 0
? 0.0f
: (float)image->gainMap->alternateHdrHeadroom.n /
image->gainMap->alternateHdrHeadroom.d;
static_cast<float>(image->gainMap->alternateHdrHeadroom.n /
image->gainMap->alternateHdrHeadroom.d);
// We are either tone mapping to the base image (i.e. leaving it as is),
// or tone mapping to the alternate image (i.e. fully applying the gain map),
// or tone mapping in between (partially applying the gain map).
Expand Down
21 changes: 7 additions & 14 deletions src/avif.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,21 +263,14 @@ avifResult avifImageCopy(avifImage * dstImage, const avifImage * srcImage, avifP
AVIF_CHECKERR(dstImage->gainMap, AVIF_RESULT_OUT_OF_MEMORY);
}
for (int c = 0; c < 3; ++c) {
dstImage->gainMap->gainMapMin[c].n = srcImage->gainMap->gainMapMin[c].n;
dstImage->gainMap->gainMapMin[c].d = srcImage->gainMap->gainMapMin[c].d;
dstImage->gainMap->gainMapMax[c].n = srcImage->gainMap->gainMapMax[c].n;
dstImage->gainMap->gainMapMax[c].d = srcImage->gainMap->gainMapMax[c].d;
dstImage->gainMap->gainMapGamma[c].n = srcImage->gainMap->gainMapGamma[c].n;
dstImage->gainMap->gainMapGamma[c].d = srcImage->gainMap->gainMapGamma[c].d;
dstImage->gainMap->baseOffset[c].n = srcImage->gainMap->baseOffset[c].n;
dstImage->gainMap->baseOffset[c].d = srcImage->gainMap->baseOffset[c].d;
dstImage->gainMap->alternateOffset[c].n = srcImage->gainMap->alternateOffset[c].n;
dstImage->gainMap->alternateOffset[c].d = srcImage->gainMap->alternateOffset[c].d;
dstImage->gainMap->gainMapMin[c] = srcImage->gainMap->gainMapMin[c];
dstImage->gainMap->gainMapMax[c] = srcImage->gainMap->gainMapMax[c];
dstImage->gainMap->gainMapGamma[c] = srcImage->gainMap->gainMapGamma[c];
dstImage->gainMap->baseOffset[c] = srcImage->gainMap->baseOffset[c];
dstImage->gainMap->alternateOffset[c] = srcImage->gainMap->alternateOffset[c];
}
dstImage->gainMap->baseHdrHeadroom.n = srcImage->gainMap->baseHdrHeadroom.n;
dstImage->gainMap->baseHdrHeadroom.d = srcImage->gainMap->baseHdrHeadroom.d;
dstImage->gainMap->alternateHdrHeadroom.n = srcImage->gainMap->alternateHdrHeadroom.n;
dstImage->gainMap->alternateHdrHeadroom.d = srcImage->gainMap->alternateHdrHeadroom.d;
dstImage->gainMap->baseHdrHeadroom = srcImage->gainMap->baseHdrHeadroom;
dstImage->gainMap->alternateHdrHeadroom = srcImage->gainMap->alternateHdrHeadroom;
dstImage->gainMap->useBaseColorSpace = srcImage->gainMap->useBaseColorSpace;
AVIF_CHECKRES(avifRWDataSet(&dstImage->gainMap->altICC, srcImage->gainMap->altICC.data, srcImage->gainMap->altICC.size));
dstImage->gainMap->altColorPrimaries = srcImage->gainMap->altColorPrimaries;
Expand Down
92 changes: 49 additions & 43 deletions src/gainmap.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,30 +12,31 @@
static void avifGainMapSetDefaults(avifGainMap * gainMap)
{
for (int i = 0; i < 3; ++i) {
gainMap->gainMapMin[i].n = 1;
gainMap->gainMapMin[i].d = 1;
gainMap->gainMapMax[i].n = 1;
gainMap->gainMapMax[i].d = 1;
gainMap->baseOffset[i].n = 1;
gainMap->baseOffset[i].d = 64;
gainMap->alternateOffset[i].n = 1;
gainMap->alternateOffset[i].d = 64;
gainMap->gainMapGamma[i].n = 1;
gainMap->gainMapGamma[i].d = 1;
}
gainMap->baseHdrHeadroom.n = 0;
gainMap->baseHdrHeadroom.d = 1;
gainMap->alternateHdrHeadroom.n = 0;
gainMap->alternateHdrHeadroom.d = 1;
gainMap->gainMapMin[i] = (avifSignedFraction) { 1, 1 };
gainMap->gainMapMax[i] = (avifSignedFraction) { 1, 1 };
gainMap->baseOffset[i] = (avifSignedFraction) { 1, 64 };
gainMap->alternateOffset[i] = (avifSignedFraction) { 1, 64 };
gainMap->gainMapGamma[i] = (avifUnsignedFraction) { 1, 1 };
}
gainMap->baseHdrHeadroom = (avifUnsignedFraction) { 0, 1 };
gainMap->alternateHdrHeadroom = (avifUnsignedFraction) { 1, 1 };
gainMap->useBaseColorSpace = AVIF_TRUE;
}

static float toFloat(int numerator, int denominator)
static float avifSignedFractionToFloat(avifSignedFraction f)
{
if (denominator == 0) {
if (f.d == 0) {
return 0.0f;
}
return (float)numerator / denominator;
return (float)f.n / f.d;
}

static float avifUnsignedFractionToFloat(avifUnsignedFraction f)
{
if (f.d == 0) {
return 0.0f;
}
return (float)f.n / f.d;
}

// ---------------------------------------------------------------------------
Expand All @@ -44,8 +45,8 @@ static float toFloat(int numerator, int denominator)
// Returns a weight in [-1.0, 1.0] that represents how much the gain map should be applied.
static float avifGetGainMapWeight(float hdrHeadroom, const avifGainMap * gainMap)
{
const float baseHdrHeadroom = toFloat(gainMap->baseHdrHeadroom.n, gainMap->baseHdrHeadroom.d);
const float alternateHdrHeadroom = toFloat(gainMap->alternateHdrHeadroom.n, gainMap->alternateHdrHeadroom.d);
const float baseHdrHeadroom = avifUnsignedFractionToFloat(gainMap->baseHdrHeadroom);
const float alternateHdrHeadroom = avifUnsignedFractionToFloat(gainMap->alternateHdrHeadroom);
if (baseHdrHeadroom == alternateHdrHeadroom) {
// Do not apply the gain map if the HDR headroom is the same.
// This case is not handled in the specification and does not make practical sense.
Expand Down Expand Up @@ -76,6 +77,8 @@ avifResult avifRGBImageApplyGainMap(const avifRGBImage * baseImage,
{
avifDiagnosticsClearError(diag);

AVIF_CHECKRES(avifGainMapValidateMetadata(gainMap, diag));

if (hdrHeadroom < 0.0f) {
avifDiagnosticsPrintf(diag, "hdrHeadroom should be >= 0, got %f", hdrHeadroom);
return AVIF_RESULT_INVALID_ARGUMENT;
Expand All @@ -86,7 +89,7 @@ avifResult avifRGBImageApplyGainMap(const avifRGBImage * baseImage,
}

for (int i = 0; i < 3; ++i) {
if (gainMap->gainMapGamma[i].n <= 0) {
if (gainMap->gainMapGamma[i].n == 0) {
avifDiagnosticsPrintf(diag, "Invalid gain map metadata, gamma should be strictly positive");
return AVIF_RESULT_INVALID_ARGUMENT;
}
Expand Down Expand Up @@ -217,21 +220,22 @@ avifResult avifRGBImageApplyGainMap(const avifRGBImage * baseImage,

float rgbMaxLinear = 0; // Max tone mapped pixel value across R, G and B channels.
float rgbSumLinear = 0; // Sum of max(r, g, b) for mapped pixels.
const float gammaInv[3] = { toFloat(gainMap->gainMapGamma[0].d, gainMap->gainMapGamma[0].n),
toFloat(gainMap->gainMapGamma[1].d, gainMap->gainMapGamma[1].n),
toFloat(gainMap->gainMapGamma[2].d, gainMap->gainMapGamma[2].n) };
const float gainMapMin[3] = { toFloat(gainMap->gainMapMin[0].n, gainMap->gainMapMin[0].d),
toFloat(gainMap->gainMapMin[1].n, gainMap->gainMapMin[1].d),
toFloat(gainMap->gainMapMin[2].n, gainMap->gainMapMin[2].d) };
const float gainMapMax[3] = { toFloat(gainMap->gainMapMax[0].n, gainMap->gainMapMax[0].d),
toFloat(gainMap->gainMapMax[1].n, gainMap->gainMapMax[1].d),
toFloat(gainMap->gainMapMax[2].n, gainMap->gainMapMax[2].d) };
const float baseOffset[3] = { toFloat(gainMap->baseOffset[0].n, gainMap->baseOffset[0].d),
toFloat(gainMap->baseOffset[1].n, gainMap->baseOffset[1].d),
toFloat(gainMap->baseOffset[2].n, gainMap->baseOffset[2].d) };
const float alternateOffset[3] = { toFloat(gainMap->alternateOffset[0].n, gainMap->alternateOffset[0].d),
toFloat(gainMap->alternateOffset[1].n, gainMap->alternateOffset[1].d),
toFloat(gainMap->alternateOffset[2].n, gainMap->alternateOffset[2].d) };
// The gain map metadata contains the encoding gamma, and 1/gamma should be used for decoding.
const float gammaInv[3] = { 1.0 / avifUnsignedFractionToFloat(gainMap->gainMapGamma[0]),
1.0 / avifUnsignedFractionToFloat(gainMap->gainMapGamma[1]),
1.0 / avifUnsignedFractionToFloat(gainMap->gainMapGamma[2]) };
const float gainMapMin[3] = { avifSignedFractionToFloat(gainMap->gainMapMin[0]),
avifSignedFractionToFloat(gainMap->gainMapMin[1]),
avifSignedFractionToFloat(gainMap->gainMapMin[2]) };
const float gainMapMax[3] = { avifSignedFractionToFloat(gainMap->gainMapMax[0]),
avifSignedFractionToFloat(gainMap->gainMapMax[1]),
avifSignedFractionToFloat(gainMap->gainMapMax[2]) };
const float baseOffset[3] = { avifSignedFractionToFloat(gainMap->baseOffset[0]),
avifSignedFractionToFloat(gainMap->baseOffset[1]),
avifSignedFractionToFloat(gainMap->baseOffset[2]) };
const float alternateOffset[3] = { avifSignedFractionToFloat(gainMap->alternateOffset[0]),
avifSignedFractionToFloat(gainMap->alternateOffset[1]),
avifSignedFractionToFloat(gainMap->alternateOffset[2]) };
for (uint32_t j = 0; j < height; ++j) {
for (uint32_t i = 0; i < width; ++i) {
float basePixelRGBA[4];
Expand Down Expand Up @@ -562,12 +566,12 @@ avifResult avifRGBImageComputeGainMap(const avifRGBImage * baseRgbImage,
}
}

float baseOffset[3] = { toFloat(gainMap->baseOffset[0].n, gainMap->baseOffset[0].d),
toFloat(gainMap->baseOffset[1].n, gainMap->baseOffset[1].d),
toFloat(gainMap->baseOffset[2].n, gainMap->baseOffset[2].d) };
float alternateOffset[3] = { toFloat(gainMap->alternateOffset[0].n, gainMap->alternateOffset[0].d),
toFloat(gainMap->alternateOffset[1].n, gainMap->alternateOffset[1].d),
toFloat(gainMap->alternateOffset[2].n, gainMap->alternateOffset[2].d) };
float baseOffset[3] = { avifSignedFractionToFloat(gainMap->baseOffset[0]),
avifSignedFractionToFloat(gainMap->baseOffset[1]),
avifSignedFractionToFloat(gainMap->baseOffset[2]) };
float alternateOffset[3] = { avifSignedFractionToFloat(gainMap->alternateOffset[0]),
avifSignedFractionToFloat(gainMap->alternateOffset[1]),
avifSignedFractionToFloat(gainMap->alternateOffset[2]) };

// If we are converting from one colorspace to another, some RGB values may be negative and an offset must be added to
// avoid clamping (although the choice of color space to do the gain map computation with
Expand Down Expand Up @@ -666,6 +670,7 @@ avifResult avifRGBImageComputeGainMap(const avifRGBImage * baseRgbImage,
const double alternateHeadroom = log2f(AVIF_MAX(altMax, kEpsilon));
if (!avifDoubleToUnsignedFraction(baseHeadroom, &gainMap->baseHdrHeadroom) ||
!avifDoubleToUnsignedFraction(alternateHeadroom, &gainMap->alternateHdrHeadroom)) {
res = AVIF_RESULT_INVALID_ARGUMENT;
goto cleanup;
}

Expand Down Expand Up @@ -698,6 +703,7 @@ avifResult avifRGBImageComputeGainMap(const avifRGBImage * baseRgbImage,
!avifDoubleToSignedFraction(gainMapMaxLog2[singleChannel ? 0 : c], &gainMap->gainMapMax[c]) ||
!avifDoubleToSignedFraction(alternateOffset[c], &gainMap->alternateOffset[c]) ||
!avifDoubleToSignedFraction(baseOffset[c], &gainMap->baseOffset[c])) {
res = AVIF_RESULT_INVALID_ARGUMENT;
goto cleanup;
}
}
Expand All @@ -708,7 +714,7 @@ avifResult avifRGBImageComputeGainMap(const avifRGBImage * baseRgbImage,
if (range <= 0.0f) {
continue;
}
const float gainMapGamma = toFloat(gainMap->gainMapGamma[c].n, gainMap->gainMapGamma[c].d);
const float gainMapGamma = avifUnsignedFractionToFloat(gainMap->gainMapGamma[c]);

for (int j = 0; j < height; ++j) {
for (int i = 0; i < width; ++i) {
Expand Down
15 changes: 5 additions & 10 deletions src/read.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1991,16 +1991,11 @@ static avifBool avifParseGainMapMetadata(avifGainMap * gainMap, avifROStream * s

// Fill the remaining values by copying those from the first channel.
for (int c = channelCount; c < 3; ++c) {
gainMap->gainMapMin[c].n = gainMap->gainMapMin[0].n;
gainMap->gainMapMin[c].d = gainMap->gainMapMin[0].d;
gainMap->gainMapMax[c].n = gainMap->gainMapMax[0].n;
gainMap->gainMapMax[c].d = gainMap->gainMapMax[0].d;
gainMap->gainMapGamma[c].n = gainMap->gainMapGamma[0].n;
gainMap->gainMapGamma[c].d = gainMap->gainMapGamma[0].d;
gainMap->baseOffset[c].n = gainMap->baseOffset[0].n;
gainMap->baseOffset[c].d = gainMap->baseOffset[0].d;
gainMap->alternateOffset[c].n = gainMap->alternateOffset[0].n;
gainMap->alternateOffset[c].d = gainMap->alternateOffset[0].d;
gainMap->gainMapMin[c] = gainMap->gainMapMin[0];
gainMap->gainMapMax[c] = gainMap->gainMapMax[0];
gainMap->gainMapGamma[c] = gainMap->gainMapGamma[0];
gainMap->baseOffset[c] = gainMap->baseOffset[0];
gainMap->alternateOffset[c] = gainMap->alternateOffset[0];
}
return AVIF_TRUE;
}
Expand Down
65 changes: 26 additions & 39 deletions tests/gtest/avif_fuzztest_helpers.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,45 +190,32 @@ ImagePtr AddGainMapToImage(
image->gainMap = avifGainMapCreate();
image->gainMap->image = gain_map.release();

image->gainMap->gainMapMin[0].n = gain_map_min_n0;
image->gainMap->gainMapMin[1].n = gain_map_min_n1;
image->gainMap->gainMapMin[2].n = gain_map_min_n2;
image->gainMap->gainMapMin[0].d = gain_map_min_d0;
image->gainMap->gainMapMin[1].d = gain_map_min_d1;
image->gainMap->gainMapMin[2].d = gain_map_min_d2;

image->gainMap->gainMapMax[0].n = gain_map_max_n0;
image->gainMap->gainMapMax[1].n = gain_map_max_n1;
image->gainMap->gainMapMax[2].n = gain_map_max_n2;
image->gainMap->gainMapMax[0].d = gain_map_max_d0;
image->gainMap->gainMapMax[1].d = gain_map_max_d1;
image->gainMap->gainMapMax[2].d = gain_map_max_d2;

image->gainMap->gainMapGamma[0].n = gain_map_gamma_n0;
image->gainMap->gainMapGamma[1].n = gain_map_gamma_n1;
image->gainMap->gainMapGamma[2].n = gain_map_gamma_n2;
image->gainMap->gainMapGamma[0].d = gain_map_gamma_d0;
image->gainMap->gainMapGamma[1].d = gain_map_gamma_d1;
image->gainMap->gainMapGamma[2].d = gain_map_gamma_d2;

image->gainMap->baseOffset[0].n = base_offset_n0;
image->gainMap->baseOffset[1].n = base_offset_n1;
image->gainMap->baseOffset[2].n = base_offset_n2;
image->gainMap->baseOffset[0].d = base_offset_d0;
image->gainMap->baseOffset[1].d = base_offset_d1;
image->gainMap->baseOffset[2].d = base_offset_d2;

image->gainMap->alternateOffset[0].n = alternate_offset_n0;
image->gainMap->alternateOffset[1].n = alternate_offset_n1;
image->gainMap->alternateOffset[2].n = alternate_offset_n2;
image->gainMap->alternateOffset[0].d = alternate_offset_d0;
image->gainMap->alternateOffset[1].d = alternate_offset_d1;
image->gainMap->alternateOffset[2].d = alternate_offset_d2;

image->gainMap->baseHdrHeadroom.n = base_hdr_headroom_n;
image->gainMap->baseHdrHeadroom.d = base_hdr_headroom_d;
image->gainMap->alternateHdrHeadroom.n = alternate_hdr_headroom_n;
image->gainMap->alternateHdrHeadroom.d = alternate_hdr_headroom_d;
image->gainMap->gainMapMin[0] = {gain_map_min_n0, gain_map_min_d0};
image->gainMap->gainMapMin[1] = {gain_map_min_n1, gain_map_min_d1};
image->gainMap->gainMapMin[2] = {gain_map_min_n2, gain_map_min_d2};

image->gainMap->gainMapMax[0] = {gain_map_max_n0, gain_map_max_d0};
image->gainMap->gainMapMax[1] = {gain_map_max_n1, gain_map_max_d1};
image->gainMap->gainMapMax[2] = {gain_map_max_n2, gain_map_max_d2};

image->gainMap->gainMapGamma[0] = {gain_map_gamma_n0, gain_map_gamma_d0};
image->gainMap->gainMapGamma[1] = {gain_map_gamma_n1, gain_map_gamma_d1};
image->gainMap->gainMapGamma[2] = {gain_map_gamma_n2, gain_map_gamma_d2};

image->gainMap->baseOffset[0] = {base_offset_n0, base_offset_d0};
image->gainMap->baseOffset[1] = {base_offset_n1, base_offset_d1};
image->gainMap->baseOffset[2] = {base_offset_n2, base_offset_d2};

image->gainMap->alternateOffset[0] = {alternate_offset_n0,
alternate_offset_d0};
image->gainMap->alternateOffset[1] = {alternate_offset_n1,
alternate_offset_d1};
image->gainMap->alternateOffset[2] = {alternate_offset_n2,
alternate_offset_d2};

image->gainMap->baseHdrHeadroom = {base_hdr_headroom_n, base_hdr_headroom_d};
image->gainMap->alternateHdrHeadroom = {alternate_hdr_headroom_n,
alternate_hdr_headroom_d};
image->gainMap->useBaseColorSpace = use_base_color_space;

return image;
Expand Down
Loading

0 comments on commit 1448be6

Please sign in to comment.