[FFmpeg-devel] [PATCH v6 1/4] lavc: add FLIF decoding support

[Moritz Barsnick]

On Wed, Aug 26, 2020 at 16:31:46 +0000, Anamitra Ghorui wrote:
> >> +uint8_t ff_flif16_rac_read_bit(FLIF16RangeCoder *rc,
> >> +                               uint8_t *target)
> >> +{
> >> +    return ff_flif16_rac_get(rc, rc->range >> 1, target);
> >> +}
> >
> > If this is called often, you may want to mark it inline.
> >
>
> On Nicolas George's suggestion, I had dropped the inline keyword because
> the number of inline functions used may put stress on the memory cache.
> I think I should only inine the "higher" functions and not the "lower"
> functions like ff_flif16_rac_get in this case. Should I?

Ah, okay, I did read Nicolas's reviews, but I didn't recall this.
Please do go with his suggestions.

> I agree that the case statements between if statements and while
> statements are not very good looking, but this allows us to extract the
> function logic without having to set the, or look at the state
> management variables and statements. Trying to make do without the
> interleaving statements would cause us to set s->segment many times,
> set multiple switch statements etc.

I understand this justification. I just wanted to point out that the
code was not parsable to my brain, due to the macros (which I do
relaize have "goto"s in them, which is okay to me) and the "case"
labels in the middle of code blocks.

I suggest others comment on this as well.

> I get your point about using do {} while (0) to break the sequence, it

That was just a general remark regarding best practice. If it doesn't
fit into your above concept, then my comment doesn't apply.

Regards,
Moritz