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.
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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
