[FFmpeg-devel] [PATCH] DeluxePaint Animation playback system

Michael Niedermayer michaelni
Mon Oct 19 22:36:11 CEST 2009


On Sun, Oct 18, 2009 at 04:19:04PM +1100, Peter Ross wrote:
> On Thu, Oct 15, 2009 at 07:01:28PM +0200, Diego Biurrun wrote:
> > On Thu, Oct 15, 2009 at 07:54:05PM +1100, Peter Ross wrote:
> > > 
> > > +++ b/libavcodec/anm.c
> > > @@ -0,0 +1,177 @@
> > > +static av_cold int decode_init(AVCodecContext *avctx) {
> > 
> > K&R function declarations please; you use them everywhere else.
> 
> Ok.
[...]
> +static av_cold int decode_init(AVCodecContext *avctx)
> +{
> +    AnmContext *s = avctx->priv_data;
> +    const uint8_t *buf;
> +    int i;
> +
> +    avctx->pix_fmt = PIX_FMT_PAL8;
> +
> +    if (avctx->extradata_size != 16*8 + 4*256)
> +        return -1;
> +
> +    s->frame.reference = 1;
> +    if (avctx->get_buffer(avctx, &s->frame) < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> +        return -1;
> +    }
> +
> +    buf = avctx->extradata + 16*8;
> +    for (i = 0; i < 256; i++)

> +        ((int*)s->frame.data[1])[i] = bytestream_get_le32(&buf);

i think this should be uint32_t not int



> +
> +    return 0;
> +}
> +
> +/**
> + * Perform decode operation
> + * @param dst Destination image buffer
> + * @param buf Source buffer (optional, see below)
> + * @param pixel Fill color (optional, see below)
> + * @param count Pixel count
> + * @param x Pointer to x-axis counter
> + * @param width Image width
> + * @param linesize Destination imaage buffer linesize
                                    ^^
typo


[...]
> +static int decode_frame(AVCodecContext *avctx,
> +                        void *data, int *data_size,
> +                        AVPacket *avpkt)
> +{
> +    AnmContext *s = avctx->priv_data;
> +    const uint8_t *buf = avpkt->data;
> +    const int buf_size = avpkt->size;
> +    const uint8_t *buf_end = buf + buf_size;
> +    uint8_t *dst     = s->frame.data[0];

> +    uint8_t *dst_end = s->frame.data[0] + s->frame.linesize[0]*avctx->height;

> +    int count;
> +
> +    if (buf[0] != 0x42) {
> +        av_log_ask_for_sample(avctx, "unknown record type\n");
> +        return buf_size;
> +    }
> +    if (buf[1]) {
> +        av_log_ask_for_sample(avctx, "padding bytes not supported\n");
> +        return buf_size;
> +    }
> +    buf += 4;
> +
> +    s->x = 0;
> +    do {
> +        /* if statements are ordered by probability */
> +#define OP(buf, pixel, count) \
> +    op(&dst, dst_end, (buf), buf_end, (pixel), (count), &s->x, avctx->width, s->frame.linesize[0])
> +

this is missing a reget_buffer()



> +        int type = bytestream_get_byte(&buf);
> +        count = type & 0x7F;
> +        type >>= 7;
> +        if (count) {
> +            OP(type ? NULL : &buf, -1, count);
> +        } else if (!type) {
> +            int pixel;
> +            count = bytestream_get_byte(&buf);  /* count==0 gives nop */
> +            pixel = bytestream_get_byte(&buf);
> +            OP(NULL, pixel, count);
> +        } else {
> +            int pixel;
> +            type = bytestream_get_le16(&buf);
> +            count = type & 0x3FFF;
> +            type >>= 14;
> +            if (!count) {
> +                if (type == 0)
> +                    break; // stop
> +                if (type == 2) {
> +                    av_log_ask_for_sample(avctx, "unknown opcode");
> +                    return AVERROR_INVALIDDATA;
> +                }
> +                continue;
> +            }
> +            pixel = type == 3 ? bytestream_get_byte(&buf) : -1;
> +            if (type == 1) count += 0x4000;
> +            OP(type == 2 ? &buf : NULL, pixel, count);
> +        }

> +    } while (dst < dst_end && buf + 1 < buf_end);

this code seems to assume linesize > 0 but it could also be < 0


[...]
> +typedef struct {
> +    int base_record;
> +    int nb_records;
> +    int size;

> +    uint16_t *record_sizes;

why is this kept for each page?
is seeking possible in this format? if not it seems to make no sense to keep
the previous


> +} Page;
[...]
> +
> +static int read_header(AVFormatContext *s,
> +                       AVFormatParameters *ap)
> +{
> +    AnmDemuxContext *anm = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    AVStream *st;
> +    int i, ret;
> +
> +    url_fskip(pb, 4); /* magic number */
> +    if (get_le16(pb) != MAX_PAGES) {
> +        av_log_ask_for_sample(s, "max_pages != " AV_STRINGIFY(MAX_PAGES) "\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    anm->nb_pages   = get_le16(pb);

> +    anm->nb_records = get_le32(pb);

the field in anm is a signed int of possibly just 32bit so it could end
negative


> +    url_fskip(pb, 2); /* max records per page */
> +    anm->page_table_offset = get_le16(pb);
> +    if (get_le32(pb) != ANIM_TAG)
> +        return AVERROR_INVALIDDATA;
> +
> +    /* video stream */
> +    st = av_new_stream(s, 0);
> +    if (!st)
> +        return AVERROR(ENOMEM);
> +    st->codec->codec_type = CODEC_TYPE_VIDEO;
> +    st->codec->codec_id   = CODEC_ID_ANM;
> +    st->codec->codec_tag  = 0; /* no fourcc */
> +    st->codec->width      = get_le16(pb);
> +    st->codec->height     = get_le16(pb);
> +    if (get_byte(pb) != 0) {
> +        av_log_ask_for_sample(s, "unknown variant");
> +        av_close_input_stream(s);
> +        return AVERROR_INVALIDDATA;
> +    }
> +    url_fskip(pb, 1); /* frame rate multiplier info */
> +
> +    /* ignore last delta record (used for looping) */
> +    if (get_byte(pb))  /* has_last_delta */
> +        anm->nb_records = FFMAX(anm->nb_records - 1, 0);
> +
> +    url_fskip(pb, 1); /* last_delta_valid */
> +
> +    if (get_byte(pb) != 0) {
> +        av_log_ask_for_sample(s, "unknown pixel type");
> +        av_close_input_stream(s);
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    if (get_byte(pb) != 1) {
> +        av_log_ask_for_sample(s, "unknown compression type");

> +        av_close_input_stream(s);
> +        return AVERROR_INVALIDDATA;

could be simplified with a goto


> +    }
> +
> +    url_fskip(pb, 1); /* other recs per frame */
> +
> +    if (get_byte(pb) != 1) {
> +        av_log_ask_for_sample(s, "unknown bitmap type");
> +        av_close_input_stream(s);
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    url_fskip(pb, 32); /* record_types */

> +    url_fskip(pb, 4);  /* nb_frames */

AVStream has a nb_frames field that maybe could be set to this assuming its
what i think it is


[...]
> +static int read_packet(AVFormatContext *s,
> +                       AVPacket *pkt)
> +{
> +    AnmDemuxContext *anm = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    Page *p;
> +    int j;
> +
> +    if (url_feof(s->pb))
> +        return AVERROR(EIO);
> +
> +    if (anm->page < 0)
> +        return 0;
> +

> +repeat:

maybe this could be a do/while loop which if it has no other disadvantages
and doesnt need more lines would IMHO be clearer than a goto


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

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091019/1fe43eee/attachment.pgp>



More information about the ffmpeg-devel mailing list