diff mbox series

[FFmpeg-devel,25/38] avfilter/avfilter: Honour the short options documentation

Message ID AM7PR03MB66604B53A4724D559CAB8E038FD89@AM7PR03MB6660.eurprd03.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,01/39] avfilter/vf_maskedminmax: Simplify init
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 fail Make fate failed
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Andreas Rheinhardt Sept. 12, 2021, 10:53 a.m. UTC
The documentation for filter arguments states that short options must
precede long options (i.e. those of the form key=value). Yet if
process_options() encounters arguments not abiding by this, it simply
treats short options after a long option as if it were parsing short
options for the first time. In particular, it overwrites options already
set earlier, possibly via other short options. This is not how it is
intended (as a comment in the code indicates).

This commit modifies the code to reject further shorthand options
after a long option has been encountered. After all, avfilter_init_str()
errors out upon unrecognized options, so it is intended to be picky.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
The while loop that is removed below is actually just an elaborate
"o = NULL", which av_opt_next() interprets as "start the iteration".

 libavfilter/avfilter.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Paul B Mahol Sept. 13, 2021, 11:55 a.m. UTC | #1
lgtm
maybe add log messages about ignored stuff
Andreas Rheinhardt Sept. 13, 2021, noon UTC | #2
Paul B Mahol:
> lgtm
> maybe add log messages about ignored stuff
> 

avfilter_init_str() already contains a log message about ignored stuff;
but it is currently dead code, because process_options() errors out if
it encounters an unknown option. That way only the first unrecognized
option will be printed.

This patch meanwhile is not about ignoring stuff at all; instead it
rejects (i.e. errors out) shorthand options after a non-shorthand option
has been encountered.

- Andreas
Nicolas George Sept. 13, 2021, 12:09 p.m. UTC | #3
Andreas Rheinhardt (12021-09-12):
> The documentation for filter arguments states that short options must
> precede long options (i.e. those of the form key=value). Yet if
> process_options() encounters arguments not abiding by this, it simply
> treats short options after a long option as if it were parsing short
> options for the first time. In particular, it overwrites options already
> set earlier, possibly via other short options. This is not how it is
> intended (as a comment in the code indicates).
> 
> This commit modifies the code to reject further shorthand options
> after a long option has been encountered. After all, avfilter_init_str()
> errors out upon unrecognized options, so it is intended to be picky.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> The while loop that is removed below is actually just an elaborate
> "o = NULL", which av_opt_next() interprets as "start the iteration".
> 
>  libavfilter/avfilter.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index cc499fd5ed..165ab1f44a 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -814,6 +814,7 @@ static int process_options(AVFilterContext *ctx, AVDictionary **options,
>      const AVOption *o = NULL;
>      int ret;
>      char *av_uninit(parsed_key), *av_uninit(value);

> +    const AVClass *priv = ctx->filter->priv_class;

I do not understand why you make this a pointer rather than an int,
since it is only used as a boolean. "int shorthand_allowed" would be
clearer IMHO.

>      const char *key;
>      int offset= -1;
>  
> @@ -824,8 +825,7 @@ static int process_options(AVFilterContext *ctx, AVDictionary **options,
>          const char *shorthand = NULL;
>          int flags = AV_DICT_DONT_STRDUP_VAL;
>  
> -        o = av_opt_next(ctx->priv, o);
> -        if (o) {
> +        if (priv && (o = av_opt_next(ctx->priv, o))) {
>              if (o->type == AV_OPT_TYPE_CONST || o->offset == offset)
>                  continue;
>              offset = o->offset;
> @@ -848,7 +848,7 @@ static int process_options(AVFilterContext *ctx, AVDictionary **options,
>          if (parsed_key) {
>              key = parsed_key;
>              flags |= AV_DICT_DONT_STRDUP_KEY;
> -            while ((o = av_opt_next(ctx->priv, o))); /* discard all remaining shorthand */
> +            priv = NULL; /* reject all remaining shorthand */
>          } else {
>              key = shorthand;
>          }

Regards,
Andreas Rheinhardt Sept. 13, 2021, 12:15 p.m. UTC | #4
Nicolas George:
> Andreas Rheinhardt (12021-09-12):
>> The documentation for filter arguments states that short options must
>> precede long options (i.e. those of the form key=value). Yet if
>> process_options() encounters arguments not abiding by this, it simply
>> treats short options after a long option as if it were parsing short
>> options for the first time. In particular, it overwrites options already
>> set earlier, possibly via other short options. This is not how it is
>> intended (as a comment in the code indicates).
>>
>> This commit modifies the code to reject further shorthand options
>> after a long option has been encountered. After all, avfilter_init_str()
>> errors out upon unrecognized options, so it is intended to be picky.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> The while loop that is removed below is actually just an elaborate
>> "o = NULL", which av_opt_next() interprets as "start the iteration".
>>
>>  libavfilter/avfilter.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
>> index cc499fd5ed..165ab1f44a 100644
>> --- a/libavfilter/avfilter.c
>> +++ b/libavfilter/avfilter.c
>> @@ -814,6 +814,7 @@ static int process_options(AVFilterContext *ctx, AVDictionary **options,
>>      const AVOption *o = NULL;
>>      int ret;
>>      char *av_uninit(parsed_key), *av_uninit(value);
> 
>> +    const AVClass *priv = ctx->filter->priv_class;
> 
> I do not understand why you make this a pointer rather than an int,
> since it is only used as a boolean. "int shorthand_allowed" would be
> clearer IMHO.
> 

This is in preparation for the next patch: In this case,
process_options() will be called for filters without AVClass. It is not
documented to be save to call the av_opt_* functions with a
non-AVOpt-enabled structure and this check automatically ensures that it
won't be called in this case. (av_opt_next() currently does not crash,
but returns NULL.)

- Andreas

>>      const char *key;
>>      int offset= -1;
>>  
>> @@ -824,8 +825,7 @@ static int process_options(AVFilterContext *ctx, AVDictionary **options,
>>          const char *shorthand = NULL;
>>          int flags = AV_DICT_DONT_STRDUP_VAL;
>>  
>> -        o = av_opt_next(ctx->priv, o);
>> -        if (o) {
>> +        if (priv && (o = av_opt_next(ctx->priv, o))) {
>>              if (o->type == AV_OPT_TYPE_CONST || o->offset == offset)
>>                  continue;
>>              offset = o->offset;
>> @@ -848,7 +848,7 @@ static int process_options(AVFilterContext *ctx, AVDictionary **options,
>>          if (parsed_key) {
>>              key = parsed_key;
>>              flags |= AV_DICT_DONT_STRDUP_KEY;
>> -            while ((o = av_opt_next(ctx->priv, o))); /* discard all remaining shorthand */
>> +            priv = NULL; /* reject all remaining shorthand */
>>          } else {
>>              key = shorthand;
>>          }
> 
> Regards,
>
diff mbox series

Patch

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index cc499fd5ed..165ab1f44a 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -814,6 +814,7 @@  static int process_options(AVFilterContext *ctx, AVDictionary **options,
     const AVOption *o = NULL;
     int ret;
     char *av_uninit(parsed_key), *av_uninit(value);
+    const AVClass *priv = ctx->filter->priv_class;
     const char *key;
     int offset= -1;
 
@@ -824,8 +825,7 @@  static int process_options(AVFilterContext *ctx, AVDictionary **options,
         const char *shorthand = NULL;
         int flags = AV_DICT_DONT_STRDUP_VAL;
 
-        o = av_opt_next(ctx->priv, o);
-        if (o) {
+        if (priv && (o = av_opt_next(ctx->priv, o))) {
             if (o->type == AV_OPT_TYPE_CONST || o->offset == offset)
                 continue;
             offset = o->offset;
@@ -848,7 +848,7 @@  static int process_options(AVFilterContext *ctx, AVDictionary **options,
         if (parsed_key) {
             key = parsed_key;
             flags |= AV_DICT_DONT_STRDUP_KEY;
-            while ((o = av_opt_next(ctx->priv, o))); /* discard all remaining shorthand */
+            priv = NULL; /* reject all remaining shorthand */
         } else {
             key = shorthand;
         }