[FFmpeg-devel] [PATCH] Port gradfun to libavfilter (GCI)

[Stefano Sabatini]

On date Wednesday 2010-12-01 22:13:25 -0500, Nolan L encoded:
> On Tue, Nov 30, 2010 at 10:00 PM, Nolan L <nol888 at gmail.com> wrote:
> >
> > Purely aesthetic change regarding a confusing segment of code. Seems as if
> > this was the only complaint, so I'm creating a new revision for that one
> > change.
> >
> 
> (Hopefully) the final revision of this port. The clarification of "direct
> rendering" allowed me to make the necessary code change to implement the
> libavfilter equivalent.
> 
> Also fixed up various other complaints.

> diff --git a/doc/filters.texi b/doc/filters.texi
> index 1cba2d6..a71e75b 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -333,6 +333,32 @@ frei0r=perspective:0.2/0.2:0.8/0.2
>  For more information see:
>  @url{http://piksel.org/frei0r}
>  
> + at section gradfun
> +
> +Fix the banding artifacts that are sometimes introduced into nearly flat
> +regions by truncation to 8bit colordepth.
> +Interpolates the gradients that should go where the bands are, and
> +dithers them.
> +
> +The filter takes two optional parameters, separated by ':',
> +"@var{strength}:@var{radius}"
> +
> + at var{strength} is the maximum amount by which the filter will change
> +any one pixel. Also the threshold for detecting nearly flat
> +regions (default: 1.2).
> +
> + at var{radius} is the neighborhood to fit the gradient to. A larger radius
> +makes for smoother gradients, but also prevents the filter from modifying
> +the pixels near detailed regions (default: 16).

Maybe you should mention that the input values are clipped in the
admissible range (and specify which).

> + at example
> +# Default parameters.
> +gradfun=1.2:16
> +
> +# Omitting radius
> +gradfun=1.2
> + at end example
> +
>  @section hflip
>  
>  Flip the input video horizontally.
> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> index 210510f..7e317c8 100644
> --- a/libavfilter/Makefile
> +++ b/libavfilter/Makefile
> @@ -28,6 +28,7 @@ OBJS-$(CONFIG_FORMAT_FILTER)                 += vf_format.o
>  OBJS-$(CONFIG_FREI0R_FILTER)                 += vf_frei0r.o
>  OBJS-$(CONFIG_HFLIP_FILTER)                  += vf_hflip.o
>  OBJS-$(CONFIG_NOFORMAT_FILTER)               += vf_format.o
> +OBJS-$(CONFIG_GRADFUN_FILTER)                += vf_gradfun.o
>  OBJS-$(CONFIG_NULL_FILTER)                   += vf_null.o
>  OBJS-$(CONFIG_OCV_SMOOTH_FILTER)             += vf_libopencv.o
>  OBJS-$(CONFIG_OVERLAY_FILTER)                += vf_overlay.o
> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
> index c3067b8..6ac517c 100644
> --- a/libavfilter/allfilters.c
> +++ b/libavfilter/allfilters.c
> @@ -48,6 +48,7 @@ void avfilter_register_all(void)
>      REGISTER_FILTER (FORMAT,      format,      vf);
>      REGISTER_FILTER (FREI0R,      frei0r,      vf);
>      REGISTER_FILTER (HFLIP,       hflip,       vf);
> +    REGISTER_FILTER (GRADFUN,     gradfun,     vf);

Nit: alphabetical order.

[...]
> diff --git a/libavfilter/vf_gradfun.c b/libavfilter/vf_gradfun.c
[...]
> +static void start_frame(AVFilterLink *inlink, AVFilterBufferRef *inpicref)
> +{
> +    AVFilterBufferRef *outpicref = avfilter_ref_buffer(inpicref, ~0);
> +    inlink->dst->outputs[0]->out_buf = outpicref;
> +    avfilter_start_frame(inlink->dst->outputs[0], outpicref);
> +}
> +
> +static void end_frame(AVFilterLink *inlink)
> +{
> +    GradFunContext *gf = inlink->dst->priv;
> +    AVFilterBufferRef *inpic = inlink->cur_buf;
> +    AVFilterBufferRef *outpic = inlink->dst->outputs[0]->out_buf;

This looks weird, you're putting in outlink->out_buf a reference to
the input buffer, then you're doing inplace processing in end_frame().

Is this really wanted? (and is this the behavior of the original
filter?).

If this is the case you can simplify below since you don't need a
distinct inpic and outpic.

> +    int p;
> +    
> +    for (p = 0; p < 4 && inpic->data[p]; p++) {
> +        int w = inlink->w;
> +        int h = inlink->h;
> +        int r = gf->radius;
> +        if (p) {
> +            w = gf->chroma_w;
> +            h = gf->chroma_h;
> +            r = gf->chroma_r;
> +        }
> +        
> +        if (FFMIN(w, h) > 2 * r)
> +            filter(gf, outpic->data[p], inpic->data[p], w, h, outpic->linesize[p], inpic->linesize[p], r);
> +        else if (outpic->data[p] != inpic->data[p])
> +            av_image_copy_plane(outpic->data[p], outpic->linesize[p], inpic->data[p], inpic->linesize[p], w, h);
> +    }
> +
> +    avfilter_draw_slice(inlink->dst->outputs[0], 0, inlink->h, 1);
> +    avfilter_end_frame(inlink->dst->outputs[0]);
> +    avfilter_unref_buffer(inpic);
> +}

[...]
> diff --git a/libavfilter/x86/Makefile b/libavfilter/x86/Makefile
> index 716048c..628e6e3 100644
> --- a/libavfilter/x86/Makefile
> +++ b/libavfilter/x86/Makefile
> @@ -1 +1,3 @@
>  MMX-OBJS-$(CONFIG_YADIF_FILTER)              += x86/yadif.o
> +OBJS-$(CONFIG_GRADFUN_FILTER)                += x86/gradfun.o
> +

Is it correct to skip MMX- in OBJS?

If MMX is disabled then this shouldn't be linked.

[...]

Regards.
-- 
FFmpeg = Foolish Faithful Mortal Pitiless Elected Guru