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

[Måns Rullgård]

Reimar D?ffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> writes:

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

The avctx->width/height should be set here for the benefit of the
(hypothetical) users of this decoder not using lavf.

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

The const bit is fine.

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

Actually, I'm thinking the bytestream reader could be used throughout.

-- 
M?ns Rullg?rd
mans at mansr.com