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

[Reimar Döffinger]

On Wed, Aug 26, 2009 at 08:45:33PM +1000, Peter Ross wrote:
> +static av_cold int decode_init(AVCodecContext *avctx){
> +    AnmContext *s = avctx->priv_data;
> +    uint8_t *buf;
> +    int i;
> +
> +    avctx->pix_fmt = PIX_FMT_PAL8;
> +
> +    if (avctx->extradata_size != 16*8 + 4*256)
> +        return -1;
> +
> +    /* manually allocate frame of width * height */
> +    s->frame.reference = 1;
> +    s->frame.buffer_hints = FF_BUFFER_HINTS_VALID;
> +    s->frame.linesize[0] = avctx->width;
> +    s->frame.data[0] = av_malloc(avctx->width*avctx->height);
> +    if (!s->frame.data[0])
> +        return AVERROR_NOMEM;
> +    s->frame.data[1] = av_malloc(AVPALETTE_SIZE);
> +    if (!s->frame.data[1]) {
> +        av_freep(&s->frame.data[0]);
> +        return AVERROR_NOMEM;
> +    }

Supporting DR1 and using reget_buffer sure would be nicer.
Yes, you'd have to split the encoded data by lines, though I think this
should be possible without making it too ugly with a bit of refactoring.

> +    buf = avctx->extradata + 16*8;
> +    for(i = 0; i < 256; i++) {
> +        ((int*)s->frame.data[1])[i] = AV_RL32(buf);
> +        buf += 4;
> +    }

Use uint32_t and bytestream_get_le32

> +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];
> +    int d = 0;
> +    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;
> +
> +#define LIMITCOUNT(c)  FFMIN( (c), avctx->width*avctx->height - d )

Wherever possible use functions instead of macros. Also
avctx->width*avctx->height should be stored somewhere.
Or actually I think you should use a dst_end, remove d and
increment dst.
Not to mention that creating and using bytestream_fill_byte and
bytestream_copy functions would slightly simplify the code.

> +        count = bytestream_get_byte(&buf);

Well, I still consider bytestream_get_byte overkill where *buf++ would
do.

> +            memcpy(dst + d, buf, LIMITCOUNT(count));

You must limit against buf size, too.

> +        } else if (count == 0x80) {  /* longop */
> +            count = bytestream_get_le16(&buf);
> +            if (count == 0) { /* stop */
> +                break;
> +            } else if (count < 0x8000) {  /* longskip */
> +                d += count;
> +            } else if (count == 0x8000) {
> +                av_log_ask_for_sample(avctx, "unknown opcode");
> +                return AVERROR_INVALIDDATA;
> +            } else if (count < 0xC000) {  /* longrun */
> +                count -= 0x8000;
> +                memcpy(dst + d, buf, LIMITCOUNT(count));

Same as for the other memcpy

> +                buf += count;
> +                d += count;
> +            } else {  /* longdump; count==0xc000 gives nop */
> +                int pixel = bytestream_get_byte(&buf);
> +                count -= 0xC000;
> +                memset(dst + d, pixel, LIMITCOUNT(count));
> +                d += count;
> +            }

These should either be ordered by probability or maybe use a
switch (count >> 14)

Also it might make sense to factor out the "count & 0x3fff" (which is
what those subtractions actually calculate).


> +#define LPF_TAG  MKTAG('L','P','F',' ')
> +#define ANIM_TAG MKTAG('A','N','I','M')
> +
> +static int probe(AVProbeData *p)
> +{
> +    /* verify tags and video dimensions */
> +    if (AV_RL32(&p->buf[0]) == LPF_TAG &&
> +        AV_RL32(&p->buf[16]) == ANIM_TAG &&

AV_RN32(p->buf     ) == AV_RN32("LPF ") &&
AV_RN32(p->buf + 16) == AV_RN32("ANIM") &&

Seems nicer to me.

> +    for (i = 0; i < MAX_PAGES; i++) {
> +        const Page *p = &anm->pt[i];
> +        if (p->nb_records > 0 && record >= p->base_record && record < p->base_record + p->nb_records) {
> +            return i;
> +        }

The {} and the p->nb_records > 0 are both pointless?

> +static int read_header(AVFormatContext *s,
> +                           AVFormatParameters *ap)
> +{
> +    AnmDemuxContext *anm = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    AVStream *st;
> +    int i;
> +
> +    if (get_le32(pb) != LPF_TAG)
> +        return AVERROR_INVALIDDATA;

I'm not sure if that kind of thing should even be checked in
read_header.

> +    /* 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)
> +        return AVERROR_INVALIDDATA;
[...]
> +    /* ignore last delta record (used for looping) */
> +    if (get_byte(pb))  /* has_last_delta */
> +        anm->nb_records = FFMAX(anm->nb_records - 1, 0);
[...]
> +    if (get_byte(pb) != 0) {
> +        av_log_ask_for_sample(s, "unknown pixel type");
> +        return AVERROR_INVALIDDATA;
> +    }

Quite inconsistent. Also aren't you leaking the created video stream?

> +    if (get_buffer(pb, st->codec->extradata, st->codec->extradata_size) < 0)
> +       return AVERROR(EIO);

You should return the error that get_buffer returned.

> +    /* read page table */
> +    url_fseek(pb, anm->page_table_offset, SEEK_SET);

Better check the return value.

> +        url_fseek(pb, anm->page_table_offset + MAX_PAGES*6 + anm->page*0x10000, SEEK_SET);

That looks rather magic. Also anm->page << 16 maybe?

> +        if (!p->record_sizes) {
> +            url_fskip(pb, 6); /* base_record, nb_records, size */
> +            if (get_le16(pb)) {
> +                av_log_ask_for_sample(s, "page_continuation_size is non-zero\n");
> +                return AVERROR_INVALIDDATA;
> +            } 

Trailing whitespace.

> +            p->record_sizes = av_malloc(sizeof(p->record_sizes[0]) * p->nb_records);
> +            if (!p->record_sizes)
> +                return AVERROR(ENOMEM);
> +            for(j = 0; j < p->nb_records; j++)
> +                p->record_sizes[j] = get_le16(pb);

The whole block could be in a separate function.

> +        }else{
> +            url_fskip(pb, 8 + 2*p->nb_records);
> +        }
> +        anm->record = 0;
> +    }
> +
> +    /* if we have fetched all records in this page, then find the
> +       next page and repeat */
> +
> +    if (anm->record >= p->nb_records) {
> +        anm->page = find_record(anm, anm->pts);
> +        if (anm->page < 0)
> +            return anm->page;
> +        anm->record = -1;
> +        goto repeat;
> +    }

You created a loop that never checks for EOF here. I have some doubts
this is all correct.

> +    /* fetch record */
> +    if (av_get_packet(s->pb, pkt, p->record_sizes[anm->record])<0)
> +        return AVERROR(ENOMEM);

Certainly not! Return whichever error av_get_packet returned.

> +    pkt->size = p->record_sizes[anm->record];

That's unnecessary for complete packets and completely wrong for partial
ones, possibly even introducing a crash.

> +    pkt->pts = anm->pts;
> +    if (pkt->pts == 0)
> +        pkt->flags |= PKT_FLAG_KEY;
> +
> +    anm->pts++;
> +    anm->record++;

That will probably mean your code can't work with AVFMT_GENERIC_INDEX.
I really think that seeking should be working.

> +    for(i = 0; i < MAX_PAGES; i++)
> +        av_free(anm->pt[i].record_sizes);

av_freep just to be safe?