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

[Anssi Hannula]

Reimar D?ffinger wrote:
> Hello,

Hi!

> On Sun, Apr 01, 2007 at 11:04:14PM +0300, Anssi Hannula wrote:
>> +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...

Indeed, but as Michael said, this should be made more efficient.

>> +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.

Right, the casts are unneeded.

>> +    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.

OK.

[...]
> 
>> +
>> +        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.

Indeed.

>> +            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.

OK.

>> +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?

The latter.

>> +    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...

OK.

>> +
>> +    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?

Yes.

However, since the palettesize is always 768 or 0, we could just
allocate the packet with size = videosize + 768 + 1, with the 768 bytes
unused when the palette is not sent. This would allow us to remove the
backward seeking.

>> +    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...

OK.

>> +    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..

In line 134 voc_get_packet() is called (the audio frame is actually a
complete VOC file).

-- 
Anssi Hannula