[FFmpeg-devel] [PATCH] RV40 Loop Filter

[Kostya]

On Mon, Oct 27, 2008 at 05:19:13PM +0100, Michael Niedermayer wrote:
> 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.
 
But binary decoder uses it instead of this for slower CPUs.
Well, while I work on it feel free to look at this filter, since it should
give better quality.
 
> >  
> > > [...]
> > > > +            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

I've commented on declaration that [0] = current block, [1] = top block,
[2] = left block, [3] = bottom block.
In most cases that would boil to "if this subblock is coded or neighbouring
subblock is coded or they are in different blocks and those blocks' motion
vectors significantly differ, then perform loop filtering".

There are several question I'd like to know an answer:
1. Why they have different edge filtering order for the first row of subblocks?
2. Why they decided to filter bottom edge of macroblock?
3. Why not stick to h.264 drafts more closely?
4. Why special treatment for B-frames loop filtering (and why loop filter them at all)?

Maybe it will be more clear on N+3th iteration, but for now I will put that
aside and will work on something else.
 
> > > > +                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

Indeed.
 
> [...]
> 
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB