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

Write the MA1B brand correctly #2515

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wantehchang
Copy link
Collaborator

A partial fix for #2514.

@wantehchang wantehchang force-pushed the write-MA1B-brand-correctly branch from 984b8ae to 3bddabc Compare December 2, 2024 23:56
@wantehchang wantehchang requested a review from y-guyon December 2, 2024 23:57
Copy link
Collaborator Author

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

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

Yannis: To help code review, I split the pull request into two commits. Commit 1 is the actual change. Commit 2 is clang-format only.

// or lower.
if (item->av1C.seqProfile != 0 || item->av1C.seqLevelIdx0 > 13) {
isMA1B = AVIF_FALSE;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yannis: I am not that familiar with this code. It is important that this for loop (which starts at line 3138) is executed for both image items and image sequences, and that we enter the if (item->encodeOutput->samples.count > 0) block (which starts at line 3140) at least once. Otherwise we will incorrectly report that isMA1B is true (the initial value). Please check this. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is my understanding:

  • A track exists in avifEncoder only if the corresponding item is present too, likely with the first frame of the sequence being used as the image item data.
  • There is no change of profile/level within a single sequence.

Assuming this is correct, the for loop and the MA1B check look good.

} //
if ((imageMetadata->depth == 8) || (imageMetadata->depth == 10)) { //
if (imageMetadata->yuvFormat == AVIF_PIXEL_FORMAT_YUV444) { //
AVIF_CHECKRES(avifRWStreamWriteChars(&s, "MA1A", 4)); // ... compatible_brands[]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the constraints listed in https://aomediacodec.github.io/av1-avif/v1.1.0.html#advanced-profile always true here?
Note that they differ for items and sequences.

// or lower.
if (item->av1C.seqProfile != 0 || item->av1C.seqLevelIdx0 > 13) {
isMA1B = AVIF_FALSE;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is my understanding:

  • A track exists in avifEncoder only if the corresponding item is present too, likely with the first frame of the sequence being used as the image item data.
  • There is no change of profile/level within a single sequence.

Assuming this is correct, the for loop and the MA1B check look good.

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