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

[Anssi Hannula]

Michael Niedermayer wrote:
> Hi

Hi!

> On Sun, Apr 01, 2007 at 11:10:34PM +0200, Reimar D?ffinger wrote:
>> Hello,
>> On Sun, Apr 01, 2007 at 11:04:14PM +0300, Anssi Hannula wrote:
>>> +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.
> 
> no definitly dont replace the constanst used per pixel by variables ...
> 
> a #define WIDTH 320 would be fine though if you like

I'll do that.

> [...]
>>> +    if ((buf++)[0] & 0x01) { /* contains palette */
>> *buf++ seems more readable to me than (buf++)[0].
> 
> theres at least one other student who used (buf++)[0], just in case you
> want to complain to him too ...
> i didnt really care so i ignored it ...

I agree with Reimar, *buf++ seems more readable :)

> 
>>> +        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.
> 
> bytestream_get_le24() theres a buf+=3 there too

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.
> 
> good argument but you forget something, the whole format is based on reading
> lists of pointers to packets so this will not work on non seekable protocols
> anyway ...

Hmm.. does seeking forward work with http/ftp?

This one is the only seek backwards (usually ( = in every c93 file I've
seen) the packets in the file are in the playing order) and it could be
removed as I described in my reply to Reimar.

-- 
Anssi Hannula