[FFmpeg-devel] [PATCH] libavfilter: constify filter list

Mark Thompson sw at jkqxz.net
Wed Jan 31 00:26:39 EET 2018


On 30/01/18 18:06, Muhammad Faiz wrote:
> On Tue, Jan 30, 2018 at 9:09 PM, Mark Thompson <sw at jkqxz.net> wrote:
>> On 30/01/18 07:24, Muhammad Faiz wrote:
>>> Move REGISTER_FILTER to FILTER_TABLE in configure.
>>> Auto generate filter extern and filter table.
>>> Sort filter table, use bsearch on avfilter_get_by_name.
>>> Define next pointer at filter extern, no need to initialize
>>> next pointer at run time, so AVFilter can be set to const.
>>> Make avfilter_register always return error.
>>> Target checkasm now depends on EXTRALIBS-avformat.
>>>
>>> Signed-off-by: Muhammad Faiz <mfcc64 at gmail.com>
>>> ---
>>
>> I like the idea of this, but I'm not sure about some of the implementation details.
>>
>> Have you considered dropping the "next" links entirely and having just the array of pointers instead?  I feel like they don't really add anything useful once we are in this form, and result in more boilerplate on every filter and some tricky generation code.
>>
>> avfilter_next() would be a bit slower (since it would have to search the array, and therefore have runtime linear in the number of filters), but avfilter_get_by_name() is a lot faster because of the binary search (and is the only common use of it) so I don't think that complaint would be a strong one.
> 
> Making avfilter_next() slower (even if it is rarely used) isn't good, I think.

I think the slowdown is irrelevant if it is rarely used.

On the other side, you get rid of a field in AVFilter and avoid having to put some pointless boilerplate in a lot of places.

