[FFmpeg-devel] [PATCH v4] avfilter/setpts: add option to decide framerate handling

[Ronald S. Bultje]

Hi,

On Sun, Jan 26, 2025 at 2:32 PM Marton Balint <cus at passwd.hu> wrote:

>
>
> On Sun, 26 Jan 2025, Gyan Doshi wrote:
>
> >
> >
> > On 2025-01-26 07:09 pm, Ronald S. Bultje wrote:
> >>  Hi,
> >>
> >>  On Sat, Jan 25, 2025 at 11:06 PM Gyan Doshi <ffmpeg at gyani.pro> wrote:
> >>
> >>>
> >>>  On 2025-01-26 12:49 am, Marton Balint wrote:
> >>>>
> >>>>  On Sat, 25 Jan 2025, Gyan Doshi wrote:
> >>>>
> >>>>>  In f121d95, the outlink framerate was unconditionally unset.
> >>>>>  This breaks/bloats outputs from CFR muxers unless the user
> explicitly
> >>>>>  sets a sane framerate. And the most common invocation for setpts
> seen
> >>>>>  in
> >>>>>  workflows, our docs and across the web is `PTS-STARTPTS` or others
> of
> >>>>>  the
> >>>>>  general form `PTS+constant` which preserves the input framerate.
> >>>>>
> >>>>>  Fixes #11428
> >>>>>  ---
> >>>>>  v4: negated option sense and renamed to vfr
> >>>>>
> >>>>>  doc/filters.texi          | 6 ++++++
> >>>>>  libavfilter/setpts.c      | 6 +++++-
> >>>>>  tests/fate/hevc.mak       | 2 +-
> >>>>>  tests/fate/mov.mak        | 2 +-
> >>>>>  tests/filtergraphs/setpts | 2 +-
> >>>>>  5 files changed, 14 insertions(+), 4 deletions(-)
> >>>>>
> >>>>>  diff --git a/doc/filters.texi b/doc/filters.texi
> >>>>>  index b926b865ae..ea11d045ec 100644
> >>>>>  --- a/doc/filters.texi
> >>>>>  +++ b/doc/filters.texi
> >>>>> @ @ -31478,6 +31478,12 @@ This filter accepts the following options:
> >>>>> @ item expr
> >>>>>  The expression which is evaluated for each frame to construct its
> >>>>>  timestamp.
> >>>>>
> >>>>>  + at item vfr (@emph{video only})
> >>>>>  +Boolean option which determines if the original framerate metadata
> >>>>>  is unset.
> >>>>>  +If set to true, be advised that a sane frame rate should be
> explicitly
> >>>>>  +specified if output is sent to a constant frame rate muxer.
> >>>>  I propose a more understandable variant for the first sentence:
> >>>>
> >>>>  Sets the filter output to variable frame rate by dropping the
> original
> >>>>  constant framerate information if present. If set to true...
> >>>  But that's not actually the case. This option does not make the output
> >>>  VFR. If it did, this option would not be needed.
> >>>  Once FR is unset, if the output goes to a CFR muxer and fps_mode is
> not
> >>>  specified, ffmpeg will emit a  CFR stream using the time_base.
> >>>  What the option does is a single narrow technical thing which has
> >>>  implications depending on context. And that was the basis for the
> >>>  original option name and description.
> >>>
> >>>  'strip_fps' and a corresponding description seems more accurate.
> >>
> >>  (I'm the bug reporter.) I actually agree. Setting FPS doesn't mean
> CFR, it
> >>  could just mean average frame rate or something. But please decide on
> >>  something, we're currently stripping said metadata and there's no way
> to
> >>  reinstate it except with lengthy hacks that we don't want people to
> start
> >>  posting on stackoverflow - because then we'll never hear the end of it.
> >
> > Ok, I'll change to strip_fps and push tomorrow (~18h) if there are no
> > objections.
>
> Ok, "strip_fps" is fine by me.


Thanks. Does anyone object to backporting this to 7.1, where xpsnr was
introduced?

Thanks,
Ronald