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

[Reimar Döffinger]

Hello,
On Sun, Apr 01, 2007 at 10:26:37PM +0100, M?ns Rullg?rd wrote:
> Reimar D?ffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> writes:
> > 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.

Well, whichever way IMO this should be set only in one place. Where
doesn't matter, unless other resolutions might be possible, then it
should be set where this info is most likely to be found/processed.

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

Yes, forget that part. Too sleepy. The cast and the () as Michael also
said are not needed though.

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

Probably, there seems to be quite a bit of simplification opportunities,
also for the boundary checks.

Greetings,
Reimar D?ffinger