[FFmpeg-devel] [PATCH] RV40 Loop Filter

[Kostya]

On Mon, Oct 27, 2008 at 09:07:31AM +0100, Michael Niedermayer wrote:
> On Sun, Oct 26, 2008 at 03:41:09PM +0200, Kostya wrote:
> > On Sat, Oct 25, 2008 at 11:14:25AM +0200, Michael Niedermayer wrote:
> > > On Sat, Oct 25, 2008 at 10:08:44AM +0300, Kostya wrote:
> > > > On Wed, Oct 22, 2008 at 10:53:23AM +0200, Michael Niedermayer wrote:
> > > > > On Tue, Oct 21, 2008 at 09:23:21AM +0300, Kostya wrote:
> > > [...]
> > > > > [...]
> > > > > > +static int rv40_set_deblock_coef(RV34DecContext *r)
> > > > > > +{
> > > > > > +    MpegEncContext *s = &r->s;
> > > > > > +    int mvmask = 0, i, j, dx, dy;
> > > > > > +    int midx = s->mb_x * 2 + s->mb_y * 2 * s->b8_stride;
> > > > > 
> > > > > > +    if(s->pict_type == FF_I_TYPE)
> > > > > > +        return 0;
> > > > > 
> > > > > why is this even called for i frames?
> > > > 
> > > > I intend to use it for calculating macroblock-specific deblock
> > > > strength in RV30.
> > > 
> > > fine but how is that related to having the pict_type check inside the
> > > function compared to outside?
> >  
> > For RV30 setting deblock coefficients would be performed for
> > I-frames as well.
> 
> so there are 2 different functions
> 
> if(rv30)
>     rv30_set_deblock_coef()
> else if(!I)
>     rv40_set_deblock_coef()
> 
> clean, simple, fast, ...
> vs.
> 
> ctx->func_ptr()
> 
> init(){
>     if(rv30)
>         ctx->func_ptr= func30
>     else
>         ctx->func_ptr= func40
> }
> 
> func40(){
>     if(I)
>         return;
> }
> 
> This is not simple, and calling functions that just return is IMHO also
> not clean.
 
ok
 
[...] 
> > +
> > +/**
> > + * This macro is used for calculating 25*x0+26*x1+26*x2+26*x3+25*x4
> > + * or 25*x0+26*x1+51*x2+26*x3
> 
> > + * @param  sub - index of the value with coefficient = 25
> 
> idx25 maybe
> 
> 
> > + * @param last - index of the value with coefficient 25 or 51
> 
> idx25_51
> 
> but still the doxy is not sufficient to understand what the function
> does and how overlapping of the 2 variables behave and are used.

I just treated this as a sum of four consequent coeffs multiplied
by 26 with some additional changes - the first one (or the last one)
should be (26-1)*coefficient, so I subtract one (here's the "sub" name)
and depending on the last coefficient position we have either 25 or
26+25 multiplier.

[...]
> 
> > +
> > +/** This structure holds conditions on applying loop filter to some edge */
> > +typedef struct RV40LoopFilterCond{
> > +    int x;              ///< x coordinate of edge start
> > +    int y;              ///< y coordinate of edge start
> 
> > +    int dir;            ///< edge filtering direction (horizontal or vertical)
> 
> and what value does dir have for each?
 
0 - horizontal, 1 - vertical
 
[filter horror]
 
> i will not accept this mess, sorry.
> If you dont (or cant) clean this up i will try eventually but that might not
> be soon.
> as is, this is too much of a mess and iam unwilling to belive that h264
> drafts required such mess.

Me neither. Another example is that they use 1/4th pel interpolation filter
(1*X[-2] - 5*X[-1] + 52*X[0] + 20*X[1] - 5*X[2] + 1*X[3] + 32) >> 6
instead of calculating halfpel value first and then averaging it,
which leads to some rounding errors.

Since I've fixed some quantization-related bugs, it looks more decent
even without loop filter, so I'm tempted to try less sophisticated
loop filter (for low-power CPUs) from the same codecs, it looks simple
and sane.
 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB