On Sat, Jun 08, 2019 at 08:49:15AM +0200, Reimar Döffinger wrote:
> > > On 08.06.2019, at 03:08, Peter Ross <pross@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.
there are some, but not that many, instances of this throughout ffmpeg
% git grep 'while.*[^!<>=]=[^=].*=='
> Also why not just check the v < limit inside the loop after the v+= and immediatedly return?> This would allow choosing the error return value, > printing a warning etc if desired, and wouldn't unintentionally (?) run the body(7) code.
i agree with this restructure.
those errors do not ordinarily happen with vp4. so it makes sense to print message in vp4_get_mb_count,
and for vp4_unpack_macroblocks to return -1 for each error case.
> This looks a bit weird. Is dc /count somehow clearer than dc/2 here?> Can dc actually be negative so that dc / 2 is different from dc >> 1?> If not the compiler probably will generate needless extra code here.
my bad, yes it is always / 2.
division is essential, because dc is signed.
> 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.
cheers,
-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)

On 09.06.2019, at 03:07, Peter Ross <pross@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@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.