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

[Diego Biurrun]

On Thu, Oct 30, 2008 at 04:51:19PM +0200, Kostya wrote:
> $subj
> 
> I've tried to generalize, clean and document it.

I've tried to look at the Doxygen, some of it is still rather hard to
understand.

> --- libavcodec/rv40.c	(revision 15732)
> +++ libavcodec/rv40.c	(working copy)
> @@ -247,7 +247,427 @@
>  
> +/**
> + * This macro is used for calculating 25*x0+26*x1+26*x2+26*x3+25*x4
> + * or 25*x0+26*x1+51*x2+26*x3
> + *
> + * @param idx25 index of the value with coefficient = 25 (could be at the end of
> +                coefficients or at the start)

the coefficients

> + * @param idx25_51 index of the value that takes coefficient 25 when this index
> +                is before strt index or after (start+3) and 51 when it falls into
> +                that range

Split this into two to three sentences.  It's much much trickier to
write long sentences in a way that they are as easy to understand as
short ones.  While you're at it, you might as well keep lines below 80
characters.

As a rule of thumb for you: English needs a lot of articles.  If you
have any kind of doubt, write "the".  When you have finished a piece of
text, find the places where you could add "the" and do it.  This
heuristics should be much more successful than your current one.

> +            y_h_deblock = cbp[POS_CUR]
> +                        // this mask corresponds to checking whether top edge can be filtered

the top edge

> +                        | ((cbp[POS_CUR] << 4) & ~MASK_Y_TOP_ROW) | (cbp[POS_TOP] >> 12)
> +                        // this mask corresponds to checking whether bottom edge can be filtered

the bottom edge

> +                        | ((cbp[POS_BOTTOM] << 20) & ~MASK_Y_TOP_ROW) | (cbp[POS_BOTTOM] << 16)
> +                        // this mask corresponds to checking whether top edge can be filtered
> +                        // because of motion vector differences between blocks

ditto

> +                        | mvmasks[POS_CUR] | (mvmasks[POS_CUR] << 16);
> +            y_v_deblock = cbp[POS_CUR]
> +                        // this mask corresponds to checking whether left edge can be filtered

ditto

> +                        | ((cbp[POS_CUR] << 1) & ~MASK_Y_LEFT_COL) | (cbp[POS_LEFT] >> 3)
> +                        // this mask corresponds to checking whether left edge can be filtered
> +                        // because of motion vector differences between blocks

ditto

Diego