[FFmpeg-devel] [PATCH] [SOCIS] [WIP] libavfilter: add vf_hue.c
Stefano Sabatini
stefasab at gmail.com
Wed Aug 1 00:14:27 CEST 2012
On date Tuesday 2012-07-31 20:55:02 +0200, Jeremy Tran encoded:
> Hello,
>
> This a port of the mplayer hue filter.
> It compiles but does not work properly.
> It puts the image in grayscale except when the hue value is set to 0
> and the saturation is set to 1 (in this case the image obviously
> doesn't change).
>
> Regards,
>
> --
> Jérémy Tran
> ACU 2013
> EPITA 2013
> From 7abfb5ddcb726071679e34c7eda2da278cecdb78 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Je=CC=81re=CC=81my=20Tran?= <jeremy.tran at epita.fr>
> Date: Tue, 31 Jul 2012 19:01:06 +0200
> Subject: [PATCH] [SOCIS] [WIP] libavfilter: add vf_hue.c
>
> This a port of the mplayer hue filter.
> It compiles but does not work properly.
> It puts the image in grayscale except when the hue
> value is set to 0 and the saturation is set to 1 (in
> this case the image doesn't change).
> ---
> libavfilter/vf_hue.c | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 184 insertions(+)
> create mode 100644 libavfilter/vf_hue.c
>
> diff --git a/libavfilter/vf_hue.c b/libavfilter/vf_hue.c
> new file mode 100644
> index 0000000..095e745
> --- /dev/null
> +++ b/libavfilter/vf_hue.c
> @@ -0,0 +1,184 @@
> +/*
> + * This file is part of FFmpeg.
Add your copyright and from the original author (Michael). Also keep
it GPL when it is not clear if can be relicensed (usually in case of
port we ask the original authors).
> + *
> + * 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
> + * Ported from MPlayer libmpcodecs/vf_hue.c
> + */
> +
> +#include "avfilter.h"
> +#include "formats.h"
> +#include "internal.h"
> +#include "video.h"
> +#include "libavutil/common.h"
> +#include "libavutil/mem.h"
> +#include "libavutil/pixdesc.h"
> +#include "libavcodec/avcodec.h"
> +
> +typedef struct {
> + uint8_t *buf[2];
> + float hue;
> + float saturation;
> + int hsub;
> + int vsub;
> +} 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);
missing input validation?
Also I'm not sure if we should add support to named options (I mean
h=VAL:s=VAL), but this is secondary for the moment.
> +
> + hue->hue = h;
> + hue->saturation = s;
> + hue->buf[0] = NULL;
> + hue->buf[1] = NULL;
> +
> + return 0;
> +}
> +
> +static int query_formats(AVFilterContext *ctx)
> +{
> + /* FIXME : find out which pixel formats are fine */
> + static const enum PixelFormat pix_fmts[] = {
> + PIX_FMT_YUV420P16LE, PIX_FMT_YUV420P16BE,
> + PIX_FMT_YUV422P16LE, PIX_FMT_YUV422P16BE,
> + PIX_FMT_YUV444P16LE, PIX_FMT_YUV444P16BE,
> + PIX_FMT_YUV444P, PIX_FMT_YUV422P,
> + PIX_FMT_YUV420P, PIX_FMT_YUV411P,
> + PIX_FMT_YUV410P, PIX_FMT_YUV440P,
> + PIX_FMT_YUVJ444P, PIX_FMT_YUVJ422P,
> + PIX_FMT_YUVJ420P, PIX_FMT_YUVJ440P,
> + PIX_FMT_YUVA420P,
> + PIX_FMT_NONE
> + };
> +
> + ff_set_common_formats(ctx, ff_make_format_list(pix_fmts));
return ff_set_common...
> +
> + return 0;
> +}
> +
> +static void process(uint8_t *udst, uint8_t *vdst, int dststride,
> + uint8_t *usrc, uint8_t *vsrc, int srcstride,
Nit: "stride" is an "MPlayerism", FFmpeg codebase use the
"linesize" term for addressing the same thing.
Also the function name could be more descriptive ("process" can be
pretty anything).
> + int w, int h,
> + float hue, float sat)
> +{
> + int i;
> + int u, v;
> + int new_u, new_v;
> + const int s = rint(sin(hue) * (1 << 16) * sat);
> + const int c = rint(cos(hue) * (1 << 16) * sat);
> +
> + while (h--) {
> + for (i = 0; i < w; i++) {
> + u = usrc[i] - 128;
> + v = vsrc[i] - 128;
> + new_u = ((c * u) - (s * v) + (1 << 15) + (128 << 16)) >> 16;
> + new_v = ((s * u) + (c * v) + (1 << 15) + (128 << 16)) >> 16;
> +
> + if (new_u & 768)
> + new_u = (-new_u) >> 31;
> + if (new_v & 768)
> + new_v = (-new_v) >> 31;
maybe add a note explaining the magic number
> +
> + udst[i] = new_u;
> + vdst[i] = new_v;
> + }
> +
> + usrc += srcstride;
> + vsrc += srcstride;
> + udst += dststride;
> + vdst += dststride;
> + }
> +}
> +
> +static int end_frame(AVFilterLink *link)
> +{
> + HueContext *hue = link->dst->priv;
> + AVFilterBufferRef *in = link->cur_buf;
> + AVFilterBufferRef *out = link->dst->outputs[0]->out_buf;
> +
> + memcpy(out->data[0], in->data[0], in->linesize[0] * link->h);
This is unnecessary and possibly wrong, since in->linesize[0] !=
out->linesize[0] (linesize is the size in bytes of each line, and it
has to be at least width * pixel_step, that is it must contain a whole
horizontal line, but it may be longer due to padding, two different
buffers have usually different linesizes/paddings).
> + out->linesize[0] = in->linesize[0];
> + out->linesize[1] = in->linesize[1];
> + out->linesize[2] = in->linesize[2];
why this?
> +
> + if (!hue->buf[0]) {
> + hue->buf[0] = av_malloc(in->linesize[1] * link->h >> hue->vsub);
> + hue->buf[1] = av_malloc(in->linesize[2] * link->h >> hue->vsub);
missing NULL checks (in case malloc fails).
Also this is leaking hard, since you never free this memory but only
in uninit.
Also the data in the output buffer is already in place, no need to
allocate it.
> + }
> +
> + if (hue->hue == 0.0f && hue->saturation == 1.0f) {
> + memcpy(out->data[1], in->data[1], in->linesize[1] * link->h >> hue->vsub);
> + memcpy(out->data[2], in->data[2], in->linesize[2] * link->h >> hue->vsub);
> + } else {
> + out->data[1] = hue->buf[0];
> + out->data[2] = hue->buf[1];
> + process(out->data[1], out->data[2], out->linesize[1],
> + in->data[1], in->data[2], in->linesize[1],
> + link->w >> hue->hsub, link->h >> hue->vsub,
> + hue->hue, hue->saturation);
> + }
I think this logic can be simplified.
If I'm not wrong you should be able to do inplace processing, so you
should avoid to request a new buffer by using the default
start_frame() callback, check how this is done in drawbox, where only
cur_buf containing the passed data is processed (so doesn't need to be
processed at all in case h==0 && s==1).
Also it should be possible to do per-slice processing, since each
slice doesn't depend on the other ones.
> +
> + return ff_end_frame(link->dst->outputs[0]);
> +}
> +
> +static int config_props(AVFilterLink *link)
link -> inlink
> +{
> + HueContext *hue = link->dst->priv;
> +
> + avcodec_get_chroma_sub_sample(link->format, &hue->hsub, &hue->vsub);
this adds a dependency on avcodec_, use direct access to
av_pix_fmt_descriptors in libavutil/pixdesc.h.
> +
> + return 0;
> +}
> +
> +static av_cold void uninit(AVFilterContext *ctx)
> +{
> + HueContext *hue = ctx->priv;
> + int i;
> +
> + for (i = 0; i < 2; i++) {
> + av_free(hue->buf[i]);
> + hue->buf[i] = NULL;
av_freep(), but again I don't think these buffers are required.
[...]
--
FFmpeg = Faithless and Fancy Mysterious Ponderous Evanescent Gnome
More information about the ffmpeg-devel
mailing list