[FFmpeg-devel] [PATCH] Rl2 Video decoder

[Sascha Sommer]

Hi,

Thanks for the review.

On Sonntag, 16. M?rz 2008, Michael Niedermayer wrote:
> On Sat, Mar 15, 2008 at 10:07:20PM +0100, Sascha Sommer wrote:
> > Hi,
> >
> > attached patch adds support for the rl2 video format as described on
> > http://wiki.multimedia.cx/index.php?title=RL2
>
> [...]
>
> > +     while(pos_in < size){
> > +         unsigned char tag = buf[pos_in++];
> > +         int len = 1;
> > +         unsigned char val;
> > +         int i;
> > +         if(tag >= 0x80){
> > +             if(pos_in >= size)
> > +                 break;
> > +             len = buf[pos_in++];
> > +             if(!len)
> > +                 break;
> > +         }
> > +         for(i=0;i<len && pos_out < frame_size &&
> > +                          back_pos < back_frame_size ;i++){
> > +
> > +             if(s->back_frame){
> > +                 if(tag == 0x00 || tag == 0x80)
> > +                     val = s->back_frame[back_pos];
> > +                 else
> > +                     val = tag | 0x80;
> > +             }else{
> > +                 if(tag < 0x80)
> > +                     val = tag;
> > +                 else if(tag == 0x80)
> > +                     val = 0;
> > +                 else
> > +                     val = tag & 0x7F;
> > +             }
> > +             out[pos_out++] = val;
> > +             ++back_pos;
> > +             ++x;
> > +             if(x >= s->avctx->width){
> > +                 pos_out += row_inc;
> > +                 x = 0;
> > +             }
> > +         }
> > +    }
>
> This can be simplified, i could point at specific things but after all
> its a qualification task so i wont ...
>
>

Better now?


> [...]
>
> > +    if(s->video_base > avctx->width * avctx->height){
>
> shouldnt that be >= ?
>

Fixed

> > +        av_log(avctx, AV_LOG_ERROR, "invalid video_base\n");
> > +        return -1;
> > +    }
> > +
> > +    /** initialize palette */
> > +    for(i=0;i<AVPALETTE_COUNT;i++){
> > +        s->palette[i] = (avctx->extradata[6+i*3] << 16) |
> > +            (avctx->extradata[6+i*3+1] << 8) |
> > (avctx->extradata[6+i*3+2]);
>
> AV_RB24()
>

Fixed

>
> [...]
>
> > +    s->frame.reference = 1;
> > +    s->frame.buffer_hints = FF_BUFFER_HINTS_VALID |
> > FF_BUFFER_HINTS_PRESERVE | +                           
> > FF_BUFFER_HINTS_REUSABLE | FF_BUFFER_HINTS_READABLE; +
>
> trailing white space
>
> > +    /** get buffer */
> > +    if(avctx->reget_buffer(avctx, &s->frame)) {
> > +        av_log(s->avctx, AV_LOG_ERROR, "reget_buffer() failed\n");
> > +        return -1;
> > +    }
>
> What of the buffer content is reused?
>

Fixed

> > +
> > +    /** run length decode */
> > +   
> > rl2_rle_decode(s,buf,buf_size,s->frame.data[0],s->frame.linesize[0]);
>
> Does doxygen nowadays support such inline comments?
>

It can be used to describe the steps of a function: For example the section 
for this function would look like:


Decode a single frame. 
Parameters:
avctx   decoder context 
  data   decoded frame 
  data_size   size of the decoded frame 
  buf   input buffer 
  buf_size   input buffer size 
Returns:
0 success, -1 on error 
 get buffer
 run length decode
 make the palette available on the way out
 report that the buffer was completely consumed 
Definition at line 171 of file rl2.c.

Regards

Sascha


-------------- next part --------------
A non-text attachment was scrubbed...
Name: rl2_decoder_try2.patch
Type: text/x-diff
Size: 7929 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080316/649dace7/attachment.patch>