On Sat, Jun 08, 2019 at 08:49:15AM +0200, Reimar Döffinger wrote:
>
>
> On 08.06.2019, at 03:08, Peter Ross <pr...@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)
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
