diff mbox series

[FFmpeg-devel,5/5] avfilter/avfilter: Apply options in one av_opt_set_dict2() call

Message ID AM7PR03MB66609BF940FD2E62991FAA288FDB9@AM7PR03MB6660.eurprd03.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,1/5] avcodec/avcodec: Set options only once via AV_OPT_SEARCH_CHILDREN | expand

Checks

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

Commit Message

Andreas Rheinhardt Sept. 15, 2021, 5:22 p.m. UTC
Up until now, avfilter_init_dict() first applied the generic options
via av_opt_set_dict(), then set the filter-specific options via
av_opt_set_dict2() applied to the private context with the
AV_OPT_SEARCH_CHILDREN flag set (this ensures that e.g. framesync
options are set, too). For filters with the init_dict callback
the remaining options were passed to it.

This has the downside that all non-generic options are copied to
a new dictionary in the first av_opt_set_dict() which involves
allocations. This commit changes this by using av_opt_set_dict2()
on the AVFilterContext to set all options (except those for init_dict)
in one step. In order to ensure that the generic options are still
applied first, the new AV_OPT_SEARCH_CHILDREN_AFTER_ME flag is used.
This also reverses the search order between the private context and
its children (if any), but this is actually more logical (the child
contexts are more generic, hence the more special private context
options should take precedence) as well as irrelevant (there is some
option duplication*, but all duplicated options are entirely equivalent).
Notice that there is also no option collision between the generic option
and any child option (i.e. one could have used AV_OPT_SEARCH_CHILDREN)
as well, but there might be some day: E.g. both the AVFilterContext
class as well as the swresample class have an option named "threads".
If the scale filters would expose this class in their preinit callback,
there would be a problem when using AV_OPT_SEARCH_CHILDREN.

Finally, in case an error happens in the second step
(e.g. due to an option being out-of-range) the name and address
of the AVFilterContext instead of the private context are used.
This allows the user to more easily map this error to the actual filter.

*: Several overlay filters duplicate framesync options (to preserve
the ability to set them via the shorthand notation).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavfilter/avfilter.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

Comments

Paul B Mahol Sept. 15, 2021, 5:35 p.m. UTC | #1
probably fine
Nicolas George Sept. 15, 2021, 5:56 p.m. UTC | #2
Andreas Rheinhardt (12021-09-15):
> Up until now, avfilter_init_dict() first applied the generic options
> via av_opt_set_dict(), then set the filter-specific options via
> av_opt_set_dict2() applied to the private context with the
> AV_OPT_SEARCH_CHILDREN flag set (this ensures that e.g. framesync
> options are set, too). For filters with the init_dict callback
> the remaining options were passed to it.
> 
> This has the downside that all non-generic options are copied to
> a new dictionary in the first av_opt_set_dict() which involves
> allocations. This commit changes this by using av_opt_set_dict2()
> on the AVFilterContext to set all options (except those for init_dict)
> in one step. In order to ensure that the generic options are still
> applied first, the new AV_OPT_SEARCH_CHILDREN_AFTER_ME flag is used.
> This also reverses the search order between the private context and
> its children (if any), but this is actually more logical (the child
> contexts are more generic, hence the more special private context
> options should take precedence) as well as irrelevant (there is some
> option duplication*, but all duplicated options are entirely equivalent).
> Notice that there is also no option collision between the generic option
> and any child option (i.e. one could have used AV_OPT_SEARCH_CHILDREN)
> as well, but there might be some day: E.g. both the AVFilterContext
> class as well as the swresample class have an option named "threads".
> If the scale filters would expose this class in their preinit callback,
> there would be a problem when using AV_OPT_SEARCH_CHILDREN.
> 
> Finally, in case an error happens in the second step
> (e.g. due to an option being out-of-range) the name and address
> of the AVFilterContext instead of the private context are used.
> This allows the user to more easily map this error to the actual filter.

I think it is ok, but I think AV_OPT_SEARCH_CHILDREN_AFTER_ME should be
the default. Also, we should try to simplify the logic to make all this
long explanation no longer relevant.

IIRC, we reserved a character in options name to introduce context. That
would fix the issue of overloaded options: lavfi.threads= vs.
sws.threads=.

> *: Several overlay filters duplicate framesync options (to preserve
> the ability to set them via the shorthand notation).

We should deprecate and eventually remove that. Shorthand notation is
for the main options, those that need to be there otherwise it will
probably do nothing useful.

Regards,
diff mbox series

Patch

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index ce63b9762f..87e28a8024 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -897,9 +897,9 @@  int avfilter_init_dict(AVFilterContext *ctx, AVDictionary **options)
 {
     int ret = 0;
 
-    ret = av_opt_set_dict(ctx, options);
+    ret = av_opt_set_dict2(ctx, options, AV_OPT_SEARCH_CHILDREN_AFTER_ME);
     if (ret < 0) {
-        av_log(ctx, AV_LOG_ERROR, "Error applying generic filter options.\n");
+        av_log(ctx, AV_LOG_ERROR, "Error applying filter options.\n");
         return ret;
     }
 
@@ -912,14 +912,6 @@  int avfilter_init_dict(AVFilterContext *ctx, AVDictionary **options)
         ctx->thread_type = 0;
     }
 
-    if (ctx->filter->priv_class) {
-        ret = av_opt_set_dict2(ctx->priv, options, AV_OPT_SEARCH_CHILDREN);
-        if (ret < 0) {
-            av_log(ctx, AV_LOG_ERROR, "Error applying options to the filter.\n");
-            return ret;
-        }
-    }
-
     if (ctx->filter->init)
         ret = ctx->filter->init(ctx);
     else if (ctx->filter->init_dict)