[FFmpeg-devel] [PATCH] frei0r wrapper

[Måns Rullgård]

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.

>> 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?

> 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.

>> > +    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.

> 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.

> +#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.

> +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 ()

> +    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.

> +        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.

> +    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.

> +               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.

> +    memset(&frei0r, 0, sizeof(frei0r));
> +}
> +

-- 
M?ns Rullg?rd
mans at mansr.com