[FFmpeg-devel,v2,09/14] avfilter/vf_scale: Honour the AVFilter.init_dict documentation

The documentation states that unrecognized options need to be returned
(just as av_opt_set_dict() would do). Yet the scale and scale2ref
filters didn't abide by this: They simply copied all options
and in case it contained an unrecognized option was among them,
they would error out later in config_props. This violates the
documentation of av_filter_init_dict().

Fix this by only keeping the recognized options and returning
the unrecognized ones.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavfilter/vf_scale.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Andreas Rheinhardt (12021-09-14):
> The documentation states that unrecognized options need to be returned
> (just as av_opt_set_dict() would do). Yet the scale and scale2ref
> filters didn't abide by this: They simply copied all options
> and in case it contained an unrecognized option was among them,
> they would error out later in config_props. This violates the
> documentation of av_filter_init_dict().
> 
> Fix this by only keeping the recognized options and returning
> the unrecognized ones.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavfilter/vf_scale.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> index a1902a13cf..c31b92b847 100644
> --- a/libavfilter/vf_scale.c
> +++ b/libavfilter/vf_scale.c
> @@ -269,6 +269,8 @@ revert:
>  
>  static av_cold int init_dict(AVFilterContext *ctx, AVDictionary **opts)
>  {
> +    const AVClass *class = sws_get_class();
> +    const AVDictionaryEntry *entry = NULL;
>      ScaleContext *scale = ctx->priv;
>      int ret;
>  
> @@ -312,15 +314,23 @@ static av_cold int init_dict(AVFilterContext *ctx, AVDictionary **opts)
>      scale->flags = 0;
>  
>      if (scale->flags_str && *scale->flags_str) {
> -        const AVClass *class = sws_get_class();
>          const AVOption    *o = av_opt_find(&class, "sws_flags", NULL, 0,
>                                             AV_OPT_SEARCH_FAKE_OBJ);
>          int ret = av_opt_eval_flags(&class, o, scale->flags_str, &scale->flags);
>          if (ret < 0)
>              return ret;
>      }
> -    scale->opts = *opts;
> -    *opts = NULL;
> +    FFSWAP(AVDictionary *, *opts, scale->opts);
> +    /* Now move all the unrecognized options back to opts. */
> +    while (entry = av_dict_get(scale->opts, "", entry, AV_DICT_IGNORE_SUFFIX)) {
> +        if (!av_opt_find(&class, entry->key, NULL, 0, AV_OPT_SEARCH_FAKE_OBJ)) {
> +            if ((ret = av_dict_set(opts, entry->key, entry->value, 0)) < 0 ||
> +                (ret = av_dict_set(&scale->opts, entry->key, NULL, 0)) < 0)
> +                return ret;
> +            /* Removing the entry from scale->opts invalidated entry. */
> +            entry = NULL;
> +        }
> +    }
>  
>      return 0;
>  }

I managed to understand what this code does, but I find it quite
confusing.

First, correct me if I am wrong, but I think the entry parameter to
av_dict_get() is always NULL.

Second, I think if you swap removing the option from scale->opts and
adding it to opts you can avoid strduping the key.

And is entry->value not leaking?

But more importantly, is it not a convoluted implementation that would
be simplified by using preinit like you did for sws?

Regards,

