[FFmpeg-devel] [PATCH] DeluxePaint Animation playback system (r2)

[Reimar Döffinger]

On Tue, Sep 01, 2009 at 10:21:03PM +1000, Peter Ross wrote:
> On Fri, Aug 28, 2009 at 04:31:51PM +0200, Reimar D?ffinger wrote:
> > On Sat, Aug 29, 2009 at 12:16:25AM +1000, Peter Ross wrote:
> > > > That will probably mean your code can't work with AVFMT_GENERIC_INDEX.
> > > > I really think that seeking should be working.
> > > 
> > > There is only one guaranteed keyframe: the first frame!
> > 
> > For normal playback I've always considered seeking to non-keyframes,
> > too, a good idea. The only real issue I have with it is that just too
> > many decoders had the habit of crashing often with that.
> 
> But it looks ugly!

Compared to no image at all it looks great.
Anyway I didn't say it should be disabled completely, I just don't like
to have code that simply can't support it at all.

> +static inline void skip(uint8_t **dst, const uint8_t *dst_end,
> +                        int count,
> +                        int *x, int width, int linesize)
> +{
> +    while (count > 0) {
> +        int striplen = FFMIN(count, width - *x);
> +        *dst  += striplen;
> +        *x    += striplen;
> +        count -= striplen;
> +        if (*x >= width) {
> +            *dst += linesize - width;
> +            *x    = 0;
> +        }
> +    }
> +}
> +
> +static inline void fill(uint8_t **dst, const uint8_t *dst_end,
> +                        int pixel, int count,
> +                        int *x, int width, int linesize)
> +{
> +    while (*dst < dst_end && count > 0) {
> +        int striplen = FFMIN(count, width - *x);
> +        memset(*dst, pixel, striplen);
> +        *dst  += striplen;
> +        *x    += striplen;
> +        count -= striplen;
> +        if (*x >= width) {
> +            *dst += linesize - width;
> +            *x    = 0;
> +        }
> +    }
> +}
> +
> +static inline void copy(uint8_t **dst, const uint8_t *dst_end,
> +                        const uint8_t **buf, const uint8_t *buf_end,
> +                        int count,
> +                        int *x, int width, int linesize)
> +{
> +    while (*dst < dst_end && *buf < buf_end && count > 0) {
> +        int striplen = FFMIN(count, FFMIN(width - *x, buf_end - *buf));
> +        memcpy(*dst, *buf, striplen);
> +        *dst  += striplen;
> +        *buf  += striplen;
> +        *x    += striplen;
> +        count -= striplen;
> +        if (*x >= width) {
> +            *dst += linesize - width;
> +            *x    = 0;
> +        }
> +    }
> +}

In the first you forgot the dst_end check, or the dst_end argument is
useless.
In the last, you should use FFMIN3
I think it might be reasonable to use a single function like

static inline void do_op(uint8_t **dst, const uint8_t *dst_end,
                         const uint8_t **buf, const uint8_t *buf_end,
                         int pixel, int count,
                         int *x, int width, int linesize)
{
    while (*dst < dst_end && count > 0) {
        int striplen = FFMIN(count, width - *x);
        if (buf) {
            striplen = FFMIN(striplen, buf_end - *buf);
            memcpy(*dst, *buf, striplen);
            *buf += striplen;
        } else if (pixel >= 0)
            memset(*dst, pixel, striplen);
        *dst  += striplen;
        *x    += striplen;
        count -= striplen;
        if (*x >= width) {
            *dst += linesize - width;
            *x    = 0;
        }
    }
}

of course with proper doxy documentation

> +        count = bytestream_get_byte(&buf);
> +        if (count > 0 && count < 0x80) {  /* copy */
> +            copy(&dst, dst_end, &buf, buf_end, count, &s->x, avctx->width, s->frame.linesize[0]);
> +        } else if (count > 0x80) {  /* shortskip */
> +            skip(&dst, dst_end, count & 0x7F, &s->x, avctx->width, s->frame.linesize[0]);
> +        } else if (count == 0) {  /* fill */
> +            int pixel;
> +            count = bytestream_get_byte(&buf);  /* count==0 gives nop */
> +            pixel = bytestream_get_byte(&buf);
> +            fill(&dst, dst_end, pixel, count, &s->x, avctx->width, s->frame.linesize[0]);
> +        } else { /* (0x80) longop */
> +            count = bytestream_get_le16(&buf);
> +            if (count > 0 && count < 0x8000) {  /* longskip */
> +                skip(&dst, dst_end, count, &s->x, avctx->width, s->frame.linesize[0]);
> +            } else if (count > 0x8000 && count < 0xC000) {  /* longcopy */
> +                copy(&dst, dst_end, &buf, buf_end, count & 0x3FFF, &s->x, avctx->width, s->frame.linesize[0]);
> +            } else if (count == 0) { /* stop */
> +                break;
> +            } else if (count >= 0xC000) {  /* longfill; count==0xc000 gives nop */
> +                int pixel = bytestream_get_byte(&buf);
> +                fill(&dst, dst_end, pixel, count & 0x3FFF, &s->x, avctx->width, s->frame.linesize[0]);
> +            } else {  /* (0x8000) unknown */
> +                av_log_ask_for_sample(avctx, "unknown opcode");
> +                return AVERROR_INVALIDDATA;
> +            }

out of which I would make something like (can probably be improved,
though I _think_ that my variant is closer to what the designers had in
mind when they created the format):
#define DO_OP(buf, pixel, count) do_op(&dst, dst_end, (buf), buf_end, (pixel), (count), &s->x, avctx->width, s->frame.linesize[0])
type = bytestream_get_byte(&buf);
count = type & 0x7f;
type >>= 7;
if (count)
    DO_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);
    DO_OP(NULL, pixel, count);
} else {
    int pixel;
    type = bytestream_get_le16(&buf);
    count = type & 0x3fff;
    type >>= 14;
    if (!count) {
        if (type == 0)
            break; // stop processing
        if (type == 2) {
            av_log_ask_for_sample(avctx, "unknown opcode");
            return AVERROR_INVALIDDATA;
        }
        // extra long skip (type == 1) or nop (type == 3)
    }
    switch (type) {
    case 1:
        // same as originally read value & 0x7fff instead of 0x3fff
        count += 0x4000;
    case 0:
        DO_OP(NULL, -1, count);
        break;
    case 2:
        DO_OP(&buf, -1, count);
        break;
    case 3:
        pixel = bytestream_get_byte(&buf);
        DO_OP(NULL, pixel, count);
        break;
    }
}
The switch could also be replaced by
    pixel = type == 3 ? bytestream_get_byte(&buf) : -1;
    if (type == 1) count += 0x4000;
    DO_OP(type == 2 ? &buf : NULL, pixel, count);