[FFmpeg-devel] [PATCH] lavfi/hue: add dynamic expression support

Stefano Sabatini stefasab at gmail.com
Sun Sep 2 01:09:31 CEST 2012


On date Saturday 2012-09-01 19:44:17 +0200, Jérémy Tran encoded:
> When using the named options syntax the hue and saturation values are
> computed for each frame.

> Is there a way to check if an expression is constant (to avoid
> computing it everytime) ?

Not at the API level. You could devise some logic for detecting if an
expression is constant (k1, k2 is constant => k1 op k2 is constant),
but I'm not sure how useful that would be (and should be addressed at
the eval framework level I guess).

> ---
>  doc/filters.texi     |   7 +++
>  libavfilter/vf_hue.c | 135 ++++++++++++++++++++++++++++++++++++++-------------
>  2 files changed, 109 insertions(+), 33 deletions(-)
> 
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 712936d..70c9991 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -2348,6 +2348,13 @@ h and s, so the following example will issue an error:
>  @example
>  hue=PI/2:1
>  @end example
> +
> + at item
> +Set the hue to PI times current time and make the saturation swing between 0
> +and 2:
> + at example
> +hue="H=PI*t: s=sin(t)+1"

More mathematics-friendly:
hue="H=2*PI*t

So you have a period of 1 second

s=sin(2*PI*t)+1

again with a period of 1 second.

Also it would be useful to have a saturation fade-in/fade-out example,
which seems like a realistic application.

> + at end example
>  @end itemize
>  
>  @subsection Commands
> diff --git a/libavfilter/vf_hue.c b/libavfilter/vf_hue.c
> index dd36d8e..4139a13 100644
> --- a/libavfilter/vf_hue.c
> +++ b/libavfilter/vf_hue.c
> @@ -26,6 +26,7 @@
>   */
>  
>  #include <float.h>
> +#include "libavutil/eval.h"
>  #include "libavutil/imgutils.h"
>  #include "libavutil/opt.h"
>  #include "libavutil/pixdesc.h"
> @@ -38,26 +39,40 @@
>  #define HUE_DEFAULT_VAL 0
>  #define SAT_DEFAULT_VAL 1
>  

> +#define CONVERT_HUE_FROM_DEGREES_TO_RADIANS(hue) \
> +    hue->hue = hue->hue_deg * M_PI / 180;

Maybe wrap it in do { ... } while (0)

> +
>  typedef struct {
>      const    AVClass *class;
>      float    hue_deg; /* hue expressed in degrees */
>      float    hue; /* hue expressed in radians */
> +    char     *hue_deg_expr;
> +    char     *hue_expr;
> +    AVExpr   *hue_deg_pexpr;
> +    AVExpr   *hue_pexpr;
>      float    saturation;
> +    char     *saturation_expr;
> +    AVExpr   *saturation_pexpr;
>      int      hsub;
>      int      vsub;
>      int32_t hue_sin;
>      int32_t hue_cos;

> +    int8_t   old_syntax;

int for stylistic consistency (int are usually used for flags, intN_t
when there is a specific reason for having a fixed number of bits).

Also I'd prefer "flat_" over "old_", "old" is not descriptive of what
the field stores as only the developer knows what "old" is.

>  } HueContext;
>  
> +enum var_name {
> +    VAR_T
> +};
> +
>  #define OFFSET(x) offsetof(HueContext, x)
>  #define FLAGS AV_OPT_FLAG_VIDEO_PARAM|AV_OPT_FLAG_FILTERING_PARAM
>  static const AVOption hue_options[] = {
> -    { "h", "set the hue angle degrees", OFFSET(hue_deg), AV_OPT_TYPE_FLOAT,
> -      { -FLT_MAX }, -FLT_MAX, FLT_MAX, FLAGS },
> -    { "H", "set the hue angle radians", OFFSET(hue), AV_OPT_TYPE_FLOAT,
> -      { -FLT_MAX }, -FLT_MAX, FLT_MAX, FLAGS },
> -    { "s", "set the saturation value", OFFSET(saturation), AV_OPT_TYPE_FLOAT,
> -      { SAT_DEFAULT_VAL }, -10, 10, FLAGS },

> +    { "h", "set the hue angle degrees", OFFSET(hue_deg_expr), AV_OPT_TYPE_STRING,
> +      { 0 }, .flags = FLAGS },
> +    { "H", "set the hue angle radians", OFFSET(hue_expr), AV_OPT_TYPE_STRING,
> +      { 0 }, .flags = FLAGS },
> +    { "s", "set the saturation value", OFFSET(saturation_expr), AV_OPT_TYPE_STRING,

add "expression" so it is will be more explicit

> +      { 0 }, .flags = FLAGS },
>      { NULL }
>  };
>  
> @@ -68,22 +83,19 @@ static inline int set_options(AVFilterContext *ctx, const char *args)
>      HueContext *hue = ctx->priv;
>      int n, ret;
>      char c1 = 0, c2 = 0;
> -    char *equal;
>  
>      if (args) {
>          /* named options syntax */
> -        if (equal = strchr(args, '=')) {
> -            hue->hue     = -FLT_MAX;
> -            hue->hue_deg = -FLT_MAX;
> -
> +        if (strchr(args, '=')) {
>              if ((ret = av_set_options_string(hue, args, "=", ":")) < 0)
>                  return ret;
> -            if (hue->hue != -FLT_MAX && hue->hue_deg != -FLT_MAX) {
> +            if (hue->hue_expr && hue->hue_deg_expr) {
>                  av_log(ctx, AV_LOG_ERROR,
>                         "H and h options are incompatible and cannot be specified "
>                         "at the same time\n");
>                  return AVERROR(EINVAL);
>              }
> +            hue->old_syntax = 0;
>          /* compatibility h:s syntax */
>          } else {
>              n = sscanf(args, "%f%c%f%c", &hue->hue_deg, &c1, &hue->saturation, &c2);
> @@ -100,6 +112,7 @@ static inline int set_options(AVFilterContext *ctx, const char *args)
>                         "must be included between range -10 and +10\n", hue->saturation);
>                  return AVERROR(EINVAL);
>              }
> +            hue->old_syntax = 1;
>          }
>      }
>  
> @@ -113,20 +126,11 @@ static av_cold int init(AVFilterContext *ctx, const char *args)
>  
>      hue->class = &hue_class;
>      av_opt_set_defaults(hue);
> +    hue->saturation = SAT_DEFAULT_VAL;
>  
>      if ((ret = set_options(ctx, args)) < 0)
>          return ret;
>  
> -    if (hue->saturation == -FLT_MAX)
> -        hue->hue = SAT_DEFAULT_VAL;
> -    if (hue->hue == -FLT_MAX)
> -        hue->hue = HUE_DEFAULT_VAL;
> -    if (hue->hue_deg != -FLT_MAX)
> -        /* Convert angle from degrees to radians */
> -        hue->hue = hue->hue_deg * M_PI / 180;
> -
> -    av_log(ctx, AV_LOG_VERBOSE, "hue:%f*PI hue_deg:%f saturation:%f\n",
> -           hue->hue/M_PI, hue->hue*180/M_PI, hue->saturation);
>      return 0;
>  }
>  
> @@ -152,13 +156,8 @@ static int query_formats(AVFilterContext *ctx)
>      return 0;
>  }
>  
> -static int config_props(AVFilterLink *inlink)
> +static inline void compute_sin_and_cos(HueContext *hue)
>  {
> -    HueContext *hue = inlink->dst->priv;
> -    const AVPixFmtDescriptor *desc = &av_pix_fmt_descriptors[inlink->format];
> -
> -    hue->hsub = desc->log2_chroma_w;
> -    hue->vsub = desc->log2_chroma_h;
>      /*
>       * Scale the value to the norm of the resulting (U,V) vector, that is
>       * the saturation.
> @@ -166,6 +165,40 @@ static int config_props(AVFilterLink *inlink)
>       */
>      hue->hue_sin = rint(sin(hue->hue) * (1 << 16) * hue->saturation);
>      hue->hue_cos = rint(cos(hue->hue) * (1 << 16) * hue->saturation);
> +}
> +
> +static int config_props(AVFilterLink *inlink)
> +{
> +    HueContext *hue = inlink->dst->priv;
> +    const AVPixFmtDescriptor *desc = &av_pix_fmt_descriptors[inlink->format];

> +    const char *var_names[] = { "t", NULL };

It's better to keep this together with the enum var_name definition,
so they won't get unsynched. Also it should be static.

> +    int ret;
> +
> +    if (hue->old_syntax) {
> +        CONVERT_HUE_FROM_DEGREES_TO_RADIANS(hue)
> +        compute_sin_and_cos(hue);
> +    } else {
> +        if (!hue->hue_expr && !hue->hue_deg_expr) {
> +            hue->hue = HUE_DEFAULT_VAL;

> +        }
> +        else {

Nit: same line

> +            if (hue->hue_deg_expr &&
> +                (ret = av_expr_parse(&hue->hue_deg_pexpr, hue->hue_deg_expr, var_names,
> +                                     NULL, NULL, NULL, NULL, 0, inlink->dst)) < 0)
> +                return AVERROR(EINVAL);
> +            if (hue->hue_expr &&
> +                (ret = av_expr_parse(&hue->hue_pexpr, hue->hue_expr, var_names,
> +                                     NULL, NULL, NULL, NULL, 0, inlink->dst)) < 0)
> +                return AVERROR(EINVAL);
> +        }
> +        if (hue->saturation_expr &&
> +            (ret = av_expr_parse(&hue->saturation_pexpr, hue->saturation_expr, var_names,
> +                                 NULL, NULL, NULL, NULL, 0, inlink->dst) < 0))
> +            return AVERROR(EINVAL);
> +    }

Printing the name of the failed expression evaluated may help.

> +
> +    hue->hsub = desc->log2_chroma_w;
> +    hue->vsub = desc->log2_chroma_h;
>  
>      return 0;
>  }

> @@ -216,6 +249,29 @@ static int draw_slice(AVFilterLink *inlink, int y, int h, int slice_dir)
>      AVFilterBufferRef *outpic = inlink->dst->outputs[0]->out_buf;
>      uint8_t *inrow[3], *outrow[3]; // 0 : Y, 1 : U, 2 : V
>      int plane;
> +    double var_values[2] = { 0 };
> +    float saturation = hue->saturation;
> +

> +    if (!hue->old_syntax) {
> +        var_values[VAR_T] = inlink->cur_buf->pts * av_q2d(inlink->time_base);

If pts is AV_NOPTS_VALUE t should be set to NaN. Check how it is done
in crop.

> +
> +        if (hue->saturation_expr) {
> +            saturation = av_expr_eval(hue->saturation_pexpr, var_values, NULL);
> +

> +            if (saturation >= -10 && saturation <= 10) {
> +                hue->saturation = saturation;
> +            }

Maybe warn and clip in the feasible range.

> +        }
> +
> +        if (hue->hue_deg_expr) {
> +            hue->hue_deg = av_expr_eval(hue->hue_deg_pexpr, var_values, NULL);
> +            CONVERT_HUE_FROM_DEGREES_TO_RADIANS(hue)
> +        } else if (hue->hue_expr) {
> +            hue->hue = av_expr_eval(hue->hue_pexpr, var_values, NULL);
> +        }
> +
> +        compute_sin_and_cos(hue);
> +    }

This should be done in start_frame() or end_frame() - whatever is more
convenient, there is no point into computing it for each slice.

Also printing the computed values with a DEBUG log at each step should
help debugging/testing.

[...]
-- 
FFmpeg = Faithful and Fabulous Mythic Proud Enigmatic Gladiator


More information about the ffmpeg-devel mailing list