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

[Reimar Döffinger]

Hello,
On Mon, Apr 02, 2007 at 01:20:43AM +0200, Michael Niedermayer wrote:
> On Sun, Apr 01, 2007 at 11:10:34PM +0200, Reimar D?ffinger wrote:
> > 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 ...

Well, ideally we wouldn't have to use them per pixel. Seems to be rather
hard in this case though.

> a #define WIDTH 320 would be fine though if you like

Would definitely make it easier to change to variables if needed.
But admittedly it's unlikely that will ever be needed, somewhen the time
will come when I will accept that generalizing code does not always make
sense ;-)

> > > +    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 don't care in the sense that I want it changed, it is just a
suggestion. Sometime people just aren't aware of the alternatives, and I
think *buf++ is the more common construct in ffmpeg.

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

No, I didn't miss that point, I just did not know if/how well the other
seeking could be avoided, so I commented on the obvious case.
But regardless of that part, there is quite a bit of seeking back and
forth in this code which makes it also harder to understand what the
code does IMO, so this is a part of code that might be worth thinking
about again and seeing if there's a simpler solution.
Like passing video and palette around as is. But that would require
either memcpying things around or resizing the AVPacket.
Is resizing the AVPacket officially supported? If yes, how is it
supposed to be done? realloc + change size variable? Or could we need a
new function for that?

Greetings,
Reimar D?ffinger