[FFmpeg-devel] [PATCH] RV40 Loop Filter (again)

[Kostya]

On Fri, Nov 14, 2008 at 06:23:30PM +0100, Michael Niedermayer wrote:
> On Fri, Nov 14, 2008 at 09:14:46AM +0200, Kostya wrote:
> > On Wed, Nov 12, 2008 at 07:46:32PM +0100, Michael Niedermayer wrote:
> > > On Wed, Nov 12, 2008 at 09:05:11AM +0200, Kostya wrote:
[...]
> >  /**
> > + * weaker deblocking very similar to the one described in 4.4.2 of JVT-A003r1
> > + */
> > +static inline void rv40_weak_loop_filter(uint8_t *src, const int step,
> > +                                         const int filter_p1, const int filter_q1,
> > +                                         const int alpha, const int beta,
> > +                                         const int lim0, const int lim1, const int lim2,
> > +                                         const int diff_p1p0, const int diff_q1q0,
> > +                                         const int diff_p1p2, const int diff_q1q2)
> > +{
> > +    uint8_t *cm = ff_cropTbl + MAX_NEG_CROP;
> > +    int t, u, diff;
> > +
> > +    t = src[0*step] - src[-1*step];
> > +    if(!t)
> > +        return;
> > +    u = (alpha * FFABS(t)) >> 7;
> > +    if(u > 3 - (filter_p1 && filter_q1))
> > +        return;
> > +
> > +    t <<= 2;
> > +    if(filter_p1 && filter_q1)
> > +        t += src[-2*step] - src[1*step];
> > +    diff = CLIP_SYMM((t + 4) >> 3, lim0);
> > +    src[-1*step] = cm[src[-1*step] + diff];
> > +    src[ 0*step] = cm[src[ 0*step] - diff];
> > +    if(FFABS(diff_p1p2) <= beta && filter_p1){
> > +        t = (diff_p1p0 + diff_p1p2 - diff) >> 1;
> > +        src[-2*step] = cm[src[-2*step] - CLIP_SYMM(t, lim2)];
> > +    }
> > +    if(FFABS(diff_q1q2) <= beta && filter_q1){
> > +        t = (diff_q1q0 + diff_q1q2 + diff) >> 1;
> > +        src[ 1*step] = cm[src[ 1*step] - CLIP_SYMM(t, lim1)];
> > +    }
> > +}
> > +
> 
> lim0,1,2 are pretty bad names
 
what about lim_p0q0, lim_p1 and lim_q1 ?
 
> [...]
> > +            p0 = RV40_STRONG_FILTER(src, step, -1, rv40_dither_l[dmode + i]);
> > +            p1 = RV40_STRONG_FILTER(src, step,  0, rv40_dither_r[dmode + i]);
> > +            diff[0] = src[-1*step];
> > +            diff[1] = src[ 0*step];
> > +            src[-1*step] = sflag ? av_clip(p0, src[-1*step] - lims, src[-1*step] + lims) : p0;
> > +            src[ 0*step] = sflag ? av_clip(p1, src[ 0*step] - lims, src[ 0*step] + lims) : p1;
> > +            diff[0] -= src[-1*step];
> > +            diff[1] -= src[ 0*step];
> > +            p0 = RV40_STRONG_FILTER(src, step, -2, rv40_dither_l[dmode + i] + diff[1]*25);
> > +            p1 = RV40_STRONG_FILTER(src, step,  1, rv40_dither_r[dmode + i] + diff[0]*25);
> 
> I have my doubts about the order of operations being correct
> are you sure its not
> a = RV40_STRONG_FILTER(src, step, -2, rv40_dither_l[dmode + i]);
> b = RV40_STRONG_FILTER(src, step, -1, rv40_dither_l[dmode + i]);
> c = RV40_STRONG_FILTER(src, step,  0, rv40_dither_r[dmode + i]);
> d = RV40_STRONG_FILTER(src, step,  1, rv40_dither_r[dmode + i]);
> ...
> ?
> 
> also the diff[] just compensates for the value being overwritten due to the
> order of operations.

I've verified that against binary decoder.
As you can see, all filters are interlocked since they all use src[-2], src[-1], src[0]
and src[1] values but only one of them is used with the old value (the farthest one in
filtering operation).
I don't see any easy way to untangle them.

> [...]
> > +            /* This pattern contains bits signalling that horizontal edges of
> > +             * the current block can be filtered.
> > +             * That happens when either of adjacent subblocks is coded or lies on
> > +             * the edge of 8x8 blocks with motion vectors differing by more than
> > +             * 3/4 pel in any component.
> > +             */
> > +            y_h_deblock =   cbp[POS_CUR]
> > +                        | ((cbp[POS_BOTTOM]     & MASK_Y_TOP_ROW)  << 16)
> > +                        | ((cbp[POS_CUR]                           <<  4) & ~MASK_Y_TOP_ROW)
> > +                        | ((cbp[POS_TOP]        & MASK_Y_LAST_ROW) >> 12)
> > +                        |   mvmasks[POS_CUR]
> > +                        | ((mvmasks[POS_BOTTOM] & MASK_Y_TOP_ROW)  << 16);
> > +            /* This pattern contains bits signalling that vertical edges of
> > +             * the current block can be filtered.
> > +             * That happens when either of adjacent subblocks is coded or lies on
> > +             * the edge of 8x8 blocks with motion vectors differing by more than
> > +             * 3/4 pel in any component.
> > +             */
> > +            y_v_deblock =   cbp[POS_CUR]
> > +                        | ((cbp[POS_CUR]                      << 1) & ~MASK_Y_LEFT_COL)
> > +                        | ((cbp[POS_LEFT] & MASK_Y_RIGHT_COL) >> 3)
> > +                        |   mvmasks[POS_CUR];
> 
> the text and mvmasks do not match

why? RV40 uses combined mvmask for both horizontal and vertical MV.
 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB