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

From: Jim DeLaHunt <from+ffmpeg-dev@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@jdlh.com>
---
 doc/filters.texi | 167 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 149 insertions(+), 18 deletions(-)

On Sun, 26 Apr 2020, list+ffmpeg-dev@jdlh.com wrote:

> From: Jim DeLaHunt <from+ffmpeg-dev@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.

IMHO it is way too verbose now.

Regards,
Marton

>
> No code, or other documentation files, are touched by this change.
>
> Signed-off-by: Jim DeLaHunt <from+ffmpeg-dev@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 
> +the input video, by copying or duplicating or dropping input frames based on 
> +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. 
> +(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.
> 
> -Possible values are:
> +Possible method names are:
> @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)
> @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. The input video passes
> +in an ending input PTS, which @var{fps} converts to an ending output PTS. 
> +@var{fps} drops any input frames with a PTS at or after this ending PTS.
> 
> 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.
> +
> @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}]].
> 
> +@var{fps} generates an output video with integer Presentation Time Stamp (PTS) 
> +values which increment by one each output frame, and with a time base set to 
> +the inverse of the given frame rate. @var{fps} copies, duplicates, or drops 
> +input frames, in sequence, to the output video. It does so according to their 
> +input PTS values, as converted to seconds (via the input time base), then 
> +rounded to output PTS values. 
> +
> +@var{fps} sets output PTS values in terms of a time line which starts at
> +zero. The integer PTS value multipled by the output time base gives a point 
> +in seconds of that output frame on that timeline. If the @var{start_time} 
> +parameter is not set, or is zero, the first output frame's PTS value is zero. 
> +Otherwise, the first PTS value is the output starting PTS value calculated
> +from the @var{start_time} parameter. 
> +
> +@var{fps} interprets input PTS values in terms of the same time line. It 
> +multiplies the input PTS value by the input time base time, to get a frame 
> +position in seconds on the time line. It rounds that position to an integer 
> +output PTS value. For example, if the input video has a frame rate
> +of 30 fps, a time base of 1/30 seconds, and its first frame has a 
> +PTS of 300, then @var{fps} treats that frame as occurring 10 seconds (300 * 1/30 
> +seconds) after the start of the video, even though it is the first frame.
> +
> +Setting a @code{start_time} value allows for padding/trimming at the 
> +start of stream. For example, @code{start_time} 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. When 
> +@code{start_time} is not set, the @var{fps} filter makes no assumption about 
> +the first frame's expected PTS, and does not pad or trim input frames which 
> +have a PTS set.
> +
> See also the @ref{setpts} filter.
> 
> +@subsection Details
> +
> +@var{fps} emits exactly one frame for each output PTS value. If there is 
> +exactly one input frame with an input PTS which converts to the current output 
> +PTS value, @var{fps} emits that frame. If there are multiple frames which 
> +convert to the same output PTS value, @var{fps} emits the final frame of that 
> +group, and drops the previous frames. If no input frame PTS converts to a given 
> +output PTS value, @var{fps} emits another copy of the previously emitted frame. 
> +When the first input frame converts to an output PTS after the first frame, then 
> +@var{fps} emits copies of that first frame until the output PTS reaches the 
> +converted value of that first frame's input PTS. 
> +
> +@var{fps} always drops input frames which have no PTS value set, regardless
> +of the @var{start_time} parameter. 
> +
> +The @var{frame rate} value may be provided in a variety of forms. Each form is
> +converted into a rational number, with an integer numerator and denominator. 
> +Each value must be zero or greater.
> +
> +@itemize
> +
> +@item
> +An integer number, e.g. @code{25}. This converts to the rational number 
> +@code{25/1}.
> +
> +@item
> +A real number, e.g. @code{3.14145926}. This converts to a rational number, 
> +e.g. @code{954708/303893}
> +
> +@item
> +A rational number. The numerator and denominator may be either integers or real
> +numbers. e.g. @code{30/1.001} or @code{30000/1001}, which both convert to 
> +@code{30000/1001}.
> +
> +@item 
> +An abbreviation. e.g @code{ntsc} as @code{30000/1001}, 
> +@code{ntsc-film} as @code{24000/1001}. See the complete list at 
> +@ref{Video rate,,the "Video rate" section in the ffmpeg-utils(1) manual,ffmpeg-utils}.
> +
> +@end itemize
> +
> +@var{fps} defines a sync point, where one input PTS value is treated as occuring
> +at the same moment as one output PTS value. IT calcluates other PTS values
> +as positive or negative time offsets from this sync point. 
> +This affects the details of how rounding works. If @var{start_time} is set, 
> +then the input and output PTS values which @var{fps} calculates from this 
> +become the sync point. Otherwise, input and output PTS values of zero are 
> +the sync point.
> +
> +Note that @var{fps} does not assume that input frames are separated by exactly 
> +1/frame_rate seconds. It takes the input PTS values literally. If the increment 
> +of PTS between frames varies along the video, fps will treat those frames as
> +happening at varying time intervals. 
> +
> +As a consequence, if you have a video which is supposed to be at a certain frame 
> +rate, but in reality the frames were not always captured at the exactly right 
> +moment, and if the input PTS values reflect that variation, then you can pass 
> +this video through an @var{fps} filter set to the same frame rate. A @var{round} 
> +method of @code{near} will pass through each input frame exactly once, and 
> +the output video will have new PTS values which reflect the exact time interval 
> +(1/frame rate) between frames.
> +
> +Because @var{fps} treats input PTS values at face value when converting them to
> +time on the time line, and because that time line starts at zero, an input 
> +video with PTS values that do not start at zero might yield unexpected results.
> +Suppose the input PTS values start with the value 300, which converts to 10
> +seconds after the sync point. Then @var{fps} will repeat the first frame of the
> +input video to fill the first 10 seconds of the output video. (However, 
> +@command{ffmpeg} may suppress those repeated frames, depending on the 
> +@option{-vsync} setting.) If you set @var{start_time} to 10 seconds, then 
> +@var{fps} sets the sync point to the PTS values converted from the 10 second 
> +moment on the time line. It no longer repeats the first frame. And, it starts 
> +the output PTS at the value corresponding to 10 seconds, instead of zero.
> +
> @subsection Examples
> 
> @itemize
> @item
> -A typical usage in order to set the fps to 25:
> +A typical usage in order to emit a video with a frame rate of 25 frames per second:
> @example
> fps=fps=25
> @end example
> 
> @item
> -Sets the fps to 24, using abbreviation and rounding method to round to nearest:
> +Emit a video with a frame rate of 24 frames per second, using an abbreviation, 
> +and rounding method to round to nearest:
> @example
> fps=fps=film:round=near
> @end example
> +
> +@item
> +If an input video is supposed to have a frame rate of 29.97 frames per second
> +(NTSC standard), but the time base is 3003/90000, and the PTS values increment 
> +variably at slightly more and less than that rate, this emits a video with the 
> +same frames, but with a new time base and PTS values that increment at exactly 
> +the NTSC rate. If some frames were dropped by the recorder, but the PTS values 
> +still reflect when the remaining frames were captured, this will also repeat 
> +frames to fill the gaps from the dropped frames.
> +@example
> +fps=fps=30/1.001:round=near
> +@end example
> +
> @end itemize
> 
> @section framepack
> -- 
> 2.26.2
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

