[FFmpeg-devel] [PATCH i/N] RV30 loop filter

[Michael Niedermayer]

On Sat, Feb 23, 2008 at 12:57:29PM +0200, Kostya wrote:
> $subj
> 
> Probably correct.

> Index: libavcodec/rv30data.h
> ===================================================================
> --- libavcodec/rv30data.h	(revision 12129)
> +++ libavcodec/rv30data.h	(working copy)
> @@ -171,4 +171,17 @@
>      2, 7, 8, 4, 0, 6, 1, 5, 3,
>      2, 8, 3, 0, 7, 4, 1, 6, 5,
>  };
> +
> +/**
> + * Loop filter limits are taken from this table.
> + */
> +static const uint8_t rv30_loop_filt_lim[7][32] = {
> +    { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 2, 2, 2, 2, 2, 3, 3, 3, 3, 4, 4, 4, 4, 5, 5, 5 },
> +    { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 2, 2, 2, 2, 2, 2, 3, 3, 3, 3, 3, 3, 4, 4, 4 },
> +    { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 2, 2, 2, 2, 2, 3, 3, 3, 3, 4, 4, 4, 4, 5, 5, 5 },
> +    { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 2, 2, 2, 2, 2, 3, 3, 3, 4, 4, 4, 5, 5, 5, 6, 6, 6 },
> +    { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 2, 2, 2, 3, 3, 3, 4, 4, 4, 5, 5, 5, 6, 6, 6, 7, 7, 7 },
> +    { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 2, 2, 3, 3, 4, 4, 4, 5, 5, 5, 6, 6, 6, 7, 7, 7, 8, 8, 8 },
> +    { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 2, 3, 4, 4, 4, 5, 5, 5, 6, 6, 6, 7, 7, 7, 8, 8, 8, 9, 9, 9 }
> +};
>  #endif /* FFMPEG_RV30DATA_H */
> Index: libavcodec/rv30.c
> ===================================================================
> --- libavcodec/rv30.c	(revision 12129)
> +++ libavcodec/rv30.c	(working copy)
> @@ -111,6 +111,103 @@
>          return rv30_b_types[code];
>  }
>  
> +static inline void rv30_weak_loop_filter(uint8_t *src, const int step,
> +                                         const int stride, const int lim)
> +{
> +    uint8_t *cm = ff_cropTbl + MAX_NEG_CROP;
> +    int i, diff;
> +
> +    if(!lim) return;
> +    for(i = 0; i < 4; i++){
> +        diff = ((src[-2*step] - src[1*step]) - (src[-1*step] - src[0*step])*4) >> 3;
> +        diff = av_clip(diff, -lim, lim);
> +        src[-1*step] = cm[src[-1*step] + diff];
> +        src[ 0*step] = cm[src[ 0*step] - diff];
> +        src += stride;
> +    }
> +}
> +
> +static void rv30_loop_filter(RV34DecContext *r)
> +{
> +    MpegEncContext *s = &r->s;
> +    int mb_pos;
> +    int i, j;

> +    int strength = 0;//FIXME how to determine correct value?
> +    const uint8_t* lim = rv30_loop_filt_lim[strength];

This does not look correct.


> +    int cbp1, cbp2, q;
> +
> +    s->mb_x = 0;
> +    for(s->mb_y = 0; s->mb_y < s->mb_height; s->mb_y++){
> +        ff_init_block_index(s);
> +        mb_pos = s->mb_y * s->mb_stride;
> +        for(s->mb_x = 0; s->mb_x < s->mb_width; s->mb_x++, mb_pos++){
> +            ff_update_block_index(s);
> +            if(!IS_INTRA(s->current_picture_ptr->mb_type[mb_pos])) continue;
> +            if(s->mb_x && IS_INTRA(s->current_picture_ptr->mb_type[mb_pos - 1])){
> +                cbp1 = r->cbp_luma[mb_pos - 1];
> +                cbp2 = r->cbp_luma[mb_pos];
> +                q = s->current_picture_ptr->qscale_table[mb_pos - 1];

> +                for(i = 0; i < 16; i += 4, cbp1 >>= 4, cbp2 >>= 4)

Please dont put so much stuff in the for() this makes the code hard to read.


> +                    if((cbp1 & 8) && (cbp2 & 1))
> +                        rv30_weak_loop_filter(s->dest[0] + i * s->linesize, 1, s->linesize, lim[q]);

cbp2 &= cbp1>>3;


> +                cbp1 = r->cbp_chroma[mb_pos - 1];
> +                cbp2 = r->cbp_chroma[mb_pos];
> +                for(i = 0; i < 8; i += 4, cbp1 >>= 2, cbp2 >>= 2){
> +                    if((cbp1 & 0x02) && (cbp2 & 0x01))
> +                        rv30_weak_loop_filter(s->dest[1] + i * s->uvlinesize, 1, s->uvlinesize, lim[q]);
> +                    if((cbp1 & 0x20) && (cbp2 & 0x10))
> +                        rv30_weak_loop_filter(s->dest[2] + i * s->uvlinesize, 1, s->uvlinesize, lim[q]);
> +                }
> +            }
> +            q = s->current_picture_ptr->qscale_table[mb_pos];
> +            for(j = 4; j < 16; j += 4){
> +                cbp1 = r->cbp_luma[mb_pos] >> ((j >> 2) - 1);
> +                for(i = 0; i < 16; i += 4, cbp1 >>= 4)
> +                    if(cbp1 & 3)
> +                        rv30_weak_loop_filter(s->dest[0] + j + i * s->linesize, 1, s->linesize, lim[q]);
> +            }
> +            cbp1 = r->cbp_chroma[mb_pos];
> +            for(i = 0; i < 8; i += 4, cbp1 >>= 2){
> +                if(cbp1 & 0x03)
> +                    rv30_weak_loop_filter(s->dest[1] + 4 + i * s->uvlinesize, 1, s->uvlinesize, lim[q]);
> +                if(cbp1 & 0x30)
> +                    rv30_weak_loop_filter(s->dest[2] + 4 + i * s->uvlinesize, 1, s->uvlinesize, lim[q]);
> +            }

This code does cbpA || cbpB the one above does cbpA && cbpB, this is strange.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- 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/20080224/4852fed1/attachment.pgp>