[FFmpeg-devel] [PATCH] frei0r wrapper

[Stefano Sabatini]

On date Sunday 2010-09-05 23:36:45 +0200, Michael Niedermayer encoded:
> On Sun, Sep 05, 2010 at 10:20:50PM +0200, Stefano Sabatini wrote:
> > On date Sunday 2010-09-05 18:54:17 +0200, Michael Niedermayer encoded:
> > > On Sun, Sep 05, 2010 at 05:33:32PM +0200, Stefano Sabatini wrote:
> > > > On date Sunday 2010-09-05 15:43:23 +0200, Michael Niedermayer encoded:
> > > > > On Mon, Aug 30, 2010 at 08:32:01PM +0200, Stefano Sabatini wrote:
> > > > > [...]
> > > > > 
> > > > > > +
> > > > > > +where @var{filter_path} is the path to the frei0r effect to load, and
> > > > > > + at var{param1}, @var{param2}, ... , @var{paramN} is a list of values
> > > > > > +separated by ":" which specify the parameters for the frei0r effect.
> > > > > > +
> > > > > > +frei0r filters are usually installed in @file{/usr/lib/frei0r-1/}.
> > > > > > +
> > > > > > +A frei0r effect parameter can be a boolean (whose values are specified
> > > > > > +with "true" and "false"), a double, a color (specified by the syntax
> > > > > > + at var{R} @var{G} @var{B}), a position (specified by the syntax
> > > > > > + at var{x} @var{y}) and a string.
> > > > > > +
> > > > > > +The number and kind of parameters depend on the loaded effect. If an
> > > > > > +effect parameter is not specified the default value is set.
> > > > > > +
> > > > > > +Follows some example:
> > > > > > + at example
> > > > > > +# apply the distort0r effect, set the first two double parameters
> > > > > > +frei0r=/usr/lib/frei0r-1/distort0r.so:0.5:0.01
> > > > > 
> > > > > requirering full pathes is lame when all filters are installed in like
> > > > > one place
> > > > 
> > > > Depends on the distro, also you can have the distro filters in one
> > > > place and your own filters into another dir.
> > > 
> > > what are you trying to say?
> > > are you always writing
> > > /bin/ls because excutables can be in several locations and they can be
> > > different according to distro?
> > 
> > That depends on PATH, I added a note in the docs telling the user that
> > she can extend her LD_LIBRARY_PATH in order to avoid to specify the
> > complete effect path.
> 
> IMHO this is still a very poor choice
> its quite unpractical to require users to manually reconfigure their system
> because some developer refuses to write the 3 standard pathes in the code.
> and thats for a odd feature, if every filter required that setup of ffmpeg
> would not be fun anymore

Added a list of prefixes to try.

> [...]
> > > > > > +    double d;
> > > > > > +    f0r_param_color_t col;
> > > > > > +    f0r_param_position_t pos;
> > > > > > +    f0r_set_param_value_f f0r_set_param_value;
> > > > > > +
> > > > > > +    if (!(f0r_set_param_value = load_sym(ctx, "f0r_set_param_value")))
> > > > > > +        return AVERROR(EINVAL);
> > > > > > +
> > > > > 
> > > > > > +    switch (info.type) {
> > > > > > +    case F0R_PARAM_BOOL:
> > > > > 
> > > > > > +    {
> > > > > 
> > > > > is that way of placing it consistent?
> > 
> > Maybe not K&R but helps readability in this case.
> 
> not for me, but i dont really care about it, if you prefer to use non K&R
> style now ill be the last person to make a fuzz about it, ive better things
> to do than {} placement discussions
> 
> 
> > 
> > > > > 
> > > > >     
> > > > > > +        if      (!strcmp(param, "true" )) d = 1.0;
> > > > > > +        else if (!strcmp(param, "false")) d = 0.0;
> > > > > > +        else goto fail;
> > > > > 
> > > > > we should allow something shorter than true false
> > > > 
> > > > on/off? Imho "true"/"false" is more readable then "1"/"0", especially
> > > > when you have a long list of params.
> > > 
> > > a long list of params like:
> > > true:false:false:true:true:true:false:true:false:true:false:true:true:false
> > > vs.
> > > y:n:n:y:y:y:n:y:n:y:n:y:y:n
> > > 
> > > ?
> > 
> > Used y/n.
> 
> thanks
> 
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > +        v = (void *)(&d);
> > > > > 
> > > > > the cast looks unneeded
> > 
> > Removed all useless casts.
> > 
> > > > > > +    }
> > > > > > +    break;
> > > > > > +
> > > > > > +    case F0R_PARAM_DOUBLE:
> > > > > > +    {
> > > > > > +        char *tail;
> > > > > > +        ld = strtold(param, &tail);
> > > > > > +        if (*tail || ld > DBL_MAX || ld < -DBL_MAX)
> > > > > > +            goto fail;
> > > > > > +        d = ld;
> > > > > > +        v = (void *)(&d);
> > > > > > +    }
> > > > > > +    break;
> > > > > > +
> > > > > 
> > > > > > +    case F0R_PARAM_COLOR:
> > > > > > +    {
> > > > > > +        if (sscanf(param, "%f %f %f", &col.r, &col.g, &col.b) != 3)
> > > > > > +            goto fail;
> > > > > > +        v = (void *)(&col);
> > > > > 
> > > > > dont the spaces need escaping on the command line?
> > > > 
> > > > Yes, suggestion for a better way of describing a color are welcome,
> > > > maybe r.g.b?
> > > 
> > > use what html does
> > 
> > I'm using R/G/B for keeping as close as possible to the frei0r API.
> 
> i dont mind but the html way of specifyimg colors should be supported too
> because its more standard

Added fallback av_parse_color() parsing.

Regards.
-- 
FFmpeg = Fast and Fundamentalist MultiPurpose Elitist Gadget