[Ffmpeg-devel] [PATCH] C93 demuxer and decoder (GSoC qualification task)

[Reimar Döffinger]

Hello,
On Sun, Apr 01, 2007 at 11:04:14PM +0300, Anssi Hannula wrote:
> As my Google Summer of Code qualification task, attached is a patch
> which adds C93 [1] demuxer and video decoder to FFmpeg. The samples from
> samples.mplayerhq.hu can be properly played out with it.
> 
> I'm not too familiar with FFmpeg codebase yet, though, so there could be
> some silly mistakes in the code ;). Please review it.
> 
> [1] http://wiki.multimedia.cx/index.php?title=C93

I'm not the "master reviewer" here, so some things I say might be wrong,
but I don't want to leave all the work to one person...

> +static int c93_decode_init(AVCodecContext *avctx)
> +{
> +    avctx->pix_fmt = PIX_FMT_PAL8;
> +    return 0;
> +}

Since you set it in the demuxer, you might consider if you can replace
the constants by avctx->width/height, it _might_ clarify the code a bit
as well.
Though don't forget to do avcodec_check_dimensions then.

> +static inline int c93_conv_offset(int offset, int x, int y, int size,
> +                                                    int stride)
> +{
> +    int prev_x = offset % 320 + x;
> +    int prev_y = offset / 320 + y;
> +    if (prev_x >= 320) {
> +        prev_x -= 320;
> +        prev_y += size;
> +    }
> +    return prev_y*stride+prev_x;
> +}

seems non-obvious enough to me to warrant a doxygen comment...

> +static int c93_decode_frame(AVCodecContext * avctx,
> +                 void *data, int *data_size, uint8_t * buf, int buf_size)
> +{
> +    C93DecoderContext * const c93 = avctx->priv_data;
> +    AVFrame * const newpic = (AVFrame*) &(c93->pictures[c93->currentpic]);
> +    AVFrame * const oldpic = (AVFrame*) &(c93->pictures[1 - c93->currentpic]);

Hmm... What's the point of those casts? They seem useless to me. Also it
seems weird to me to cast to (AVFrame*) but actually assigning to
AVFrame * const.

> +    c93->currentpic = c93->currentpic ? 0 : 1;

c93->currentpic = !c93->currentpic;
or
c93->currentpic = 1 - c93->currentpic;

whichever you choose, IMO make it the same as you use for setting
oldpic.

> +    if ((buf++)[0] & 0x01) { /* contains palette */

*buf++ seems more readable to me than (buf++)[0].

> +        for (i = 0; i < 256; i++, buf += 3) {
> +            pal1[i] = pal2[i] = (buf[2] << 16) | (buf[1] << 8) | buf[0];
> +        }

Looks like what the AV_RL24 macro does to me.

> +
> +        switch (bt1) {
> +        case C93_8X8_FROM_PREV:
> +            offset = AV_RL16(buf);
> +            buf += 2;
> +            for (j = 0; j < 8; j++)
> +                for (i = 0; i < 8; i++) {
> +                    int loc = c93_conv_offset(offset, i, j, 8, stride);
> +                    if (loc >= 192 * stride + 320) {
> +                        av_log(avctx, AV_LOG_ERROR, "invalid offset %d for"
> +                                " mode 2 at %dx%d\n", offset, x, y);
> +                        return -1;
> +                    }
> +                    out[(y+j)*stride+x+i] = oldpic->data[0][loc];
> +                }

These two loops look similar enough to those of the other case that you
should try factoring it out into an extra function.

> +            break;
> +
> +        case C93_4X4_FROM_PREV:
> +            for (k = 0; k < 4; k++) {
> +                y_off = k > 1 ? 4 : 0;
> +                x_off = (k % 2) ? 4 : 0;

Avoid % where it is pointless. Here k & 1 will do at least as well.

> +static int c93_probe(AVProbeData *p)
> +{
> +    if (p->buf_size < 13)
> +        return 0;
> +
> +    if (p->buf[0] == 0x01 && p->buf[1] == 0x00 &&
> +        p->buf[4] == 0x01 + p->buf[2] &&
> +        p->buf[8] == p->buf[4] + p->buf[6] &&
> +        p->buf[12] == p->buf[8] + p->buf[10])
> +        return AVPROBE_SCORE_MAX / 2;

Did you have any kind of misdetection or is the / 2 just because the
check seems not too reliable?

> +    videosize = get_le16(pb);
> +    url_fskip(pb, videosize);
> +    palettesize = get_le16(pb);
> +
> +    ret = av_new_packet(pkt, videosize + palettesize + 1);
> +    if (ret < 0)
> +        return ret;
> +
> +    pkt->data[0] = palettesize ? 0x01 : 0x00;

Could use !!palettesize instead...

> +
> +    if (palettesize) {
> +        if (palettesize != 768) {
> +            av_log(s, AV_LOG_ERROR, "invalid palette size %u\n", palettesize);
> +            av_free_packet(pkt);
> +            return AVERROR_INVALIDDATA;
> +        }
> +        ret = get_buffer(pb, pkt->data + 1, palettesize);
> +        if (ret < palettesize) {
> +            av_free_packet(pkt);
> +            return AVERROR_IO;
> +        }
> +    }
> +    url_fskip(pb, -(palettesize + videosize + 2));

seeking, especially seeking backwards should be avoided where possible.
Just try playing the file directly from http or ftp and you'll see why.
I guess this would be a bit simpler if we had a proper way to handle
palette. As I understand the code sending the palette in a second packet
is not an option since it is needed already for the frame that's stored
directly in front of it?

> +    ret = get_buffer(pb, pkt->data + palettesize + 1, videosize);
> +    if (ret < videosize) {
> +        av_free_packet(pkt);
> +        return AVERROR_IO;
> +    }
> +    url_fskip(pb, palettesize + 2);
> +    pkt->size = palettesize + videosize + 1;

No need to set that since that's exactly the number you gave to
av_new_packet...

> +    pkt->stream_index = 0;
> +    c93->decode_audio = c93->current_block * 32 + c93->current_frame;
> +    /* only the first frame is guaranteed to not reference previous frames */
> +    if (c93->decode_audio == 0) {
> +        pkt->flags |= PKT_FLAG_KEY;
> +        pkt->data[0] |= 0x02;
> +    }

Can you explain this audio stuff? It seems weird, you create an audio
stream but never add any packets to it..

Greetings,
Reimar D?ffinger