[FFmpeg-devel] [PATCH] Complete rewrite of the "fps" video filter section. More accurate.

[Carl Eugen Hoyos]

Am Mo., 27. Apr. 2020 um 08:18 Uhr schrieb <list+ffmpeg-dev at jdlh.com>:

The suggested patch is currently not ok.

> From: Jim DeLaHunt <from+ffmpeg-dev at jdlh.com>
>
> This is a complete rewrite of the documentation for the "fps" video
> filter. It describes the filter's behaviour more clearly and accurately.
> I based the rewrite on reading the source code in vf_fps.c closely.
>
> No code, or other documentation files, are touched by this change.
>
> Signed-off-by: Jim DeLaHunt <from+ffmpeg-dev at jdlh.com>
> ---
>  doc/filters.texi | 167 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 149 insertions(+), 18 deletions(-)
>
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 71a6787289..bd4a1ad2a9 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -11139,27 +11139,34 @@ format=pix_fmts=yuv420p|yuv444p|yuv410p
>  @anchor{fps}
>  @section fps
>
> -Convert the video to specified constant frame rate by duplicating or dropping
> -frames as necessary.
> +Generate a video, having the specified constant frame rate, from the frames of

I am not saying that "convert" is ideal but "Generate a video" is simply wrong.

> +the input video, by copying or duplicating or dropping input frames based on

What is the difference between "copying" and "duplicating"?

> +their input presentation time stamps (PTSs). The output video has new PTSs. You
> +can choose the method for rounding from input PTS to output PTS.
>
>  It accepts the following parameters:
>  @table @option
>
>  @item fps
> -The desired output frame rate. The default is @code{25}.
> +The output frame rate, as a number of frames per second. This value can be an
> +integer, real, or rational number, or an abbreviation. The default is @code{25}.
>
>  @item start_time
> -Assume the first PTS should be the given value, in seconds. This allows for
> -padding/trimming at the start of stream. By default, no assumption is made
> -about the first frame's expected PTS, so no padding or trimming is done.
> -For example, this could be set to 0 to pad the beginning with duplicates of
> -the first frame if a video stream starts after the audio stream or to trim any
> -frames with a negative PTS.

> +The time, in seconds from the start of the input stream, which is converted to
> +an input starting PTS value and an output starting PTS value.

> +If set, @var{fps} drops input frames
> +which have PTS values less than the input starting PTS. If not set, the input
> +and output starting PTS values are zero, but @var{fps} drops no input frames based
> +on PTS.

This is different than the explanation above and (hopefully) wrong.

> +(See details below.)
>
>  @item round
> -Timestamp (PTS) rounding method.
> +Rounding method to use when calculating output Presentation Timestamp
> +(PTS) integer values from input PTS values. If the calculated output PTS value
> +is not exactly an integer, then the method determines which of the two
> +neighbouring integer values to choose.

I do not see any improvement with this change.

> -Possible values are:
> +Possible method names are:

Not being a native speaker, the change makes this harder to read.
("Verschlimmbesserung")

>  @table @option
>  @item zero
>  round towards 0
> @@ -11170,43 +11177,167 @@ round towards -infinity
>  @item up
>  round towards +infinity
>  @item near
> -round to nearest

> +round to nearest (and if exactly at midpoint, away from 0)

I wonder if this implementation detail should be documented.
Are you sure that the current behaviour is intended and must
not change?

>  @end table
>  The default is @code{near}.
>
>  @item eof_action
> -Action performed when reading the last frame.
> +Action which @var{fps} takes with the final input frame.

"Verschlimmbesserung", see above.

> The input video passes
> +in an ending input PTS, which @var{fps} converts to an ending output PTS.

> + at var{fps} drops any input frames with a PTS at or after this ending PTS.

What is this supposed to mean / isn't this wrong?

>  Possible values are:
>  @table @option
>  @item round
> -Use same timestamp rounding method as used for other frames.
> +Use same rounding method as for other frames, to convert the ending input PTS
> +to output PTS.

Useless change.

> +
>  @item pass
> -Pass through last frame if input duration has not been reached yet.
> +Round the ending input PTS using @code{up}. This can have the effect of passing
> +through one last input frame.
>  @end table
> +
>  The default is @code{round}.
>
>  @end table
>
> -Alternatively, the options can be specified as a flat string:
> +Alternatively, the options may be specified as a flat string:
>  @var{fps}[:@var{start_time}[:@var{round}]].

The remaining part is far too long and not acceptable.

Allow me to repeat: The patch is currently not ok.

Carl Eugen