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