On 2020-04-27 01:04, Marton Balint wrote:

>
> On Sun, 26 Apr 2020, list+ffmpeg-dev@jdlh.com wrote:
>
>> From: Jim DeLaHunt <from+ffmpeg-dev@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.
>
> IMHO it is way too verbose now.
>
> Regards,
> Marton
>
I agree, there are a lot more words. On the other hand it now describes 
what the filter actually does, without forcing a user to read the code. 
(And the code isn't straightforward, there is an event-driven style to 
video filter code which takes a bit of learning.)

However, I'm confident it could be improved.

It would be useful for reviewers to comment on:

 1. Is there information which should be kept, but expressed with fewer
    words?
 2. Is there information which is true description of the filter, but
    which the documentation should not bother to disclose?

I'm of the opinion that FFmpeg users are impeded a lot more by necessary 
facts left out of documentation than by too many words left in.
>>
>> No code, or other documentation files, are touched by this change.
>>
>> Signed-off-by: Jim DeLaHunt <from+ffmpeg-dev@jdlh.com>
Cheers,

       —Jim DeLaHunt, software engineer, Vancouver, Canada

On 27/04/2020 07:17, list+ffmpeg-dev@jdlh.com wrote:
> From: Jim DeLaHunt <from+ffmpeg-dev@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@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
> +the input video, by copying or duplicating or dropping input frames based on
> +their input presentation time stamps (PTSs). The output video has new PTSs. You

