[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