[Ffmpeg-devel] [PATCH] ported SGI decoder to the new API

[Michael Niedermayer]

Hi

On Sun, Apr 01, 2007 at 08:38:24PM +0800, Xiaohui Sun wrote:
[...]
> Index: libavcodec/rle.c
> ===================================================================
> --- libavcodec/rle.c	(revision 0)
> +++ libavcodec/rle.c	(revision 0)
> @@ -0,0 +1,253 @@

your rle encoder has as many lines of code as the whole targa encoder
this can be done simpler

[...]

> +static int expand_rle_row(uint8_t *in_buf, uint8_t* end_buf,
> +            unsigned char *out_ptr, uint8_t* end_ptr,
> +            int chan_offset, int pixelstride)
> +{
> +    unsigned char pixel, count;
> +    unsigned char *orig = out_ptr;
> +
> +    out_ptr += chan_offset;
> +
> +    while (1) {
> +        if(in_buf + 1 > end_buf) return -1;
> +        pixel = bytestream_get_byte(&in_buf);
> +        if (!(count = (pixel & 0x7f))) {
> +            return (out_ptr - orig) / pixelstride;
> +        }
> +
> +        if (pixel & 0x80) {
> +            if ((in_buf + count >= end_buf) || (out_ptr + count > end_ptr)) return -1;
> +            while (count--) {
> +                *out_ptr = bytestream_get_byte(&in_buf);
> +                out_ptr += pixelstride;
> +            }
> +        } else {
> +            if ((in_buf + 1 >= end_buf) || (out_ptr + count > end_ptr)) return -1;
> +            pixel = bytestream_get_byte(&in_buf);
> +
> +            while (count--) {
> +                *out_ptr = pixel;
> +                out_ptr += pixelstride;
> +            }
> +        }
> +    }

you check for buffer overflows now, sadly the checks are buggy
also they can be simplified


[...]
> +            start_offset = bytestream_get_be32(&start_table);
> +            len = bytestream_get_be32(&length_table);
> +            if(in_buf + start_offset + len > end_buf) {
> +                return AVERROR_INVALIDDATA;
> +            }

this check still is not catching all cases, also it seems you missunderstood
me, i didnt mean that you check the values in an otherwise unused table but
rather that you properly check the values you do use for overflow


[...]
> +        for (x = s->width; x > 0; x--) {
> +            ptr = in_buf;
> +            offset = 0;
> +            for(z = 0; z < s->depth; z ++) {
> +                ptr += offset;
> +                bytestream_put_byte(&dest_row, *ptr);
> +            }
> +            in_buf ++;
> +        }

this is buggy

[...]
> +    s->linesize = p->linesize[0];
> +    p->pict_type = FF_I_TYPE;
> +    p->key_frame = 1;
> +    ptr = p->data[0];
> +
> +    end_ptr = ptr + FFABS(s->linesize * s->height);

this is buggy


[...]
> +        case PIX_FMT_RGB32:
> +            dimension = SGI_MULTI_CHAN;
> +            depth = SGI_RGBA;
> +            break;

i think this is broken on either little or big endian

[...]
> +    return (buf - orig_buf);

superfluous ()


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070401/776be5a4/attachment.pgp>