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

[Derek Buitenhuis]

Quick scan / review follows.

> +#include <stdbool.h>

Do we allow C99 bool in FFmpeg? I thought we didn't.

> +#define FORCE_INLINE __attribute__((always_inline))
> +#define RESTRICT __restrict

av_restrict
av_always_inline

> +static inline int floorn(int n, int m)
> +{
> +    return n - n % m;
> +}
> +
> +static inline int ceiln(int n, int m)
> +{
> +    return n % m ? n + (m - n % m) : n;
> +}

Can this not be done with av_rescale_rnd or something?

> +
> +FORCE_INLINE inline float convolution_edge(bool horizontal, const float *filter,
> +                                           int filt_width, const float *src,
> +                                           int w, int h, int stride, int i,
> +                                           int j)

inline mixed with force inline doesn't make any sense.

> +{
> +    int radius = filt_width / 2;
> +
> +    float accum = 0;
> +    for (int k = 0; k < filt_width; ++k) {
> +        int i_tap = horizontal ? i : i - radius + k;
> +        int j_tap = horizontal ? j - radius + k : j;

The checks for horizontal should probably be moved outside the loop.

> +
> +        if (horizontal) {
> +            if (j_tap < 0)
> +                j_tap = -j_tap;
> +            else if (j_tap >= w)
> +                j_tap = w - (j_tap - w + 1);
> +        } else {
> +            if (i_tap < 0)
> +                i_tap = -i_tap;
> +            else if (i_tap >= h)
> +                i_tap = h - (i_tap - h + 1);
> +        }
> +
> +        accum += filter[k] * src[i_tap * stride + j_tap];
> +    }
> +    return accum;
> +}

[...
> +#define OFFSET(x) offsetof(MOTIONContext, x)
Not used.

> +#define MAX_ALIGN 32

> +static inline double get_motion_avg(double motion_sum, uint64_t nb_frames)
> +{
> +    return motion_sum / nb_frames;
> +}

Necessary to be it own func?

> +static double image_sad_c(const float *img1, const float *img2, int w,
> +                          int h, int img1_stride, int img2_stride)

Why does this re-implement basic DSP functions like SAD?

> +
> +static void set_meta(AVDictionary **metadata, const char *key, float d)
> +{
> +    char value[128];
> +    snprintf(value, sizeof(value), "%0.2f", d);

Do we care if this fails on super large values? (Or can it?)

> +    av_dict_set(metadata, key, value, 0);

Missing return value check.

- Derek