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.

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

Conversation

maryla-uc
Copy link
Collaborator

The 'tmap' item data containing the gain map metadata was mistakenly being put at the end of the file.

The 'tmap' item data containing the gain map metadata was mistakenly being put at the end of the file.
@maryla-uc maryla-uc requested a review from y-guyon October 17, 2024 09:45
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.

tests/gtest/avifincrtest_helpers.cc Outdated Show resolved Hide resolved
tests/gtest/avifincrtest_helpers.cc Outdated Show resolved Hide resolved
tests/gtest/avifincrtest_helpers.cc Outdated Show resolved Hide resolved
@@ -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.

@maryla-uc maryla-uc merged commit 048488e into AOMediaCodec:main Oct 17, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants