On Wed, Jan 31, 2018 at 5:26 AM, Mark Thompson <[email protected]> wrote:
> On 30/01/18 18:06, Muhammad Faiz wrote:
>> On Tue, Jan 30, 2018 at 9:09 PM, Mark Thompson <[email protected]> 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 <[email protected]>
>>>> ---
>>>
>>> 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
[email protected]
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
