[FFmpeg-devel] [PATCH] Unsharp filter

Stefano Sabatini stefano.sabatini-lala
Wed Mar 24 00:31:14 CET 2010


On date Tuesday 2010-03-23 17:37:21 -0400, Daniel G. Taylor encoded:
> Hey,

Hi, and thanks for the patch!

Follows a list of nitpicks and maybe some more useful comment.

[...]
> Index: doc/libavfilter.texi
> ===================================================================
> --- doc/libavfilter.texi	(revision 22644)
> +++ doc/libavfilter.texi	(working copy)
> @@ -217,6 +217,21 @@
>  Adding this in the beginning of filter chains should make filtering
>  faster due to better use of the memory cache.
>  
> + at section unsharp
> +
> +Sharpen or blur the input video. It takes four parameters: the effect type, width, height, and amount. The type can be one of 'l' for luma, 'c' for chroma, or 'b' for both. The width and height are integers defining how large the affected area is, while the amount defines the intensity. A negative amount blurs while a positive amount sharpens. Passing no arguments to the filter is the same as passing l:3:3:1.5.

Please fix formatting, these lines are looooong.
Also I'm not fond of one char flags, having word may help for example
whne reading scripts.

Also I'd prefer a more formal description for the taken parameters, of
the kind:

It accepts the parameters @var{effect_type}:@var{width}:@var{height}:@var{amount}.
@var{type} can be blah blah...

> +
> + at example
> +# Use the default values
> +./ffmpeg -i in.avi -vfilters "[in] unsharp [out]"

[NIT] I tend to prefer the use of bare graph description without the
rest of the ff* command, it is shorter and make more sense as the
graph description is also meaningful for an application using
libavfilter, for which the ff* tools syntax is irrelelvant.

> +
> +# Specify a strong sharpen effect
> +./ffmpeg -i in.avi -vfilters "[in] unsharp=l:5:5:1.5 [out]" out.avi
> +
> +# Specify a strong blur effect
> +./ffmpeg -i in.avi -vfilters "[in] unsharp=l:5:5:-1.5 [out]" out.avi
> + at end example
> +
>  @section vflip
>  
>  Flip the input video vertically.
> Index: libavfilter/allfilters.c
> ===================================================================
> --- libavfilter/allfilters.c	(revision 22644)
> +++ libavfilter/allfilters.c	(working copy)
> @@ -42,6 +42,7 @@
>      REGISTER_FILTER (PIXELASPECT, pixelaspect, vf);
>      REGISTER_FILTER (SCALE,       scale,       vf);
>      REGISTER_FILTER (SLICIFY,     slicify,     vf);
> +    REGISTER_FILTER (UNSHARP,     unsharp,     vf);
>      REGISTER_FILTER (VFLIP,       vflip,       vf);
>  
>      REGISTER_FILTER (NULLSRC,     nullsrc,     vsrc);
> Index: libavfilter/vf_unsharp.c
> ===================================================================
> --- libavfilter/vf_unsharp.c	(revision 0)
> +++ libavfilter/vf_unsharp.c	(revision 0)
> @@ -0,0 +1,258 @@
> +/*
> + * Ported to FFmpeg from MPlayer libmpcodecs/unsharp.c
> + * Original copyright (C) 2002 Remi Guyomarch <rguyom at pobox.com>
> + * Port copyright (C) 2010 Daniel G. Taylor <dan at programmer-art.org>
> + *
> + * 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
> + * Lesser 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 libavfilter/vf_unsharp.c
> + * blur / sharpen filter
> + */
> +
> +#include "avfilter.h"
> +#include "libavutil/common.h"
> +#include "libavutil/mem.h"
> +
> +#define MIN_SIZE 3
> +#define MAX_SIZE 8

Please put these macros near to where they are used, also maybe you
can just use their values.

> +
> +typedef struct FilterParam
> +{
> +    int msizeX, msizeY;
> +    int amount;
> +    int stepsX, stepsY;
> +    int scalebits;
> +    int32_t halfscale;
> +    uint32_t *SC[(MAX_SIZE * MAX_SIZE) - 1];
> +} FilterParam;

Please document all these variables. 

Stylistic overall nitpick: this_variable style is preferred over
camelVariable, also

if (...) {
   ...
}

aka K&R, is favored over

if (...)
{
   ...
}

and similar, this to preserve overall code consistency in the project.

> +typedef struct
> +{
> +    FilterParam luma;   ///< luma parameters (width, height, amount)
> +    FilterParam chroma; ///< chroma parameters (width, height, amount)
> +} UnsharpContext;
> +
> +//===========================================================================//
> +
> +/* This code is based on :
> +
> +An Efficient algorithm for Gaussian blur using finite-state machines
> +Frederick M. Waltz and John W. V. Miller
> +
> +SPIE Conf. on Machine Vision Systems for Inspection and Metrology VII
> +Originally published Boston, Nov 98
> +
> +http://www.engin.umd.umich.edu/~jwvm/ece581/21_GBlur.pdf
> +
> +*/
> +
> +static void unsharpen( uint8_t *dst, uint8_t *src, int dstStride, int srcStride, int width, int height, FilterParam *fp )
                         ^
unsharpen(uint8_t -> no space before and after paretheses).

> +{
> +    uint32_t **SC = fp->SC;
> +    uint32_t SR[(MAX_SIZE * MAX_SIZE) - 1], Tmp1, Tmp2;
> +
> +    int32_t res;
> +    int x, y, z;
> +
> +    if (!fp->amount)
> +    {
> +        if (dstStride == srcStride)
> +            memcpy(dst, src, srcStride * height);
> +        else
> +            for (y = 0; y < height; y++, dst += dstStride, src += srcStride)
> +                memcpy(dst, src, width);
> +        return;
> +    }
> +
> +    for (y = 0; y < 2 * fp->stepsY; y++)
> +    {
> +        memset(SC[y], 0, sizeof(SC[y][0]) * (width + 2 * fp->stepsX));
> +    }
> +
> +    for (y =- fp->stepsY; y < height + fp->stepsY; y++)
> +    {
> +        memset(SR, 0, sizeof(SR[0]) * (2 * fp->stepsX - 1));
> +        for (x =- fp->stepsX; x < width + fp->stepsX; x++)
> +        {
> +            Tmp1 = x<=0 ? src[0] : x>=width ? src[width-1] : src[x];
> +            for (z = 0; z < fp->stepsX * 2; z += 2)
> +            {
> +            	Tmp2 = SR[z + 0] + Tmp1; SR[z + 0] = Tmp1;
> +            	Tmp1 = SR[z + 1] + Tmp2; SR[z + 1] = Tmp2;

Weird indent.

> +            }
> +            for (z = 0; z < fp->stepsY * 2; z += 2)
> +            {
> +            	Tmp2 = SC[z + 0][x + fp->stepsX] + Tmp1; SC[z + 0][x + fp->stepsX] = Tmp1;
> +            	Tmp1 = SC[z + 1][x + fp->stepsX] + Tmp2; SC[z + 1][x + fp->stepsX] = Tmp2;
> +            }
> +            if (x >= fp->stepsX && y >= fp->stepsY)
> +            {
> +            	uint8_t* srx = src - fp->stepsY * srcStride + x - fp->stepsX;
> +            	uint8_t* dsx = dst - fp->stepsY * dstStride + x - fp->stepsX;
> +
> +            	res = (int32_t)*srx + ((((int32_t) * srx - (int32_t)((Tmp1 + fp->halfscale) >> fp->scalebits)) * fp->amount) >> 16);
> +            	*dsx = av_clip_uint8(res);
> +            }
> +        }
> +        if (y >= 0)
> +        {
> +            dst += dstStride;
> +            src += srcStride;
> +        }
> +    }
> +}
> +
> +static void set_filter_param(FilterParam *fp, int msizeX, int msizeY, double amount)
> +{
> +    fp->msizeX = msizeX;
> +    fp->msizeY = msizeY;
> +    fp->amount = amount * 65536.0;
> +
> +    fp->stepsX = msizeX / 2;
> +    fp->stepsY = msizeY / 2;
> +    fp->scalebits = (fp->stepsX + fp->stepsY) * 2;
> +    fp->halfscale = 1 << ((fp->stepsX + fp->stepsY) * 2 - 1);
> +}
> +
> +static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
> +{
> +    UnsharpContext *unsharp = ctx->priv;
> +    char type;
> +    int msizeX, msizeY;
> +    double amount;
> +
> +    if (args)
> +    {
> +        sscanf(args, "%c:%d:%d:%lf", &type, &msizeX, &msizeY, &amount);
> +
> +        msizeX = FFMIN(MAX_SIZE, FFMAX(MIN_SIZE, msizeX));
> +        msizeY = FFMIN(MAX_SIZE, FFMAX(MIN_SIZE, msizeY));
> +    }
> +    else
> +    {
> +        type = 'l';
> +        msizeX = 3;
> +        msizeY = 3;
> +        amount = 1.5f;
> +    }

This is broken if not all the values are set in args. Maybe you could
just initialize the default values *before* to read the args. Also
please specify in the docs the default values.

> +
> +    if (type == 'l' || type == 'b')
> +    {
> +        set_filter_param(&unsharp->luma, msizeX, msizeY, amount);
> +    }
> +
> +    if (type == 'c' || type == 'b')
> +    {
> +        set_filter_param(&unsharp->chroma, msizeX, msizeY, amount);
> +    }
> +
> +    return 0;
> +}
> +
> +static int query_formats(AVFilterContext *ctx)
> +{
> +    enum PixelFormat pix_fmts[] = {
> +        PIX_FMT_YUV420P, PIX_FMT_NONE
> +    };
> +
> +    avfilter_set_common_formats(ctx, avfilter_make_format_list(pix_fmts));
> +
> +    return 0;
> +}
> +
> +static void init_fp(const char *effect_type, int width, FilterParam *fp)

nit: init_filter_param -> more clear, also:
init_filter_param(FilterParam *fp, const char *effect_type, int width)

looks more natural.

> +{
> +    int z;
> +    const char *effect;
> +
> +    effect = fp->amount == 0 ? "don't touch" : fp->amount < 0 ? "blur" : "sharpen";

I believe "none" instead of "don't touch" looks more clear.

> +    av_log(NULL, AV_LOG_INFO, "unsharp: %dx%d:%0.2f (%s %s)\n", fp->msizeX, fp->msizeY, fp->amount / 65535.0, effect, effect_type);

Maybe you could pass to the function the filter context rather than
use NULL, or event print this directly in config_props.

Also I tend to prefer something of the kind: 
[0xdeadbeef at unsharpen] msizex:%d msizey:%d amount:%f effect:%s type:%s

should be more readable.

> +
> +    memset(fp->SC, 0, sizeof(fp->SC));
> +    for (z = 0; z < 2 * fp->stepsY; z++)
> +    {
> +        fp->SC[z] = av_malloc(sizeof(*(fp->SC[z])) * (width + 2 * fp->stepsX));
> +    }
> +}
> +
> +static int config_props(AVFilterLink *link)
> +{
> +    UnsharpContext *unsharp = link->dst->priv;
> +
> +    init_fp("luma", link->w, &unsharp->luma);
> +    init_fp("chroma", link->w, &unsharp->chroma);

vertical align

> +
> +    return 0;
> +}
> +
> +static void free_fp(FilterParam *fp)
> +{
> +    int z;
> +
> +    for (z = 0; z < 2 * fp->stepsY; z++)
> +    {
> +        av_free(fp->SC[z]);
> +    }
> +}
> +
> +static av_cold void uninit(AVFilterContext *ctx)
> +{
> +    UnsharpContext *unsharp = ctx->priv;
> +
> +    free_fp(&unsharp->luma);
> +    free_fp(&unsharp->chroma);
> +}
> +
> +static void end_frame(AVFilterLink *link)
> +{
> +    UnsharpContext *unsharp = link->dst->priv;
> +    AVFilterPicRef *in  = link->cur_pic;
> +    AVFilterPicRef *out = link->dst->outputs[0]->outpic;
> +
> +    unsharpen(out->data[0], in->data[0], out->linesize[0], in->linesize[0], link->w, link->h, &unsharp->luma);
> +    unsharpen(out->data[1], in->data[1], out->linesize[1], in->linesize[1], link->w / 2, link->h / 2, &unsharp->chroma);
> +    unsharpen(out->data[2], in->data[2], out->linesize[2], in->linesize[2], link->w / 2, link->h / 2, &unsharp->chroma);
> +
> +    avfilter_unref_pic(in);
> +    avfilter_end_frame(link->dst->outputs[0]);
> +}
> +
> +AVFilter avfilter_vf_unsharp = {
> +    .name      = "unsharp",
> +    .description = NULL_IF_CONFIG_SMALL("Sharpen / blur input."),

Description is a sentence describing what the filter *does*, rather
than a long name.

> +
> +    .priv_size = sizeof(UnsharpContext),
> +
> +    .init = init,
> +    .uninit = uninit,
> +    .query_formats = query_formats,
> +
> +    .inputs    = (AVFilterPad[]) {{ .name             = "default",
> +                                    .type             = CODEC_TYPE_VIDEO,
> +                                    .end_frame        = end_frame,
> +                                    .config_props     = config_props,
> +                                    .min_perms        = AV_PERM_READ, },
> +                                  { .name = NULL}},
> +
> +    .outputs   = (AVFilterPad[]) {{ .name             = "default",
> +                                    .type             = CODEC_TYPE_VIDEO, },
> +                                  { .name = NULL}},
> +};
> Index: libavfilter/Makefile
> ===================================================================
> --- libavfilter/Makefile	(revision 22644)
> +++ libavfilter/Makefile	(working copy)
> @@ -22,6 +22,7 @@
>  OBJS-$(CONFIG_PIXELASPECT_FILTER)            += vf_aspect.o
>  OBJS-$(CONFIG_SCALE_FILTER)                  += vf_scale.o
>  OBJS-$(CONFIG_SLICIFY_FILTER)                += vf_slicify.o
> +OBJS-$(CONFIG_UNSHARP_FILTER)                += vf_unsharp.o
>  OBJS-$(CONFIG_VFLIP_FILTER)                  += vf_vflip.o
>  
>  OBJS-$(CONFIG_NULLSRC_FILTER)                += vsrc_nullsrc.o

Regards and thanks again.
-- 
FFmpeg = Furious and Fantastic Miracolous Powered Eccentric Gadget



More information about the ffmpeg-devel mailing list