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

[Kostya]

On Sat, Mar 08, 2008 at 06:50:10PM +0100, Michael Niedermayer wrote:
> On Sat, Mar 08, 2008 at 07:15:18PM +0200, Kostya wrote:
> > On Sun, Feb 24, 2008 at 12:37:24AM +0100, Michael Niedermayer wrote:
> > > On Sat, Feb 23, 2008 at 12:57:29PM +0200, Kostya wrote:
> > > > $subj
> > > > 
> > > > Probably correct.
> > > 
> > [...]
> > > > +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.
> > 
> > I believe it is - rv30_loop_filt_lim is an array of 32-element mapping
> > tables between quantizer and limit values.
> 
> I mean strength=0 above vs. your claim about "Probably correct." above.
> This just does not look correct at all, the FIXME also contradicts that this
> is correct.
 
I meant the whole flow; strength parameter must be initialized somehow, but
zero value for strength for now has some sense too.
 
> [...]
> > > > +                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.
> > 
> > before it was luma, here it is chroma and I just merged U and V filter calls together
> 
> The first code excutes rv30_weak_loop_filter when both blocks are coded
> The second code excutes rv30_weak_loop_filter when either block is coded
> They are both for chroma
 
Ah, I forgot to state if((cbp1 & 0x03) == 0x03).
 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB