[Ffmpeg-devel] [PATCH] ported SGI decoder to the new API
Xiaohui Sun
sunxiaohui
Sun Apr 1 14:38:24 CEST 2007
Xiaohui Sun wrote:
> 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.
>
ok, thank you for reminding me :)
>
> [...]
> > + unsigned int linesize;
>
> linesize is signed not unsigned, and yes it can be negative for
> "bottom up"
> stored images
>
fixed
>
> > +} 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
>
done
>
> [...]
> > + if(in_buf + tablen * 2 > end_buf) {
> > + return AVERROR_INVALIDDATA;
> > + }
>
> indention looks wrong
>
done
>
> > +
> > + 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
>
done
>
> > + 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
>
done, I use addition each time.
>
> [...]
> > +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?
done
>
> > +
> > + /* 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
done
>
>
> [...]
>
> > + 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);
> }
I have changed that.
>
> 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
I have added rle.h/rle.c in this patch, and let targaenc.c call it.
I have noticed Bartlomiej is implementing this, so checked his code and
put an additional parameter to handle different RLE encodings in
TIFF/TGA/SGI.
>
>
> [...]
>
> > + 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
done
>
>
> [...]
> > + 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
done
>
>
> [...]
> > + /* total length */
> > + n_bytes = buf - orig_buf;
> > +
> > + return n_bytes;
>
> the n_bytes variable is unneeded
done
>
>
> [...]
>
>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: sgi_port.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070401/9ce709e6/attachment.asc>
More information about the ffmpeg-devel
mailing list