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

[Michael Niedermayer]

Hi

On Fri, Mar 23, 2007 at 07:54:22PM +0800, sunxiaohui wrote:
> Hi,
> I have ported the SGI decoder to the new API.  


[...]
> +#include "avcodec.h"
> +#include "bytestream.h"
> +
> +/* sgi image file signature */
> +#define SGI_MAGIC 474

comments should be doxygen compatible


[...]
> +#define SGI_SINGLE_CHAN 2
> +#define SGI_MULTI_CHAN 3

unused


> +
> +typedef struct SgiContext {
> +    AVFrame picture;
> +} SgiContext;
> +
> +typedef struct SGIInfo{
> +    short magic;
> +    char rle;
> +    char bytes_per_channel;
> +    unsigned short dimension;
> +    unsigned short xsize;
> +    unsigned short ysize;
> +    unsigned short zsize;
> +    unsigned short linesize;

why are they not int?


> +} SGIInfo;

why are there 2 structs (SGIInfo and SgiContext) and not 1


> +
> +/* expand an rle row into a channel */
> +static int expand_rle_row(uint8_t *buf, unsigned char *optr,
> +        int chan_offset, int pixelstride)
> +{
> +    unsigned char pixel, count;
> +    int length = 0;
> +
> +#ifndef WORDS_BIGENDIAN
> +    /* rgba -> bgra for rgba32 on little endian cpus */
> +    if (pixelstride == 4 && chan_offset != 3) {
> +       chan_offset = 2 - chan_offset;
> +    }
> +#endif
> +
> +    optr += chan_offset;
> +
> +    while (1) {
> +        pixel = bytestream_get_byte(&buf);
> +
> +        if (!(count = (pixel & 0x7f))) {
> +            return length;
> +        }
> +        if (pixel & 0x80) {
> +            while (count--) {
> +                length++;
> +                optr += pixelstride;
> +            }
> +        } else {
> +            pixel = bytestream_get_byte(&buf);
> +
> +            while (count--) {
> +                length++;
> +                optr += pixelstride;
> +            }
> +        }
> +    }

what is this supposed to do?


[...]
> +    tablen = ysize * zsize * sizeof(long);
> +    start_table = (unsigned long *)av_malloc(tablen);
> +    if (!bytestream_get_buffer(&cur_buf, (uint8_t*)start_table, tablen)) {

unneeded memcpy() and unneeded casts


> +        ret = AVERROR_IO;
> +        goto fail;
> +    }
> +
> +    /* skip run length table */
> +     cur_buf += tablen;

wrong indention


> +    
> +    for (z = 0; z < zsize; z++) {
> +        for (y = 0; y < ysize; y++) {
> +            dest_row = ptr + (ysize - 1 - y) * (xsize * zsize);

whatever this is supposed to do the multiplication can overflow
which considering this is a destination pointer to write is not good


> +            start_offset = AV_RB32(&start_table[y + z * ysize]);
> +

> +            if ((cur_buf - buf) != start_offset){
> +                cur_buf = buf + start_offset;
> +            }

the check is useless

[...]
> +/* read an uncompressed sgi image */
> +static int read_uncompressed_sgi(unsigned char* ptr,
> +                        uint8_t *buf, int buf_size, SGIInfo* s_info)
> +{
> +    int x, y, z, chan_offset, ret = 0;
> +    uint8_t *dest_row = ptr;
> +
> +    /* skip header */
> +    buf += SGI_HEADER_SIZE - 12;
> +
> +    for (z = 0; z < s_info->zsize; z++) {
> +
> +#ifndef WORDS_BIGENDIAN
> +        /* rgba -> bgra for rgba32 on little endian cpus */
> +        if (s_info->zsize == 4 && z != 3)
> +            chan_offset = 2 - z;
> +        else
> +#endif
> +            chan_offset = z;
> +
> +        for (y = s_info->ysize - 1; y >= 0; y--) {
> +            dest_row = ptr + (y * s_info->linesize);
> +            for (x = s_info->xsize; x > 0; x--) {
> +                dest_row[chan_offset] = bytestream_get_byte(&buf);
> +                dest_row += s_info->zsize;
> +            }
> +        }
> +    }

isnt this just a memcpy() (with a loop for linesize)?


[...]

> +    if(buf_size < 2){
> +    av_log(avctx, AV_LOG_ERROR, "buf size too small (%d)\n", buf_size);

wrong indention


> +        return -1;
> +    }
> +
> +    /* test for sgi magic */
> +    if (AV_RB16(buf) != SGI_MAGIC) {
> +       av_log(avctx, AV_LOG_ERROR, "bad magic number\n");

indention should be 4 spaces in ffmpeg


> +       return -1;
> +    }
> +
> +    /* skip magic */
> +    buf += 2;
> +    s_info.rle = bytestream_get_byte(&buf);
> +    s_info.bytes_per_channel = bytestream_get_byte(&buf);
> +    s_info.dimension = (unsigned short)bytestream_get_be16(&buf);
> +    s_info.xsize = (unsigned short) bytestream_get_be16(&buf);
> +    s_info.ysize = (unsigned short) bytestream_get_be16(&buf);
> +    s_info.zsize = (unsigned short) bytestream_get_be16(&buf);

senseless casts

bytes_per_channel, rle and dimension are not used outside this function so
they dont need to be stored in the context

xsize/ysize could be renamed to width/height


> +
> +    if(s_info.zsize > 4096)
> +        s_info.zsize= 0;

what is this good for, >4096 and 0 will both fail the checks below


[...]

> +    s_info.linesize = p->linesize[0];

this is wrong, linesize doesnt have to fit into a unsigned short


[...]
> +static int sgi_init(AVCodecContext *avctx){
> +    SgiContext *s = avctx->priv_data;
> +
> +    avcodec_get_frame_defaults((AVFrame*)&s->picture);
> +    avctx->coded_frame= (AVFrame*)&s->picture;

senseless casts


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct awnser.
-------------- 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/20070323/dbc9392e/attachment.pgp>