[FFmpeg-devel] [PATCH] RV40 Loop Filter

[Michael Niedermayer]

On Mon, Oct 27, 2008 at 05:12:33PM +0200, Kostya wrote:
> 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.

a different filter could cause artifacts for some videos, if that happens we
are back to square one.


>  
> > [...]
> > > +            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 

the problem is that its not even obvious what is in *deblock because the parts
its build of are unclear.
if it where uvcbp_left, uvcbp_top, ... it alraedy would be more readable


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

-> it should be called strength or filter_strength

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081027/7923e79b/attachment.pgp>