[FFmpeg-devel] [PATCH] RV40 loop filter, try 2

Michael Niedermayer michaelni
Sun Nov 2 23:25:19 CET 2008


On Sun, Nov 02, 2008 at 05:32:56PM +0200, Kostya wrote:
> On Sun, Nov 02, 2008 at 01:54:56PM +0100, Michael Niedermayer wrote:
> > On Fri, Oct 31, 2008 at 10:00:47AM +0200, Kostya wrote:
> > > On Fri, Oct 31, 2008 at 01:18:12AM +0100, Michael Niedermayer wrote:
> > > > On Thu, Oct 30, 2008 at 04:51:19PM +0200, Kostya wrote:
> > > > > $subj
> > > > > 
> > > > > I've tried to generalize, clean and document it.
> > > > 
> > > > its better but this still needs a lot more work to be readable.
> > > > the bit masks still are practically unreadable.
> > > > Its not so much a question of what is added into them but what the
> > > > variables actually are.
> > > > what i mean is things like
> > > > 
> > > > cbp // these are 16bits representing the coded block flags of the 4x4 luma
> > > >        blocks like:
> > > >     0  1  2  3
> > > >     4  5  6  7
> > > >     8  9 10 11
> > > >    12 13 14 15 where 0 is lsb and 15 is msb
> > > >    (msb)3x3, 2x3, 1x3, 0x3, 3x2, ...
> > > > 
> > > > this is especially absolutely essential for all the trickier loop filter
> > > > masks. mvmask, these h/v masks, ...
> > > 
> > > documented
> > 
> > IMHO the documentation is still extreemly poor
> > 
> > The documentation on its own must be able to awnser _which_ block of which
> > macroblock is how related to lets say bit 0x0008.
> > 
> > With such documentation it would be easier to understan dthe code and
> > find a better way to implement it
> 
> Hmm, I thought CBPs are formed in the same way for all codecs. 

you could store things in raster scan or zigzag scan at least
0 1 4 5
2 3 6 7
8 9 C D
A B E F
is what i mean


> 
> [...]
> > > Pixels lying on some filtered edge, pixels adjacent to it and outer ones. 
> > 
> > yes thats how i interpreted the names but this is not what the variables
> > contain. Thats why i said its confusing, i guess saying it nonsese would
> > be better.
>  
> Depends on your edge definition (e.g. it's an imaginary line between two
> macroblocks or a stripe of pixels along that imaginary line).

its nonsens for either
these variables represent left and right no inner or outer IIRC


>  
> [...]
> > > +static int rv40_set_deblock_coef(RV34DecContext *r)
> > > +{
> > > +    MpegEncContext *s = &r->s;
> > > +    int mvmask = 0, i, j;
> > > +    int midx = s->mb_x * 2 + s->mb_y * 2 * s->b8_stride;
> > > +    int16_t (*motion_val)[2] = s->current_picture_ptr->motion_val[0][midx];
> > > +    if(s->pict_type == FF_I_TYPE)
> > > +        return 0;
> > > +    for(i = 0; i < 2; i++){
> > > +        if(!s->first_slice_line && check_mv_difference(motion_val + i, s->b8_stride)){
> > > +            mvmask |= 0x03 << (i*2);
> > > +        }
> > > +        if(check_mv_difference(motion_val + s->b8_stride + i, s->b8_stride)){
> > > +            mvmask |= 0x300 << (i*2);
> > > +        }
> > > +    }
> > > +    for(j = 0; j < 16; j += 8){
> > > +        if(s->mb_x && check_mv_difference(motion_val, 1)){
> > > +            mvmask |= 0x11 << j;
> > > +        }
> > > +        if(check_mv_difference(motion_val + 1, 1)){
> > > +            mvmask |= 0x44 << j;
> > > +        }
> > > +        motion_val += s->b8_stride;
> > > +    }
> > > +    return mvmask;
> > > +}
> > 
> > are you sure this code is correct? More specifically are you sure that
> > mv differences across vertical edges can cause filtering accros horizontal
> > edges to be enabled?
> > It really looks like 2 different things are stored in the same bits.
>  
> Can anyone get their specs and see?

with enough NDA and USD iam sure anyone can.

and the h264 spec contains this mv diff > 3 stuff as well ...


