[FFmpeg-devel] [PATCH] frei0r wrapper

[Stefano Sabatini]

On date Wednesday 2010-09-08 16:16:59 +0100, M?ns Rullg?rd encoded:
> Stefano Sabatini <stefano.sabatini-lala at poste.it> writes:
> 
> >  configure                |    8 +
> >  doc/filters.texi         |   46 ++++++
> >  libavfilter/Makefile     |    1 +
> >  libavfilter/allfilters.c |    1 +
> >  libavfilter/vf_frei0r.c  |  353 ++++++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 409 insertions(+), 0 deletions(-)
> >  create mode 100644 libavfilter/vf_frei0r.c
> >
> > diff --git a/configure b/configure
> > index ae3d738..7a2117d 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
> > @@ -1006,6 +1008,7 @@ HAVE_LIST="
> >      fast_cmov
> >      fcntl
> >      fork
> > +    frei0r
> >      getaddrinfo
> >      gethrtime
> >      GetProcessMemoryInfo
> 
> You can't have something in both of those lists.  This one should be
> in CONFIG_LIST only.

Fixed.

> 
> > @@ -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"
> > +
> >  # protocols
> >  gopher_protocol_deps="network"
> >  http_protocol_deps="network"
> > @@ -2704,6 +2710,7 @@ check_mathfunc truncf
> >  
> >  # these are off by default, so fail if requested and not available
> >  enabled avisynth   && require2 vfw32 "windows.h vfw.h" AVIFileInit -lavifil32
> > +enabled frei0r     && check_header frei0r.h
> 
> || die "suitable message"

Fixed.
 
> > diff --git a/libavfilter/vf_frei0r.c b/libavfilter/vf_frei0r.c
> > new file mode 100644
> > index 0000000..4cb1cd5
> > --- /dev/null
> > +++ b/libavfilter/vf_frei0r.c
> > @@ -0,0 +1,353 @@
> > +/*
> > + * Stefano Sabatini 2010
> > + * This file is part of FFmpeg.
> > + *
> > + * FFmpeg is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * FFmpeg is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with FFmpeg; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> > + */
> > +
> > +/**
> > + * @file
> > + * frei0r wrapper
> > + */
> > +
> > +/* #define DEBUG */
> > +
> > +#include <dlfcn.h>
> > +#include <frei0r.h>
> > +#include <float.h>
> > +#include "avfilter.h"
> > +#include "parseutils.h"
> > +
> > +typedef f0r_instance_t (*f0r_construct_f)(unsigned int width, unsigned int height);
> > +typedef void (*f0r_destruct_f)(f0r_instance_t instance);
> > +typedef void (*f0r_deinit_f)(void);
> > +typedef int (*f0r_init_f)(void);
> > +typedef void (*f0r_get_plugin_info_f)(f0r_plugin_info_t *info);
> > +typedef void (*f0r_get_param_info_f)(f0r_param_info_t *info, int param_index);
> > +typedef void (*f0r_update_f)(f0r_instance_t instance, double time, const uint32_t *inframe, uint32_t *outframe);
> > +typedef void (*f0r_update2_f)(f0r_instance_t instance, double time, const uint32_t *inframe1, const uint32_t *inframe2, const uint32_t *inframe3, uint32_t *outframe);
> > +typedef void (*f0r_set_param_value_f)(f0r_instance_t instance, f0r_param_t param, int param_index);
> > +typedef void (*f0r_get_param_value_f)(f0r_instance_t instance, f0r_param_t param, int param_index);
> > +
> > +typedef struct Frei0rContext {
> > +    f0r_update_f update;
> > +    void *dll;
> > +    char dll_filename[128];
> 
> 128 is a bit short for a filename.

Increased to 256. 

> 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, anyway
changed to dl_handle.

> > +    f0r_instance_t instance;
> > +    f0r_plugin_info_t plugin_info;
> > +
> > +    f0r_get_param_info_f  get_param_info;
> > +    f0r_get_param_value_f get_param_value;
> > +    f0r_set_param_value_f set_param_value;
> > +    f0r_construct_f       construct;
> > +    f0r_destruct_f        destruct;
> > +    f0r_deinit_f          deinit;
> > +    char params[256];
> > +} Frei0rContext;
> > +
> > +static void *load_sym(AVFilterContext *ctx, const char *sym_name)
> > +{
> > +    Frei0rContext *frei0r = ctx->priv;
> > +    void *sym = dlsym(frei0r->dll, sym_name);
> > +    if (!sym)
> > +        av_log(ctx, AV_LOG_ERROR,
> > +               "Could not find symbol '%s' in module '%s'\n", sym_name, frei0r->dll_filename);
> > +    return sym;
> > +}
> > +
> > +static int set_param(AVFilterContext *ctx, f0r_param_info_t info, int index, char *param)
> > +{
> > +    Frei0rContext *frei0r = ctx->priv;
> > +    union {
> > +        double d;
> > +        f0r_param_color_t col;
> > +        f0r_param_position_t pos;
> > +    } val;
> > +
> > +    switch (info.type) {
> > +    case F0R_PARAM_BOOL:
> > +        if      (!strcmp(param, "y")) val.d = 1.0;
> > +        else if (!strcmp(param, "n")) val.d = 0.0;
> > +        else goto fail;
> > +    break;
> 
> Strange indentation.
> 
> > +    case F0R_PARAM_DOUBLE:
> > +    {
> > +        char *tail;
> > +        val.d = strtod(param, &tail);
> > +        if (*tail || val.d == HUGE_VAL)
> > +            goto fail;
> > +    }
> > +    break;
> 
> Ditto.
> 
> > +    case F0R_PARAM_COLOR:
> > +        if (sscanf(param, "%f/%f/%f", &val.col.r, &val.col.g, &val.col.b) != 3) {
> > +            uint8_t rgba[4];
> > +            if (av_parse_color(rgba, param, ctx) < 0)
> > +                goto fail;
> > +            val.col.r = rgba[0] / 255.0;
> > +            val.col.g = rgba[1] / 255.0;
> > +            val.col.b = rgba[2] / 255.0;
> > +        }
> > +    break;
> 
> And here.
> 
> > +    case F0R_PARAM_POSITION:
> > +        if (sscanf(param, "%lf/%lf", &val.pos.x, &val.pos.y) != 2)
> > +            goto fail;
> > +    break;
> 
> Here too.

Fixed weird indentation.
 
> > +    default: /* F0R_PARAM_STRING */
> > +        break;
> 
> But not here.  However, the default label is rather pointless since it
> contains no code.

Removed.

> > +    }
> > +
> > +    frei0r->set_param_value(frei0r->instance, &val, index);
> > +    return 0;
> > +
> > +fail:
> > +    av_log(ctx, AV_LOG_ERROR, "Invalid value '%s' for parameter '%s'",
> > +           param, info.name);
> > +    return AVERROR(EINVAL);
> > +}
> > +
> 
> [...]
> 
> > +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.

> 
> > +    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.
> 
> > +            break;
> > +    }
> > +
> > +    if (!frei0r->dll) {
> > +        av_log(ctx, AV_LOG_ERROR, "Could not find module '%s'\n", dll_filename);
> > +        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);
> 
> Useless ().

Removed.

Updated patch in attachment.
-- 
FFmpeg = Fascinating & Funny Mastering Pacific Elastic Guru