On 09.06.2019, at 03:07, Peter Ross <[email protected]> wrote:
> On Sat, Jun 08, 2019 at 08:49:15AM +0200, Reimar Döffinger wrote:
>>
>>
>> On 08.06.2019, at 03:08, Peter Ross <[email protected]> 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.
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