Should just be `PTS'.

> +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}.
>   

I don't think this change adds any value here.

>   @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.
> +(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.
>   
> -Possible values are:
> +Possible method names are:

They are still just values for the option.

>   @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)
>   @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. The input video passes
> +in an ending input PTS, which @var{fps} converts to an ending output PTS.
> +@var{fps} drops any input frames with a PTS at or after this ending PTS.
>   
>   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.
> +
>   @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}]].
>   
> +@var{fps} generates an output video with integer Presentation Time Stamp (PTS)

You already specify what PTS is above.

> +values which increment by one each output frame, and with a time base set to
> +the inverse of the given frame rate. @var{fps} copies, duplicates, or drops
> +input frames, in sequence, to the output video. It does so according to their
> +input PTS values, as converted to seconds (via the input time base), then
> +rounded to output PTS values.
> +
> +@var{fps} sets output PTS values in terms of a time line which starts at
> +zero. The integer PTS value multipled by the output time base gives a point

multiplied

> +in seconds of that output frame on that timeline. If the @var{start_time}
> +parameter is not set, or is zero, the first output frame's PTS value is zero.
> +Otherwise, the first PTS value is the output starting PTS value calculated
> +from the @var{start_time} parameter.
> +
> +@var{fps} interprets input PTS values in terms of the same time line. It
> +multiplies the input PTS value by the input time base time, to get a frame
> +position in seconds on the time line. It rounds that position to an integer
> +output PTS value. For example, if the input video has a frame rate
> +of 30 fps, a time base of 1/30 seconds, and its first frame has a
> +PTS of 300, then @var{fps} treats that frame as occurring 10 seconds (300 * 1/30
> +seconds) after the start of the video, even though it is the first frame.
> +
> +Setting a @code{start_time} value allows for padding/trimming at the
> +start of stream. For example, @code{start_time} 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. When
> +@code{start_time} is not set, the @var{fps} filter makes no assumption about
> +the first frame's expected PTS, and does not pad or trim input frames which
> +have a PTS set.
> +
>   See also the @ref{setpts} filter.
>   
> +@subsection Details
> +
> +@var{fps} emits exactly one frame for each output PTS value. If there is
> +exactly one input frame with an input PTS which converts to the current output
> +PTS value, @var{fps} emits that frame. If there are multiple frames which
> +convert to the same output PTS value, @var{fps} emits the final frame of that
> +group, and drops the previous frames. If no input frame PTS converts to a given
> +output PTS value, @var{fps} emits another copy of the previously emitted frame.
> +When the first input frame converts to an output PTS after the first frame, then
> +@var{fps} emits copies of that first frame until the output PTS reaches the
> +converted value of that first frame's input PTS.
> +
> +@var{fps} always drops input frames which have no PTS value set, regardless
> +of the @var{start_time} parameter.
> +
> +The @var{frame rate} value may be provided in a variety of forms. Each form is
> +converted into a rational number, with an integer numerator and denominator.
> +Each value must be zero or greater.
> +
> +@itemize
> +
> +@item
> +An integer number, e.g. @code{25}. This converts to the rational number
> +@code{25/1}.
> +
> +@item
> +A real number, e.g. @code{3.14145926}. This converts to a rational number,
> +e.g. @code{954708/303893}
> +
> +@item
> +A rational number. The numerator and denominator may be either integers or real
> +numbers. e.g. @code{30/1.001} or @code{30000/1001}, which both convert to
> +@code{30000/1001}.
> +
> +@item
> +An abbreviation. e.g @code{ntsc} as @code{30000/1001},
> +@code{ntsc-film} as @code{24000/1001}. See the complete list at
> +@ref{Video rate,,the "Video rate" section in the ffmpeg-utils(1) manual,ffmpeg-utils}.
> +
> +@end itemize
> +
> +@var{fps} defines a sync point, where one input PTS value is treated as occuring

occurring

> +at the same moment as one output PTS value. IT calcluates other PTS values

calculates

> +as positive or negative time offsets from this sync point.
> +This affects the details of how rounding works. If @var{start_time} is set,
> +then the input and output PTS values which @var{fps} calculates from this
> +become the sync point. Otherwise, input and output PTS values of zero are
> +the sync point.
> +
> +Note that @var{fps} does not assume that input frames are separated by exactly
> +1/frame_rate seconds. It takes the input PTS values literally. If the increment
> +of PTS between frames varies along the video, fps will treat those frames as
> +happening at varying time intervals.
> +
> +As a consequence, if you have a video which is supposed to be at a certain frame
> +rate, but in reality the frames were not always captured at the exactly right
> +moment, and if the input PTS values reflect that variation, then you can pass
> +this video through an @var{fps} filter set to the same frame rate. A @var{round}
> +method of @code{near} will pass through each input frame exactly once, and
> +the output video will have new PTS values which reflect the exact time interval
> +(1/frame rate) between frames.
> +
> +Because @var{fps} treats input PTS values at face value when converting them to
> +time on the time line, and because that time line starts at zero, an input
> +video with PTS values that do not start at zero might yield unexpected results.
> +Suppose the input PTS values start with the value 300, which converts to 10
> +seconds after the sync point. Then @var{fps} will repeat the first frame of the
> +input video to fill the first 10 seconds of the output video. (However,
> +@command{ffmpeg} may suppress those repeated frames, depending on the
> +@option{-vsync} setting.) If you set @var{start_time} to 10 seconds, then
> +@var{fps} sets the sync point to the PTS values converted from the 10 second
> +moment on the time line. It no longer repeats the first frame. And, it starts
> +the output PTS at the value corresponding to 10 seconds, instead of zero.
> +
>   @subsection Examples
>   
>   @itemize
>   @item
> -A typical usage in order to set the fps to 25:
> +A typical usage in order to emit a video with a frame rate of 25 frames per second: >
>   @example
>   fps=fps=25
>   @end example
>   
>   @item
> -Sets the fps to 24, using abbreviation and rounding method to round to nearest:
> +Emit a video with a frame rate of 24 frames per second, using an abbreviation,
> +and rounding method to round to nearest:
>   @example
>   fps=fps=film:round=near
>   @end example
> +
> +@item
> +If an input video is supposed to have a frame rate of 29.97 frames per second
> +(NTSC standard), but the time base is 3003/90000, and the PTS values increment
> +variably at slightly more and less than that rate, this emits a video with the
> +same frames, but with a new time base and PTS values that increment at exactly
> +the NTSC rate. If some frames were dropped by the recorder, but the PTS values
> +still reflect when the remaining frames were captured, this will also repeat
> +frames to fill the gaps from the dropped frames.
> +@example
> +fps=fps=30/1.001:round=near
> +@end example
> +
>   @end itemize
>   
>   @section framepack
> 

Just a few nitpicks.

Also your usage of `emit' may be confusing to non-native English 
speakers and I don't think your writing has those people in mind 
particularly. I would just s/emit/output/ for the most part (check first 
this works).

Like Marton said, it's probably a bit on the verbose side, just have a 
read over and see if you can say the same thing with less words. I don't 
think being verbose is necessarily bad either, as you said there's less 
chance that a user would have to end up looking into code to know how to 
use it.

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

The suggested patch is currently not ok.

> From: Jim DeLaHunt <from+ffmpeg-dev@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@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.

> +@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

On 2020-04-28 07:33, Josh de Kock wrote:
> On 27/04/2020 07:17, list+ffmpeg-dev@jdlh.com wrote:
>> From: Jim DeLaHunt <from+ffmpeg-dev@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.
>>
Thank you for your review, Josh. The patch will be the better for your 
comments.

My responses interleaved.

>>   @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
>> +the input video, by copying or duplicating or dropping input frames 
>> based on
>> +their input presentation time stamps (PTSs). The output video has 
>> new PTSs. You
>
> Should just be `PTS'.

This brings up that I expand "PTS" to "presentation time stamp" three 
times in this patch, and my spacing and capitalisation are inconsistent 
across those three cases. I have deleted the later two expansions, and 
reduced my word count.

But this is the first use of the acronym PTS, and I think it is 
important to spell it out. If the FFmpeg documentation had a Glossary, I 
would like to point to "PTS" there. If there was a Concepts Guide that 
explained PTS, I could point readers there. But FFmpeg has neither. 
Without them, using acronyms without definition presents an obstacle to 
the reader which I think is unacceptable.

My scope is limited to this one section. I wish to spell out the acronym 
here. Later, if someone adds a Glossary or a Concepts Guide, then 
clean-up from that can include removing the expansion of PTS here. But 
not until then.

Yes, applying this principle would result in a lot more acronyms being 
expanded. This would be an improvement for FFmpeg users just trying to 
get their work done.


>> +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}.
>
> I don't think this change adds any value here.

The first sentence defines what "frame rate" means. It is valuable here 
because of the lack of a Glossary. FFmpeg should, somewhere, define what 
"frame rate" means.  The existing language fails to define the term 
"frame rate".

FFmpeg should say that frame rate is per second, not per minute. It 
should say that frame rate counts frames per unit time, not time per 
frame (1/25). It is not a sin for documentation to state the obvious, 
because readers new to FFmpeg and its jargon will find everything to be 
obvious.

The second sentence is a quick summary of what types the value of the 
parameter may take. The existing language fails to do this. The types 
are fleshed out below, in the parameters table.

>>   @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.
>> +(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.
>>   -Possible values are:
>> +Possible method names are:
>
> They are still just values for the option.

I think this is a wording style point.  This section uses the word 
"value" to indicate numerical quantities, and the strings passed to the 
`round` parameter are not numerical. It seems clearer to use a different 
word, and to tie it to "method". But if this is all that stands in the 
way of getting the patch accepted, I will concede.

>>   @table @option
…[snip]…
>>   -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}]].
>>   +@var{fps} generates an output video with integer Presentation Time 
>> Stamp (PTS)
>
> You already specify what PTS is above.
Agreed. I don't think it's harmful, but people want me to reduce my word 
count. Expansion deleted.
>> +values which increment by one each output frame, and with a time 
>> base set to
>> +the inverse of the given frame rate. @var{fps} copies, duplicates, 
>> or drops
>> +input frames, in sequence, to the output video. It does so according 
>> to their
>> +input PTS values, as converted to seconds (via the input time base), 
>> then
>> +rounded to output PTS values.
>> +
>> +@var{fps} sets output PTS values in terms of a time line which 
>> starts at
>> +zero. The integer PTS value multipled by the output time base gives 
>> a point
>
> multiplied … occurring … calculates …

Thank you for pointing out these spelling errors. Corrected.

…[snip]…

>> +
>>   @end itemize
>>     @section framepack
>>
>
> Just a few nitpicks.
Thank you.
>
> Also your usage of `emit' may be confusing to non-native English 
> speakers and I don't think your writing has those people in mind 
> particularly. I would just s/emit/output/ for the most part (check 
> first this works).
I see your point. I need to consider which verbs I use, and see if I can 
choose clearer ones.
> Like Marton said, it's probably a bit on the verbose side, just have a 
> read over and see if you can say the same thing with less words. I 
> don't think being verbose is necessarily bad either, as you said 
> there's less chance that a user would have to end up looking into code 
> to know how to use it.

Thank you. I will submit a revised patch, with fewer words.

Best regards,
       —Jim DeLaHunt, software engineer, Vancouver Canada

On 2020-04-28 08:50, Carl Eugen Hoyos wrote:

> Am Mo., 27. Apr. 2020 um 08:18 Uhr schrieb <list+ffmpeg-dev@jdlh.com>:
>
> The suggested patch is currently not ok.
Thank you for your review, Carl Eugen. The patch will be the better for 
your comments.

My responses interleaved.

…[snip]…

>> -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.

I will look at all the word choices for verbs and see if I can make this 
clearer.

The word "generate" was a response to the the original text using the 
word "convert". "Convert" gave me the impression that the filter might 
just change time base and frame rate metadata parameters, but leave the 
rest unchanged. The *fps* filter does more than that. It alters the PTS 
values. It can add frames. It can drop frames. To my reading the output 
video is a new thing, not the original thing with some tweaks.

>> +the input video, by copying or duplicating or dropping input frames based on
> What is the difference between "copying" and "duplicating"?

Good point. I will look harder at these word choices.

What I am trying to convey is that the filter can do one of three things 
to the input frames: pass them through, or drop some of them, or repeat 
some of them. I will look for words which communicate this three-way path.

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

I based my new text on reading the code. Take a look for yourself: 
ffmpeg/libavfilter/vf_fps.c:159-174 
<https://github.com/FFmpeg/FFmpeg/blob/1128aa875367f66ac11adc30364d5652919a2591/libavfilter/vf_fps.c#L159-L174>, 
within config_props(), and ffmpeg/libavfilter/vf_fps.c:195-204 
<https://github.com/FFmpeg/FFmpeg/blob/1128aa875367f66ac11adc30364d5652919a2591/libavfilter/vf_fps.c#L195-L204>, 
within read_frame().

I think the first sentence of the old text, "Assume the first PTS should 
be the given value", doesn't match what the code does. The code does not 
include a clear high-level statement of what the parameter does. I think 
that means we have to take the implementation as correct, and document 
what it actually does.

The rest of the old text is actually a guide on how to use the 
`start_time` parameter. I kept that, but moved it down to after the 
parameter definition table.

>> +(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.

The old text fails to answer these questions: what is being rounded? 
What is the calculation which leads to the result being rounded? What is 
the frame of reference for "towards 0", "towards infinity"? What 
possible values does the rounding choose among? The new text tries to 
answer those questions.

There may be a better way to answer these questions. I will try to think 
of better wording. But I think the documentation should provide answers.

>> -Possible values are:
>> +Possible method names are:
> Not being a native speaker, the change makes this harder to read.
> ("Verschlimmbesserung")
As I replied to Josh, I think this is a wording style point.  This 
section uses the word "value" to indicate numerical quantities, and the 
strings passed to the `round` parameter are not numerical. It seems 
clearer to use a different word, and to tie it to "method". But if this 
is all that stands in the way of getting the patch accepted, I will concede.
>>   @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?

Again, the code does not provide a high-level statement of what it 
intends. It does not separate essence of interface from accident of 
implementation. However, one clue is that the `near` value for `round` 
corresponds to AV_ROUND_NEAR_INF, which is defined in 
ffmpeg/libavutil/mathematics.c:84 
<https://github.com/FFmpeg/FFmpeg/blob/a0ac49e38ee1d1011c394d7be67d0f08b2281526/libavutil/mathematics.h#L84>, 
where it says:

AV_ROUND_NEAR_INF = 5, ///< Round to nearest and halfway cases away from zero.

So, is that comment in *mathematics.c* an interface specification?  Is 
the use of that value in *vf_fps.c* an interface specification? It sure 
would be nice if the code had a high-level statement of intent. But it 
seems not to.

I don't feel strongly about this point. If you all tell me to leave this 
halfway case behaviour out of the documentation, I can live with that.

>>   @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 new text makes two changes to the old:

1. The new text is clear that the last /input/ frame, not output frame, 
is what the parameter controls. The old text is ambiguous.

2. The new text uses the active voice ("@var{fps} takes"). The old text 
uses "when" plus a present progressive clause. The style guides I 
remember say that active voice is more lively than indirect 
constructions. In any case, it looks like the old text should have used 
"while" instead of "when" 
<https://english.stackexchange.com/a/17318/33109>.  (Thank you for 
leading me to a discussion of subject complements, by the way.)

