[FFmpeg-devel] [PATCH] Rl2 Video decoder

[Sascha Sommer]

Hi,

On Montag, 17. M?rz 2008, Reimar D?ffinger wrote:
> 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.
>

I did not change this but I can if it is needed.

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

Done.

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

Should be fixed now...

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

I'm only checking for one value now. Swapping the if and while does not lead 
to faster decoding with gcc 4.2.1 and that way there is no code duplication.

> > +             *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...

The memcpy and memset would still need checks for len and calulation of the 
arguments. I also noticed that one of the samples did not decode properly. 
Therefore some more fixes were in order. Copying the whole back frame is 
simpler but not faster (for the 3 samples that exist for that format...)

Regards

Sascha