[FFmpeg-devel] [PATCH] RV40 Loop Filter

[Michael Niedermayer]

On Sat, Oct 25, 2008 at 10:08:44AM +0300, Kostya wrote:
> On Wed, Oct 22, 2008 at 10:53:23AM +0200, Michael Niedermayer wrote:
> > On Tue, Oct 21, 2008 at 09:23:21AM +0300, Kostya wrote:
[...]
> > [...]
> > > +static int rv40_set_deblock_coef(RV34DecContext *r)
> > > +{
> > > +    MpegEncContext *s = &r->s;
> > > +    int mvmask = 0, i, j, dx, dy;
> > > +    int midx = s->mb_x * 2 + s->mb_y * 2 * s->b8_stride;
> > 
> > > +    if(s->pict_type == FF_I_TYPE)
> > > +        return 0;
> > 
> > why is this even called for i frames?
> 
> I intend to use it for calculating macroblock-specific deblock
> strength in RV30.

fine but how is that related to having the pict_type check inside the
function compared to outside?


[...]
> > > +                if(dx > 3 || dy > 3){
> > > +                    mvmask |= 0x03 << (i*2 + j*8);
> > > +                }
> > > +            }
> > > +        }
> > > +        midx += s->b8_stride;
> > > +    }
> > 
> > i think the if() can be moved out of the loop like
> > if(first_slice_line)
> >     mvmask &= 123;
> 
> IMO it can't.
> It constructs mask based on motion vectors difference in the
> horizontal/vertical neighbouring blocks after all. 

one way (there surely are thousend others)

get_mask(int delta)
    for()
        for()
            v0= motion_val[x+y*stride]
            v1= motion_val[x+y*stride+delta]
            if(FFABS(v0[0]-v1[0])>3 || FFABS(v0[1]-v1[1])>3)
                mask |= 1<<(2*x+8*y);
    return mask

hmask= get_mask(1     );
vmask= get_mask(stride);
if(!mb_x)
    hmask &= 0x...
if(first_slice_line)
    vmask &= 0x...
mask = hmask | (hmask<<1) | vmask | (vmask<<4);

besides, the way mask bits are combined looks strange/wrong


>  
> > > +    return mvmask;
> > > +}
> > > +
> > > +static void rv40_loop_filter(RV34DecContext *r)
> > > +{
> > > +    MpegEncContext *s = &r->s;
> > > +    int mb_pos;
> > > +    int i, j;
> > > +    uint8_t *Y, *C;
> > > +    int alpha, beta, betaY, betaC;
> > > +    int q;
> > > +    // 0 - cur block, 1 - top, 2 - left, 3 - bottom
> > > +    int btype[4], clip[4], mvmasks[4], cbps[4], uvcbps[4][2];
> > > +
> > 
> > > +    if(s->pict_type == FF_B_TYPE)
> > > +        return;
> > 
> > why is this even called for b frames?
> 
> Because the spec says so :)
> RV40 has many special cases for B-frame loop filter which
> I didn't care to implement.

:/
i hope it cannot use B frames as reference?


[...]
> [lots of loop filter invoking] 
> > 
> > the word mess is probably the best way to describe this
> > as far as i can tell you are packing all the bits related to deblocking
> > and then later duplicate code each with hardcoded masks to extract them
> > again.
> 
> We have a saying here "To make a candy from crap", which I think describes
> current situation. I'd like to shot the group of men who proposed the loop
> filter in the form RV40 has it.

there arent many codecs around that are cleanly designed ...
Some things here and there are ok but terrible messes like this are more
common.
We dont have too much of a choice, to support things the mess has to be
implemented. If it can be done cleaner/simpler thats a big advantage in the
long term, easier to maintain, understand, optimize; smaller and faste, ...


> 
> The problem is that edges should be filtered in that order with clipping
> values depending on clipping values selected depending on whether
> neighbouring block coded is not and if it belongs to the same MB or not.
> It's possible to all of the into loop, but it will have too many additional
> conditions to my taste. I've merged some of them though.

iam not suggesting to build a complex and ugly loop, rather something like
storing all the numbers that might differ in a 2d array and then
having a loop go over this.
the mb edge flags, coded info and all that would be in the array so that
reading it is a matter of coded[y][x], mb_edge[y][x], mb_type[y][x]
i think this would be cleaner IMHO

Ill review the new patch soon

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

It is not what we do, but why we do it that matters.
-------------- 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/20081025/a85aea39/attachment.pgp>