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

[Stefano Sabatini]

On date Friday 2010-09-24 08:13:51 +0100, M?ns Rullg?rd wrote:
> 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.

POSIX => dlopen
but not:
dlopen => POSIX

so if dlopen is present you can't infer that strtok_r exists.

And note that we already have a check for strerror_r, which is
supported by POSIX but is not supported in MinGW, following the same
logic it makes sense to keep the check for strtok_r.
 
> > +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?

Good point, it can be removed.
 
> > +        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?

Basically I rewrote it to be more strict with the frei0r specs.

Regards.
-- 
FFmpeg = Fierce & Faithless Mythic Prodigious Emblematic Gospel