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

[Andreas Rheinhardt]

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 at 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,
>