[FFmpeg-devel] [PATCH] VP8 decoder

[Aurelien Jacobs]

On Tue, Jun 15, 2010 at 09:12:30AM -0400, David Conrad wrote:
> Hi,
> 
> Since I might not have much time to work on this soon, I figure a round of review might be good.
> 
> It's bitexact for all streams I have that do not use the bilinear filter (so not test vectors 3,4,7)
> 
> Speed is currently ~10% faster than libvpx for intra-only, and 40-50% faster for normal video, probably mostly due to faster C loop filter functions.

Wow !! Nice job !!

Overall, I am under the impression that a few more stuff could be shared
with vp56, but I haven't looked deeply, and it might not be possible
without speedloos.
I have also noted quite a few tiny comments which are not really useful
or don't mean much except to the one who wrote them. A comment cleanup
would be useful.

> diff --git a/libavcodec/vp56.h b/libavcodec/vp56.h
> index edbdcdd..40216b2 100644
> --- a/libavcodec/vp56.h
> +++ b/libavcodec/vp56.h
> @@ -237,6 +237,10 @@ static inline int vp56_rac_get(VP56RangeCoder *c)
>      return bit;
>  }
>  
> +// rounding is different than vp56_rac_get
> +// TODO: special version (probably only useful in coeff decode)
> +#define vp8_rac_get(c) vp56_rac_get_prob(c, 128)

Maybe make this a static inline ?

> @@ -248,12 +252,40 @@ static inline int vp56_rac_gets(VP56RangeCoder *c, int bits)
>      return value;
>  }
>  
> +static inline int vp8_rac_get_uint(VP56RangeCoder *c, int bits)
> +{
> +    int value = 0;
> +
> +    while (bits--) {
> +        value = (value << 1) | vp8_rac_get(c);
> +    }
> +
> +    return value;
> +}

OK.

> +static inline int vp8_rac_get_sint(VP56RangeCoder *c, int bits)
> +{
> +    int v = vp8_rac_get_uint(c, bits);
> +
> +    if (vp8_rac_get(c))
> +        v = -v;
> +
> +    return v;
> +}

This function is always called with the folling patern:
    vp8_rac_get(c) ? vp8_rac_get_sint(c, n_bits) : 0;
This could be factorized (see attached patch, against your git tree).

> +static inline int vp8_rac_get_nn(VP56RangeCoder *c)
> +{
> +    int v = vp8_rac_get_uint(c, 7) << 1;
> +    return v + !v;
> +}

OK.

> +// how probabilities are associated with decisions is different than vp56
> +// well, the new scheme fits in the old but this way has one fewer branches per decision
> +static inline int vp8_rac_get_tree(VP56RangeCoder *c, const int8_t (*tree)[2],
> +                                   const uint8_t *probs)
> +{
> +    int i = 0;
> +
> +    do {
> +        i = tree[i][vp56_rac_get_prob(c, probs[i])];
> +    } while (i > 0);
> +
> +    return -i;
> +}
> +
> +/**
> + * This is identical to vp8_rac_get_tree except for the possibility of starting
> + * on a node other than the root node, needed for coeff decode where this is
> + * used to save a bit after a 0 token (by disallowing EOB to immediately follow.)
> + */
> +static inline int vp8_rac_get_tree_with_offset(VP56RangeCoder *c, const int8_t (*tree)[2],
> +                                               const uint8_t *probs, int i)
> +{
> +    do {
> +        i = tree[i][vp56_rac_get_prob(c, probs[i])];
> +    } while (i > 0);
> +
> +    return -i;
> +}

Does it make any sens to keep both of those functions ?
Can't you just keep the second one and use it with last parameter set to
0 instead of using the first one ?

> +// DCTextra

Strange comment (as many others)...

> +static inline int vp8_rac_get_coeff(VP56RangeCoder *c, const uint8_t *prob)
> +{
> +    int v = 0;
> +
> +    do {
> +        v = (v<<1) + vp56_rac_get_prob(c, *prob++);
> +    } while (*prob);
> +
> +    return v;
> +}

OK.

I have not reviewed the rest of the patch in detail...

Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vp8_rac_get_sint.diff
Type: text/x-diff
Size: 2483 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100615/cce0a7b5/attachment.diff>