[FFmpeg-devel] [PATCH 1/2] avfilter/vf_fps: clean-up filter options

[Tobias Rapp]

On 23.09.2017 17:05, Michael Niedermayer wrote:
> On Fri, Sep 22, 2017 at 08:52:26AM +0200, Tobias Rapp wrote:
>> On 22.09.2017 01:58, Michael Niedermayer wrote:
>>> On Thu, Sep 21, 2017 at 04:55:51PM +0200, Tobias Rapp wrote:
>>>> Align order of "start_time" option to documentation and add missing
>>>> AV_OPT_FLAG_FILTERING_PARAM flag. Fix indent of "round" named constants
>>>> and clear unused field values.
>>>>
>>>> Also fix some documentation cosmetics.
>>>>
>>>> Signed-off-by: Tobias Rapp <t.rapp at noa-archive.com>
>>>> ---
>>>>   doc/filters.texi     |  4 ++--
>>>>   libavfilter/vf_fps.c | 12 ++++++------
>>>>   2 files changed, 8 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/doc/filters.texi b/doc/filters.texi
>>>> index 830de54..96b3dfd 100644
>>>> --- a/doc/filters.texi
>>>> +++ b/doc/filters.texi
>>>> @@ -8670,12 +8670,12 @@ It accepts the following parameters:
>>>>   The desired output frame rate. The default is @code{25}.
>>>>   @item round
>>>> -Rounding method.
>>>> +Timestamp (PTS) rounding method.
>>>>   Possible values are:
>>>>   @table @option
>>>>   @item zero
>>>> -zero round towards 0
>>>> +round towards 0
>>>>   @item inf
>>>>   round away from 0
>>>>   @item down
>>>> diff --git a/libavfilter/vf_fps.c b/libavfilter/vf_fps.c
>>>> index 1e5d07e..0db2b48 100644
>>>> --- a/libavfilter/vf_fps.c
>>>> +++ b/libavfilter/vf_fps.c
>>>> @@ -65,13 +65,13 @@ typedef struct FPSContext {
>>>>   #define F AV_OPT_FLAG_FILTERING_PARAM
>>>>   static const AVOption fps_options[] = {
>>>>       { "fps", "A string describing desired output framerate", OFFSET(framerate), AV_OPT_TYPE_VIDEO_RATE, { .str = "25" }, 0, INT_MAX, V|F },
>>>> -    { "start_time", "Assume the first PTS should be this value.", OFFSET(start_time), AV_OPT_TYPE_DOUBLE, { .dbl = DBL_MAX}, -DBL_MAX, DBL_MAX, V },
>>>>       { "round", "set rounding method for timestamps", OFFSET(rounding), AV_OPT_TYPE_INT, { .i64 = AV_ROUND_NEAR_INF }, 0, 5, V|F, "round" },
>>>> -    { "zero", "round towards 0",      OFFSET(rounding), AV_OPT_TYPE_CONST, { .i64 = AV_ROUND_ZERO     }, 0, 5, V|F, "round" },
>>>> -    { "inf",  "round away from 0",    OFFSET(rounding), AV_OPT_TYPE_CONST, { .i64 = AV_ROUND_INF      }, 0, 5, V|F, "round" },
>>>> -    { "down", "round towards -infty", OFFSET(rounding), AV_OPT_TYPE_CONST, { .i64 = AV_ROUND_DOWN     }, 0, 5, V|F, "round" },
>>>> -    { "up",   "round towards +infty", OFFSET(rounding), AV_OPT_TYPE_CONST, { .i64 = AV_ROUND_UP       }, 0, 5, V|F, "round" },
>>>> -    { "near", "round to nearest",     OFFSET(rounding), AV_OPT_TYPE_CONST, { .i64 = AV_ROUND_NEAR_INF }, 0, 5, V|F, "round" },
>>>> +        { "zero", "round towards 0",                 0, AV_OPT_TYPE_CONST, { .i64 = AV_ROUND_ZERO     }, 0, 0, V|F, "round" },
>>>> +        { "inf",  "round away from 0",               0, AV_OPT_TYPE_CONST, { .i64 = AV_ROUND_INF      }, 0, 0, V|F, "round" },
>>>> +        { "down", "round towards -infty",            0, AV_OPT_TYPE_CONST, { .i64 = AV_ROUND_DOWN     }, 0, 0, V|F, "round" },
>>>> +        { "up",   "round towards +infty",            0, AV_OPT_TYPE_CONST, { .i64 = AV_ROUND_UP       }, 0, 0, V|F, "round" },
>>>> +        { "near", "round to nearest",                0, AV_OPT_TYPE_CONST, { .i64 = AV_ROUND_NEAR_INF }, 0, 0, V|F, "round" },
>>>> +    { "start_time", "Assume the first PTS should be this value.", OFFSET(start_time), AV_OPT_TYPE_DOUBLE, { .dbl = DBL_MAX}, -DBL_MAX, DBL_MAX, V|F },
>>>>       { NULL }
>>>
>>> This breaks shorthand notation like fps=30:-0.01
>>
>> The order of options according to the documentation is
>> [fps]:[round]:[start_time] (see
>> https://ffmpeg.org/ffmpeg-filters.html#fps). It even explicitly
>> says:
>>
>> "Alternatively, the options can be specified as a flat string: fps[:round]."
>>
>> But if preferred I can update the documentation instead.
> 
> Whichever makes more users happy.
> 
> (we maybe should avoid 2 different implementation behaviours if we
>   dont already have 2, i have not checked if we had a different order in
>   the code in the past)

Looking at the history of vf_fps.c apparently the current order of 
options exists since August 2013 (commit 
b69b075ac607419a840da798b089de8ea7630d4b). So it might be better to just 
update documentation to the established implementation.

Regards,
Tobias