[FFmpeg-devel] [PATCH] lavfi: add curves filter.

[Clément Bœsch]

On Tue, Mar 05, 2013 at 07:11:10PM +0100, Stefano Sabatini wrote:
> On date Tuesday 2013-03-05 18:09:26 +0100, Clément Bœsch encoded:
[...]
> > +typedef struct {
> > +    const AVClass *class;
> > +    char *comp_dots_str[NB_COMP];
> > +    struct keydot *comp_dots[NB_COMP];
> > +    uint8_t graph[NB_COMP*256];
> 
> I think a distinct graph per component would be more readable:
> uint8_t graph[256][NB_COMP]
> 

Changed to graph[NB_COMP][256]

> > +} CurvesContext;
> > +
> > +#define OFFSET(x) offsetof(CurvesContext, x)
> > +#define FLAGS AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM
> > +static const AVOption curves_options[] = {
> > +    { "r",     "set red dots coordinates",   OFFSET(comp_dots_str[0]), AV_OPT_TYPE_STRING, {.str=NULL}, .flags = FLAGS },
> > +    { "red",   "set red dots coordinates",   OFFSET(comp_dots_str[0]), AV_OPT_TYPE_STRING, {.str=NULL}, .flags = FLAGS },
> > +    { "g",     "set green dots coordinates", OFFSET(comp_dots_str[1]), AV_OPT_TYPE_STRING, {.str=NULL}, .flags = FLAGS },
> > +    { "green", "set green dots coordinates", OFFSET(comp_dots_str[1]), AV_OPT_TYPE_STRING, {.str=NULL}, .flags = FLAGS },
> > +    { "b",     "set blue dots coordinates",  OFFSET(comp_dots_str[2]), AV_OPT_TYPE_STRING, {.str=NULL}, .flags = FLAGS },
> > +    { "blue",  "set blue dots coordinates",  OFFSET(comp_dots_str[2]), AV_OPT_TYPE_STRING, {.str=NULL}, .flags = FLAGS },
> > +    { NULL }
> > +};
> > +
> > +AVFILTER_DEFINE_CLASS(curves);
> > +
> 
> > +static struct keydot *new_dot(double x, double y, struct keydot *next)
> 
> Nit: I don't like C++ jargon, "make_dot" looks better to me.
> 

Renamed.

> > +{
> > +    struct keydot *dot = av_mallocz(sizeof(*dot));
> > +
> > +    if (!dot)
> > +        return NULL;
> > +    dot->x = x;
> > +    dot->y = y;
> > +    dot->next = next;
> > +    return dot;
> > +}
> 
> > +
> > +static int parse_dots_str(AVFilterContext *ctx, struct keydot **dots, const char *s)
> > +{
> > +    char *p = (char *)s; // strtod won't alter the string
> > +    struct keydot *last = NULL;
> > +
> > +    /* construct a linked list based on the key dots string */
> > +    while (p && *p) {
> > +        struct keydot *dot = new_dot(0, 0, NULL);
> > +        if (!dot)
> > +            return AVERROR(ENOMEM);
> 
> > +        dot->x = av_strtod(p, &p); if (p && *p) p++;
> 
> you're not checking for the separator here.
> 

Yeah, the idea is that if the documented separator ('/') gets replaced in
the future for some reason (more explicit one, conflict with a new
filtergraph syntax, etc) it will still works. Also, some people might
prefer some other character, but I still don't want to take the risk to
document them.

> > +        dot->y = av_strtod(p, &p); if (p && *p) p++;
> 
> what happens if the string is incomplete (p is NULL)?
> 

Actually it shouldn't happen (unless some strtod implem are setting p to
NULL) since it won't enter the main loop.

> > +        if (dot->x < 0 || dot->x > 1 || dot->y < 0 || dot->y > 1) {
> > +            av_log(ctx, AV_LOG_ERROR, "Invalid key dot coordinates (%f;%f), "
> > +                   "x and y must be in the [0;1] range.\n", dot->x, dot->y);
> > +            return AVERROR(EINVAL);
> > +        }
> > +        if (!*dots)
> > +            *dots = dot;
> > +        if (last) {
> > +            if ((int)(last->x * 255) >= (int)(dot->x * 255)) {
> > +                av_log(ctx, AV_LOG_WARNING, "Key dot coordinates (%f;%f) "
> 
> AV_LOG_ERROR?
> 

Fixed.

> > +                       "and (%f;%f) are too close from each other on the "
> > +                       "x-axis\n", last->x, last->y, dot->x, dot->y);
> > +                return AVERROR(EINVAL);
> > +            }
> > +            last->next = dot;
> > +        }
> > +        last = dot;
> > +    }
> > +
> 

I'll send a new patch later after honoring the other comments. Thanks for
the review.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130305/545f2851/attachment.asc>