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

[Michael Niedermayer]

Hi

On Tue, Apr 03, 2007 at 12:46:36AM +0800, Xiaohui Sun wrote:
> I have updated the patch.

[...]
> Index: libavcodec/rle.c
> ===================================================================
> --- libavcodec/rle.c	(revision 0)
> +++ libavcodec/rle.c	(revision 0)

i think this should be diffed against the old rle code (currently in targaenc.c)

[...]
> +#include "common.h"
> +#include "bytestream.h"
> +#include "rle.h"
> +
> +/**
> + * Count up to 127 consecutive pixels which are either all the same or
> + * all differ from the previous and next pixels with one bpp
> + * @param start pointer to the first pixel
> + * @param len maximum number of pixels
> + * @param same 1 if searching for identical pixel values, 0 for differing
> + * @return number of matching consecutive pixels found
> + */
> +static av_always_inline int count_pixels_1bpp(uint8_t *start, int len, int same)
> +{
> +    uint8_t *pos;
> +    int count = 1;
> +
> +    for (pos = start + 1; count < FFMIN(127, len); pos ++, count ++) {
> +        if (same != !(*(pos - 1) != * pos)) {
> +            if (!same) {
> +                /* if bpp == 1, then 0 1 1 0 is more efficiently encoded as a single
> +                 * raw block of pixels.  for larger bpp, RLE is as good or better */
> +                if (count + 1 < FFMIN(127, len) && *pos != *(pos + 1))
> +                    continue;
> +
> +                /* if RLE can encode the next block better than as a raw block,
> +                 * back up and leave _all_ the identical pixels for RLE */
> +                count --;
> +            }
> +            break;
> +        }
> +    }
> +    return count;
> +}
> +
> +/**
> + * Count up to 127 consecutive pixels which are either all the same or
> + * all differ from the previous and next pixels with more than one bpp
> + * @param start pointer to the first pixel
> + * @param len maximum number of pixels
> + * @param bpp bytes per pixel
> + * @param same 1 if searching for identical pixel values, 0 for differing
> + * @return number of matching consecutive pixels found
> + */
> +static av_always_inline int count_pixels_nbpp(uint8_t *start, int len, int bpp, int same)
> +{
> +    uint8_t *pos;
> +    int count = 1;
> +
> +    for (pos = start + bpp; count < FFMIN(127, len); pos += bpp, count ++) {
> +        if (same != !memcmp(pos - bpp, pos, bpp)) {
> +            if (!same) {
> +                /* if RLE can encode the next block better than as a raw block,
> +                 * back up and leave _all_ the identical pixels for RLE */
> +                count --;
> +            }
> +            break;
> +        }
> +    }
> +    return count;
> +}

code duplication

[...]
> +/**
> + * expand an RLE row into a channel
> + * @param in_buf input buffer
> + * @param out_ptr output buffer
> + * @param end_ptr end of line in output buffer
> + * @param chan_offset offsets into input buffer
> + * @param pixelstride pixel stride of input buffer
> + * @return Size of output in bytes, -1 if buffer overflows
> + */
> +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;

it would be cleaner if the out_ptr argument would point to the output
array instead of passing the chan_offset into the functin and then changing
the out_ptr


> +
> +    while (1) {

> +        if(in_buf + 1 > end_buf) return -1;

this check isnt really needed as the input array is guranteed to be 8 bytes
larger then end_buf


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

excelent, finally the check looks good


> +            if (expand_rle_row(in_buf + start_offset, end_buf, dest_row,
> +                dest_row + s->linesize, z, s->depth) != s->width)
> +                return AVERROR_INVALIDDATA;

linesize can be negative



[...]
> +static int decode_frame(AVCodecContext *avctx,
> +                        void *data, int *data_size,
> +                        uint8_t *buf, int buf_size)
> +{
> +    SgiState *s = avctx->priv_data;
> +    AVFrame *picture = data;
> +    AVFrame *p = &s->picture;
> +    uint8_t* orig_buf = buf, *end_buf = buf + buf_size;

the orig_buf variable seems to be unneeded


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

linesize can be negative

also i think ptr / buf are confusing names, out_ptr / in_buf would be
clearer


[...]
> +static av_always_inline int encode_rle_1bpp(int w, uint8_t* in_buf,
> +                        unsigned char* out_buf, uint8_t* end_buf)
> +{
> +    int x, count;
> +    uint8_t* orig_buf = out_buf;
> +
> +    for (x = 0; x < w; x += count) {
> +        /* see if we can encode the next set of pixels with RLE */
> +        if ((count = ff_count_pixels(in_buf, w - x, 1, 1)) > 1) {
> +            if (out_buf + 2 > end_buf) return -1;
> +            bytestream_put_byte(&out_buf, count);
> +            bytestream_put_byte(&out_buf, *in_buf);
> +        } else {
> +            /* fall back on uncompressed */
> +            count = ff_count_pixels(in_buf, w-x, 1, 0);
> +
> +            if (out_buf + count + 1 > end_buf) return -1;
> +            bytestream_put_byte(&out_buf, 0x80 | count);
> +            bytestream_put_buffer(&out_buf, in_buf, count);
> +        }
> +        in_buf += count;
> +    }

> +    *(out_buf++) = 0;

it seems this byte can end outside the array


[...]
> +    /* make an intermediate consecutive buffer */
> +    if ((encode_buf = (char *)av_malloc(width)) == NULL)

unneeded cast


> +        return -1;
> +
> +    for (z = 0; z < depth; z++) {
> +        in_buf = p->data[0] + p->linesize[0] * (height - 1) + z;
> +
> +        for (y = 0; y < height; y++) {
> +            bytestream_put_be32(&offsettab, buf - orig_buf);
> +

> +            for (x = 0; x < width; x++)
> +                *(encode_buf + x) = *(in_buf + depth * x);

i think encode_buf[x] = in_buf[depth*z] is more readable and definitly more
commonly used


> +
> +            if((length = encode_rle_1bpp(width, encode_buf, buf, end_buf)) == -1)
> +                return -1;

memleak


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

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- 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/20070403/d36c07d9/attachment.pgp>