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

Michael Niedermayer michaelni
Tue Mar 27 14:39:46 CEST 2007


Hi

On Tue, Mar 27, 2007 at 03:34:39PM +0800, Xiaohui Sun wrote:
> Hi,
>      I have updated the path, Thank you again for your suggestions about
> my code.
> 
> the new patch
> --  ported SGI encoder
> --  seperate encoder/decoder into two files
> --  fixed some problems of the previous code

[...]
> Index: libavcodec/avcodec.h
> ===================================================================
> --- libavcodec/avcodec.h	(revision 8531)
> +++ libavcodec/avcodec.h	(working copy)
> @@ -148,6 +148,7 @@
>      CODEC_ID_DSICINVIDEO,
>      CODEC_ID_TIERTEXSEQVIDEO,
>      CODEC_ID_TIFF,
> +    CODEC_ID_SGI,
>      CODEC_ID_GIF,
>      CODEC_ID_FFH264,
>      CODEC_ID_DXA,

breaks ABI, please add the new codec id at the end of the video codecs


[...]
> +/* expand an rle row into a channel */
> +static int expand_rle_row(uint8_t *buf, unsigned char *optr,
> +        int chan_offset, int pixelstride, int y)
> +{

comment is not doxygen compatible


> +    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

it would be simpler to set the pix_fmt correctly on little-endian instead
of changing the sample order


> +
> +    optr += chan_offset;
> +
> +    while (1) {
> +        pixel = bytestream_get_byte(&buf);
> +        if (!(count = (pixel & 0x7f))) {
> +            return length;
> +        }
> +        if (pixel & 0x80) {
> +            while (count--) {
> +                *optr = bytestream_get_byte(&buf);
> +                length++;
> +                optr += pixelstride;
> +            }
> +        } else {
> +            pixel = bytestream_get_byte(&buf);
> +
> +            while (count--) {
> +                *optr = pixel;
> +                length++;
> +                optr += pixelstride;
> +            }
> +        }
> +    }

the code lacks checks against overflowing the output buffer


> +}
> +
> +/* read a run length encoded sgi image */
> +static int read_rle_sgi(unsigned char* ptr,
> +                 uint8_t *buf, int buf_size, SgiState* s)
> +{
> +    uint8_t *dest_row;
> +    unsigned long *start_table;
> +    unsigned int y, z, width, height, depth, tablen;
> +    long start_offset;
> +    int ret = 0;
> +    uint8_t *cur_buf = buf;
> +
> +    width = s->width;
> +    height = s->height;
> +    depth = s->depth;
> +
> +    /* skip header */
> +    cur_buf += SGI_HEADER_SIZE - 12;
> +
> +    /* size of rle offset and length tables */
> +    tablen = height * depth * sizeof(long);
> +    start_table = (unsigned long *)cur_buf;
> +
> +    /* skip run length table */
> +    cur_buf += tablen;
> +
> +    for (z = 0; z < depth; z++) {
> +        for (y = 0; y < height; y++) {
> +            dest_row = ptr + (height - 1 - y) * (width * depth);
> +            start_offset = AV_RB32(&start_table[y + z * height]);
> +            cur_buf = buf + start_offset - 12;

the pointer should be checked for being within the input buffer, also 
unsigned long is not guranteed to be 32bit but for example is 64bit on
amd64


[...]
> +fail:
> +    return ret;

if the only thing goto fail does is return ret then the fail label and all
gotos can be avoided by directly returning which is cleaner anyway


[...]

> +    /* skip header */
> +    buf += SGI_HEADER_SIZE - 12;

this code is duplicated and could easily be factored out


> +
> +    for (z = 0; z < s->depth; z++) {
> +
> +#ifndef WORDS_BIGENDIAN
> +        /* rgba -> bgra for rgba32 on little endian cpus */
> +        if (s->depth == 4 && z != 3)
> +            chan_offset = 2 - z;
> +        else
> +#endif
> +            chan_offset = z;
> +

> +        for (y = s->height - 1; y >= 0; y--) {
> +            dest_row = ptr + (y * s->linesize);
> +            for (x = s->width; x > 0; x--) {
> +                dest_row[chan_offset] = bytestream_get_byte(&buf);
> +                dest_row += s->depth;
> +            }
> +        }

sequential writes with random reads are faster then sequential reads with
random writes


[...]
> +    if(avctx->get_buffer(avctx, p) < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> +        return -1;
> +    }
> +
> +    p->pict_type = FF_I_TYPE;
> +    p->key_frame = 1;
> +    ptr = p->data[0];
> +    p->linesize[0] = s->width * s->depth;

you cannot override the linesize provided by get_buffer()


[...]
> +/**
> + * sgi image file signature
> + */
> +#define SGI_MAGIC 474
> +
> +#define SGI_HEADER_SIZE 512
> +
> +#define SGI_GRAYSCALE 1
> +#define SGI_RGB 3
> +#define SGI_RGBA 4

code duplication


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

code duplication


> +
> +static int rle_row(AVCodecContext *avctx, unsigned char **buf,
> +                char *row, int stride, int rowsize)
> +{
> +    int length, count, i, x;
> +    char *start, repeat = 0;
> +
> +    for (x = rowsize, length = 0; x > 0;) {
> +        start = row;
> +        row += (2 * stride);
> +        x -= 2;
> +
> +        while (x > 0 && (row[-2 * stride] != row[-1 * stride] ||
> +                    row[-1 * stride] != row[0])) {
> +            row += stride;
> +            x--;
> +        };
> +
> +        row -= (2 * stride);

superfluous ()


> +        x += 2;

the row += (2 * stride); x-=2 and theire reversal can be avoided


> +
> +        count = (row - start) / stride;

division is slow and should not be used in the innermost loop


> +        while (count > 0) {
> +
> +            i = count > 126 ? 126 : count;
> +            count -= i;
> +
> +            bytestream_put_byte(buf, 0x80 | i);
> +            length++;
> +
> +            while (i > 0) {
> +                bytestream_put_byte(buf, *start);
> +                start += stride;
> +                i--;
> +                length++;
> +            };
> +        };
> +
> +        if (x <= 0)
> +            break;
> +
> +        start = row;
> +        repeat = row[0];
> +
> +        row += stride;
> +        x--;
> +
> +        while (x > 0 && *row == repeat) {
> +            row += stride;
> +            x--;
> +        };
> +
> +        count = (row - start) / stride;
> +        while (count > 0) {
> +            i = count > 126 ? 126 : count;
> +            count -= i;
> +
> +            bytestream_put_byte(buf, i);
> +            length++;
> +
> +            bytestream_put_byte(buf, repeat);
> +            length++;
> +        };
> +    };

with an input of 1 1
this loop would produce 0x82 0x01 0x01
instead of the shorter 0x02 0x01

also its code duplication, the rle encoding code from targaenc.c should be
used or if your code is faster and works correctly then you should place it
in rle.c/h and make targaenc.c use it


[...]
> +static int encode_frame(AVCodecContext *avctx, unsigned char *buf, 
> +			int buf_size, void *data){
> +    SgiContext *s = avctx->priv_data;
> +    AVFrame * const p = &s->picture;
> +    long *offsettab, *lengthtab;
> +    int i, y, z;
> +    int tablesize, chan_offset;
> +    uint8_t *srcrow;
> +    unsigned int width, height, depth, dimension;
> +    unsigned int n_bytes;
> +    unsigned char *buf0 = buf;
> +

there are no checks for buf_size being large enough


> +    *p = *(AVFrame*)data;
> +    p->pict_type = FF_I_TYPE;
> +    p->key_frame = 1;
> +
> +    width = avctx->width;
> +    height = avctx->height;

> +    depth = avctx->height;

this is wrong


[...]
> +    lengthtab = av_malloc(tablesize);
> +    offsettab = av_malloc(tablesize);

these are unneeded the values can be written directly


[...]
> +    /* write offset table */
> +    for (i = 0; i < (height * depth); i++) {
> +        bytestream_put_be32(&buf0, offsettab[i]);
> +    }
> +
> +    /* write length table */
> +    for (i = 0; i < (height * depth); i++) {
> +        bytestream_put_be32(&buf0, lengthtab[i]);
> +    }

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

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/20070327/1514c682/attachment.pgp>



More information about the ffmpeg-devel mailing list