[FFmpeg-devel] [PATCH] Rl2 Video decoder

[Michael Niedermayer]

On Thu, Mar 20, 2008 at 07:45:21PM +0100, Sascha Sommer wrote:
> Hi,
> 
> > 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 ++
> >
> 
> Changed the code a bit. Some other changes were also needed because one sample 
> did not decode properly.
> 
> >

> > > +        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
> >
> > [...]
> 
> Why? Just removing it here won't work because in case of decoding a video with 
> back_frame the back_frame itself has to be decoded without backframe.
> Of course one could solve this differently if this is what you meant?

hmm no, forget what i said.


[...]
> +typedef struct Rl2Context {
> +    AVCodecContext *avctx;
> +    AVFrame frame;
> +
> +    unsigned short video_base; ///< initial drawing offset

> +    int base_x;                ///< x in the current line
> +    int base_y;                ///< how often the stride needs to be incremented

I think they would fit better as local variables of rl2_rle_decode(), after
all there is no other function useing them.


> +    unsigned int clr_count;    ///< number of used colors (currently unused)
> +    unsigned char* back_frame; ///< background frame
> +    unsigned int palette[AVPALETTE_COUNT];
> +} Rl2Context;
> +
> +/**
> + * 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
> + * @param ignore_base ignore video_base
> + */
> +static void rl2_rle_decode(Rl2Context *s,const unsigned char* in,int size,
> +                               unsigned char* out,int stride,int ignore_base){

Instead of passing ignore_base you could pass base itself and set it to 0
for the ignore case.


> +     int stride_adj = stride - s->avctx->width;
> +     const unsigned char* back_frame = s->back_frame;
> +     const unsigned char* in_end = in + size;
> +     const unsigned char* out_end = out + stride * s->avctx->height;
> +     unsigned char* line_end = out + s->avctx->width;
> +


> +     /** copy start of the background frame */
> +     if(s->back_frame){
> +         int i;
> +         for(i=0;i<s->base_y;i++){
> +             memcpy(out,back_frame,s->avctx->width);
> +             out += stride;
> +             back_frame += s->avctx->width;
> +         }
> +         if(s->base_x){
> +             memcpy(out, back_frame, s->base_x);
> +             back_frame += s->base_x;
> +         }
> +         line_end = out + s->avctx->width;
> +         out += s->base_x;
> +     }else if(!ignore_base){
> +         line_end = out + s->base_y * stride + s->avctx->width;
> +         out += s->video_base + s->base_y * stride_adj;
> +         back_frame += s->video_base;
> +     }

for(i=0;i<=base_y;i++){
    if(s->back_frame)
        memcpy(out,back_frame,s->avctx->width);
    out += stride;
    back_frame += s->avctx->width;
}
back_frame += base_x - s->avctx->width;
line_end = out - stride_adj;
out += base_x - stride;


> +
> +     /** decode the variable part of the frame */
> +     while(in < in_end){
> +         unsigned char val = *in++;
> +         int len = 1;
> +         if(val >= 0x80){
> +             if(in >= in_end)
> +                 break;
> +             len = *in++;
> +             if(!len)
> +                 break;
> +         }
> +

> +         if(out >= out_end - len)

if(len >= out_end - out)

out_end - len could theoretically overflow



> +             break;
> +
> +         if(s->back_frame)
> +             val |= 0x80;
> +         else 
> +             val &= ~0x80;
> +
> +         while(len--){
> +             *out++ = (val == 0x80)? *back_frame:val;
> +             back_frame++;
> +             if(out == line_end){
> +                  out += stride_adj;
> +                  line_end += stride;
> +                  if(out >= out_end - len)
> +                      break;
> +             }
> +         }
> +    }
> +
> +    /** copy the rest from the background frame */
> +    if(s->back_frame){
> +        while(out < out_end){

> +             *out++ = *back_frame++;
> +             if(out == line_end){
> +                  out += stride_adj;
> +                  line_end += stride;
> +             }

following might be faster, but ignore it if its not

memcpy(out, back_frame, line_end - out);
back_frame += line_end - out;
out= line_end + stride_adj;
line_end += stride;

patch looks ok except these

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

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- 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/20080321/68dd6ff7/attachment.pgp>