[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 Sabatini <stefano.sabatini-lala at poste.it> writes:

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

We check for strerror_r because it causes an actual build failure if
we do not.  That is not the case for strtok_r.  Gratuitous checks only
make configure run slower and bloat the build system for no gain.

For just about any function we use, no matter how standard, there
surely exists some system, somewhere that doesn't have it.

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

Without bothering to respond to my comments, and without posting it
for review.

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