[FFmpeg-devel] [PATCH] vf_hue: add named options support

[Stefano Sabatini]

On date Tuesday 2012-08-14 11:13:22 +0200, Jérémy Tran encoded:
> Old syntax has been kept for compatibility reasons.
> ---
>  doc/filters.texi     | 17 ++++++++----
>  libavfilter/vf_hue.c | 73 ++++++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 68 insertions(+), 22 deletions(-)
> 
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 763085c..44e8344 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -2191,12 +2191,19 @@ a float number which specifies chroma temporal strength, defaults to
>  
>  Modify the hue and/or the saturation of the input.
>  
> -This filter accepts the optional parameters: @var{hue}:@var{saturation}.
> +This filter accepts the optional parameters:
>  
> - at var{hue} must be a float number that specifies the hue angle as a
> -number of degrees, and defaults to 0.0.
> - at var{saturation} must be a float number that specifies the saturation
> -in the [-10,10] range, and defaults to 1.0.
> + at table @option
> + at item h
> +a float number that specifies the hue angle as a
> +number of degrees, and defaults to 0.0
> + at item H
> +a float number that specifies the hue angle as a
> +number of radians, and defaults to 0.0
> + at item s
> +a float number that specifies the saturation
> +in the [-10,10] range, and defaults to 1.0
> + at end table

Uhm docs should mention that the filter supports both syntaxes, and
possibly provide examples with both of them.

>  @section idet
>  
> diff --git a/libavfilter/vf_hue.c b/libavfilter/vf_hue.c
> index 16fa4f4..e2a404f 100644
> --- a/libavfilter/vf_hue.c
> +++ b/libavfilter/vf_hue.c
> @@ -25,7 +25,10 @@
>   * Ported from MPlayer libmpcodecs/vf_hue.c.
>   */
>  
> +#include <float.h>
> +#include "libavutil/eval.h"
>  #include "libavutil/imgutils.h"
> +#include "libavutil/opt.h"
>  #include "libavutil/pixdesc.h"
>  
>  #include "avfilter.h"
> @@ -34,7 +37,9 @@
>  #include "video.h"
>  
>  typedef struct {
> +    const    AVClass *class;
>      float    hue;

> +    float    hrad; /* hue expr in radian */

expressed?

>      float    saturation;
>      int      hsub;
>      int      vsub;
> @@ -42,37 +47,69 @@ typedef struct {
>      int32_t hue_cos;
>  } HueContext;
>  
> +#define OFFSET(x) offsetof(HueContext, x)
> +static const AVOption hue_options[] = {
> +    { "h", NULL, OFFSET(hue), AV_OPT_TYPE_FLOAT, { 0 }, -FLT_MAX, FLT_MAX, AV_OPT_FLAG_VIDEO_PARAM },
> +    { "H", NULL, OFFSET(hrad), AV_OPT_TYPE_FLOAT, { 0 }, -FLT_MAX, FLT_MAX, AV_OPT_FLAG_VIDEO_PARAM },
> +    { "s", NULL, OFFSET(saturation), AV_OPT_TYPE_FLOAT, { 1 }, -10, 10, AV_OPT_FLAG_VIDEO_PARAM },
> +    { NULL }
> +};
> +
> +AVFILTER_DEFINE_CLASS(hue);
> +
>  static av_cold int init(AVFilterContext *ctx, const char *args)
>  {
>      HueContext *hue = ctx->priv;
>      float h = 0, s = 1;
>      int n;
>      char c1 = 0, c2 = 0;
> -
> -    if (args) {
> -        n = sscanf(args, "%f%c%f%c", &h, &c1, &s, &c2);
> -        if (n != 0 && n != 1 && (n != 3 || c1 != ':')) {
> -            av_log(ctx, AV_LOG_ERROR,
> -                   "Invalid syntax for argument '%s': "
> -                   "must be in the form 'hue[:saturation]'\n", args);
> -            return AVERROR(EINVAL);
> +    char *equal;
> +
> +    hue->class = &hue_class;
> +
> +    /* named options syntax */
> +    if (equal = strchr(args, '=')) {
> +        av_opt_set_defaults(hue);
> +        av_set_options_string(hue, args, "=", ":");
> +    /* compatibility syntax */

> +    } else {
> +        if (args) {
> +            n = sscanf(args, "%f%c%f%c", &h, &c1, &s, &c2);
> +            if (n != 0 && n != 1 && (n != 3 || c1 != ':')) {
> +                av_log(ctx, AV_LOG_ERROR,
> +                       "Invalid syntax for argument '%s': "
> +                       "must be in the form 'hue[:saturation]'\n", args);
> +                return AVERROR(EINVAL);
> +            }
> +            if (s < -10 || s > 10) {
> +                av_log(ctx, AV_LOG_ERROR,
> +                       "Invalid value for saturation %0.1f: "
> +                       "must be included between range -10 and +10\n", s);
> +                return AVERROR(EINVAL);
> +            }

Please don't reindent old code, it should simplify the patch (reindent
can be applied as a separate cosmetics commit).

>          }
> +        hue->hue = h;
> +        hue->saturation = s;
>      }
>  
> -    if (s < -10 || s > 10) {
> -        av_log(ctx, AV_LOG_ERROR,
> -               "Invalid value for saturation %0.1f: "
> -               "must be included between range -10 and +10\n", s);
> -        return AVERROR(EINVAL);

> +    /* radian has priority on degree */
> +    if (hue->hrad) {
> +        hue->hue = hue->hrad;
> +    } else {
> +        /* Convert angle from degree to radian */
> +        hue->hue *= M_PI / 180;

For avoiding confusion I'd rather rename hue -> hue_deg

   if (!hue->hue)
       hue->hue = hue->hue_deg * M_PI / 180;

while hue is expressed by default in radians.

[...]

Looks good otherwise.
-- 
FFmpeg = Fanciful & Fantastic Merciless Perfectionist Exploitable Generator