[FFmpeg-devel] [PATCH] RV40 Loop Filter

[Michael Niedermayer]

On Mon, Oct 27, 2008 at 07:09:34PM +0200, Kostya wrote:
> 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.

then it might be ok ... hopefully ... but then i dont belive in reals
inteligence enough ...


> 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".

but that can be implemented much cleaner than the way it is ...


> 
> 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?

well i dont even know in what order they do filter them ...


> 2. Why they decided to filter bottom edge of macroblock?

do they filter an edge twice?


> 3. Why not stick to h.264 drafts more closely?

they probably thought their mess results in better quality or they just
failed to implement h264 correctly ...


> 4. Why special treatment for B-frames loop filtering 

well i do not know how different it is, but there are 2 motion vectors
instead of one that leads to special cases.
This is especially nasty for H264 because one has to check more than
forward vs. forward and backward vs. backward in the mv<4 check its
also possible thet forward has to be checked against backward but that
is h264 not rv, i dunno about rv ...


> (and why loop filter them at all)?

looks better i guess (less flickering (applying different filters to every 3rd
frame is not going to look perfect), less blocking artifacts)

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

It is not what we do, but why we do it that matters.
-------------- 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/3e9b302a/attachment.pgp>