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.
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)
signature.asc
Description: PGP signature
_______________________________________________ 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".
