[FFmpeg-devel] [PATCH] VP8 decoder

[Ronald S. Bultje]

Hi,

On Tue, Jun 15, 2010 at 4:52 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
> 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.

Haven't looked into this yet. My suggestion would be to add it to SVN,
then work on it as we go, making sure we don't break it. More eyes
would be good here in general.

>> +// 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 ?

Done.

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

Applied your patch.

>> +// 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 ?

We can still keep one and have it call the other, which I did.

>> +// DCTextra
>
> Strange comment (as many others)...

David?

Patch follows in a bit...

Ronald