> The way it filter edges is still unclear to me (but you've guessed that
> long ago).
>  
> [...]
> > > +            if(s->mb_y){
> > > +                mvmasks[POS_TOP] = r->deblock_coefs[mb_pos - s->mb_stride] & MASK_Y_LAST_ROW;
> > > +                mbtype [POS_TOP] = s->current_picture_ptr->mb_type[mb_pos - s->mb_stride];
> > > +                cbp    [POS_TOP] = r->cbp_luma[mb_pos - s->mb_stride] & MASK_Y_LAST_ROW;
> > > +                uvcbp[POS_TOP][0] =  r->cbp_chroma[mb_pos - s->mb_stride]       & MASK_C_LAST_ROW;
> > > +                uvcbp[POS_TOP][1] = (r->cbp_chroma[mb_pos - s->mb_stride] >> 4) & MASK_C_LAST_ROW;
> > > +            }
> > > +            if(s->mb_x){
> > > +                mvmasks[POS_LEFT] = r->deblock_coefs[mb_pos - 1] & MASK_Y_RIGHT_COL;
> > > +                mbtype [POS_LEFT] = s->current_picture_ptr->mb_type[mb_pos - 1];
> > > +                cbp    [POS_LEFT] = r->cbp_luma[mb_pos - 1] & MASK_Y_RIGHT_COL;
> > > +                uvcbp[POS_LEFT][0] =  r->cbp_chroma[mb_pos - 1]       & MASK_C_RIGHT_COL;
> > > +                uvcbp[POS_LEFT][1] = (r->cbp_chroma[mb_pos - 1] >> 4) & MASK_C_RIGHT_COL;
> > > +            }
> > > +            if(s->mb_y < s->mb_height - 1){
> > > +                mvmasks[POS_BOTTOM] = r->deblock_coefs[mb_pos + s->mb_stride] & MASK_Y_TOP_ROW;
> > > +                mbtype [POS_BOTTOM] = s->current_picture_ptr->mb_type[mb_pos + s->mb_stride];
> > > +                cbp    [POS_BOTTOM] = r->cbp_luma[mb_pos + s->mb_stride] & MASK_Y_TOP_ROW;
> > > +                uvcbp[POS_BOTTOM][0] =  r->cbp_chroma[mb_pos + s->mb_stride]       & MASK_C_TOP_ROW;
> > > +                uvcbp[POS_BOTTOM][1] = (r->cbp_chroma[mb_pos + s->mb_stride] >> 4) & MASK_C_TOP_ROW;
> > > +            }
> > 
> > what is this code doing? it seems to just mask unused bits out
>  
> Yes. But some of them are used later in condition checking.

then move the masking where its needed, it prevents factoring the code above
into a function.


> Also I'm not sure I've thrown out all the B-frame specific cases.
>  
> > > +            for(i = 0; i < 4; i++){
> > > +                strength[i] = (IS_INTRA(mbtype[i]) || IS_SEPARATE_DC(mbtype[i])) ? 2 : 1;
> > > +                clip[i] = rv40_filter_clip_tbl[strength[i]][q];
> > > +            }
> > 
> > > +            /* This pattern contains bits signalling that horizontal edges of
> > > +             * the current block can be filtered.
> > > +             * It is composed from the coded block pattern for the current MB,
> > > +             * coded block pattern for the bottom MB, superposition of the current
> > > +             * and the top macroblock CBP shifted down by one row and an additional
> > > +             * mask derived from motion vectors.
> > > +             */
> > > +            y_h_deblock = cbp[POS_CUR]
> > > +                        | ((cbp[POS_CUR] << 4) & ~MASK_Y_TOP_ROW) | (cbp[POS_TOP] >> 12)
> > > +                        | ((cbp[POS_BOTTOM] << 20) & ~MASK_Y_TOP_ROW) | (cbp[POS_BOTTOM] << 16)
> > > +                        | mvmasks[POS_CUR] | (mvmasks[POS_CUR] << 16);
> > 
> > what is this code supposed to do?
> > this does (x<<4) & ~0xF and (x<<20) & ~0xF 
> > these operations obviously make no sense at all
> > 
> > also mvmasks[POS_CUR] | (mvmasks[POS_CUR] << 16) makes no sense in
> > combination with the other code
> 
> it's black magic to me 

great


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

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- 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/20081102/5143a193/attachment.pgp>



More information about the ffmpeg-devel mailing list