Nicolas George:
> Andreas Rheinhardt (12021-09-14):
>> The documentation states that unrecognized options need to be returned
>> (just as av_opt_set_dict() would do). Yet the scale and scale2ref
>> filters didn't abide by this: They simply copied all options
>> and in case it contained an unrecognized option was among them,
>> they would error out later in config_props. This violates the
>> documentation of av_filter_init_dict().
>>
>> Fix this by only keeping the recognized options and returning
>> the unrecognized ones.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>  libavfilter/vf_scale.c | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
>> index a1902a13cf..c31b92b847 100644
>> --- a/libavfilter/vf_scale.c
>> +++ b/libavfilter/vf_scale.c
>> @@ -269,6 +269,8 @@ revert:
>>  
>>  static av_cold int init_dict(AVFilterContext *ctx, AVDictionary **opts)
>>  {
>> +    const AVClass *class = sws_get_class();
>> +    const AVDictionaryEntry *entry = NULL;
>>      ScaleContext *scale = ctx->priv;
>>      int ret;
>>  
>> @@ -312,15 +314,23 @@ static av_cold int init_dict(AVFilterContext *ctx, AVDictionary **opts)
>>      scale->flags = 0;
>>  
>>      if (scale->flags_str && *scale->flags_str) {
>> -        const AVClass *class = sws_get_class();
>>          const AVOption    *o = av_opt_find(&class, "sws_flags", NULL, 0,
>>                                             AV_OPT_SEARCH_FAKE_OBJ);
>>          int ret = av_opt_eval_flags(&class, o, scale->flags_str, &scale->flags);
>>          if (ret < 0)
>>              return ret;
>>      }
>> -    scale->opts = *opts;
>> -    *opts = NULL;
>> +    FFSWAP(AVDictionary *, *opts, scale->opts);
>> +    /* Now move all the unrecognized options back to opts. */
>> +    while (entry = av_dict_get(scale->opts, "", entry, AV_DICT_IGNORE_SUFFIX)) {
>> +        if (!av_opt_find(&class, entry->key, NULL, 0, AV_OPT_SEARCH_FAKE_OBJ)) {
>> +            if ((ret = av_dict_set(opts, entry->key, entry->value, 0)) < 0 ||
>> +                (ret = av_dict_set(&scale->opts, entry->key, NULL, 0)) < 0)
>> +                return ret;
>> +            /* Removing the entry from scale->opts invalidated entry. */
>> +            entry = NULL;
>> +        }
>> +    }
>>  
>>      return 0;
>>  }
> 
> I managed to understand what this code does, but I find it quite
> confusing.
> 
> First, correct me if I am wrong, but I think the entry parameter to
> av_dict_get() is always NULL.

It is not. It is only reset when the last entry key was not recognized
as option; if it was recognized, entry is not NULL. In particular, in
the (hopefully) common case where all options are recognized, the
dictionary is just traversed linearly.

> 
> Second, I think if you swap removing the option from scale->opts and
> adding it to opts you can avoid strduping the key.
> 

You seem to think about "stealing" the key from the returned entry. This
does not work at all here, because for av_dict_set(&scale->opts,
entry->key, NULL, 0) to actually find the entry to remove, the entry
still needs to have the key. But if it has the key, it will free the key
upon deleting the entry.
More generally, stealing a key/value is forbidden: "The returned entry
key or value must not be changed, or it will cause undefined behavior."
Several parts of the AVDict API itself as well as user code requires
this to be so.
(I know you do this in tee.c where it happens to work; btw: I have a few
open patches for it that you may look at.)

> And is entry->value not leaking?

No, default behaviour for av_dict_set() (the flags AV_DICT_MULTIKEY,
AV_DICT_DONT_OVERWRITE, AV_DICT_APPEND modify this) is to replace the
first current entry whose key matches with the new entry (or just delete
said entry when the new value is NULL). When an entry is freed, both of
its key and value are freed, too.

> 
> But more importantly, is it not a convoluted implementation that would
> be simplified by using preinit like you did for sws?
> 

Unfortunately, the situation here is more complicated: We have not one
object on which we can simply apply options, but several (there are
separate scalers for interlaced pictures); and they are freed and
recreated on parameter changes. So one would at least need a persistent
master scale context which is used to apply the initial user options and
also used to copy options from it to the other scalers. At that point I
looked at the place where the options from the dictionary get applied
and saw that it is in the middle of applying other options, so I
believed that I could not maintain current behaviour by using a master
context and instead opted for this.

Now I look at this again and see that it is possible to override certain
parameters which should not be overrideable at all: E.g. overriding
dstw/dsth/srcw/srch easily leads to crashes. So this should be reordered
and in this case the master scaler approach might be feasible. I can't
promise anything though.

Currently there is a bit of direct duplication in the options of this
filter and the swscale options: param0, param1. And there is more
duplication when one also considers equivalent, but only similar named
options: flags and sws_flags; and the *_chr_pos options. Of course my
intention is to deprecate these options and directly use the swscale
options*. Yet there is an issue with how the options are set: If the
deprecated options get the AV_OPT_FLAG_DEPRECATED (as they should), then
using AV_OPT_FLAG_SEARCH_CHILDREN_AFTER_ME would show deprecation
warnings. To fix this, AV_OPT_FLAG_SEARCH_CHILDREN_AFTER_ME (and
probably also AV_OPT_FLAG_SEARCH_CHILDREN) should prefer non-deprecated
options if they exist even if they are found later. The end result
should be that only users that set one of those options directly using
av_opt_set without any SEARCH_CHILDREN flag should get the deprecation
warning.
Luckily for you, this will make a simple goto approach for
AV_OPT_FLAG_SEARCH_CHILDREN_AFTER_ME unfeasible.

- Andreas

*: When these options are gone, the shorthands for them won't work any
more; I hope this will ultimately be acceptable.