[FFmpeg-devel] [PATCH] lavfi/hue: apply major simplifications, and switch to AVOption-based system

[Clément Bœsch]

[...]
> From c6e9df87ca91b4d90b1c06f088cbeffd746ca65c Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefasab at gmail.com>
> Date: Thu, 11 Apr 2013 01:12:33 +0200
> Subject: [PATCH] lavfi/hue: apply major simplifications, and switch to
>  AVOption-based system
> 
> This also drops support for "flat syntax" and "reinit" command.
> 

Fine with me.

> "reinit" command is not very robust and complicates the logic more than
> necessary, since requires to reset all the options in the command.
> 

This reinit thing should be handled differently anyway. It should be done
generically by calling again a one or more callbacks.

> *This is a syntax break*.
> ---
>  doc/filters.texi       |   40 +++++----------
>  libavfilter/avfilter.c |    1 -
>  libavfilter/vf_hue.c   |  127 +++++++++++++++++-------------------------------
>  3 files changed, 58 insertions(+), 110 deletions(-)
> 
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 3438308..b990b52 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -3653,24 +3653,27 @@ a float number which specifies chroma temporal strength, defaults to
>  
>  Modify the hue and/or the saturation of the input.
>  
> -This filter accepts the following optional named options:
> +This filter accepts the following options:
>  
>  @table @option
>  @item h
> -Specify the hue angle as a number of degrees. It accepts a float
> -number or an expression, and defaults to 0.0.
> -
> - at item H
> -Specify the hue angle as a number of radians. It accepts a float
> -number or an expression, and defaults to 0.0.
> +Specify the hue angle as a number of degrees. It accept an expression,

accepts

[...]
> +#define SET_EXPR(expr, option)                                          \
> +    if (hue->expr##_expr) {                                             \
> +        ret = set_expr(&hue->expr##_pexpr, hue->expr##_expr, option, ctx); \
> +        if (ret < 0)                                                    \
> +            return ret;                                                 \
> +    }
> +    SET_EXPR(saturation, "s");
> +    SET_EXPR(hue_deg,    "h");
> +    SET_EXPR(hue,        "H");
>  

please use the do { ... } while (0) form. It will avoid empty statements,
possibly bad branching later, etc.

[...]
> -    if (!hue->flat_syntax) {
> +    /* todo: reindent */

nit: TODO, easier to grep

>          hue->var_values[VAR_T]   = TS2T(inpic->pts, inlink->time_base);
>          hue->var_values[VAR_PTS] = TS2D(inpic->pts);
>  
> @@ -323,7 +280,6 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *inpic)
>                 hue->var_values[VAR_T], (int)hue->var_values[VAR_N]);
>  
>          compute_sin_and_cos(hue);
> -    }
>  
>      hue->var_values[VAR_N] += 1;
>  
> @@ -345,10 +301,17 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *inpic)
>  static int process_command(AVFilterContext *ctx, const char *cmd, const char *args,
>                             char *res, int res_len, int flags)
>  {
> -    if (!strcmp(cmd, "reinit"))
> -        return set_options(ctx, args);
> -    else
> -        return AVERROR(ENOSYS);
> +    HueContext *hue = ctx->priv;
> +
> +#define SET_CMD(expr, option)                                          \
> +    if      (!strcmp(cmd, option))                                     \

weird spacing

> +        return set_expr(&hue->expr##_pexpr, args, cmd, ctx);
> +
> +    SET_CMD(hue_deg,    "h");
> +    SET_CMD(hue,        "H");
> +    SET_CMD(saturation, "s");
> +
> +    return AVERROR(ENOSYS);

LGTM otherwise.

-- 
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/20130411/9d45f75d/attachment.asc>