I care about #1, because I want the documentation to say what the filter 
does. I am flexible on #2.

>
>> The input video passes
>> +in an ending input PTS, which @var{fps} converts to an ending output PTS.
>> +@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?

I guess the wording is unclear. The code is even more unclear.

Take a look: ffmpeg/libavfilter/vf_fps.c:234-245 
<https://github.com/FFmpeg/FFmpeg/blob/1128aa875367f66ac11adc30364d5652919a2591/libavfilter/vf_fps.c#L234-L245>, 
within write_frame(). There is a comment, "If we have status (EOF) set, 
drop frames when we hit the status timestamp." I have a hard time 
relating it to what the code does.

If you can clarify what the code a) actually does, and b) is supposed to 
do, I am glad to try to describe that more clearly.

>>   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.

"Useless"? No, it restates for clarity what the rounding operates on.

However, "not very useful, and not worth the cost in extra words"?  
Maybe so. I will take another look at these words when I edit to reduce 
word count.

>> +
>>   @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.

By "the remaining part" I understand you to mean the remaining three 
paragraphs in the main section, and the entire subsection "Details". I 
presume you still want to keep the "Examples" subsection.

I think what I put in those paragraphs is all true. It all accurately 
describes the code. It all explains points which I found to be necessary 
to understand as I was trying, as a user, to figure out if the *fps* 
filter would accomplish a purpose I wanted to do.

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

Your earlier comments are about specific points. Some I agree with, some 
I disagree with. But I can imagine working with you to resolve them.

Your final comment, "The remaining part is far too long and not 
acceptable", comes across as a rejection of the very idea of improving 
*FFmpeg* documentation.

*FFmpeg* is a complex tool. Even apparently simple filters are complex. 
The present documentation is inadequate to explain to a user how the 
code behaves. This presents a big obstacle for *FFmpeg* users. Traffic 
on the ffmpeg-users list proves this constantly.

What do you have against removing this obstacle? What do you have 
against documentation which describes what the code actually does?

I will go through all these comments and make a revised patch. Let's see 
how that looks.

> Carl Eugen
Best regards,

      —Jim DeLaHunt, software engineer, Vancouver, Canada

Thank you for the review. Here is a rewrite of the improvement to the fps filter
documentation, based on those comments.  Thought it was too wordy before? It now 
has 11% fewer words.                             

To confirm, no other documentation and no code are affected by this patch.

Comments?
    —Jim DeLaHunt, software engineer, Vancouver, Canada