[FFmpeg-devel] [PATCH] libavfilter: constify filter list
Mark Thompson
sw at jkqxz.net
Tue Jan 30 16:09:43 EET 2018
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.
> 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?
> 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?
> };
>
> #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.
>
> - REGISTER_FILTER(NULLSINK, nullsink, vsink);
> +static AVOnce check_validity_once = AV_ONCE_INIT;
> +#define CHECK_VALIDITY() ff_thread_once(&check_validity_once, check_validity)
> +#else
> +#define CHECK_VALIDITY() ((void)0)
> +#endif
>
> - /* multimedia filters */
> - REGISTER_FILTER(ABITSCOPE, abitscope, avf);
> - REGISTER_FILTER(ADRAWGRAPH, adrawgraph, avf);
> - REGISTER_FILTER(AHISTOGRAM, ahistogram, avf);
> - REGISTER_FILTER(APHASEMETER, aphasemeter, avf);
> - REGISTER_FILTER(AVECTORSCOPE, avectorscope, avf);
> - REGISTER_FILTER(CONCAT, concat, avf);
> - REGISTER_FILTER(SHOWCQT, showcqt, avf);
> - REGISTER_FILTER(SHOWFREQS, showfreqs, avf);
> - REGISTER_FILTER(SHOWSPECTRUM, showspectrum, avf);
> - REGISTER_FILTER(SHOWSPECTRUMPIC, showspectrumpic, avf);
> - REGISTER_FILTER(SHOWVOLUME, showvolume, avf);
> - REGISTER_FILTER(SHOWWAVES, showwaves, avf);
> - REGISTER_FILTER(SHOWWAVESPIC, showwavespic, avf);
> - REGISTER_FILTER(SPECTRUMSYNTH, spectrumsynth, vaf);
> +void avfilter_register_all(void)
> +{
> + CHECK_VALIDITY();
> +}
>
> - /* multimedia sources */
> - REGISTER_FILTER(AMOVIE, amovie, avsrc);
> - REGISTER_FILTER(MOVIE, movie, avsrc);
> +const AVFilter *avfilter_next(const AVFilter *prev)
> +{
> + CHECK_VALIDITY();
Calling avfilter_next() without having called avfilter_register_all() violates the API, though?
(Or is there an intent to deprecate avfilter_register_all() immediately after this?)
> + return prev ? prev->next : filter_list[0];
> +}
>
> - /* those filters are part of public or internal API => registered
> - * unconditionally */
> - REGISTER_FILTER_UNCONDITIONAL(asrc_abuffer);
> - REGISTER_FILTER_UNCONDITIONAL(vsrc_buffer);
> - REGISTER_FILTER_UNCONDITIONAL(asink_abuffer);
> - REGISTER_FILTER_UNCONDITIONAL(vsink_buffer);
> - REGISTER_FILTER_UNCONDITIONAL(af_afifo);
> - REGISTER_FILTER_UNCONDITIONAL(vf_fifo);
> +static int compare_name(const void *key, const void *elem)
> +{
> + const char *name = key;
> + const AVFilter *const *filter = elem;
> + return strcmp(name, (*filter)->name);
> }
>
> -void avfilter_register_all(void)
> +const AVFilter *avfilter_get_by_name(const char *name)
> {
> - static AVOnce control = AV_ONCE_INIT;
> + const AVFilter **filter;
>
> - ff_thread_once(&control, register_all);
> + CHECK_VALIDITY();
> + filter = bsearch(name, filter_list, FF_ARRAY_ELEMS(filter_list) - 1,
> + sizeof(filter_list[0]), compare_name);
> + return filter ? *filter : NULL;
> }
> ...
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index ea75467a75..b89c28d57e 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -575,49 +575,10 @@ int avfilter_process_command(AVFilterContext *filter, const char *cmd, const cha
> return AVERROR(ENOSYS);
> }
>
> -static AVFilter *first_filter;
> -static AVFilter **last_filter = &first_filter;
> -
> -const AVFilter *avfilter_get_by_name(const char *name)
> -{
> - const AVFilter *f = NULL;
> -
> - if (!name)
> - return NULL;
> -
> - while ((f = avfilter_next(f)))
> - if (!strcmp(f->name, name))
> - return (AVFilter *)f;
> -
> - return NULL;
> -}
> -
> -static AVMutex filter_register_mutex = AV_MUTEX_INITIALIZER;
> -
> int avfilter_register(AVFilter *filter)
> {
> - AVFilter **f;
> -
> - /* the filter must select generic or internal exclusively */
> - av_assert0((filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE) != AVFILTER_FLAG_SUPPORT_TIMELINE);
> -
> - ff_mutex_lock(&filter_register_mutex);
> - f = last_filter;
> -
> - while (*f)
> - f = &(*f)->next;
> - *f = filter;
> - filter->next = NULL;
> - last_filter = &filter->next;
> -
> - ff_mutex_unlock(&filter_register_mutex);
> -
> - return 0;
> -}
> -
> -const AVFilter *avfilter_next(const AVFilter *prev)
> -{
> - return prev ? prev->next : first_filter;
> + av_log(NULL, AV_LOG_ERROR, "External filter registration is currently unsupported.\n");
> + return AVERROR(EINVAL);
+1 to explicitly blocking external filter registration.
> }
>
> int avfilter_pad_count(const AVFilterPad *pads)
> diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> index 62eed2168f..24e46a9b40 100644
> --- a/libavfilter/avfilter.h
> +++ b/libavfilter/avfilter.h
> @@ -289,7 +289,7 @@ typedef struct AVFilter {
> * Used by the filter registration system. Must not be touched by any other
> * code.
> */
> - struct AVFilter *next;
> + const struct AVFilter *next;
>
> /**
> * Make the filter instance process a command.
>
> ...
> diff --git a/libavfilter/vf_datascope.c b/libavfilter/vf_datascope.c
> index 467663556e..bb9e2befe2 100644
> --- a/libavfilter/vf_datascope.c
> +++ b/libavfilter/vf_datascope.c
> @@ -390,6 +390,7 @@ static int config_output(AVFilterLink *outlink)
> return 0;
> }
>
> +#if CONFIG_DATASCOPE_FILTER
This sort of fixup should be an independent change.
> static const AVFilterPad inputs[] = {
> {
> .name = "default",
> @@ -409,7 +410,7 @@ static const AVFilterPad outputs[] = {
> { NULL }
> };
>
> -AVFilter ff_vf_datascope = {
> +const AVFilter ff_vf_datascope = {
> .name = "datascope",
> .description = NULL_IF_CONFIG_SMALL("Video data analysis."),
> .priv_size = sizeof(DatascopeContext),
> @@ -418,7 +419,9 @@ AVFilter ff_vf_datascope = {
> .inputs = inputs,
> .outputs = outputs,
> .flags = AVFILTER_FLAG_SLICE_THREADS,
> + .next = ff_next_vf_datascope,
> };
> +#endif /* CONFIG_DATASCOPE_FILTER */
>
> typedef struct PixscopeContext {
> const AVClass *class;
> @@ -642,6 +645,7 @@ static int pixscope_filter_frame(AVFilterLink *inlink, AVFrame *in)
> return ff_filter_frame(outlink, out);
> }
>
> +#if CONFIG_PIXSCOPE_FILTER
> static const AVFilterPad pixscope_inputs[] = {
> {
> .name = "default",
> @@ -660,7 +664,7 @@ static const AVFilterPad pixscope_outputs[] = {
> { NULL }
> };
>
> -AVFilter ff_vf_pixscope = {
> +const AVFilter ff_vf_pixscope = {
> .name = "pixscope",
> .description = NULL_IF_CONFIG_SMALL("Pixel data analysis."),
> .priv_size = sizeof(PixscopeContext),
> @@ -669,7 +673,9 @@ AVFilter ff_vf_pixscope = {
> .inputs = pixscope_inputs,
> .outputs = pixscope_outputs,
> .flags = AVFILTER_FLAG_SUPPORT_TIMELINE_GENERIC,
> + .next = ff_next_vf_pixscope,
> };
> +#endif /* CONFIG_PIXSCOPE_FILTER */
>
> typedef struct PixelValues {
> uint16_t p[4];
> @@ -1022,6 +1028,7 @@ static int oscilloscope_filter_frame(AVFilterLink *inlink, AVFrame *frame)
> return ff_filter_frame(outlink, frame);
> }
>
> +#if CONFIG_OSCILLOSCOPE_FILTER
> static const AVFilterPad oscilloscope_inputs[] = {
> {
> .name = "default",
> @@ -1041,7 +1048,7 @@ static const AVFilterPad oscilloscope_outputs[] = {
> { NULL }
> };
>
> -AVFilter ff_vf_oscilloscope = {
> +const AVFilter ff_vf_oscilloscope = {
> .name = "oscilloscope",
> .description = NULL_IF_CONFIG_SMALL("2D Video Oscilloscope."),
> .priv_size = sizeof(OscilloscopeContext),
> @@ -1051,4 +1058,6 @@ AVFilter ff_vf_oscilloscope = {
> .inputs = oscilloscope_inputs,
> .outputs = oscilloscope_outputs,
> .flags = AVFILTER_FLAG_SUPPORT_TIMELINE_GENERIC,
> + .next = ff_next_vf_oscilloscope,
> };
> +#endif /* CONFIG_OSCILLOSCOPE_FILTER */
> ...
> diff --git a/tests/checkasm/Makefile b/tests/checkasm/Makefile
> index afbd09b940..4a96159c1a 100644
> --- a/tests/checkasm/Makefile
> +++ b/tests/checkasm/Makefile
> @@ -61,7 +61,7 @@ tests/checkasm/checkasm.o: CFLAGS += -Umain
> CHECKASM := tests/checkasm/checkasm$(EXESUF)
>
> $(CHECKASM): $(CHECKASMOBJS) $(FF_STATIC_DEP_LIBS)
> - $(LD) $(LDFLAGS) $(LDEXEFLAGS) $(LD_O) $(CHECKASMOBJS) $(FF_STATIC_DEP_LIBS) $(EXTRALIBS-avcodec) $(EXTRALIBS-avfilter) $(EXTRALIBS-avutil) $(EXTRALIBS)
> + $(LD) $(LDFLAGS) $(LDEXEFLAGS) $(LD_O) $(CHECKASMOBJS) $(FF_STATIC_DEP_LIBS) $(EXTRALIBS-avcodec) $(EXTRALIBS-avformat) $(EXTRALIBS-avfilter) $(EXTRALIBS-avutil) $(EXTRALIBS)
>
> checkasm: $(CHECKASM)
>
>
Thanks,
- Mark
More information about the ffmpeg-devel
mailing list