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

[Michael Niedermayer]

Hi

On Thu, Apr 05, 2007 at 09:42:59AM +0800, Xiaohui Sun wrote:
[...]

> + * @param out_ptr output buffer

this pointer does not point to the output buffer but rather to the line
after the output buffer


> + * @param in_buf input buffer
> + * @param end_buf end of input buffer
> + * @param s the current image state
> + * @return 0 if no error, else return error number
> + */
> +static int read_rle_sgi(unsigned char* out_ptr, uint8_t *in_buf,
> +                        uint8_t *end_buf, SgiState* s)

s/end_buf/in_end/


> +{
> +    uint8_t *dest_row = out_ptr;
> +    unsigned int len = s->height * s->depth * 4;
> +    uint8_t *start_table = in_buf;
> +    unsigned int y, z;
> +    unsigned int start_offset;
> +
> +    /* size of  RLE offset and length tables */
> +    if(len * 2  > end_buf - in_buf) {
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    in_buf -= SGI_HEADER_SIZE;
> +    for (z = 0; z < s->depth; z++) {
> +        for (y = 0; y < s->height; y++) {
> +            dest_row -= s->linesize;
> +            start_offset = bytestream_get_be32(&start_table);
> +            if(start_offset > end_buf - in_buf) {
> +                return AVERROR_INVALIDDATA;
> +            }
> +            if (expand_rle_row(in_buf + start_offset, end_buf, dest_row + z,
> +                dest_row + FFABS(s->linesize), s->depth) != s->width)
> +                return AVERROR_INVALIDDATA;
> +        }

> +        dest_row = out_ptr;

if you move this line before the for() then you avoid the second such
assignment above


> +    }
> +    return 0;
> +}
> +
> +/**
> + * read an uncompressed SGI image
> + * @param out_ptr output buffer

> + * @param end_ptr end ofoutput buffer

typo also s/end_ptr/out_end/


> + * @param in_buf input buffer

> + * @param end_buf end of input buffer

s/end_buf/in_end/


[...]
> +    uint8_t *dest_row = end_ptr;

this is uneeded


> +    uint8_t *ptr = in_buf;
> +    unsigned int offset = s->height * s->width;
> +
> +    /* test buffer size */
> +    if (offset * s->depth > end_buf - in_buf) {
> +       return -1;
> +    }
> +
> +    for (y = s->height - 1; y >= 0; y--) {

> +        dest_row = out_ptr + (y * FFABS(s->linesize));

this looks wrong


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

> +            ptr = ++in_buf;

if you move this assignment before the for loop then you avoid the second
redundant assignment farther above


[...]
> +    dimension = bytestream_get_be16(&in_buf);
> +    s->width = bytestream_get_be16(&in_buf);
> +    s->height = bytestream_get_be16(&in_buf);
> +    s->depth = bytestream_get_be16(&in_buf);

these could be verticall aligned like

dimension = bytestream_get_be16(&in_buf);
s->width  = bytestream_get_be16(&in_buf);
s->height = bytestream_get_be16(&in_buf);
s->depth  = bytestream_get_be16(&in_buf);

looks nicer ... (not really important though)


[...]
> +    /* name */
> +    memset(buf, 0, 80);
> +    buf += 80;
> +
> +     /* colormap */
> +    bytestream_put_be32(&buf, 0L);
> +
> +    /* the rest of the 512 byte header is unused */
> +    memset(buf, 0, 404);
> +    buf += 404;

thats just a single memset() ...


[...]
> +            if((length = ff_rle_encode(buf, end_buf - buf - 1, encode_buf, 1, width, 0, 0, 0x80, 0)) == -1) {

if(... < 1)


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

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- 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/20070405/628f624e/attachment.pgp>