[FFmpeg-devel] [PATCH] RV40 Loop Filter

[Kostya]

On Mon, Oct 27, 2008 at 03:55:38PM +0100, Michael Niedermayer wrote:
> On Sun, Oct 26, 2008 at 03:41:09PM +0200, Kostya wrote:
[...]
> 
> have you confirmed that this loop filter is bit exact?
> Id like to make really sure that this is correct before i spend time
> on finding a clean implementation.

Yes, it is. But as I've mentioned, there is simpler and saner filter
there which I tend to use instead of this.
 
> [...]
> > +            if(s->width * s->height <= 0x6300){
> > +                betaY += beta;
> > +            }
> 
> 0x6300= 176*144
> 
> [...]
> > +                c_v_deblock[i] = ((uvcbp[0][i] << 1) & ~0x5) | (uvcbp[2][i] >> 1)
> > +                               | (uvcbp[3][i] << 4) | uvcbp[0][i];
> > +                c_h_deblock[i] = (uvcbp[3][i] << 4) | uvcbp[0][i] | (uvcbp[1][i] >> 2)
> > +                               | (uvcbp[3][i] << 6) | (uvcbp[0][i] << 2);
> > +                uvcbp[0][i] = (uvcbp[3][i] << 4) | uvcbp[0][i];
> 
> this can be simplified a lot, the term "(uvcbp[3][i] << 4) | uvcbp[0][i]"
> occurs several times in there
> besides id like to repeat that this should be using a 2d array NOT bits.
> if you insist on using bits, then they MUST be documented, that is each bit
> must be documented so its clear to which block or edge it belongs.

those are rather obvious, it's *_deblock variables that give me a headache 
 
> > +                if(!s->mb_x){
> > +                    c_v_deblock[i] &= ~0x5;
> > +                }
> > +                if(!s->mb_y){
> > +                    c_h_deblock[i] &= ~0x3;
> > +                }
> 
> > +                if(s->mb_y == s->mb_height - 1 || mbtype[0] == 2 || mbtype[3] == 2){
> 
> mbtype should never be compared against litteral numbers, always
> named values like from an enum.
 
that's a misleading name here, sorry
first I store real macroblock type here, then just something
resembling filtering strength in H264 draft (=1 for most MB types,
=2 for intra MBs and 1MV P-frame interblock with separately coded
DCs).
 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB