[FFmpeg-devel] [PATCH] avfilter: add normalize filter

Richard Ling divetec at rling.com
Thu Sep 21 15:43:14 EEST 2017

> Thanks for the patch. Unfortunately, your mail software mangled it with
> line breaks, it cannot be applied as is. Still, see a few comments
> below.
I should have posted to myself first to make sure it worked OK.
I will do that before posting another patch to the list.

> +The amount of temporal smoothing, expressed in seconds. the input range
> +each channel is smoothed using a rolling average over that many seconds
> +video. Defaults to 0.0 (no temporal smoothing).  The maximum is 60

If I read the code correctly, the rolling average is asymmetrical:
> the current frame is computed using the n previous frames rather than
> the n/2 previous and n/2 next. Which, of course, is the best that can be
> done without buffering that many frames.
> But the documentation should probably specify it.
Yes, it is asymmetrical.
OK, I'll change to ".a rolling average over that many seconds of PREVIOUS
Although, it only makes sense if there is a way to allocate a buffer for
the rolling average based on the frame rate, and you imply below that there
is not.

> > +normalize=black:white:0
> Better use named options in example. See the drama about that last
> summer.

OK, I'll do that.

> > +#ifndef MIN
> > +#define MIN(x,y)        ((x) < (y) ? (x) : (y))
> > +#endif
> > +#ifndef MAX
> > +#define MAX(x,y)        ((x) > (y) ? (x) : (y))
> > +#endif

OK, will use those, I didn't know they existed.

> +#define INIT(c)     (min[c].in = max[c].in = in->data[0][s->co[c]])
> +#define EXTEND(c)   (min[c].in = MIN(min[c].in, inp[s->co[c]])), \
> +                    (max[c].in = MAX(max[c].in, inp[s->co[c]]))
> +
> +    INIT(0);
> +    INIT(1);
> +    INIT(2);

I think a loop over c, same as below, would be better style.
Agreed, will change it

> +// For future use...
> +static av_cold int init(AVFilterContext *ctx)
> +{
> +    return 0;
> +}

It can be added when needed.
Agreed, I will remove that function.

> +    // Convert smoothing value (seconds) to history_len (a count of
> to
> +    // average, must be at least 1).
> +    s->history_len = (int)(s->smoothing / av_q2d(inlink->time_base)) + 1;
> +    // In case the frame rate is unusually high, cap it to
> +    // to avoid allocating stupid amounts of memory.

According to the comments, you are mistaking time_base with the stream's
> frame rate. They are not the same, and the streams are not guaranteed to
> be constant frame rate anyway.
> I think you should consider using an exponential moving average instead:
> you can get rid of all the history code. Furthermore, exponential moving
> average supports variable frame rate with just a bit of maths: set the
> decay coefficient to exp(-dt/T); extra bonus: compute exp(-x) as
> 1/exp(x) using a short power series to approximate the exponential.

I'm concerned that with exponential rolling average, it is not easy to
choose a suitable decay rate.
Even with a very fast decay rate, the output range depends on every frame
ever seen, no matter how long ago.  There is no upper limit on age.  The
problem gets worse with slower decay rate.  But with a faster decay rate,
there is not enough smoothing (too much bias to most recent frame(s)) so
flickering will return.
The square wave of a rolling average makes more intuitive sense to me. But
I haven't tested it (I've tested existing method extensively).

Is it possible to test for variable frame rate video and make the filter
issue a warning and passthrough (or simply fail, ie force an error)?
I have no need for working with such video.  Other existing temporal
filters must have the same problem. It could be a future enhancement for
the filter, when (if) it is ever needed.

> Regards,
> --
>   Nicolas George


More information about the ffmpeg-devel mailing list