diff mbox

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

Message ID 1506005752-14650-1-git-send-email-t.rapp@noa-archive.com
State Superseded
Headers show

Commit Message

Tobias Rapp Sept. 21, 2017, 2:55 p.m. UTC
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@noa-archive.com>
---
 doc/filters.texi     |  4 ++--
 libavfilter/vf_fps.c | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

Comments

Michael Niedermayer Sept. 21, 2017, 11:58 p.m. UTC | #1
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@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

[...]
Tobias Rapp Sept. 22, 2017, 6:52 a.m. UTC | #2
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@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.

Regards,
Tobias
Michael Niedermayer Sept. 23, 2017, 3:05 p.m. UTC | #3
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@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)

[...]
Tobias Rapp Sept. 25, 2017, 9:51 a.m. UTC | #4
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@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
diff mbox

Patch

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 }
 };