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

Encode an anim instead of the same frame X times #1916

Merged
merged 1 commit into from
May 22, 2024

Conversation

y-guyon
Copy link
Collaborator

@y-guyon y-guyon commented Jan 8, 2024

No description provided.

@y-guyon y-guyon requested a review from vrabaud January 9, 2024 10:22
avifPixelFormat pixel_format,
bool has_alpha,
const std::vector<uint8_t>& samples) {
std::vector<ImagePtr> frames;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe encapsulate the function in template in the .cc (with depth as a parameter and called here with /*depth*/=8)? Just in case the function becomes more complex? Not necessary,

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 did this at first but the fuzzing engine generates garbled reproducer snippets when there are templates. Maybe this got fixed since then.

fuzztest::Just(height), fuzztest::Just(pixel_format),
fuzztest::Just(has_alpha),
fuzztest::Arbitrary<std::vector<uint8_t>>().WithSize(GetNumSamples(
num_frames, width, height, pixel_format, has_alpha)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

That will force all frames to have alpha or none to have alpha. Maybe we should have a vector of bools?
Same for width and height but I actually do not know whether AVIF supports animations with frames of different size: maybe you can run this test locally to see if it passes with frames of different size?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These proposals can be added in another PR.

Copy link
Collaborator Author

@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.

As discussed, let's wait until internal b/avif_fuzztest_enc_dec_anim are verified as fixed before merging this PR, or create another fuzztest target if it takes too much time.

avifPixelFormat pixel_format,
bool has_alpha,
const std::vector<uint8_t>& samples) {
std::vector<ImagePtr> frames;
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 did this at first but the fuzzing engine generates garbled reproducer snippets when there are templates. Maybe this got fixed since then.

fuzztest::Just(height), fuzztest::Just(pixel_format),
fuzztest::Just(has_alpha),
fuzztest::Arbitrary<std::vector<uint8_t>>().WithSize(GetNumSamples(
num_frames, width, height, pixel_format, has_alpha)));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These proposals can be added in another PR.

@vrabaud
Copy link
Collaborator

vrabaud commented Jan 23, 2024

With libaom updated to 3.8.1, we should be good to merge this right?

@wantehchang
Copy link
Collaborator

Vincent: I didn't review this pull request. Does this pull request need a bug fix in libaom 3.8.1?

@vrabaud
Copy link
Collaborator

vrabaud commented Jan 24, 2024

I thought all internal bugs depending on this fuzzer had been fixed in 3.8.1 but there is still b/319140742. Once it is fixed, we can change this fuzzer.

@y-guyon y-guyon merged commit b3d1219 into AOMediaCodec:main May 22, 2024
13 checks passed
@y-guyon y-guyon deleted the fuzztest_anim branch May 22, 2024 08:39
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.

3 participants