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

[Michael Niedermayer]

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

:)))))))
sadly ive written my review without checking mails so i didnt realize
i was doing half redundant work :)


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

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


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


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

PS: thanks for the reviewing help, its appreciated and might become
even more so if 10 people send a patch on the last day, after all
we must select students before googles deadlines ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070402/425e52ac/attachment.pgp>