[FFmpeg-devel] [PATCH] Native VP9 decoder.

[Derek Buitenhuis]

On 9/29/2013 9:38 PM, Ronald S. Bultje wrote:
>> Doesn't it rely on the vp8 decoder?
>>
> 
> No. Maybe the bilinear filter will be shared but that can be done at the
> object level and doesn't require a full decoder dependency.

Got it.

>>> +typedef struct {
>>> +    unsigned seg_id:3, intra:1, comp:1, ref[2], mode[4], uvmode, skip:1;
>>> +    enum FilterMode filter;
>>> +    VP56mv mv[4 /* b_idx */][2 /* ref */];
>>
>> My first time seening such inline comments -- bizarre ;).
>>
> 
> I like this. If you prefer enums or so I can add them, but this is pretty
> simple.

I have no objections to either.

>>> +static av_always_inline int inv_recenter_nonneg(int v, int m)
>>> +{
>>> +    return v > 2 * m ? v : v & 1 ? m - ((v + 1) >> 1) : m + (v >> 1);
>>
>> Did a code obfuscator generate this?
>>
> 
> Heh :) It's a range-check. At the end, you're trying to accomplish that for
> a given range 0-A, with current value B, you can code (in the bitstream) a
> varlen-coded difference, where the length depends on how far off you are
> from B. This difference is then specified as an absolute number to have
> equal distribution on either side of B.
> 
> Example: you're a range 0-255 and your current value is 10. To code a
> difference of -10 to 10 (output range 0-20) means the absolute difference
> can have two sign values (- and +). However, beyond absolute difference 10,
> the sign is known (it is always +) because otherwise the output would be
> outside the valid range 0-255. So for absdiff 0-10, we have a signbit (so
> coded difference values 0-20 mean 0-10 with the lowest bit being the sign).
> Beyond codeddiff 20, we know the sign, so it's only a difference value
> (where coded from 20 means absolute from 10).
> 
> This code and the branch in the caller basically do that.

OK with me if you add this, or something like this, as a comment, and/or
split up the large return line into multiple lines.

>>> +
>>> +    if (s->keyframe || s->intraonly) {
>>> +        memset(s->counts.coef, 0, sizeof(s->counts.coef) +
>> sizeof(s->counts.eob));
>>
>> Is eob sanity checked anywhere?
>>
> 
> What do you mean? It's a counter for updating next-frame probabilities to
> the distribution in this (i.e. adaptivity).

My bad, I misread.

>> Will this be looked into, or will this if 0 be removed?
>>
> 
> Clement was working on this.

OK, but before pushing, this should be replaced with Clement's work,
or be removed.

More review to come.

- Derek