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

[Michael Niedermayer]

On Sat, Nov 15, 2008 at 03:44:26PM +0200, Kostya wrote:
> On Sat, Nov 15, 2008 at 01:24:32PM +0100, Michael Niedermayer wrote:
> > 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:
[...]
> > >  
> > > > > > [...]
> > > > > > > +            /* 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.
> 
> I'll make that "any edge, vertical or horizontal" then.

That still is not clear, i mean if i have 2 squares i can speak of the
edge that is between them, which is either horizontal or vertical but not
both at the same time. 
If now you speak of the horizontal _and_ vertical edge then really the
problem is which other edge is meant

 -------------     -------------
|             | E |             |
|             | D |             |
|             | G |             |
|             | E |             |
|             | 1 |             |
 -------------     -------------

where in this is the second edge RV40 is using ?




> With this codec
> logic starts to abandon me.
> 
> > The problem iam having with this is that the current code
> > 1. Makes no sense
> 
> so it suits the codec well :S

i know myself how fun it is to RE such codecs ... after all ive given up
these "j-frames"


> 
> > 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 ...
>  
> We have I-frames where my decoder is bitexact (before loop filter for sure
> and after loop filter too), P-frames (there are bitexactness problems with them)
> and B-frames (no one cares about them much if picture is more or less correct).

Its great to hear I frames are bitexact, but I frames dont have motion
vectors so the kind of combined mask we talk about isnt used.


>  
> > > 
> > > > 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?
> 
> Laziness, I suppose.
> Maybe I'll send a patch for that next week if Ivan won't finish his.

Note, it can be slow as hell, i just want it so we know if things are or
are not bitexact.
I would feel alot more confident in reviewing this insane design once i know
it is bitexact and thus correct.

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

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- 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/da8a5f71/attachment.pgp>