[FFmpeg-devel] [PATCHv4] VP4 video decoder

[Tomas Härdin]

tis 2019-05-21 klockan 17:44 +1000 skrev Peter Ross:
> ---
> 
> what's changed:
> * apply #if CONFIG_VP4_DECODER around large vp4 code blocks
> * improved vp4_read_mb_value thanks to reminars suggestions
> * improved configure vp3_decoder_select
> 
> [...]
>  
> +#define BLOCK_X (2 * mb_x + (k & 1))
> +#define BLOCK_Y (2 * mb_y + (k >> 1))
> +
> +#if CONFIG_VP4_DECODER
> +static int vp4_read_mb_value(GetBitContext *gb)
> +{
> +    int v = 1;
> +    int bits = show_bits(gb, 9);

This call to show_bits() looks unnecessary

> +    while ((bits = show_bits(gb, 9)) == 0x1ff) {
> +        skip_bits(gb, 9);
> +        v += 256;
> +    }

I have a feeling this loop should have a stop condition like v <
SOME_LARGE_VALUE, say INT_MAX-255 or yuv_macroblock_count, to reject
corrupt/malicious files and not cause undefined behavior

> +    if (bits < 0x100) {
> +        skip_bits(gb, 1);
> +    } else if (bits < 0x180) {
> +        skip_bits(gb, 2);
> +        v += 1;
> +    }
> +#define body(n) { \
> +    skip_bits(gb, 2 + n); \
> +    v += (1 << n) + get_bits(gb, n); }
> +#define else_if(thresh, n) else if (bits < thresh) body(n)
> +    else_if(0x1c0, 1)
> +    else_if(0x1e0, 2)
> +    else_if(0x1f0, 3)
> +    else_if(0x1f8, 4)
> +    else_if(0x1fc, 5)
> +    else_if(0x1fe, 6)
> +    else body(7)
> +#undef body
> +#undef else_if
> +    return v;
> +}

> @@ -1012,8 +1214,8 @@ static int unpack_vlcs(Vp3DecodeContext *s, GetBitContext *gb,
>              bits_to_get = coeff_get_bits[token];
>              if (bits_to_get)
>                  bits_to_get = get_bits(gb, bits_to_get);
> -            coeff = coeff_tables[token][bits_to_get];
>  
> +            coeff = coeff_tables[token][bits_to_get];

Stray hunk

> +#if CONFIG_VP4_DECODER
> +            if (s->version >= 2) {
> +                int mb_height, mb_width;
> +                int mb_width_mul, mb_width_div, mb_height_mul, mb_height_div;
> +
> +                mb_height = get_bits(&gb, 8);
> +                mb_width  = get_bits(&gb, 8);
> +                if (mb_height != s->macroblock_height ||
> +                    mb_width != s->macroblock_width)
> +                    av_log(s->avctx, AV_LOG_WARNING, "VP4 header: Warning, macroblock dimension mismatch");
> +
> +                mb_width_mul = get_bits(&gb, 5);
> +                mb_width_div = get_bits(&gb, 3);
> +                mb_height_mul = get_bits(&gb, 5);
> +                mb_height_div = get_bits(&gb, 3);
> +                if (mb_width_mul != 1 || mb_width_div != 1 || mb_height_mul != 1 || mb_height_div != 1)
> +                      av_log(s->avctx, AV_LOG_WARNING, "VP4 header: Warning, unexpected macroblock dimension multipler/divider");
> +
> +                if (get_bits(&gb, 2))
> +                    av_log(s->avctx, AV_LOG_WARNING, "VP4 header: Warning, unknown bits set");

Maybe these should be errors and/or requests for samples? It macroblock
count changes that may indicate a resolution change

/Tomas