[FFmpeg-devel] [PATCH] RV40 loop filter, try 2
Kostya
kostya.shishkov
Sun Nov 2 16:32:56 CET 2008
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.
[...]
> > 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).
[...]
> > +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?
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.
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
> > + /* This pattern contains bits signalling that left edge of
> > + * the current block can be filtered.
> > + * It is composed from the coded block pattern for the current MB,
> > + * superposition of the current and the left macroblock CBP shifted
> > + * right by one column and an additional mask derived from motion vectors.
> > + */
>
> > + y_v_deblock = cbp[POS_CUR]
> > + | ((cbp[POS_CUR] << 1) & ~MASK_Y_LEFT_COL) | (cbp[POS_LEFT] >> 3)
> > + | mvmasks[0];
>
> 0 supposed to be POS_CUR ?
Yes.
I'm goint to lay that code down at least for a month.
Maybe it will be clearer then. For now there are still things to do.
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
More information about the ffmpeg-devel
mailing list