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

Michael Niedermayer michaelni
Sat Nov 15 13:24:32 CET 2008


On Sat, Nov 15, 2008 at 12:49:47PM +0200, Kostya wrote:
> On Sat, Nov 15, 2008 at 11:10:26AM +0100, Michael Niedermayer wrote:
> > On Sat, Nov 15, 2008 at 11:27:41AM +0200, Kostya wrote:
> > > 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:
> [...]
> > > > > +            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.
> > 
> > p0= (26*(s[-1] + s[ 0] + s[1]) + 25*(s[-2] + s[2]) + offl)>>7;
> > q0= (26*(s[ 0] + s[ 1] + s[2]) + 25*(s[-1] + s[3]) + offr)>>7;
> > p0= clipwhatever(p0)
> > q0= clipwhatever(q0)
> > p1= (26*(s[-2] + s[-1] + p0  ) + 25*(s[-3] + s[1]) + offl)>>7;
> > q1= (26*(q0    + s[ 2] + s[3]) + 25*(s[ 0] + s[4]) + offr)>>7;
>  
> Exactly. But it's easier to me to include difference into bias then change filter. 

IMHO above is clearer than your code, besides being faster i suspect


>  
> > > > [...]
> > > > > +            /* 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.
> > 
> > The text does not speak of combining anything, and the combination must be
> > documented very exactly IMHO.
> 
> It says about edge of blocks with differing MVs, nothing is said about the direction,
> so it's any edge. 

it clearly says:
    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.

This clearly means the edge that is filtered, not some unrelated edge.
The problem iam having with this is that the current code
1. Makes no sense
2. the decoder is not bitexact
-> its hard for me to be sure the code is correct or even does what you
   think it does ...



> 
> > Also is the decoder bitexact already?
> > If not, why not?
> 
> I planned to verify another cases.
> For now I can name only one known source of inexactness: motion compensation for luma.
> H.264 uses 
> (src[0] + (1*src[-2] - 5*src[-1] + 20*src[0] + 20*src[1] - 5*src[2] + src[3] + 16)>>5 + 1) >> 1
> 
> RV40 uses
> 
> (1*src[-2] - 5*src[-1] + 52*src[0] + 20*src[1] - 5*src[2] + src[3] + 32) >> 6
> 
> For example,
> 20 20 20 |20| 22 23 23
> results in 21 for H.264 and 20 for RV40

What stops you from implementing the RV40 MC variant?

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

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates
-------------- 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/20081115/ee068d66/attachment.pgp>



More information about the ffmpeg-devel mailing list