[FFmpeg-devel] [PATCHv5 2/2] VP4 video decoder

[Reimar Döffinger]

On 09.06.2019, at 03:07, Peter Ross <pross at xvid.org> wrote:

> On Sat, Jun 08, 2019 at 08:49:15AM +0200, Reimar Döffinger wrote:
>> 
>> 
>> On 08.06.2019, at 03:08, Peter Ross <pross at xvid.org> wrote:
>> 
>>> ---
>>> comments against v4 patch addressed. thanks.
>>> 
>>> +#if CONFIG_VP4_DECODER
>>> +static int vp4_get_mb_count(Vp3DecodeContext *s, GetBitContext *gb)
>>> +{
>>> +    int v = 1;
>>> +    int bits;
>>> +    while ((bits = show_bits(gb, 9)) == 0x1ff && v < s->yuv_macroblock_count) {
>> 
>> I know some people prefer it, so leave it in that case.
>> But I think avoiding an assignment in the while makes the code
>> sufficiently more readable that it's worth the extra line of code.
> 
> this adds three lines though...
> 
>    while(1) {
>        bits = show_bits(gb, 9);
>        if (bits == 0x1ff)
>            break;
> 
> if reduced to 'while ((bits = show_bits(gb, 9)) == 0x1ff) {' i think it is readable enough.

I was thinking of
int bits = show_bits(gb, 9);
while (bits == 0x1ff){
...
v += ...
if (v >= ...) {some error handling }
consume bits (sorry, forgot how that is done - and possibly should be done before the error handling?)
bits = show_bits(gb, 9);
}

> there are some, but not that many, instances of this throughout ffmpeg
> % git grep 'while.*[^!<>=]=[^=].*=='

Yes, I know.
I admit it's no big deal, it's just one of those things I just think is not worth doing in like 90% of
cases, but I can live with people disagreeing on that.
So I show you my idea of how I'd have written it and you can consider what looks best to you.

>> I hope these are indeed my final comments now :)
>> It looks really good to me as far as I am qualified to comment.
> 
> your comments are very much appreciated.
> it takes time to do these reviews, and the result is worth it imho.

Glad to hear that, luckily for the few patches I am interested in it is not that much time.
That is because I only look at the patches and don't even try to understand
the full context, thus I am always in favour of having also a maintainer +1.