[FFmpeg-devel] [PATCH] add ivf muxer

[Reimar Döffinger]

On Tue, Dec 14, 2010 at 10:24:06AM +0100, Tomas H?rdin wrote:
> On Mon, 2010-12-13 at 20:44 +0100, Reimar D?ffinger wrote:
> > +static int ivf_write_header(AVFormatContext *s)
> > +{
> > +    AVCodecContext *ctx;
> > +    ByteIOContext *pb = s->pb;
> > +
> > +    if (s->nb_streams != 1) {
> > +        av_log(s, AV_LOG_ERROR, "Format supports only exactly one
> > video stream\n");
> 
> "only supports" is perhaps more gramatically correct?

I don't think so, not when it's used with numbers
(more precisely, "only exactly" belong to "one" so
you can't split them apart - though I guess for the
"only" you could argue about that).
Though a native speaker probably knows better.

> > +        return AVERROR(EINVAL);
> > +    }
> > +    ctx = s->streams[0]->codec;
> > +    if (ctx->codec_type != CODEC_TYPE_VIDEO || ctx->codec_id !=
> > CODEC_ID_VP8) {
> > +        av_log(s, AV_LOG_ERROR, "Currently only VP8 is supported!
> > \n");
> 
> Does IVF support any other formats at all? Is there a spec?
> The demuxer seems to imply that there's support for all AVI FOURCCs, but
> maybe that's just code sharing and being a little tolerant. 

I think it at least "supports" VP7 and it might be used
for a VP8.1 or VP9 as well.
It definitely is possible to support a lot more formats in
principle, just seems like a bad idea...


> > +    put_le64(pb, s->streams[0]->duration); // TODO: duration or
> > number of frames?!?
> 
> This sort of depends on whether VP8/IVF supports VFR, right? The demuxer
> doesn't give many hints, but with CFR you'll have duration == number of
> frames. Hm.

Which means it does not at all depend on whether it supports VFR.
If it doesn't it is as you said completely irrelevant, so going with
this (simpler) solution is perfectly fine.
My assumption is though that it does.

> > +AVOutputFormat ivf_muxer = {
> > +    "ivf",
> > +    NULL_IF_CONFIG_SMALL("On2 IVF"),
> > +    "",
> > +    "ivf",
> > +    0,
> > +    CODEC_ID_NONE,
> > +    CODEC_ID_VP8,
> > +    ivf_write_header,
> > +    ivf_write_packet,
> 
> Use named initializers for all of these?
> 
> > +};
> 
> Maybe add some documentation, and a Changelog entry,

Changelog seems a bit overkill, I doubt anyone would care?
But yes I'll update the docs.