[FFmpeg-devel] [PATCH] Unsharp filter

[Stefano Sabatini]

On date Thursday 2010-03-25 13:10:52 -0400, Daniel G. Taylor encoded:
> 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.

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

diff --git a/configure b/configure
index 0682006..44b1d3a 100755
--- a/configure
+++ b/configure
@@ -90,6 +90,7 @@ Configuration options:
   --disable-swscale        disable libswscale build
   --enable-postproc        enable GPLed postprocessing support [no]
   --enable-avfilter        video filter support [no]
+  --enable-avfilter-gpl    video filters dependent on gpl [no]
   --enable-avfilter-lavf   video filters dependent on avformat [no]
   --enable-beosthreads     use BeOS threads [no]
   --enable-os2threads      use OS/2 threads [no]
@@ -888,6 +889,7 @@ CONFIG_LIST="
     avcodec
     avdevice
     avfilter
+    avfilter_gpl
     avfilter_lavf
     avformat
     avisynth
@@ -1416,6 +1418,8 @@ udp_protocol_deps="network"
 # filters
 movie_filter_deps="avfilter_lavf"
 avfilter_lavf_deps="avformat"
+unsharp_filter_deps="avfilter_gpl"
+avfilter_gpl_deps="gpl"
 
 # libraries
 avdevice_deps="avcodec avformat"
@@ -2334,6 +2338,7 @@ die_license_disabled gpl libx264
 die_license_disabled gpl libxvid
 die_license_disabled gpl postproc
 die_license_disabled gpl x11grab
+die_license_disabled gpl avfilter_gpl
 
 die_license_disabled nonfree libfaac
 
Indeed it was quite tricky, this is my solution, let's see if mans
likes it.

Anyway I'd prefer to relicense the filter as LGPL and avoid all the
configuration trickery if you manage to contact the original author
and convince him to relicense (that can be eventually done after
inclusion).

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

OK.

> Take care,
> -- 
> Daniel G. Taylor
> http://programmer-art.org/

> Index: doc/libavfilter.texi
> ===================================================================
> --- doc/libavfilter.texi	(revision 22666)
> +++ doc/libavfilter.texi	(working copy)
> @@ -217,6 +217,28 @@
>  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 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
> +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
> +causing a blur effect and positive values sharpening the image. All
> +parameters are optional and default to the equivalent of 'luma:5:5:1.0'.

Just another nit: more line breaks may help readability, you can use
the @* command for forcing a line break without to start a new
paragraph (for example to separate the description of each parameter),
or even better if you feel hairy you can create a table like is done
for the pad filter parameters.

[...]

Apart from this I'm fine with the patch.

Regards.
-- 
FFmpeg = Fundamental and Fierce Maxi Picky Embarassing Gadget