Closed wantehchang closed 2 months ago
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.
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.
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.
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.