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

Properly check if an identical chunk has been written before. #1956

Merged
merged 3 commits into from
Jan 22, 2024

Conversation

vrabaud
Copy link
Collaborator

@vrabaud vrabaud commented Jan 19, 2024

Fixes internal bugs: b/321189607 and b/321189607

@vrabaud vrabaud requested a review from y-guyon January 19, 2024 12:51
Copy link
Collaborator

@y-guyon y-guyon left a comment

Choose a reason for hiding this comment

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

This piece of code was written 4 years ago but maybe someone more familiar with tracks than me could review.

src/write.c Outdated
chunkOffset = avifEncoderFindExistingChunk(s, mdatStartOffset, sample->data.data, sample->data.size);
size_t total_size = sample->data.size;
for (uint32_t sampleIndex = 1; sampleIndex < item->encodeOutput->samples.count; ++sampleIndex) {
total_size += item->encodeOutput->samples.sample[sampleIndex].data.size;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the data contiguous?

Suggestion:

AVIF_CHECKERR(
  item->encodeOutput->samples.sample[sampleIndex].data.data ==
    item->encodeOutput->samples.sample[0].data.data + total_size,
  AVIF_RESULT_UNKNOWN_ERROR);

(not just an assertion, for extra security)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, the data is written contiguously a few lines below but I'll add a check.
The problem happened starting with #463

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, we do not care right? The stream is already formed so as long as we have a match of size total_size, we are good and the deduplication works.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I missed it's on the input, that makes sense. Done

@wantehchang wantehchang requested a review from vigneshvg January 19, 2024 23:43
Fixes internal bugs: b/321189607 and b/321189607
@vrabaud vrabaud merged commit b01a8af into AOMediaCodec:main Jan 22, 2024
20 checks passed
@vrabaud vrabaud deleted the version_number branch January 22, 2024 22:03
vrabaud added a commit to vrabaud/libavif that referenced this pull request Jan 22, 2024
vrabaud added a commit that referenced this pull request Jan 23, 2024
@wantehchang wantehchang mentioned this pull request Feb 1, 2024
@wantehchang
Copy link
Collaborator

@vrabaud Vincent: The first comment in this pull request says "Fixes internal bugs: b/321189607 and b/321189607", but the two bug numbers are the same. Is there a second bug that this pull request fixes?

@vrabaud
Copy link
Collaborator Author

vrabaud commented Feb 2, 2024

Nope, just a mistake on my end ...

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.

4 participants