[FFmpeg-devel] [PATCH] RV40 Loop Filter
Kostya
kostya.shishkov
Mon Oct 27 13:26:45 CET 2008
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
More information about the ffmpeg-devel
mailing list