[FFmpeg-devel] [PATCH] Unsharp filter

[Daniel G. Taylor]

On 03/24/2010 08:01 PM, Stefano Sabatini wrote:
> On date Wednesday 2010-03-24 12:03:54 -0400, Daniel G. Taylor encoded:
> [...]
>> +Sharpen or blur the input video. It accepts the following parameters:
>> + at var{effect_type}:@var{msize_x}:@var{msize_y}:@var{amount}.
>> + at var{effect_type} can be one of 'luma', 'chroma', or 'both'.
>> + at var{msize_x} and @var{msize_y} define the effect matrix size should be
>
> missing "," before "size should be..."

Fixed.

>> +integer values between 3 and 13. @var{amount} defines the strength of
>> +the effect with sane values in the -2.0 to 5.0 range with negative values
>
> missing ", " before "with negative values..."

Fixed.

>> [...]
>> +# Use the default values with FFmpeg
>
> I prefer just ffmpeg (maybe @filename{ffmpeg} or the equivalent),
> FFmpeg is mainly used for referring the the whole project.

Fixed.

>> [...]
>> +static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
>> +{
>> +    UnsharpContext *unsharp = ctx->priv;
>> +    char type;
>> +    int msize_x, msize_y;
>> +    double amount;
>> +
>> +    type = 'l';
>> +    msize_x = 5;
>> +    msize_y = 5;
>> +    amount = 1.0f;
>
> you can merge definition and declaration and save some lines

Fixed.

>> [...]
>
>> +    unsharpen(out->data[0], in->data[0], out->linesize[0], in->linesize[0], link->w, link->h,&unsharp->luma);
>
> NIT+ vertical align

Fixed.

>> [...]
>> +AVFilter avfilter_vf_unsharp = {
>> +    .name      = "unsharp",
>> +    .description = NULL_IF_CONFIG_SMALL("Sharpen or blur the input video"),
>
> missing final dot.

Fixed.

> Missing empty definition of draw_slice(), try for example
> "unsharp, hflip" to see what happens, also don't forget that you can
> trace what the hell is going in the filterchain enabling DEBUG in
> avfilter.c and using -loglevel debug in the ff* tools.

Thanks for the tip. I've not only fixed that but also found my last 
memory leak and fixed that as well by looking at how the rotate filter 
works.

> Also remember the GPL issue, check the function die_license_disabled()
> in the configure.

This is the one thing I cannot for the life of me figure out. How can I 
make it so that the filter is disabled by default, but automatically 
enabled when --enable-gpl is passed? Any tips would be much appreciated.

> Another very minor reserve which I have is the name of the filter,
> maybe "sharpenblur" or something mentioning also "blur" may help
> someone to immediately detect the filter, but then I leave to you the
> final choice as the author of the filter (also maybe is a good idea to
> keep the same name as the original MPlayer filter).

I think in this case it's probably best to keep the name since it is a 
port, uses the same algorithm and has the same features, etc. Also it's 
not that useful for blurs - past a certain small negative amount it no 
longer does what you would expect from a blur filter and gives very 
strange output. It _is_ good at small blurs and a good amount of 
sharpening, but I imagine the most common use case here will be just 
small amounts of sharpening by itself or after a scale. For that the 
"unsharp" name should be fine in my opinion.

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