[FFmpeg-cvslog] r25165 - in trunk: Changelog configure doc/filters.texi libavfilter/Makefile libavfilter/allfilters.c libavfilter/avfilter.h libavfilter/vf_frei0r.c

[Måns Rullgård]

stefano <subversion at mplayerhq.hu> writes:

> Author: stefano
> Date: Fri Sep 24 02:32:22 2010
> New Revision: 25165
>
> Log:
> Add frei0r filter.
>
> See thread:
> Subject: [FFmpeg-devel] [POC] frei0r wrapper
> Date: Tue, 24 Aug 2010 21:37:32 +0200
>
> Added:
>    trunk/libavfilter/vf_frei0r.c
> Modified:
>    trunk/Changelog
>    trunk/configure
>    trunk/doc/filters.texi
>    trunk/libavfilter/Makefile
>    trunk/libavfilter/allfilters.c
>    trunk/libavfilter/avfilter.h
>
> Modified: trunk/Changelog
> ==============================================================================
> --- trunk/Changelog	Thu Sep 23 23:33:29 2010	(r25164)
> +++ trunk/Changelog	Fri Sep 24 02:32:22 2010	(r25165)
> @@ -36,6 +36,7 @@ version <next>:
>  - G.722 ADPCM audio encoder/decoder
>  - R10k video decoder
>  - ocv_smooth filter
> +- frei0r wrapper filter
>
>  version 0.6:
>
> Modified: trunk/configure
> ==============================================================================
> --- trunk/configure	Thu Sep 23 23:33:29 2010	(r25164)
> +++ trunk/configure	Fri Sep 24 02:32:22 2010	(r25165)
> @@ -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-libopencv       enable video filtering via libopencv [no]
> @@ -873,6 +874,7 @@ CONFIG_LIST="
>      ffprobe
>      ffserver
>      fft
> +    frei0r
>      golomb
>      gpl
>      gray
> @@ -1050,6 +1052,7 @@ HAVE_LIST="
>      poll_h
>      setrlimit
>      strerror_r
> +    strtok_r
>      struct_addrinfo
>      struct_ipv6_mreq
>      struct_sockaddr_in6

I told you not to check for strtok_r.  It exists on all POSIX systems,
and frei0r doesn't work anywhere else (dlopen).  Please remove this
pointless check, and also please respect the review comments in future.

> +static void *load_path(AVFilterContext *ctx, const char *prefix, const char *name)
> +{
> +    char path[1024];
> +
> +    snprintf(path, sizeof(path), "%s%s.so", prefix, name);
> +    av_log(ctx, AV_LOG_DEBUG, "Looking for frei0r effect in '%s'\n", path);
> +    return dlopen(path, RTLD_NOW|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[1024], *path;
> +
> +    *frei0r->params = 0;
> +
> +    if (args)
> +        sscanf(args, "%1023[^:]:%255c", dl_name, frei0r->params);
> +
> +    /* see: http://piksel.org/frei0r/1.2/spec/1.2/spec/group__pluglocations.html */
> +    if ((path = av_strdup(getenv("FREI0R_PATH")))) {
> +        char *p, *ptr = NULL;
> +        for (p = path; p = strtok_r(p, ":", &ptr); p = NULL)
> +            if (frei0r->dl_handle = load_path(ctx, p, dl_name))
> +                break;
> +        av_free(path);
> +    }
> +    if (!frei0r->dl_handle && (path = av_strdup(getenv("HOME")))) {

Why strdup?

> +        char prefix[1024];
> +        snprintf(prefix, sizeof(prefix), "%s/.frei0r-1/lib/", path);
> +        frei0r->dl_handle = load_path(ctx, prefix, dl_name);
> +        av_free(path);
> +    }
> +    if (!frei0r->dl_handle)
> +        frei0r->dl_handle = load_path(ctx, "/usr/local/lib/frei0r-1/", dl_name);
> +    if (!frei0r->dl_handle)
> +        frei0r->dl_handle = load_path(ctx, "/usr/lib/frei0r-1/", dl_name);

You have changed this substantially from what was reviewed, and not in
the way I requested.  What is the point of reviews if you are going to
ignore them?

> +    if (!frei0r->dl_handle) {
> +        av_log(ctx, AV_LOG_ERROR, "Could not find module '%s'\n", dl_name);
> +        return AVERROR(EINVAL);
> +    }
> +

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