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

[Reimar Döffinger]

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.
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.


> +        memset(s->superblock_coding + i, 2 * bit, current_run);
> +        has_partial |= bit == 0;
> +        bit ^= 1;

Maybe it would be too obfuscating, but you could flip these last 2 lines around and have
has_partial |= bit;
...

> +        if (current_run)
> +            av_log(s->avctx, AV_LOG_ERROR, "Invalid run length\n");

Maybe "Invalid partial block run length"? To distinguish the different run lengths.

> +/* note: dc_pred points to the current block */
> +static int vp4_dc_pred(const Vp3DecodeContext *s, const VP4Predictor * dc_pred, const int * last_dc, int type, int plane)
> +{
> +    int count = 0;
> +    int dc = 0;
[...]
> +    return count == 2 ? dc / count : last_dc[type];

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.


> 
> +    static const int motion_shift[2] = { 1, 2 };
> +    static const int subpel_mask[2]  = { 1, 3 };


> +    x = 8 * bx + motion_x / (1 << motion_shift[!!plane]);
> +    y = 8 * by + motion_y / (1 << motion_shift[!!plane]);
> +
> +    x_subpel = motion_x & subpel_mask[!!plane];
> +    y_subpel = motion_y & subpel_mask[!!plane];

I'd probably go for
// using division instead of shift to correctly handle negative values
int motion_div = plane ? 4 : 2;
int subpel_mask = plane ? 3 : 1;
Instead of the array lookup...

I hope these are indeed my final comments now :)
It looks really good to me as far as I am qualified to comment.