[FFmpeg-devel] [PATCH] libavfilter: add vf_hue.c

Stefano Sabatini stefasab at gmail.com
Sat Aug 11 01:20:45 CEST 2012


On date Friday 2012-08-10 22:49:06 +0200, Jérémy Tran encoded:
> This is a port of the MPlayer hue filter (libmpcodecs/vf_hue.c)
> ---
>  doc/filters.texi         |  12 +++
>  libavfilter/Makefile     |   1 +
>  libavfilter/allfilters.c |   1 +
>  libavfilter/vf_hue.c     | 188 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 202 insertions(+)
>  create mode 100644 libavfilter/vf_hue.c
> 
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 18be723..ea8263d 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -2187,6 +2187,18 @@ a float number which specifies chroma temporal strength, defaults to
>  @var{luma_tmp}*@var{chroma_spatial}/@var{luma_spatial}
>  @end table
>  
> + at section hue
> +
> +Hue and saturation filter.  This filter modifies the hue and/or the saturation

First sentence tells what the filter does, not what it is.

> +of the input.
> +
> + at table @option
> + at item hue
> +a float number that specifies the hue.

Range? Default value?

> + at item saturation
> +a float number that specifies the saturation.

Ditto.

Also missing description of the syntax (how to specify hue and
saturation?)

> + at end table
> +
>  @section idet
>  
>  Interlaceing detect filter. This filter tries to detect if the input is
> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> index 727ab4e..66b5d52 100644
> --- a/libavfilter/Makefile
> +++ b/libavfilter/Makefile
> @@ -98,6 +98,7 @@ OBJS-$(CONFIG_FREI0R_FILTER)                 += vf_frei0r.o
>  OBJS-$(CONFIG_GRADFUN_FILTER)                += vf_gradfun.o
>  OBJS-$(CONFIG_HFLIP_FILTER)                  += vf_hflip.o
>  OBJS-$(CONFIG_HQDN3D_FILTER)                 += vf_hqdn3d.o
> +OBJS-$(CONFIG_HUE_FILTER)                    += vf_hue.o
>  OBJS-$(CONFIG_IDET_FILTER)                   += vf_idet.o
>  OBJS-$(CONFIG_LUT_FILTER)                    += vf_lut.o
>  OBJS-$(CONFIG_LUTRGB_FILTER)                 += vf_lut.o
> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
> index 403383d..f097bcf 100644
> --- a/libavfilter/allfilters.c
> +++ b/libavfilter/allfilters.c
> @@ -88,6 +88,7 @@ void avfilter_register_all(void)
>      REGISTER_FILTER (GRADFUN,     gradfun,     vf);
>      REGISTER_FILTER (HFLIP,       hflip,       vf);
>      REGISTER_FILTER (HQDN3D,      hqdn3d,      vf);
> +    REGISTER_FILTER (HUE,         hue,         vf);
>      REGISTER_FILTER (IDET,        idet,        vf);
>      REGISTER_FILTER (LUT,         lut,         vf);
>      REGISTER_FILTER (LUTRGB,      lutrgb,      vf);
> diff --git a/libavfilter/vf_hue.c b/libavfilter/vf_hue.c
> new file mode 100644
> index 0000000..a5bd635
> --- /dev/null
> +++ b/libavfilter/vf_hue.c
> @@ -0,0 +1,188 @@
> +/*
> + * Copyright (c) 2012 Michael Niedermayer

>From the MPlayer SVN log:
r11250 | michael | 2003-10-23 20:36:02 +0200 (Thu, 23 Oct 2003) | 2 lines
vf_hue

I'll retain the original year (and having ancient year is somewhat cooler...)

> + * Copyright (c) 2012 Jeremy Tran
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with FFmpeg; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +

> +/**
> + * @file
> + * Apply a hue filter to the input video

Apply a hue/saturation filter to the input video

> + * Ported from MPlayer libmpcodecs/vf_hue.c
> + */
> +
> +#include "libavutil/common.h"
> +#include "libavutil/imgutils.h"
> +#include "libavutil/mem.h"
> +#include "libavutil/pixdesc.h"
> +
> +#include "avfilter.h"
> +#include "formats.h"
> +#include "internal.h"
> +#include "video.h"
> +
> +typedef struct {
> +    float    hue;
> +    float    saturation;
> +    int      hsub;
> +    int      vsub;

> +    uint32_t sin;
> +    uint32_t cos;

int32_t?

> +} HueContext;
> +
> +static av_cold int init(AVFilterContext *ctx, const char *args)
> +{
> +    HueContext *hue = ctx->priv;
> +    float h, s;
> +

> +    if (args)
> +        sscanf(args, "%f:%f", &h, &s);

I tend to prefer the use of named options, despite the fact of
breaking compatibility with mp=hue, because of the increased
robustness/flexibility/overall consistency.

What's the opinion of other devs?

Alternatively we could try to support both syntaxes (check how it is
done in buffersrc).

> +
> +    if (h < 0 || h > 360 || s < 0 || s > 2) {

It could be useful to support negative h values as well. Also I wonder
if we should support angles expressed in radians (with named options
this could be done by using two distinct names h/H).

> +        av_log(ctx, AV_LOG_ERROR,
> +               "Invalid value for h:%0.1f or s:%0.1f\n",
> +               h, s);
> +        return AVERROR(EINVAL);
> +    }

Adding different error messages for different errors is helpful
toward users, at a modest code complexity price.

> +    /* Convert angle from degree to radian */
> +    hue->hue = h * M_PI / 180;
> +    hue->saturation = s;
> +
> +    return 0;
> +}
> +
> +static int query_formats(AVFilterContext *ctx)
> +{
> +    static const enum PixelFormat pix_fmts[] = {
> +        PIX_FMT_YUV444P,      PIX_FMT_YUV422P,
> +        PIX_FMT_YUV420P,      PIX_FMT_YUV411P,
> +        PIX_FMT_YUV410P,      PIX_FMT_YUV440P,
> +        PIX_FMT_YUVA420P,
> +        PIX_FMT_NONE
> +    };
> +
> +    ff_set_common_formats(ctx, ff_make_format_list(pix_fmts));
> +
> +    return 0;
> +}
> +
> +static int config_props(AVFilterLink *inlink)
> +{
> +    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.
> +     * This will be useful in the process_chrominance function.
> +     */
> +    hue->sin = rint(sin(hue->hue) * (1 << 16) * hue->saturation);
> +    hue->cos = rint(cos(hue->hue) * (1 << 16) * hue->saturation);
> +
> +    return 0;
> +}
> +

> +/*
> + * If we consider U and V as the components of a 2D vector then its angle
> + * is the hue and the norm is the saturation
> + */

Move the comment inside the function, otherwise the reader will parse
it as function documentation.

> +static void process_chrominance(uint8_t *udst, uint8_t *vdst, const int dst_linesize,
> +                                uint8_t *usrc, uint8_t *vsrc, const int src_linesize,
> +                                int w, int h,

> +                                const uint32_t c, const uint32_t s)

maybe int32_t since the values may be negative

> +{
> +    int i;

> +    int u, v;
> +    int new_u, new_v;

int32_t should be more meaningful.

> +
> +    while (h--) {
> +        for (i = 0; i < w; i++) {
> +            /* Normalize the components from range [16;140] to [-112;112] */
> +            u = usrc[i] - 128;
> +            v = vsrc[i] - 128;
> +            /*
> +             * Apply the rotation of the vector : (c * u) - (s * v)
> +             *                                    (s * u) + (c * v)
> +             * De-normalize the components (without forgetting to scale 128
> +             * by << 16)
> +             * Finally scale back the result by >> 16
> +             */
> +            new_u = ((c * u) - (s * v) + (1 << 15) + (128 << 16)) >> 16;
> +            new_v = ((s * u) + (c * v) + (1 << 15) + (128 << 16)) >> 16;
> +
> +            /* Prevent a potential overflow */
> +            udst[i] = av_clip_uint8_c(new_u);
> +            vdst[i] = av_clip_uint8_c(new_v);
> +        }
> +
> +        usrc += src_linesize;
> +        vsrc += src_linesize;
> +        udst += dst_linesize;
> +        vdst += dst_linesize;
> +    }
> +}
[...]

Rest looks fine.

Make sure that the new filter issues the same output as mp=hue (we
have several tools for that, compare output issued with showinfo,
md5/framecrc/framemd5 output formats, md5 protocol, possibly using
lavfi test sources).
-- 
FFmpeg = Fabulous & Fiendish Most Pure Erotic Game


More information about the ffmpeg-devel mailing list