[FFmpeg-devel] [PATCH] Rl2 Video decoder

[Michael Niedermayer]

On Mon, Mar 17, 2008 at 01:56:08PM +0100, Sascha Sommer wrote:
> Hi,
> 
> > > +static void rl2_rle_decode(Rl2Context *s,const unsigned char* buf,int
> > > size, +                               unsigned char* out,int stride){
> > > +     int row_inc = stride - s->avctx->width;
> > > +     /* calculate additional video_base offset due to stride != width */
> > > +     int offset = (s->video_base / s->avctx->width) * row_inc;
> > > +     int frame_size = s->avctx->width * s->avctx->height - offset;
> > > +     int pos_in = 0;
> > > +     int pos_out = s->video_base;
> > > +     unsigned char* back_frame = s->back_frame - s->video_base;
> > > +     out += offset;
> > > +
> > > +     while(pos_in < size){
> > > +         unsigned char tag = buf[pos_in++];
> > > +         int len = 1;
> > > +         unsigned char val = 0;
> > > +         int i;
> > > +         if(tag >= 0x80){
> > > +             if(pos_in >= size)
> > > +                 break;
> > > +             len = buf[pos_in++];
> > > +             if(!len)
> > > +                 break;
> > > +         }
> > > +         if(s->back_frame)
> > > +             val = tag | 0x80;
> > > +         else
> > > +             val = tag & ~0x80;
> > > +
> > > +         for(i=0;i<len && pos_out < frame_size; i++){
> > > +             if(s->back_frame && (!tag || tag == 0x80))
> > > +                 val = back_frame[pos_out];
> > > +             out[pos_out] = val;
> > > +             ++pos_out;
> > > +             if(!(pos_out % s->avctx->width))
> > > +                  out += row_inc;
> > > +         }
> >
> > modulo is as division a very slow operation so it should be avoided
> > and yes the 0x80 was what i meant
> > But there are still a few simplifcations possible
> > Theres are several  redundant variables, there are some checks which are
> > redundant as well
> > And i would check len against the remaining size instead of checking
> > against both len and the size in every iteration.
> > Then i also would use pointers like in / in_end instead of buf/pos_in/size
> > it avoids the buf+pos_in addition.
> >
> 
> New patch attached.
[...]

> +     while(in < in_end){
> +         unsigned char tag = *in++;
> +         int len = 1;
> +         unsigned char val;
> +         if(tag >= 0x80){
> +             if(in >= in_end)
> +                 break;
> +             len = *in++;
> +             if(!len)
> +                 break;
> +         }
> +         if(s->back_frame)
> +             val = tag | 0x80;
> +         else
> +             val = tag & ~0x80;
> +
> +         if(out + len >= out_end)
> +             break;
> +
> +         while(len--){
> +             if(s->back_frame){
> +                 if(!tag || tag == 0x80)
> +                     val = *back_frame;
> +                 ++back_frame;
> +             }
> +             *out++ = val;
> +             if(++x == s->avctx->width){
> +                  out += row_inc;
> +                  x = 0;
> +             }

This still contains several redundant checks
A (unlikely) buffer overflow
A redundant variable

if(out == line_end){
    out += row_inc;
    line_end += stride;
}
avoids the ++


[...]
> +        uint8_t* back_frame = av_mallocz(avctx->width*avctx->height);
> +        if(!back_frame)
> +            return -1;
> +        rl2_rle_decode(s,avctx->extradata + EXTRADATA1_SIZE,back_size,
> +                           back_frame,avctx->width);
> +        s->back_frame = back_frame;

the back_frame variable isnt really needed

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

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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/20080317/28dc72f0/attachment.pgp>