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

[Richard Ling]

>
>
> 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
of
> +each channel is smoothed using a rolling average over that many seconds
of
> +video. Defaults to 0.0 (no temporal smoothing).  The maximum is 60
seconds.

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
video".
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
>
> FFMIN(), FFMAX()
>

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
frames
> 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
MAX_HISTORY_LEN
> +    // 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
>
>
>
Cheers

R.