On Wed, Jan 31, 2018 at 5:26 AM, Mark Thompson <s...@jkqxz.net> wrote:
> On 30/01/18 18:06, Muhammad Faiz wrote:
>> On Tue, Jan 30, 2018 at 9:09 PM, Mark Thompson <s...@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 <mfc...@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.

The other solution is to initialize next pointer on
avfilter_register_all() as before,  add new iterate API, and deprecate
both avfilter_register_all() and avfilter_next().


>
> 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.)

Oh, I forgot it.


>
>>>
>>>>  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.

Without ff_next_* stuff, it will generate only one file. With
ff_next_* stuff, it will generate two files in both cases.


>
>> 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.

So, this is the first which is moved from all*.c to configure. The
others will follow. Also this will remove 'all*.c is newer than
config.h' message. The only remaining will be 'configure is newer than
config.h' message, which is natural (when we modify configure, we
should rerun it).

I think, the purpose of storing REGISTER_*() in all*.c is to avoid
generating C-code from configure. Because now we generate C-code, it
is irrelevant. The size of configure isn't really a problem.


>
> 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?

I think, it will just add more complexity.

>
>>>
>>>>  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

Thank's.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Reply via email to