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

Read the first two bytes of mini box directly #2408

Conversation

wantehchang
Copy link
Collaborator

The first two bytes of the mini box can be read directly without checking any condition. Parse the fields in the first two bytes using elementary operations (bitwise AND and right shift). No need to use avifROStreamReadBits() yet.

The first two bytes of the mini box can be read directly without
checking any condition. Parse the fields in the first two bytes using
elementary operations (bitwise AND and right shift). No need to use
avifROStreamReadBits() yet.
@maryla-uc
Copy link
Collaborator

Hi Wan-Teh, could you help clarify what the benefit of this approach is?
I still don't understand why this code needs refactoring.
Just because it's possible to read two bytes at once doesn't mean we have to read two bytes at once if there's no clear benefit? The current code is short and readable.
If I were to refactor it I would change the avifROStreamRead* functions to return an avifStatus to remove the need to specify AVIF_RESULT_BMFF_PARSE_FAILED on every line.

@wantehchang
Copy link
Collaborator Author

Hi Maryla and Yannis,

The new code is also short and readable. (The new code is actually shorter.) The new code uses elementary operations (bitwise AND and right shift). The masks used in the bitwise AND operations are commonly-used 2^n - 1 values (1, 3, 7), and the right shift amounts are the widths of the bit(n) fields (1, 2, 3). No unusual constants are used. Programmers working on parsing file formats should find the new code easy to understand.

Having a powerful, general-purpose tool available should not prevent us from using simple, elementary operations when they suffice. In this particular case, we can use a simpler solution because of our deeper understanding of the problem. (We know a co-author of the spec deliberately packed the first two bytes to make this possible.) It is a shame to not take advantage of that.

@wantehchang wantehchang deleted the read-first-two-bytes-from-mini-box branch August 23, 2024 14:40
@maryla-uc
Copy link
Collaborator

I think it's better to stay consistent with the rest of the stream parsing in this file rather than special case these two bytes.

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