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

Michael Niedermayer michaelni
Thu Mar 29 18:58:44 CEST 2007


Hi

On Thu, Mar 29, 2007 at 06:54:19PM +0800, Xiaohui Sun wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Diego Biurrun wrote:
> > On Thu, Mar 29, 2007 at 05:25:49PM +0800, Xiaohui Sun wrote:
> >>    I have updated the patch.
> >>
> >> --- libavcodec/sgi.h	(revision 0)
> >> +++ libavcodec/sgi.h	(revision 0)
> >> @@ -0,0 +1,36 @@
> >> +
> >> +#endif
> >> \ No newline at end of file
> > 
> > Our repository is set up to reject files with missing newlines at the
> > end.
> > 
> 
> I am sorry for that, and resend patch :)
[...]
> +static int expand_rle_row(uint8_t *buf, unsigned char *optr,
> +        int chan_offset, int pixelstride)
> +{
> +    unsigned char pixel, count;
> +    int length = 0;
> +    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;
> +            }

length can be calculated from optr and the buffer start, which would avoid
the length++ per sample


[...]
> +static int read_rle_sgi(unsigned char* ptr,
> +                 uint8_t *buf, int buf_size, SgiState* s)
> +{
> +    uint8_t *dest_row;
> +    uint8_t *start_table;
> +    int y, z, tablen;
> +    long start_offset;
> +    int ret = 0;
> +    uint8_t *cur_buf = buf;
> +
> +    /* skip header, SGI_HEADER_SIZE - 12 */
> +    cur_buf += 500;
> +
> +    /* size of  RLE offset and length tables */
> +    tablen = s->height * s->depth * 4;
> +    start_table = cur_buf;
> +
> +    /* skip run length table */
> +    cur_buf += tablen;
> +
> +    for (z = 0; z < s->depth; z++) {
> +        dest_row = ptr + s->height * s->linesize;
> +        for (y = 0; y < s->height; y++) {
> +            dest_row -= s->linesize;
> +            start_offset = AV_RB32(start_table);
> +            start_table += 4;

one of the bytestream_get_ functions should be used here


> +            cur_buf = buf + start_offset - 12;
> +
> +            if (expand_rle_row(cur_buf, dest_row, z, s->depth) != s->width) {

as already said the start_offset pointer should be checked for validity before
using it


> +                ret =  AVERROR_INVALIDDATA;
> +                return ret;
> +            }
> +        }
> +    }
> +    return ret;
> +}
> +
> +/**
> + * read an uncompressed SGI image
> + * @param ptr output buffer
> + * @param buf input buffer
> + * @param buf_size input buffer size
> + * @param s the current image state
> + * @return Number of matching consecutive pixels found
> + */
> +static int read_uncompressed_sgi(unsigned char* ptr,
> +                uint8_t *buf, int buf_size, SgiState* s)
> +{
> +    int x, y, z, chan_offset, ret = 0;
> +    uint8_t *dest_row = ptr;
> +
> +    /* skip header, SGI_HEADER_SIZE - 12 */
> +    buf += 500;

i didnt mean you to replace the SGI_HEADER_SIZE - 12 by 500 but rather to move
the duplicated addition out of these functions


> +
> +    for (z = 0; z < s->depth; z++) {
> +        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;
> +            }
> +        }

the code still performs several write passes


[...]
> +    if (s->depth == SGI_GRAYSCALE) {
> +        avctx->pix_fmt = PIX_FMT_GRAY8;
> +    } else if (s->depth == SGI_RGB) {
> +        avctx->pix_fmt = PIX_FMT_RGB24;
> +    } else if (s->depth == SGI_RGBA) {
> +        avctx->pix_fmt = PIX_FMT_RGBA32;

this is wrong for big endian or little endian cpus


> +    } else {
> +        av_log(avctx, AV_LOG_ERROR, "wrong picture format\n");
> +        return -1;
> +    }
> +

> +    avctx->codec_id = CODEC_ID_SGI;

uneeded codec_id has to be at that value already

[...]
> Index: libavcodec/sgienc.c
> ===================================================================
> --- libavcodec/sgienc.c	(revision 0)
> +++ libavcodec/sgienc.c	(revision 0)
> @@ -0,0 +1,258 @@
[...]
> +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;
> +}

this function is still duplicated


> +
> +/**
> + * Compare two pixels.
> + * @param bpp Bytes per pixel
> + * @param p1 the first pixel
> + * @param p2 the second pixel
> + * @return 0 if the two pixels are the same, 
> + * 1 if the two pixel s are different
> + */
> +static inline int pixel_cmp(uint8_t* p1, uint8_t* p2, int bpp)
> +{
> +    if (bpp == 1)
> +        return (*p1 != *p2);
> +    else 
> +        return memcmp(p1, p2, bpp);
> +}
> +
> +/**
> + * Count up to 127 consecutive pixels which are either all the same or
> + * all differ from the previous and next pixels.
> + * @param start Pointer to the first pixel
> + * @param len Maximum number of pixels
> + * @param step Image pixel step
> + * @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 int count_pixels(uint8_t *start, int len, int step, int bpp, int same)
> +{
> +    uint8_t *pos;
> +    int count = 1;
> +    int span = step * bpp;
> +    len = (len + step  - 1) / step;
> +
> +    for (pos = start + span; count < FFMIN(127, len); pos += span, count ++) {
> +        if (same != !pixel_cmp(pos - span, pos, bpp)) {
> +            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 (bpp == 1 && count + 1 < FFMIN(127, len) && *pos != *(pos + span))
> +                    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;
> +}

code duplication, also the inline int pixel_cmp() makes no sense


> +
> +/**
> + * RLE compress one line of image, with maximum size of out_size
> + * @param w Image width
> + * @param bpp Bytes per pixel
> + * @param step Image pixel step
> + * @param ptr Input buffer
> + * @param outbuf Output buffer
> + * @param out_size Maximum output size
> + * @return size of output in bytes, or -1 if larger than out_size
> + */
> +int encode_rle(int w, int bpp, int step, char* ptr, char* outbuf, int out_size)
> +{
> +    int x, y, count;
> +    char* out = outbuf;
> +    int span = step * bpp;
> +
> +    for (x = 0; x < w; x += count * step) {
> +        /* see if we can encode the next set of pixels with RLE */
> +        if ((count = count_pixels(ptr, w-x, step, bpp, 1)) > 1) {
> +            if (out + bpp + 1 > outbuf + out_size) return -1;
> +            *out++ = count;
> +            if (bpp == 1)
> +                *out = *ptr;
> +            else
> +                memcpy(out, ptr, bpp);
> +            out += bpp;
> +        } else {
> +            /* fall back on uncompressed */
> +            count = count_pixels(ptr, w-x, step, bpp, 0);
> +            *out++ = 0x80 | count;
> +
> +            if (out + bpp * count + 1 > outbuf + out_size) return -1;
> +            if (step == 1)
> +                memcpy(out, ptr, bpp * count);
> +            else {
> +                for (y = 0; y < count; y++)
> +                    memcpy(out + y, ptr + span * y, bpp);
> +            }
> +            out += bpp * count;
> +        }
> +        ptr += count * span;
> +    }
> +    *out++ = 0;
> +    return (out - outbuf);
> +}

function is more complex then needed also it needs to have a ff_ prefix


[...]
> +    unsigned char *buf0 = buf;
[...]
> +    buf0 = buf0 + SGI_HEADER_SIZE;

whats this good for?

> +
> +    return n_bytes;
[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Democracy is the form of government in which you can choose your dictator
-------------- 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/20070329/e5afebf8/attachment.pgp>



More information about the ffmpeg-devel mailing list