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

[Peter Ross]

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.

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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190609/64bc31fe/attachment.sig>