Related: the one remaining use of avfilter_next() inside lavfi, in filter_child_class_next(), should also be replaced with array operations.  (I can easily do that if you don't want to bother as part of this patch.)

>>
>>>  Makefile                           |   4 +-
>>>  configure                          | 440 ++++++++++++++++++++++++++++++++++++-
>>> ...
>>>  tests/checkasm/Makefile            |   2 +-
>>>  303 files changed, 1229 insertions(+), 796 deletions(-)
>>>
>>> diff --git a/Makefile b/Makefile
>>> index 9defddebfd..f607579369 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -56,6 +56,7 @@ tools/uncoded_frame$(EXESUF): ELIBS = $(FF_EXTRALIBS)
>>>  tools/target_dec_%_fuzzer$(EXESUF): $(FF_DEP_LIBS)
>>>
>>>  CONFIGURABLE_COMPONENTS =                                           \
>>> +    $(SRC_PATH)/configure                                           \
>>>      $(wildcard $(FFLIBS:%=$(SRC_PATH)/lib%/all*.c))                 \
>>>      $(SRC_PATH)/libavcodec/bitstream_filters.c                      \
>>>      $(SRC_PATH)/libavformat/protocols.c                             \
>>> @@ -142,7 +143,8 @@ distclean:: clean
>>>       $(RM) .version avversion.h config.asm config.h mapfile  \
>>>               ffbuild/.config ffbuild/config.* libavutil/avconfig.h \
>>>               version.h libavutil/ffversion.h libavcodec/codec_names.h \
>>> -             libavcodec/bsf_list.c libavformat/protocol_list.c
>>> +             libavcodec/bsf_list.c libavformat/protocol_list.c \
>>> +             libavfilter/filter_list.h libavfilter/filter_list.c
>>>  ifeq ($(SRC_LINK),src)
>>>       $(RM) src
>>>  endif
>>> diff --git a/configure b/configure
>>> index fcfa7aa442..3261f5fd1a 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -3177,6 +3177,381 @@ unix_protocol_deps="sys_un_h"
>>>  unix_protocol_select="network"
>>>
>>>  # filters
>>> +FILTER_TABLE="
>>> +abench                  af
>>> +acompressor             af
>>> +acontrast               af
>>> ...
>>> +spectrumsynth           vaf
>>> +amovie                  avsrc
>>> +movie                   avsrc
>>> +"
>>> +
>>> +UNCONDITIONAL_FILTER_TABLE="
>>> +abuffer         asrc
>>> +buffer          vsrc
>>> +abuffersink     asink
>>> +buffersink      vsink
>>> +afifo           af
>>> +fifo            vf
>>> +"
>>> +
>>
>> I don't really like having this table in configure.  Since you're generating the filter_list.h header with the external definitions from it anyway, why not write that and use it as the source rather than having the table here?
> 
> Imho, parsing source code and then generating source code is
> pointless, it is redundant. Previously, it was just parsing source
> code without generating source code. And now it is just generating
> source code without parsing source code. There are no duplicates here.

This is parsing a huge variable in configure and generating two different files from it.  Parsing one file and generating one file seems clearer, though I guess that's mostly subjective.

> Also storing filter table in configure is more consistent, I think.
> Because filter dependencies are in configure.

Storing it in configure feels less consistent to me - no other components lists like this are in configure, they are all in their own files (either as macros the allfoo.c files like the one you are removing or as external declarations).  Also configure is already inconveniently huge, and this is adding many more lines to it.

As a middle ground between the two options, perhaps a non-C file outside configure ("libavfilter/filter_list", I guess) which can just be loaded directly into a shell variable by configure?

>>
>>>  afftfilt_filter_deps="avcodec"
>>>  afftfilt_filter_select="fft"
>>>  afir_filter_deps="avcodec"
>>> @@ -3530,7 +3905,18 @@ MUXER_LIST=$(find_things    muxer    _MUX     libavformat/allformats.c)
>>>  DEMUXER_LIST=$(find_things  demuxer  DEMUX    libavformat/allformats.c)
>>>  OUTDEV_LIST=$(find_things   outdev   OUTDEV   libavdevice/alldevices.c)
>>>  INDEV_LIST=$(find_things    indev    _IN      libavdevice/alldevices.c)
>>> -FILTER_LIST=$(find_things   filter   FILTER   libavfilter/allfilters.c)
>>> +
>>> +extract_list_from_table(){
>>> +    cols=$1
>>> +    suffix=$2
>>> +    shift 2
>>> +    while test -n "$1"; do
>>> +        echo "${1}${suffix}"
>>> +        shift $cols
>>> +    done
>>> +}
>>> +
>>> +FILTER_LIST=$(extract_list_from_table 2 _filter $FILTER_TABLE)
>>>
>>>  find_things_extern(){
>>>      thing=$1
>>> @@ -7030,6 +7416,58 @@ print_enabled_components(){
>>>  print_enabled_components libavcodec/bsf_list.c AVBitStreamFilter bitstream_filters $BSF_LIST
>>>  print_enabled_components libavformat/protocol_list.c URLProtocol url_protocols $PROTOCOL_LIST
>>>
>>> +# filters
>>> +extract_enabled_filter(){
>>> +    while test -n "$1"; do
>>> +        if enabled "${1}_filter"; then
>>> +            echo "$1 $2"
>>> +        fi
>>> +        shift 2
>>> +    done
>>> +}
>>> +
>>> +extract_sorted_filter(){
>>> +    while test -n "$1"; do
>>> +        echo "$1 $2"
>>> +        shift 2
>>> +    done | sort
>>> +}
>>> +
>>> +print_filter_extern(){
>>> +    while test -n "$1"; do
>>> +        echo "extern const AVFilter ff_${2}_${1};"
>>> +        if test -n "$3"; then
>>> +            echo "#define ff_next_${2}_${1} &ff_${4}_${3}"
>>> +        else
>>> +            echo "#define ff_next_${2}_${1} NULL"
>>> +        fi
>>> +        shift 2
>>> +    done
>>> +}
>>> +
>>> +print_filter_array(){
>>> +    echo "static const AVFilter *const filter_list[] = {"
>>> +    while test -n "$1"; do
>>> +        echo "    &ff_${2}_${1},"
>>> +        shift 2
>>> +    done
>>> +    echo "    NULL"
>>> +    echo "};"
>>> +}
>>> +
>>> +sorted_filter_table=$(extract_sorted_filter $(extract_enabled_filter $FILTER_TABLE) $UNCONDITIONAL_FILTER_TABLE)
>>> +
>>> +echo "/* Automatically generated by configure - do not modify! */" > $TMPH
>>> +echo "#ifndef AVFILTER_FILTER_LIST_H" >> $TMPH
>>> +echo "#define AVFILTER_FILTER_LIST_H" >> $TMPH
>>> +print_filter_extern $sorted_filter_table >> $TMPH
>>> +echo "#endif" >> $TMPH
>>> +cp_if_changed $TMPH libavfilter/filter_list.h
>>> +
>>> +echo "/* Automatically generated by configure - do not modify! */" > $TMPH
>>> +print_filter_array $sorted_filter_table >> $TMPH
>>> +cp_if_changed $TMPH libavfilter/filter_list.c
>>> +
>>>  # Settings for pkg-config files
>>>
>>>  cat > $TMPH <<EOF
>>> diff --git a/libavfilter/aeval.c b/libavfilter/aeval.c
>>> index cdddbaf31d..d5963367a1 100644
>>> --- a/libavfilter/aeval.c
>>> +++ b/libavfilter/aeval.c
>>> @@ -322,7 +322,7 @@ static const AVFilterPad aevalsrc_outputs[] = {
>>>      { NULL }
>>>  };
>>>
>>> -AVFilter ff_asrc_aevalsrc = {
>>> +const AVFilter ff_asrc_aevalsrc = {
>>>      .name          = "aevalsrc",
>>>      .description   = NULL_IF_CONFIG_SMALL("Generate an audio signal generated by an expression."),
>>>      .query_formats = query_formats,
>>> @@ -332,6 +332,7 @@ AVFilter ff_asrc_aevalsrc = {
>>>      .inputs        = NULL,
>>>      .outputs       = aevalsrc_outputs,
>>>      .priv_class    = &aevalsrc_class,
>>> +    .next          = ff_next_asrc_aevalsrc,
>>
>> If we're going to go with this approach, I think this field should be macroed somehow because it is entirely boilerplate.
>>
>> Maybe an AVFILTER_NAME() macro which sets the "name" and "next" fields?
> 
> I guess people will like something start with FF_, so I will try with
> FF_DEFINE_AVFILTER_NAME().

It shouldn't matter - it's not a symbol and won't be user-visible.

>>>  };
>>>
>>>  #endif /* CONFIG_AEVALSRC_FILTER */
>>> @@ -475,7 +476,7 @@ static const AVFilterPad aeval_outputs[] = {
>>>      { NULL }
>>>  };
>>>
>>> -AVFilter ff_af_aeval = {
>>> +const AVFilter ff_af_aeval = {
>>>      .name          = "aeval",
>>>      .description   = NULL_IF_CONFIG_SMALL("Filter audio signal according to a specified expression."),
>>>      .query_formats = aeval_query_formats,
>>> @@ -485,6 +486,7 @@ AVFilter ff_af_aeval = {
>>>      .inputs        = aeval_inputs,
>>>      .outputs       = aeval_outputs,
>>>      .priv_class    = &aeval_class,
>>> +    .next          = ff_next_af_aeval,
>>>  };
>>>
>>>  #endif /* CONFIG_AEVAL_FILTER */
>>>
>>> ...
>>> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
>>> index 9adb1090b7..8bab79ff96 100644
>>> --- a/libavfilter/allfilters.c
>>> +++ b/libavfilter/allfilters.c
>>> @@ -19,412 +19,59 @@
>>>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>>   */
>>>
>>> +#include "libavutil/avassert.h"
>>>  #include "libavutil/thread.h"
>>>  #include "avfilter.h"
>>>  #include "config.h"
>>>
>>> +#include "libavfilter/filter_list.h"
>>> +#include "libavfilter/filter_list.c"
>>>
>>> -#define REGISTER_FILTER(X, x, y)                                        \
>>> -    {                                                                   \
>>> -        extern AVFilter ff_##y##_##x;                                   \
>>> -        if (CONFIG_##X##_FILTER)                                        \
>>> -            avfilter_register(&ff_##y##_##x);                           \
>>> -    }
>>> -
>>> -#define REGISTER_FILTER_UNCONDITIONAL(x)                                \
>>> -    {                                                                   \
>>> -        extern AVFilter ff_##x;                                         \
>>> -        avfilter_register(&ff_##x);                                     \
>>> -    }
>>>
>>> -static void register_all(void)
>>> +#if defined(ASSERT_LEVEL) && ASSERT_LEVEL > 1
>>> +static void check_validity(void)
>>>  {
>>> -    REGISTER_FILTER(ABENCH,         abench,         af);
>>> -    REGISTER_FILTER(ACOMPRESSOR,    acompressor,    af);
>>> -    REGISTER_FILTER(ACONTRAST,      acontrast,      af);
>>> ...
>>> -    REGISTER_FILTER(TESTSRC,        testsrc,        vsrc);
>>> -    REGISTER_FILTER(TESTSRC2,       testsrc2,       vsrc);
>>> -    REGISTER_FILTER(YUVTESTSRC,     yuvtestsrc,     vsrc);
>>> +    int k;
>>> +    for (k = 0; k < FF_ARRAY_ELEMS(filter_list) - 2; k++) {
>>> +        av_assert2(filter_list[k]->next == filter_list[k+1] ||
>>> +                   (av_log(NULL, AV_LOG_FATAL, "%s filter: invalid next pointer.\n", filter_list[k]->name),0));
>>> +        av_assert2(strcmp(filter_list[k]->name, filter_list[k+1]->name) < 0 ||
>>> +                   (av_log(NULL, AV_LOG_FATAL, "%s filter: unsorted with %s.\n", filter_list[k]->name, filter_list[k+1]->name),0));
>>> +    }
>>> +    av_assert2(!filter_list[k]->next);
>>> +    av_assert2(!filter_list[k+1]);
>>> +}
>>
>> Cute :)  I think it should be assert0() if we go with the links, though - almost noone builds with --assert-level=2, and the overhead of this check is not very high.
> 
> It is inside #if ASSERT_LEVEL > 1. I think we should not include this
> check on production use.

I was thinking that it's a problem because it would waste developer time on weird problems when making new filters by copying existing ones (and forgetting to change the "next" link), but if the explicit value is macroed away then it's not going to happen.

Thanks,

- Mark


More information about the ffmpeg-devel mailing list