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

[Jim DeLaHunt]

On 2020-04-28 07:33, Josh de Kock wrote:
> On 27/04/2020 07:17, list+ffmpeg-dev at jdlh.com wrote:
>> 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.
>>
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}]].
>>   + at 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.
>> +
>> + at 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