Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure the gain map metadata is towards the start of the file. #2479

Merged
merged 3 commits into from
Oct 17, 2024
Merged

Make sure the gain map metadata is towards the start of the file. #2479

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7280dbe
Make sure the gain map metadata is towards the start of the file.
maryla-uc Oct 16, 2024
c50dfa7
Address review comments
maryla-uc Oct 17, 2024
014e44e
Fix gradient value too large
maryla-uc Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/write.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR: Shouldn't Exif/XMP be at the end?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the same as the gain map metadata: if we put it at the end of the file, avifDecoderParse() will block untill it receives the whole file, unless ignoreExif and ignoreXMP are set. In the documentation for those fields we say some of these payloads are (unfortunately) packed at the end of the file so we consider this to be a bad thing.

// * 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:
Expand All @@ -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;
Expand Down
8 changes: 5 additions & 3 deletions tests/gtest/avifgainmaptest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand Down
53 changes: 38 additions & 15 deletions tests/gtest/avifincrtest_helpers.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this still holds with monochrome+alpha+gainmap, but this function is hard to tune anyway. Feel free to change or remove conditions if they are cumbersome.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I changed it a bit randomly, but after a while I realized the problem was not this formula but actually this part:

  // There is no valid AV1 payload smaller than 10 bytes, so all but one cell
  // should be decoded if at most 10 bytes are missing.
  if ((available_byte_count + 10) >= byte_count) {

Because my image had tiles that were all identical, and so all the tiles pointed to the same blob in mdat and were all decoded at once. So I fixed it by changing the test image.
I left this just in case, although looking at it again my change was wrong (and I changed it again), but I'm not sure this code is really exercised right now.

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<uint32_t>(
(height / cell_height) * available_byte_count / byte_count);
Expand Down Expand Up @@ -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");
}
maryla-uc marked this conversation as resolved.
Show resolved Hide resolved
AVIF_CHECKRES(parse_result);

// Decoding is incremental.
Expand All @@ -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;
maryla-uc marked this conversation as resolved.
Show resolved Hide resolved
}
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);
maryla-uc marked this conversation as resolved.
Show resolved Hide resolved
}
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;
Expand Down
8 changes: 4 additions & 4 deletions tests/gtest/aviftest_helpers.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -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)) {
Expand Down
4 changes: 3 additions & 1 deletion tests/gtest/aviftest_helpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Loading