[FFmpeg-devel] [PATCH] RV30/40 decoder

[Kostya]

On Sat, Nov 24, 2007 at 12:58:54PM +0100, Michael Niedermayer wrote:
> On Sun, Nov 18, 2007 at 11:11:24AM +0200, Kostya wrote:
> > Well, it roughly the same feature-wise as it was,
> > I just don't think I will improve it soon, yet
> > it is playable (and maybe will attract samples
> > and patches, I'm an optimist).
> 
> more reviewing, also your chances of seeing this applied would improve
> if you splited it in maybe 10+ patches!
> the problem is every time i look at it i find new issues but its too big
> (400k uncompressed) to really review all at once, one inevitably becomes
> tired so the quality of the review degrades and many issues are missed
> and with the next iteration another subset of the issues is found and
> so on ...

250k of them are in rv34vlc.h and ~50k in other headers,
another 50k in common core (rv34.c). 
 
> heres the rv34.c review:
> 
> 
> > +/** Translation of RV40 macroblock types to lavc ones */
> > +static const int rv34_mb_type_to_lavc[12] = {
> > +    MB_TYPE_INTRA, MB_TYPE_INTRA16x16, MB_TYPE_16x16,   MB_TYPE_8x8,
> > +    MB_TYPE_16x16, MB_TYPE_16x16,      MB_TYPE_SKIP,    MB_TYPE_DIRECT2,
> > +    MB_TYPE_16x8,  MB_TYPE_8x16,       MB_TYPE_DIRECT2, MB_TYPE_16x16
> > +};
> 
> the resulting types look invalid and incomplete, also the comment doesnt match
> the variable name rv40 vs 34

that's the common problem there, will correct

[...] 
> > +    for(mask = 8; mask; mask >>= 1, curshift++){
> > +        if(!(pattern & mask)) continue;
> > +        t = get_vlc2(gb, vlc->cbp[table][table2].table, vlc->cbp[table][table2].bits, 1);
> > +        cbp |= rv34_cbp_code[t] << curshift[0];
> > +    }
> 
> the vlc can be changed so as to make rv34_cbp_code unneeded!

Hmm, I will try to make gen_vlc() accept symbol table as well.   

[...]
> > +    case RV34_MB_B_FORWARD:
> > +        rv34_pred_b_vector(A[0], B[0], C[0], no_A[0], no_B[0], no_C[0], &mx[0], &my[0]);
> > +        r->dmv[1][0] = 0;
> > +        r->dmv[1][1] = 0;
> > +        break;
> > +    case RV34_MB_B_BACKWARD:
> > +        r->dmv[1][0] = r->dmv[0][0];
> > +        r->dmv[1][1] = r->dmv[0][1];
> > +        r->dmv[0][0] = 0;
> > +        r->dmv[0][1] = 0;
> 
> why are the delta mv not stored in the correct entry when read?
> also why is the other dmv not 0 changing the delta mv during prediction
> seems very hackish

DMVs are read into array starting from zero index for all blocks, but later
for B-frame blocks dmv[0][] and dmv[1][] are used to update mv[0][] and mv[1][]
respectively. I may disable or drop B-frame MV part of code as it's not correct.

> > +        rv34_pred_b_vector(A[1], B[1], C[1], no_A[1], no_B[1], no_C[1], &mx[1], &my[1]);
> > +        break;
> > +    case RV34_MB_B_DIRECT:
> > +        rv34_pred_b_vector(A[0], B[0], C[0], no_A[0], no_B[0], no_C[0], &mx[0], &my[0]);
> > +        rv34_pred_b_vector(A[1], B[1], C[1], no_A[1], no_B[1], no_C[1], &mx[1], &my[1]);
> > +        break;
> > +    default:
> > +        no_A[0] = no_A[1] = no_B[0] = no_B[1] = no_C[0] = no_C[1] = 1;
> > +    }
> 
> please read the mpeg4 and h264 specs about what direct mode is
> this terminology is not correct
> direct is NOT the same as bidirectional

ok 
 
[...]
> > +/**
> > + * Table for obtaining quantizer difference
> > + */
> > +static const int8_t rv34_dquant_tab[] = {
> > +  0,  0,  2,  1, -1,  1, -1,  1, -1,  1, -1,  1, -1,  1, -1,  1,
> > + -1,  1, -1,  1, -1,  1, -2,  2, -2,  2, -2,  2, -2,  2, -2,  2,
> > + -2,  2, -2,  2, -2,  2, -2,  2, -2,  2, -3,  3, -3,  3, -3,  3,
> > + -3,  3, -3,  3, -3,  3, -3,  3, -3,  3, -3,  2, -3,  1, -3, -5
> > +};
> 
> isnt this the same as modified_quant_tab ?

Yes, it could be used instead (if it was not static)

Patch will be sent later.
 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB