[Ffmpeg-devel] [PATCH] ported SGI decoder to the new API
Michael Niedermayer
michaelni
Sat Mar 31 20:57:55 CEST 2007
Hi
On Sat, Mar 31, 2007 at 04:28:19PM +0800, Xiaohui Sun wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi,
> I have updated the path.
>
> - changed the RLE functions to more efficiency.
> - fixed other problems
could you please in the future reply to reviews and clearly say which
comments you have taken care of and which not, and why not.
[...]
> + unsigned int linesize;
linesize is signed not unsigned, and yes it can be negative for "bottom up"
stored images
> +} SgiState;
> +
> +/**
> + * expand an RLE row into a channel
> + * @param in_buf input buffer
> + * @param out_buf 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_buf, int chan_offset, int pixelstride)
> +{
> + unsigned char pixel, count;
> + unsigned char *orig = out_buf;
> +
> + out_buf += chan_offset;
> +
> + while (1) {
> + if(in_buf + 1 > end_buf) return -1;
> + pixel = bytestream_get_byte(&in_buf);
> + if (!(count = (pixel & 0x7f))) {
> + return (out_buf - orig) / pixelstride;
> + }
> +
> + if (pixel & 0x80) {
> + if(in_buf + count >= end_buf) return -1;
> + while (count--) {
> + *out_buf = bytestream_get_byte(&in_buf);
> + out_buf += pixelstride;
> + }
> + } else {
> + if(in_buf + 1 >= end_buf) return -1;
> + pixel = bytestream_get_byte(&in_buf);
> +
> + while (count--) {
> + *out_buf = pixel;
> + out_buf += pixelstride;
> + }
> + }
> + }
> +}
this code still lacks checks against buffer overflows
[...]
> + if(in_buf + tablen * 2 > end_buf) {
> + return AVERROR_INVALIDDATA;
> + }
indention looks wrong
> +
> + for (z = 0; z < s->depth; z++) {
> + dest_row = out_buf + s->height * s->linesize;
> + for (y = 0; y < s->height; y++) {
> + dest_row -= s->linesize;
> + start_offset = bytestream_get_be32(&start_table);
> + if(in_buf + start_offset > end_buf) {
> + return AVERROR_INVALIDDATA;
> + }
this is better but it still does not catch all cases
> + if (expand_rle_row(in_buf + start_offset, end_buf, dest_row, z, s->depth) != s->width)
> + return AVERROR_INVALIDDATA;
[...]
> + for(z = 0; z < s->depth; z ++) {
> + data = *(in_buf + z * offset);
> + bytestream_put_byte(&dest_row, data);
> + }
the *offset can be avoided
[...]
> +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;
> + unsigned int dimension, bytes_per_channel, rle;
> + int ret = 0;
> + uint8_t *ptr;
> +
> + if (buf_size < 2){
> + av_log(avctx, AV_LOG_ERROR, "buf size too small (%d)\n", buf_size);
> + return -1;
> + }
why check for 2? why not SGI_HEADER_SIZE?
> +
> + /* test for SGI magic */
> + if (AV_RB16(buf) != SGI_MAGIC) {
> + av_log(avctx, AV_LOG_ERROR, "bad magic number\n");
> + return -1;
> + }
> +
> + /* skip magic */
> + buf += 2;
the AV_RB16() / buf+=2 can be simplified
[...]
> + if (rle) {
> + ret = read_rle_sgi(ptr, orig_buf, end_buf, s);
> + } else {
> + ret = read_uncompressed_sgi(ptr, orig_buf + SGI_HEADER_SIZE, end_buf, s);
> + }
my repeated suggestions to factor the addition out did not mean to move them
around randomly in each submission but rather:
orig_buf += SGI_HEADER_SIZE
if (rle) {
ret = read_rle_sgi(ptr, orig_buf, end_buf, s);
} else {
ret = read_uncompressed_sgi(ptr, orig_buf, end_buf, s);
}
you could of course have said that the offsets to the lines in rle are from
the buffer start (i missed that) and as such my suggested change would not
change the number of additions (true for the current code) though with
proper working validity checks on the offsets it might still be simpler, we
will see as soon as the code has such checks ...
[...]
> +/**
> + * 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 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.
> + * @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;
> +}
this is still duplicated relative to targaenc.c
[...]
> + case PIX_FMT_RGB24:
> + dimension = SGI_MULTI_CHAN;
> + depth = SGI_RGB;
> + break;
> + case PIX_FMT_RGBA32:
> + dimension = SGI_MULTI_CHAN;
> + depth = SGI_RGBA;
> + break;
indention is wrong and the pix_fmt missmatches what is in AVCodec.pix_fmts
[...]
> + for (z = 0; z < depth; z++) {
> + out_buf = p->data[0] + p->linesize[0] * (height - 1) + z;
out_buf sounds like output buffer to me, but this is the input image which is
to be encoded, maybe another name would be better
[...]
> + /* total length */
> + n_bytes = buf - orig_buf;
> +
> + return n_bytes;
the n_bytes variable is unneeded
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No great genius has ever existed without some touch of madness. -- Aristotle
-------------- 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/20070331/84bfab06/attachment.pgp>
More information about the ffmpeg-devel
mailing list