[FFmpeg-devel] [PATCH] RV40 Loop Filter, hopefully final version
Kostya
kostya.shishkov
Sun Nov 23 08:15:03 CET 2008
On Sun, Nov 23, 2008 at 03:01:15AM +0100, Michael Niedermayer wrote:
> On Sat, Nov 22, 2008 at 04:11:15PM +0200, Kostya wrote:
> > This has been tested and verified to be bitexact if correct
> > MC functions are used.
[...]
[Loop filter code]
>
> The variable names of s0 s1 s2 s3 are poor, the give no hint on what
> they are
renamed
[...]
> > /**
> > + * RV40 loop filtering function
> > + */
> > +static void rv40_loop_filter(RV34DecContext *r)
> > +{
> > + MpegEncContext *s = &r->s;
> > + int mb_pos;
> > + int i, j, k;
> > + uint8_t *Y, *C;
> > + int alpha, beta, betaY, betaC;
> > + int q;
> > + int mbtype[4]; ///< current macroblock and its neighbours types
>
>
> > + /**
> > + * macroblock filtering strength
> > + * 2 for intra coded MB and MB with DCs coded separately, 1 otherwise
> > + */
> > + int strength[4];
>
> so its just a flag? if so is_intra seems a better name, otherwise the comment
> is not correct ...
remade into flag
also it's not intra - it is set also for P-frame block with separately coded DCs
> > + int clip[4]; ///< MB filter clipping value calculated from filtering strength
> > + /**
> > + * coded block patterns for luma part of current macroblock and its neighbours
> > + * Format:
> > + * LSB corresponds to the top left block,
> > + * each nibble represents one row of subblocks.
> > + */
> > + int cbp[4];
> > + /**
> > + * coded block patterns for chroma part of current macroblock and its neighbours
> > + * Format is the same as for luma with two subblocks in a row.
> > + */
> > + int uvcbp[4][2];
> > + /**
> > + * This mask represents the pattern of luma subblocks that should be filtered
> > + * in addition to the coded ones because because they lie at the edge of
> > + * 8x8 block with different enough motion vectors
> > + */
> > + int mvmasks[4];
> > +
> > + for(s->mb_y = 0; s->mb_y < s->mb_height; s->mb_y++){
> > + mb_pos = s->mb_y * s->mb_stride;
> > + for(s->mb_x = 0; s->mb_x < s->mb_width; s->mb_x++, mb_pos++){
>
> > + int btype = s->current_picture_ptr->mb_type[mb_pos];
>
> i dont think btype is a good name for mb_type
renamed
> [...]
>
> > + y_h_deblock = cbp[POS_CUR]
> > + | ((cbp[POS_BOTTOM] & MASK_Y_TOP_ROW) << 16)
> [...]
> [...]
> > + | mvmasks[POS_CUR]
> > + | ((mvmasks[POS_BOTTOM] & MASK_Y_TOP_ROW) << 16);
> [...]
> > + cbp[POS_CUR] = cbp[POS_CUR]
> > + | (cbp[POS_BOTTOM] << 16)
> > + | mvmasks[POS_CUR]
> > + | ((mvmasks[POS_BOTTOM] & MASK_Y_TOP_ROW) << 16);
>
> something looks redundant here
> also i have my doubts about the need of the &
dropped redundant ands but the formula is left the same because of interlocking:
both patterns also use original cbp[POS_CUR] value
The only way to change that is to introduce few temp vars that will be used once.
> > + /* Calculating chroma patterns is similar and easier since there is
> > + * no motion vector pattern for them.
> > + */
> > + for(i = 0; i < 2; i++){
>
> > + c_v_deblock[i] = ((uvcbp[POS_CUR][i] << 1) & ~MASK_C_RIGHT_COL)
> > + | ((uvcbp[POS_LEFT][i] & MASK_C_RIGHT_COL) >> 1)
>
>
> > + | ((uvcbp[POS_BOTTOM][i] & MASK_C_TOP_ROW) << 4)
> > + | uvcbp[POS_CUR][i];
> > + c_h_deblock[i] = ((uvcbp[POS_BOTTOM][i] & MASK_C_TOP_ROW) << 4)
> > + | uvcbp[POS_CUR][i]
> [...]
> > + uvcbp[POS_CUR][i] = ((uvcbp[POS_BOTTOM][i] & MASK_C_TOP_ROW) << 4) | uvcbp[POS_CUR][i];
>
> more redudnandies
same here
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
More information about the ffmpeg-devel
mailing list