[FFmpeg-devel] [PATCH v2] avformat/ivfenc: Set the "number of frames" in IVF header

[Ronald S. Bultje]

Hi,

On Thu, Jun 29, 2023 at 5:44 AM Anton Khirnov <anton at khirnov.net> wrote:

> Quoting Dai, Jianhui J (2023-06-29 08:03:18)
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> > > Anton Khirnov
> > > Sent: Wednesday, June 28, 2023 11:25 PM
> > > To: ffmpeg-devel at ffmpeg.org
> > > Subject: Re: [FFmpeg-devel] [PATCH v2] avformat/ivfenc: Set the
> "number of
> > > frames" in IVF header
> > >
> > > Quoting Dai, Jianhui J (2023-06-05 02:53:35)
> > > > diff --git a/libavformat/ivfdec.c b/libavformat/ivfdec.c index
> > > > 511f2387ed..01012db948 100644
> > > > --- a/libavformat/ivfdec.c
> > > > +++ b/libavformat/ivfdec.c
> > > > @@ -53,6 +53,7 @@ static int read_header(AVFormatContext *s)
> > > >      st->codecpar->height     = avio_rl16(s->pb);
> > > >      time_base.den         = avio_rl32(s->pb);
> > > >      time_base.num         = avio_rl32(s->pb);
> > > > +    // Infer duration from "number of frames".
> > > >      st->duration          = avio_rl32(s->pb);
> > >
> > > This should be setting st->nb_frames then rather than duration.
> > > And the muxer should be using that field as well instead of its custom
> version.
> >
> > ACK.
> > Do you suggest letting `duration` unset?
> > It is interesting that the 'duration' is often right in this way, if
> > the time_base.den/time_base.num == fps which is the popular
> > configuration.
>
> Yes, but AFAIU there is no way to tell whether the file is CFR, so then
> the value would be unreliable. So I'd prefer to leave it unset.
>

I see this discussion now...

I don't think I agree with the above. First of all, IVF has two fields
there (it seems): duration, and n_frames. We have the ability to set both
in the muxer, and therefore should set both. Setting only once was silly,
but changing it to set only the other is not better. Similarly, the demuxer
should set both. I think it's particularly bad that we change a muxer that
has been setting a particular field for years (and the accompanying demuxer
code to the the AVStream member) to suddenly not set that field/member
anymore. This is technically a regression.

Ronald