[FFmpeg-devel] [PATCH] frei0r wrapper
Stefano Sabatini
stefano.sabatini-lala
Mon Sep 13 13:18:53 CEST 2010
On date Monday 2010-09-13 10:31:52 +0100, M?ns Rullg?rd encoded:
> Stefano Sabatini <stefano.sabatini-lala at poste.it> writes:
>
> >> > +typedef struct Frei0rContext {
> >> > + f0r_update_f update;
> >> > + void *dll;
> >> > + char dll_filename[128];
> >>
> >> 128 is a bit short for a filename.
> >
> > Increased to 256.
>
> Still easily exceeded. 1024 is the bare minimum I'd use. The system
> limit is often 4096.
Increased to 1024.
> >> Also, does it really need to be stored here?
> >
> > It was to help error feedback, but I found an alternative way which
> > doesn't require that.
> >
> >> Also "dll" has an unpleasant Windows ring to it.
> >
> > Stands for Dynamic Linking Loader so no Windows specific,
>
> Have you ever seen another system use the DLL acronym?
Well, during the first write I was referring to the code of veejay,
which was using that acronym.
> > anyway changed to dl_handle.
> >
> >> > +static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
> >> > +{
> >> > + Frei0rContext *frei0r = ctx->priv;
> >> > + f0r_init_f f0r_init;
> >> > + f0r_get_plugin_info_f f0r_get_plugin_info;
> >> > + f0r_plugin_info_t *pi;
> >> > + int i;
> >> > + const char *dll_prefixes[]= {
> >> > + "", "/usr/lib/frei0r-1/", "/usr/local/lib/frei0r-1/"
> >> > + };
> >>
> >> That's a rather limited set. Is there some environment variable
> >> customarily used to find these files? If so, use it. If not, pick a
> >> suitable name and use that.
> >
> > Checked the specs, now I'm using FREI0R_PATH.
>
> Where is this information? Just curious.
http://piksel.org/frei0r/1.2/spec/1.2/spec/group__pluglocations.html
Referenced in the code.
> >> > + char dll_filename[128];
> >> > +
> >> > + *(frei0r->params) = 0;
> >> > +
> >> > + if (args)
> >> > + sscanf(args, "%127[^:]:%255c", dll_filename, frei0r->params);
> >> > +
> >> > + for (i = 0; i < FF_ARRAY_ELEMS(dll_prefixes); i++) {
> >> > + snprintf(frei0r->dll_filename, sizeof(frei0r->dll_filename), "%s%s", dll_prefixes[i], dll_filename);
> >> > + if ((frei0r->dll = dlopen(frei0r->dll_filename, RTLD_NOW)))
> >>
> >> Why RTLD_NOW? You also should specify one of RTLD_GLOBAL or
> >> RTLD_LOCAL. I suggest the latter.
> >
> > Used RTLD_LAZY|RTLD_LOCAL, note that I'm not such an expert and I
> > didn't noticed any difference so I'm simply trusting your word.
>
> RTLD_LOCAL is a good choice for loading self-contained plugins. As
> for LAZY vs NOW, LAZY loads faster, but if an undefined symbol can't
> be resolved when it is first referenced, the entire application will
> die. With RTLD_NOW, dlopen() reports an error if any symbol fails to
> be resolved, and the caller has a chance to handle it. The choice
> depends on how much you trust the loaded modules to be correct and
> on the impact of aborting later.
>
> For on-demand loading of third-party plugins into a larger
> application, e.g. a web browser, RTLD_NOW would be preferred to avoid
> having the entire browser die from attempting to use a bad plugin.
>
> In our case, little is lost even if it dies during filtering. OTOH,
> the plugins probably have few external dependencies and thus should
> load quickly even with RTLD_NOW. Chose whichever you want.
RTLD_NOW seems more robust (more robust is better than faster and
brittle) and there is a non-trascurable chance that that code will
effectively be embedded in a browser (e.g. chromium).
> > Updated patch in attachment.
> > --
> > FFmpeg = Fascinating & Funny Mastering Pacific Elastic Guru
> >
> > From 4cf8bce742c580dae100ffd2f523ccf1769afe7d Mon Sep 17 00:00:00 2001
> > From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> > Date: Tue, 24 Aug 2010 19:48:57 +0200
> > Subject: [PATCH] Add frei0r filter.
> >
> > ---
> > configure | 9 ++
> > doc/filters.texi | 47 ++++++
> > libavfilter/Makefile | 1 +
> > libavfilter/allfilters.c | 1 +
> > libavfilter/vf_frei0r.c | 358 ++++++++++++++++++++++++++++++++++++++++++++++
> > 5 files changed, 416 insertions(+), 0 deletions(-)
> > create mode 100644 libavfilter/vf_frei0r.c
> >
> > diff --git a/configure b/configure
> > index ae3d738..9bb8b51 100755
> > --- a/configure
> > +++ b/configure
> > @@ -162,6 +162,7 @@ Configuration options:
> > External library support:
> > --enable-avisynth enable reading of AVISynth script files [no]
> > --enable-bzlib enable bzlib [autodetect]
> > + --enable-frei0r enable frei0r video filtering
> > --enable-libopencore-amrnb enable AMR-NB de/encoding via libopencore-amrnb [no]
> > --enable-libopencore-amrwb enable AMR-WB decoding via libopencore-amrwb [no]
> > --enable-libdc1394 enable IIDC-1394 grabbing using libdc1394
> > @@ -872,6 +873,7 @@ CONFIG_LIST="
> > ffprobe
> > ffserver
> > fft
> > + frei0r
> > golomb
> > gpl
> > gray
> > @@ -1047,6 +1049,7 @@ HAVE_LIST="
> > poll_h
> > setrlimit
> > strerror_r
> > + strtok_r
> > struct_addrinfo
> > struct_ipv6_mreq
> > struct_sockaddr_in6
> > @@ -1379,6 +1382,9 @@ vfwcap_indev_extralibs="-lavicap32"
> > x11_grab_device_indev_deps="x11grab XShmCreateImage"
> > x11_grab_device_indev_extralibs="-lX11 -lXext -lXfixes"
> >
> > +# filters
> > +frei0r_filter_deps="frei0r dlopen strtok_r"
> > +
> > # protocols
> > gopher_protocol_deps="network"
> > http_protocol_deps="network"
> > @@ -2631,6 +2637,7 @@ check_func mkstemp
> > check_func ${malloc_prefix}posix_memalign && enable posix_memalign
> > check_func setrlimit
> > check_func strerror_r
> > +check_func strtok_r
> > check_func_headers io.h setmode
> > check_func_headers lzo/lzo1x.h lzo1x_999_compress
> > check_lib2 "windows.h psapi.h" GetProcessMemoryInfo -lpsapi
>
> strtok_r is a standard POSIX function. There is no need to check for it.
IIRC there are other functions which are POSIX and nonetheless are not
included by some of the supported systems (strerror_r and MinGW).
>
> > +#include <dlfcn.h>
> > +#include <frei0r.h>
> > +#include <float.h>
>
> What do you need float.h for? I don't see anything from it being used.
Removed.
> > +static void *load_path(AVFilterContext *ctx, const char *prefix, const char *name)
> > +{
> > + char path[256];
> > +
> > + snprintf(path, sizeof(path), "%s%s", prefix, name);
> > + av_log(ctx, AV_LOG_DEBUG, "Looking for frei0r effect in '%s'\n", path);
> > + return dlopen(path, RTLD_LAZY|RTLD_LOCAL);
> > +}
> > +
> > +static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
> > +{
> > + Frei0rContext *frei0r = ctx->priv;
> > + f0r_init_f f0r_init;
> > + f0r_get_plugin_info_f f0r_get_plugin_info;
> > + f0r_plugin_info_t *pi;
> > + char dl_name[256], *frei0r_path;
> > +
> > + *(frei0r->params) = 0;
>
> Useless ()
Fixed.
> > + if (args)
> > + sscanf(args, "%255[^:]:%255c", dl_name, frei0r->params);
> > +
> > + if (frei0r_path = av_strdup(getenv("FREI0R_PATH"))) {
>
> GCC warns about assignment in conditional here. Please add some
> parens to shut it up.
Fixed.
> > + char *p, *ptr = NULL;
> > + for (p = frei0r_path; p = strtok_r(p, ":", &ptr); p = NULL)
> > + if (frei0r->dl_handle = load_path(ctx, p, dl_name))
> > + break;
> > + av_free(frei0r_path);
> > + }
> > +
> > + if (!frei0r->dl_handle)
> > + frei0r->dl_handle = load_path(ctx, "", dl_name);
>
> Do not search FREI0R_PATH for absolute pathname arguments.
Removed.
> > + if (!frei0r->dl_handle) {
> > + av_log(ctx, AV_LOG_ERROR, "Could not find module '%s'\n", dl_name);
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + if (!(f0r_init = load_sym(ctx, "f0r_init" )) ||
> > + !(f0r_get_plugin_info = load_sym(ctx, "f0r_get_plugin_info")) ||
> > + !(frei0r->get_param_info = load_sym(ctx, "f0r_get_param_info" )) ||
> > + !(frei0r->get_param_value = load_sym(ctx, "f0r_get_param_value")) ||
> > + !(frei0r->set_param_value = load_sym(ctx, "f0r_set_param_value")) ||
> > + !(frei0r->update = load_sym(ctx, "f0r_update" )) ||
> > + !(frei0r->construct = load_sym(ctx, "f0r_construct" )) ||
> > + !(frei0r->destruct = load_sym(ctx, "f0r_destruct" )) ||
> > + !(frei0r->deinit = load_sym(ctx, "f0r_deinit" )))
> > + return AVERROR(EINVAL);
> > +
> > + if (f0r_init() < 0) {
> > + av_log(ctx, AV_LOG_ERROR, "Could not init the frei0r module");
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + f0r_get_plugin_info(&frei0r->plugin_info);
> > + pi = &frei0r->plugin_info;
> > + if (pi->plugin_type != F0R_PLUGIN_TYPE_FILTER) {
> > + av_log(ctx, AV_LOG_ERROR,
> > + "Invalid type '%s' for the plugin, a filter plugin was expected\n",
> > + pi->plugin_type == F0R_PLUGIN_TYPE_FILTER ? "filter" :
>
> The type will never be filter here.
Removed.
> > + pi->plugin_type == F0R_PLUGIN_TYPE_SOURCE ? "source" :
> > + pi->plugin_type == F0R_PLUGIN_TYPE_MIXER2 ? "mixer2" :
> > + pi->plugin_type == F0R_PLUGIN_TYPE_MIXER3 ? "mixer3" : "unknown");
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + av_log(ctx, AV_LOG_INFO,
> > + "name:%s author:'%s' explanation:'%s' color_model:%s "
> > + "frei0r_version:%d version:%d.%d num_params:%d\n",
> > + pi->name, pi->author, pi->explanation,
> > + pi->color_model == F0R_COLOR_MODEL_BGRA8888 ? "bgra8888" :
> > + pi->color_model == F0R_COLOR_MODEL_RGBA8888 ? "rgba8888" :
> > + pi->color_model == F0R_COLOR_MODEL_PACKED32 ? "packed32" : "unknown",
> > + pi->frei0r_version, pi->major_version, pi->minor_version, pi->num_params);
> > +
> > + return 0;
> > +}
> > +
> > +static av_cold void uninit(AVFilterContext *ctx)
> > +{
> > + Frei0rContext *frei0r = ctx->priv;
> > +
> > + if (frei0r->destruct)
> > + frei0r->destruct(frei0r->instance);
> > + if (frei0r->deinit)
> > + frei0r->deinit();
> > + if (frei0r->dl_handle)
> > + dlclose(frei0r->dl_handle);
>
> Is uninit() called even if init() fails? If not, the ifs above are
> unnecessary.
You can do avfilter_open(), and then call avfilter_destroy() without
to call avfilter_init().
Regards.
--
FFmpeg = Frightening Fostering Meaningless Programmable Extravagant God
More information about the ffmpeg-devel
mailing list