[Ffmpeg-devel] [PATCH] Chinese AVS video decoder

[Rich Felker]

On Sun, Jul 02, 2006 at 12:36:26AM +0100, M?ns Rullg?rd wrote:
> Some comments based on discussions on IRC and other observations.
> 
> > +typedef struct {
> > +    int16_t x;
> > +    int16_t y;
> > +    int16_t dist;
> > +    int16_t ref;
> > +} vector_t;
> 
> If this is marked with __attribute__((align(8))) gcc will be able to
> read/write the entire thing with one instruction on 64-bit machines.

Maybe you think this is ugly, but what about the standard way, making
it a union and also including an int64_t member?

> > +/* kth-order exponential golomb code */
> > +static inline int get_ue_code(GetBitContext *gb, int order) {
> > +    if(order)
> > +        return (get_ue_golomb(gb) << order) + get_bits(gb,order);
> 
> The order of evaluation of operands is unspecified, so that could read
> the bits in the wrong order.  The get_ue_golomb() and get_bits() calls
> must be in different statements (well, strictly speaking, any sequence
> point will do).

Yep, good catch!

> > +    for(i=0;i<65;i++) {
> > +        level_code = get_ue_code(gb,r->golomb_order);
> > +        if(level_code >= ESCAPE_CODE) {
> > +            run = (level_code - ESCAPE_CODE) >> 1;
> > +            esc_code = get_ue_code(gb,esc_golomb_order);
> > +            level = esc_code + (run > r->max_run ? 1 : r->level_add[run]);
> > +            while(level > r->inc_limit)
> > +                r++;
> > +            mask = -(level_code & 1);
> > +            level = (level^mask) - mask;
> > +        } else {
> > +            if(level_code < 0)
> > +                return -1;
> > +            level = r->rltab[level_code][0];
> > +            if(!level) //end of block signal
> > +                break;
> > +            run   = r->rltab[level_code][1];
> > +            r += r->rltab[level_code][2];
> > +        }
> > +        level_buf[i] = level;
> > +        run_buf[i] = run;
> > +    }
> > +    /* inverse scan and dequantization */
> > +    for(i=i-1;i>=0;i--) {
> 
> That's an odd-looking for() construct.  I'd write while(--i) instead.

I love odd-looking for statements, but I'll agree that many people
don't.. :)

> > +        veccpy(&h->mv[MV_FWD_B2],(vector_t *)&un_mv);
> > +        veccpy(&h->mv[MV_FWD_B3],(vector_t *)&un_mv);
> > +        veccpy(&h->mv[MV_BWD_B2],(vector_t *)&un_mv);
> > +        veccpy(&h->mv[MV_BWD_B3],(vector_t *)&un_mv);
> 
> Those casts (and others like them) remove a const qualifier.  Make the
> second parameter to veccpy const instead.  Or just skip veccpy
> entirely and write the assignment directly.  I don't see why there
> would ever be any need for a more complicated function.

Agree.

Rich