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

[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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210916/140c504a/attachment.sig>