[FFmpeg-devel] [PATCH] Rl2 Video decoder

[Reimar Döffinger]

On Mon, Mar 17, 2008 at 01:56:08PM +0100, Sascha Sommer wrote:
> +/**
> + * Run Length Decode a single 320x200 frame
> + * @param s rl2 context
> + * @param buf input buffer
> + * @param size input buffer size
> + * @param out ouput buffer
> + * @param stride stride of the output buffer
> + */
> +static void rl2_rle_decode(Rl2Context *s,const unsigned char* in,int size,
> +                               unsigned char* out,int stride){
> +     int row_inc = stride - s->avctx->width;
> +     unsigned char* back_frame = s->back_frame;
> +     const unsigned char* in_end = in + size;
> +     unsigned char* out_end = out + stride*s->avctx->height;

Btw. I do not really like using "unsigned char", first because it is
much longer that just "uint8_t" and secondly because (almost?) all other
FFmpeg code uses uint8_t.

> +     int x = s->video_base % s->avctx->width;
> +     out += s->video_base + (s->video_base / s->avctx->width) * row_inc;

I think it would be better to split video_base into x and y already in
rl2_decode_init

> +     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;

I think this is wrong in many ways, e.g. out + len could overflow,
and this does not take into account the row_inc that will be added in
the while loop.

> +         while(len--){
> +             if(s->back_frame){
> +                 if(!tag || tag == 0x80)
> +                     val = *back_frame;
> +                 ++back_frame;
> +             }

The order of these ifs and the while really should be swapped, also
since you do "tag | 0x80" above already, you should just compare
that result against 0x80 instead of comparing tag both against 0 and
0x80.

> +             *out++ = val;
> +             if(++x == s->avctx->width){
> +                  out += row_inc;
> +                  x = 0;
> +             }

I have the feeling this whole stride-handling will be easier if you use
uint8_t *cur_line and int x (pointer to start of line and current
offset in that line) and then reuse memset/memcpy (using them in 2
places, first process till end of current line and process full lines
(needs incrementing stride) and then partial last line if any).
In case of s->back_frame I even wonder if it wouldn't be both faster and
simpler to just copy the whole back frame first and then just overwrite
some parts...

Greetings,
Reimar D?ffinger