[FFmpeg-devel,3/5] avutil/opt: Add search flag to search children after the current object

The current way searches children first which is odd and not always
intended.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
Missing APIchanges entry and version bump.

 libavutil/opt.c | 12 ++++++++++++
 libavutil/opt.h |  7 +++++++
 2 files changed, 19 insertions(+)

Andreas Rheinhardt (12021-09-15):
> The current way searches children first which is odd and not always
> intended.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> Missing APIchanges entry and version bump.
> 
>  libavutil/opt.c | 12 ++++++++++++
>  libavutil/opt.h |  7 +++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index 41284d4ecd..de06728cd1 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -1679,6 +1679,11 @@ const AVOption *av_opt_find2(void *obj, const char *name, const char *unit,
>          return NULL;
>  
>      if (search_flags & AV_OPT_SEARCH_CHILDREN) {
> +        /* Searching children both last and first makes no sense. */
> +        if (search_flags & AV_OPT_SEARCH_CHILDREN_AFTER_ME)
> +            return NULL;
> +

> +search_children:

No gotos for anything less obvious than cleanup after error, please. Yes
it makes this patch simpler, but it will make the next ones much harder.
I can see a few reasonable ways of avoiding it, I do not know which one
you would prefer. The cleanest would probably be to split searching in
the children in a separate function, and call it conditionally once at
the beginning and once at the end.

Also, would there be a drawback in making it the default? If I
understand correctly, you said there are currently no meaningful
collisions, so we can choose now without breaking anything. If we
consider that children are usually more generic code, it makes sense to
search them later by default.

>          if (search_flags & AV_OPT_SEARCH_FAKE_OBJ) {
>              void *iter = NULL;
>              const AVClass *child;
> @@ -1691,6 +1696,11 @@ const AVOption *av_opt_find2(void *obj, const char *name, const char *unit,
>                  if (o = av_opt_find2(child, name, unit, opt_flags, search_flags, target_obj))
>                      return o;
>          }
> +        /* If the AV_OPT_SEARCH_CHILDREN_AFTER_ME flag is set,
> +         * then we have already unsuccesfully checked our own options
> +         * and it is certain that this option is unrecognized. */
> +        if (search_flags & AV_OPT_SEARCH_CHILDREN_AFTER_ME)
> +            return NULL;
>      }
>  
>      while (o = av_opt_next(obj, o)) {
> @@ -1706,6 +1716,8 @@ const AVOption *av_opt_find2(void *obj, const char *name, const char *unit,
>              return o;
>          }
>      }
> +    if (search_flags & AV_OPT_SEARCH_CHILDREN_AFTER_ME)
> +        goto search_children;
>      return NULL;
>  }
>  
> diff --git a/libavutil/opt.h b/libavutil/opt.h
> index 2820435eec..5ce4a8d03d 100644
> --- a/libavutil/opt.h
> +++ b/libavutil/opt.h
> @@ -558,6 +558,13 @@ int av_opt_eval_q     (void *obj, const AVOption *o, const char *val, AVRational
>  
>  #define AV_OPT_SEARCH_CHILDREN   (1 << 0) /**< Search in possible children of the
>                                                 given object first. */
> +/**
> + * Search in possible children of the given object after having
> + * searched the options of the object itself.
> + * This flag must not be combined with AV_OPT_SEARCH_CHILDREN.
> + */
> +#define AV_OPT_SEARCH_CHILDREN_AFTER_ME   (1 << 3)
> +
>  /**
>   *  The obj passed to av_opt_find() is fake -- only a double pointer to AVClass
>   *  instead of a required pointer to a struct containing AVClass. This is

Regards,

Nicolas George:
> Andreas Rheinhardt (12021-09-15):
>> The current way searches children first which is odd and not always
>> intended.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> Missing APIchanges entry and version bump.
>>
>>  libavutil/opt.c | 12 ++++++++++++
>>  libavutil/opt.h |  7 +++++++
>>  2 files changed, 19 insertions(+)
>>
>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>> index 41284d4ecd..de06728cd1 100644
>> --- a/libavutil/opt.c
>> +++ b/libavutil/opt.c
>> @@ -1679,6 +1679,11 @@ const AVOption *av_opt_find2(void *obj, const char *name, const char *unit,
>>          return NULL;
>>  
>>      if (search_flags & AV_OPT_SEARCH_CHILDREN) {
>> +        /* Searching children both last and first makes no sense. */
>> +        if (search_flags & AV_OPT_SEARCH_CHILDREN_AFTER_ME)
>> +            return NULL;
>> +
> 
>> +search_children:
> 
> No gotos for anything less obvious than cleanup after error, please. Yes
> it makes this patch simpler, but it will make the next ones much harder.
> I can see a few reasonable ways of avoiding it, I do not know which one
> you would prefer. The cleanest would probably be to split searching in
> the children in a separate function, and call it conditionally once at
> the beginning and once at the end.
> 
> Also, would there be a drawback in making it the default? If I
> understand correctly, you said there are currently no meaningful
> collisions, so we can choose now without breaking anything. If we
> consider that children are usually more generic code, it makes sense to
> search them later by default.
> 

What do you mean with "by default"? All flags to av_opt_find2() need to
be explicitly set, so there are no real default flags for it. But some
(two) parts of the AVOpt-API just use it; these could be switched. At
least, I don't see a negative consequence of it.
If you mean that the behaviour of AV_OPT_SEARCH_CHILDREN should be
adapted (making the new flag unnecessary), then you'd propose breaking
documented API: "Search in possible children of the
 given object first."

The rest of the code could mostly also be switched; the only scenario
(that I am aware of) where it makes a difference is for libavcodec,
where there is a clash, so that (unfortunately) it can't use the new
flag.* IMO the current way of doing things has the problem that every of
your options can be interposed away by a child; this increases
complexity for no gain. For example, using AV_OPT_SEARCH_CHILDREN in a
similar patch as 1/5 or 4/5 would be impossible in libavformat, because
the AVIOContext is a child of it and when using custom I/O it can have
arbitrary options which could then redirect/interpose the generic
libavformat options. (Apart from that, setting options on the
AVIOContext would be a change of behaviour that I wanted to avoid with
these patches.)

Notice that it is generally not correct that the children are more
generic. I.e. the first level
(AVFilterContext/AVCodecContext/AVFormatContext) is more generic than
the level below it, but given that their children should be explicitly
designed to be children of the former, they should not clash with the
options of the former (but unfortunately do in libavcodec), so giving
precedence to the first level over the others is entirely natural. And
so is giving precedence to the second level over the third level (I
cannot think of a reason where the third level should win in a clash
with the second level; the opposite is not true: If the third level is
(like framesync) a more generic object, overriding some options from it
for a particular filter makes sense**).

*: For the jpeg2000 decoder this clash seems to result from the
ffmpeg-libav split: Libav has deprecated and removed the generic lowres
option early on, FFmpeg kept it to this day. So for this very decoder
Libav added a lowres option. The reason for this seems to be that this
codec is based around progressive refinement layers and when setting
lowres to something > 0, some of these refinement layers can be simply
skipped which can be done quite cleanly. And the photocd decoder seemed
to copy the option from jpeg2000 without noticing that there is a
generic option for it.

(I don't like the generic lowres flag, unless the format really contains
a lower-resolution image itself.)
**: But notice that if an API user iterates over all children of a given
object, they can nevertheless the options of the third level. But at
least the AVDict-initializations would be covered.

>>          if (search_flags & AV_OPT_SEARCH_FAKE_OBJ) {
>>              void *iter = NULL;
>>              const AVClass *child;
>> @@ -1691,6 +1696,11 @@ const AVOption *av_opt_find2(void *obj, const char *name, const char *unit,
>>                  if (o = av_opt_find2(child, name, unit, opt_flags, search_flags, target_obj))
>>                      return o;
>>          }
>> +        /* If the AV_OPT_SEARCH_CHILDREN_AFTER_ME flag is set,
>> +         * then we have already unsuccesfully checked our own options
>> +         * and it is certain that this option is unrecognized. */
>> +        if (search_flags & AV_OPT_SEARCH_CHILDREN_AFTER_ME)
>> +            return NULL;
>>      }
>>  
>>      while (o = av_opt_next(obj, o)) {
>> @@ -1706,6 +1716,8 @@ const AVOption *av_opt_find2(void *obj, const char *name, const char *unit,
>>              return o;
>>          }
>>      }
>> +    if (search_flags & AV_OPT_SEARCH_CHILDREN_AFTER_ME)
>> +        goto search_children;
>>      return NULL;
>>  }
>>  
>> diff --git a/libavutil/opt.h b/libavutil/opt.h
>> index 2820435eec..5ce4a8d03d 100644
>> --- a/libavutil/opt.h
>> +++ b/libavutil/opt.h
>> @@ -558,6 +558,13 @@ int av_opt_eval_q     (void *obj, const AVOption *o, const char *val, AVRational
>>  
>>  #define AV_OPT_SEARCH_CHILDREN   (1 << 0) /**< Search in possible children of the
>>                                                 given object first. */
>> +/**
>> + * Search in possible children of the given object after having
>> + * searched the options of the object itself.
>> + * This flag must not be combined with AV_OPT_SEARCH_CHILDREN.
>> + */
>> +#define AV_OPT_SEARCH_CHILDREN_AFTER_ME   (1 << 3)
>> +
>>  /**
>>   *  The obj passed to av_opt_find() is fake -- only a double pointer to AVClass
>>   *  instead of a required pointer to a struct containing AVClass. This is
> 
> Regards,
>