[FFmpeg-devel] [PATCH] Unsharp filter

[Daniel G. Taylor]

On 03/23/2010 07:31 PM, Stefano Sabatini wrote:
> 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.
>    

Thanks for the comments! Update patch attached, comments inline.

> [...]
>    
>> 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...
>    

Fixed, parameters updated, defaults are more clear.

>> +
>> + 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.
>    

Added bare graph descriptions, left one example using FFmpeg for people 
to see that they can apply the bare graph descriptions to.

>> [...]
>> +
>> +#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.
>    

They are used throughout the file in various places and the MAX_SIZE is 
used just a couple lines below the declaration. Copying the values 
everywhere seems like a bad idea. If I should move these - where would a 
good location be? I also updated them to reflect better sane values as 
they are used to clip the input 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.
>    

Fixed.

> 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.
>    

Fixed everywhere in the file.

>> +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).
>    

Fixed, was a remnant of copied code that I had missed.

>> [...]
>> +    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.
>    

Fixed.

>> [...]
>> +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.
>    

Fixed, see new parameter parsing code.

>> [...]
>> +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.
>    

Fixed.

>> +{
>> +    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.
>    

Fixed.

>> +    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.
>    

Fixed. I wasn't aware I could pass the filter context and have it do 
that - good to know!

>> +
>> +    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
>    

Fixed.

>> [...]
>> +
>> +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.
>    

Fixed.

>> [...]
>>      
> Regards and thanks again.
>    

Thanks for the quick reply. Let me know what else needs to be done to 
get this into trunk and/or SoC libavfilter.

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: 11193 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100324/7ca0290e/attachment.diff>