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

[Xiaohui Sun]

Hi,
   I 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)
>

I used the current rle code.

> [...]
> > +#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
see above

>
> [...]
> > +/**
> > + * 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
>
changed

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

>
> [...]
> > +            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
>
I modified, but not sure.

>
>
> [...]
> > +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
done

>
>
> [...]
> > +    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
changed, but not sure

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

>
> [...]
> > +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
modified

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

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

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

>
>
> [...]

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: sgi_port.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070404/98c5166a/attachment.asc>