[FFmpeg-devel] [PATCH] RV40 Loop Filter, hopefully final version

[Kostya]

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