[FFmpeg-devel] [PATCH] IFF: Add error checking to byterun1 decoder

[Sebastian Vater]

Ronald S. Bultje a ?crit :
> Hi,
>
> On Sun, May 16, 2010 at 6:35 PM, Sebastian Vater
> <cdgs.basty at googlemail.com> wrote:
>   
>> Stefano Sabatini a ?crit :
>>     
>>> On date Monday 2010-05-17 00:08:55 +0200, Sebastian Vater encoded:
>>>
>>>       
>>>> @@ -306,7 +313,7 @@ static int decode_frame_byterun1(AVCodecContext *avctx,
>>>>      const uint8_t *buf = avpkt->data;
>>>>      int buf_size = avpkt->size;
>>>>      const uint8_t *buf_end = buf+buf_size;
>>>> -    int y, plane;
>>>> +    int y, plane, err;
>>>>
>>>>      if (avctx->reget_buffer(avctx, &s->frame) < 0){
>>>>          av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
>>>> @@ -319,7 +326,11 @@ static int decode_frame_byterun1(AVCodecContext *avctx,
>>>>                  uint8_t *row = &s->frame.data[0][ y*s->frame.linesize[0] ];
>>>>                  memset(row, 0, avctx->width);
>>>>                  for (plane = 0; plane < avctx->bits_per_coded_sample; plane++) {
>>>> -                    buf += decode_byterun(s->planebuf, s->planesize, buf, buf_end);
>>>> +                    if ((err = decode_byterun(s->planebuf, s->planesize, buf, buf_end)) < 0) {
>>>> +                        av_log(avctx, AV_LOG_ERROR, "IFF byterun1 stream truncated\n");
>>>> +                        return err;
>>>> +                    }
>>>> +                    buf += err;
>>>>
>>>>         
>>> buf += err looks strange, buf += ret should look saner.
>>>
>>>       
>> Fixed by changing err(or) to res(ult).
>>     
> [..]
>   
>> @@ -336,7 +351,11 @@ static int decode_frame_byterun1(AVCodecContext *avctx,
>>      } else {
>>          for(y = 0; y < avctx->height ; y++ ) {
>>              uint8_t *row = &s->frame.data[0][y*s->frame.linesize[0]];
>> -            buf += decode_byterun(row, avctx->width, buf, buf_end);
>> +            if ((res = decode_byterun(row, avctx->width, buf, buf_end)) < 0) {
>> +                av_log(avctx, AV_LOG_ERROR, "IFF byterun1 stream truncated\n");
>> +                return res;
>> +            }
>> +            buf += res;
>>          }
>>      }
>>     
>
> You could also pass &buf as a function argument, have decode_byterun()
> update buf internally, so this duplication is removed also. Then
> return 0 on success or <0 on error like any other FFmpeg function.
>   

Maybe this helps to prevent un-inlining the function with the error
messages in it (see below)...

But not today, I'm awfully tired, didn't sleep last night and had to
write another exam for 7 hours today. I'm really chessmate for today...

> And I agree with Stefano that the log msg should be in
> decode_byterun() also, a goto here is fine with me.
>   

Well, I tried this already today, but exactly this change makes the
function not inline anymore :(

And that's what I don't like really.

BTW, I'm thinking of another optimization patch...the memset 0 stuff in
the decoding stuff...

This can be replaced with a decode_first_plane like function, which
doesn't OR into output buffer but sets it instead.
So bitplanes from 1 to n are handled simply as normal ORed as they're
now, while bitplane 0 is simply set, so the memset can be discarded, too.

-- 

Best regards,
                   :-) Basty/CDGS (-: