[FFmpeg-devel] [PATCH] Unsharp filter
Daniel G. Taylor
dan
Tue Mar 23 22:37:21 CET 2010
Hey,
On 03/19/2010 02:05 AM, Bobby Bingham wrote:
> On Thu, 18 Mar 2010 13:14:36 -0400
> "Daniel G. Taylor"<dan at programmer-art.org> wrote:
>
>
>> Hey,
>>
>> I'm interested in contributing to libavfilter. I have ported the
>> unsharp filter from MPlayer, which is attached. I still don't fully
>>
> Great! It's always good to have more filters, both for using and to
> serve as examples for future filter writers.
>
Thanks for the quick review. I've spent a few days trying to fix up all
the issues and have attached an updated patch. Updated review would be
appreciated.
>> understand how everything works, so please go easy on me in the
>> review.
>>
>> The attached patch is against FFmpeg + SoC libavfilter. Usage
>> examples are in the updated documentation.
>>
>> Take care,
>>
>
>> ...
>>
>> Flip the input video vertically.
>> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
>> index 9e59b0b..041bef0 100644
>> --- a/libavfilter/Makefile
>> +++ b/libavfilter/Makefile
>> @@ -32,6 +32,7 @@ OBJS-$(CONFIG_SLICIFY_FILTER) += vf_slicify.o
>> OBJS-$(CONFIG_SPLIT_FILTER) += vf_split.o
>> OBJS-$(CONFIG_TRANSPOSE_FILTER) += vf_transpose.o
>> OBJS-$(CONFIG_VFLIP_FILTER) += vf_vflip.o
>> +OBJS-$(CONFIG_UNSHARP_FILTER) += vf_unsharp.o
>>
>> OBJS-$(CONFIG_BUFFER_FILTER) += vsrc_buffer.o
>> OBJS-$(CONFIG_MOVIE_FILTER) += vsrc_movie.o
>>
> You'll need to recreate this patch from current SVN. Spacing has
> changed in this file recently. Please keep the list in alphabetic
> order as well.
>
Fixed.
>> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
>> index 73e5fbe..b925960 100644
>> --- a/libavfilter/allfilters.c
>> +++ b/libavfilter/allfilters.c
>> @@ -51,6 +51,7 @@ void avfilter_register_all(void)
>> REGISTER_FILTER(SPLIT,split,vf);
>> REGISTER_FILTER(TRANSPOSE,transpose,vf);
>> REGISTER_FILTER(VFLIP,vflip,vf);
>> + REGISTER_FILTER(UNSHARP,unsharp,vf);
>>
>> REGISTER_FILTER(BUFFER,buffer,vsrc);
>> REGISTER_FILTER(MOVIE,movie,vsrc);
>> diff --git a/libavfilter/vf_unsharp.c b/libavfilter/vf_unsharp.c
>> new file mode 100644
>> index 0000000..1cd033b
>> --- /dev/null
>> +++ b/libavfilter/vf_unsharp.c
>> @@ -0,0 +1,244 @@
>> +/*
>> + * 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.
>>
> I'm sure some developers would appreciate it if you could ask the
> original developer about the possibility of relicensing under the LGPL
> instead. If not, I believe there is some configure magic required to
> make this only build when the user specified --enable-gpl.
>
I actually emailed him before writing to this list. So far no reply, so
we may be stuck with GPL for now. I believe a slice-based
reimplementation from the original paper could be licensed LGPL, though.
>> + *
>> + * 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 Lesser 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
>> + */
>>
> This is not the same license mentioned above.
>
Fixed.
>> +
>> +/**
>> + * @file libavfilter/vf_unsharp.c
>> + * blur / sharpen filter
>> + */
>> +
>> +#include "avfilter.h"
>> +#include "libavutil/common.h"
>> +#include "libavutil/mem.h"
>> +
>> +#define MIN_MATRIX_SIZE 3
>>
> unused
>
Fixed.
>> +#define MAX_MATRIX_SIZE 63
>> +
>> +typedef struct FilterParam
>> +{
>> + int msizeX, msizeY;
>> + double amount;
>> + uint32_t *SC[MAX_MATRIX_SIZE - 1];
>> +} FilterParam;
>> +
>> +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
>> +
>> +https://docs.google.com/viewer?url=http://www.engin.umd.umich.edu/~jwvm/ece581/21_GBlur.pdf
>> +
>>
> Why not link directly to the original PDF?
>
Fixed.
>> +*/
>> +
>> +static void unsharpen( uint8_t *dst, uint8_t *src, int dstStride, int srcStride, int width, int height, FilterParam *fp )
>> +{
>> + uint32_t **SC = fp->SC;
>> + uint32_t SR[MAX_MATRIX_SIZE - 1], Tmp1, Tmp2;
>> + uint8_t* src2 = src; // avoid gcc warning
>>
> It looks like src2 always equals src, and is hence unnecessary.
>
Fixed.
>> +
>> + int32_t res;
>> + int x, y, z;
>> + int amount = fp->amount * 65536.0;
>> + int stepsX = fp->msizeX / 2;
>> + int stepsY = fp->msizeY / 2;
>>
> Instead of recalculating these same three values every from, calculate
> them in init and store them in the context.
>
Fixed.
>> + int scalebits = (stepsX + stepsY) * 2;
>> + int32_t halfscale = 1<< ((stepsX + stepsY) * 2 - 1);
>> +
>> + if (!fp->amount)
>> + {
>> + if (src == dst)
>> + return;
>>
> This cannot happen.
>
Removed.
>> +
>> + if (dstStride == srcStride)
>> + memcpy(dst, src, srcStride*height);
>> + else
>> + for (y = 0; y< height; y++, dst += dstStride, src += srcStride)
>> + memcpy( dst, src, width );
>> + return;
>> + }
>>
> Tabs are forbidden in ffmpeg svn.
>
Remnant from copied code, fixed.
>> +
>> + for (y = 0; y< 2 * stepsY; y++)
>> + {
>> + memset(SC[y], 0, sizeof(SC[y][0]) * (width + 2 * stepsX));
>> + }
>> +
>> + for (y =- stepsY; y< height + stepsY; y++)
>> + {
>> + if(y< height)
>> + src2 = src;
>> +
>> + memset(SR, 0, sizeof(SR[0]) * (2 * stepsX - 1));
>> +
>> + for (x =- stepsX; x< width + stepsX; x++)
>> + {
>> + Tmp1 = x<=0 ? src2[0] : x>=width ? src2[width-1] : src2[x];
>> + for (z = 0; z< stepsX * 2; z += 2)
>> + {
>> + Tmp2 = SR[z + 0] + Tmp1; SR[z + 0] = Tmp1;
>> + Tmp1 = SR[z + 1] + Tmp2; SR[z + 1] = Tmp2;
>> + }
>> + for (z = 0; z< stepsY * 2; z += 2)
>> + {
>> + Tmp2 = SC[z + 0][x + stepsX] + Tmp1; SC[z + 0][x + stepsX] = Tmp1;
>> + Tmp1 = SC[z + 1][x + stepsX] + Tmp2; SC[z + 1][x + stepsX] = Tmp2;
>> + }
>> + if (x>= stepsX&& y>= stepsY)
>> + {
>> + uint8_t* srx = src - stepsY*srcStride + x - stepsX;
>> + uint8_t* dsx = dst - stepsY*dstStride + x - stepsX;
>> +
>> + res = (int32_t)*srx + ((((int32_t) * srx - (int32_t)((Tmp1 + halfscale)>> scalebits)) * amount)>> 16);
>> + *dsx = res> 255 ? 255 : res< 0 ? 0 : (uint8_t)res;
>>
> av_clip_uint8
>
Fixed.
>> + }
>> + }
>> + if (y>= 0)
>> + {
>> + dst += dstStride;
>> + src += srcStride;
>> + }
>> + }
>> +}
>>
> I'll read the paper and come back and review this function. It does
> look possible to support slices, rather than filtering the entire frame
> at the end.
>
I'm afraid I don't understand the algorithm well enough yet to
re-implement it using slices so I opted for working with whole frames
like the original did. I'd still love to hear your input on this.
>> +
>> +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);
>> + else
>> + {
>> + type = 'l';
>> + msizeX = 3;
>> + msizeY = 3;
>> + amount = 1.5f;
>> + }
>> +
>> + if (type == 'l' || type == 'b')
>> + {
>> + unsharp->luma.msizeX = msizeX;
>> + unsharp->luma.msizeY = msizeY;
>> + unsharp->luma.amount = amount;
>> + }
>> +
>> + if (type == 'c' || type == 'b')
>> + {
>> + unsharp->chroma.msizeX = msizeX;
>> + unsharp->chroma.msizeY = msizeY;
>> + unsharp->chroma.amount = amount;
>> + }
>>
> You should do some bounds checking on msize*.
>
Done, though more checks could be made in the future.
>> +
>> + return 0;
>> +}
>> +
>> +static int query_formats(AVFilterContext *ctx)
>> +{
>> + enum PixelFormat pix_fmts[] = {
>> + PIX_FMT_YUV420P, PIX_FMT_NONE
>> + };
>>
> Does this really only work for this format?
>
I don't know. I don't entirely understand how all the pixel formats
work, and the big list in libavutil isn't that useful for figuring out
what I can and cannot support. Help / explanations / whatever would be
appreciated here!
>> +
>> + avfilter_set_common_formats(ctx, avfilter_make_format_list(pix_fmts));
>> +
>> + return 0;
>> +}
>> +
>> +static int config_props(AVFilterLink *link)
>> +{
>> + UnsharpContext *unsharp = link->dst->priv;
>> + FilterParam *fp;
>> + int z, stepsX, stepsY;
>> + char *effect;
>>
> Either declare this const char * or put its value directly into the
> av_log calls below to avoid warnings about discarding qualifiers.
>
Fixed.
>> +
>> + fp =&unsharp->luma;
>> + effect = fp->amount == 0 ? "don't touch" : fp->amount< 0 ? "blur" : "sharpen";
>> +
>> + av_log(NULL, AV_LOG_ERROR, "unsharp: %dx%d:%0.2f (%s luma)\n", fp->msizeX, fp->msizeY, fp->amount, effect);
>> +
>> + memset(fp->SC, 0, sizeof(fp->SC));
>> + stepsX = fp->msizeX / 2;
>> + stepsY = fp->msizeY / 2;
>> + for (z = 0; z< 2 * stepsY; z++)
>> + {
>> + fp->SC[z] = memalign(16, sizeof(*(fp->SC[z])) * (link->w + 2 * stepsX));
>>
> Use av_malloc instead of memalign. Also, you never free this memory.
>
Fixed and fixed.
>> + }
>> +
>> + fp =&unsharp->chroma;
>> + effect = fp->amount == 0 ? "don't touch" : fp->amount< 0 ? "blur" : "sharpen";
>> +
>> + av_log(NULL, AV_LOG_ERROR, "unsharp: %dx%d:%0.2f (%s chroma)\n", fp->msizeX, fp->msizeY, fp->amount, effect);
>> +
>> + memset(fp->SC, 0, sizeof(fp->SC));
>> + stepsX = fp->msizeX / 2;
>> + stepsY = fp->msizeY / 2;
>> + for (z = 0; z< 2 * stepsY; z++)
>> + {
>> + fp->SC[z] = memalign(16, sizeof(*(fp->SC[z])) * (link->w + 2 * stepsX));
>> + }
>>
> This is a duplicate of the above code. Factor it into a separate function please.
>
Fixed.
>> +
>> + return 0;
>> +}
>> +
>> +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], link->w, link->w, link->w, link->h,&unsharp->luma);
>> + unsharpen(out->data[1], in->data[1], link->w, link->w, link->w, link->h,&unsharp->chroma);
>> + unsharpen(out->data[2], in->data[2], link->w, link->w, link->w, link->h,&unsharp->chroma);
>> +
>>
> You pass the width as the image strides. This is not correct. Use the
> linesize member from AVFilterPicRef.
>
Fixed.
> The actual dimensions of the chroma planes also do not match that of the
> luma plane. Check out av_pix_fmt_descriptors in libavutil (someone
> correct me if this isn't the current preferred way to get this
> information).
>
This is still confusing to me. There is a lot of information in that
file but I'm just not sure how exactly to use it! Any documentation or
books to read would be useful!
For now I've copied how it's done in mplayer, just divide the
width/height by two for the chroma planes and only support YUV420P. Not
sure why it worked for my samples before.
>> + avfilter_end_frame(link->dst->outputs[0]);
>> +}
>>
> You need to call avfilter_unref_pic(in). You're leaking memory like mad.
>
Fixed. Good call, it's been a while since I've coded C. I have a feeling
I'm still leaking memory, but I'm not sure where or how exactly. Any
tips would be appreciated.
>> +
>> +AVFilter avfilter_vf_unsharp = {
>> + .name = "unsharp",
>> + .description = NULL_IF_CONFIG_SMALL("Sharpen / blur input."),
>> +
>> + .priv_size = sizeof(UnsharpContext),
>> +
>> + .init = init,
>> + .query_formats = query_formats,
>> +
>> + .inputs = (AVFilterPad[]) {{ .name = "default",
>> + .type = CODEC_TYPE_VIDEO,
>> + .end_frame = end_frame,
>> + .config_props = config_props, },
>>
> You should specify .min_perms = AV_PERM_READ, because your code needs
> permission to read from the source frame. I can't think of a reason
> you wouldn't always get that permission, regardless, but it's better to
> ask for it anyway.
>
Fixed.
Take care,
--
Daniel G. Taylor
http://programmer-art.org/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unsharp.diff
Type: text/x-diff
Size: 10230 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100323/902713ad/attachment.diff>
More information about the ffmpeg-devel
mailing list