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

[Ronald S. Bultje]

Hi,

On Sat, Jun 24, 2017 at 3:50 PM, Ashish Singh <ashk43712 at gmail.com> wrote:

> +typedef struct VMAFContext {
>
[..]

> +    double curr_vmaf_score;
>

Unused.


> +    uint64_t nb_frames;
>

Unused.


> +    pthread_attr_t attr;
>

This is essentially unused, so might just as well pass NULL into
pthread_create().


> +    int psnr;
> +    int ssim;
> +    int ms_ssim;
>

Hm... I see you're trying to replicate the commandline arguments here, but
we already have psnr/ssim filters by themselves. I'm not sure we need those
options here. (If others think we should keep them or you have a strong
opinion on keeping it, we can keep it.)


> +    FILE *stats_file;
> +    char *stats_file_str;
>

Unused.


> +    int stats_version;
>

Unused, and also unneeded. stats_version was added to some other filters so
we could update the stats_file format without breaking backwards
compatibility for applications expecting the old log format. This filter
has no "old" or "new" log format yet, so this isn't needed.


> +    int stats_header_written;
> +    int stats_add_max;
> +    int nb_components;
>

Unused.


> +static const AVOption vmaf_options[] = {
>
[..]

> +    {"stats_file", "Set file where to store per-frame difference
> information", OFFSET(stats_file_str), AV_OPT_TYPE_STRING, {.str=NULL}, 0,
> 0, FLAGS },
> +    {"f",          "Set file where to store per-frame difference
> information", OFFSET(stats_file_str), AV_OPT_TYPE_STRING, {.str=NULL}, 0,
> 0, FLAGS },
> +    {"stats_version", "Set the format version for the stats file.",
>          OFFSET(stats_version),  AV_OPT_TYPE_INT,    {.i64=1},    1, 2,
> FLAGS },
> +    {"output_max",  "Add raw stats (max values) to the output log.",
>       OFFSET(stats_add_max), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
>

Unused.


> +    {"model_path",  "Set the model to be used for computing vmaf.",
>       OFFSET(model_path), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 1, FLAGS},
>

I would suggest to add a non-NULL default model path here (instead of {
.str = NULL }). Maybe the default path can be detected by configure?


> +    {"psnr",  "Enables computing psnr along with vmaf.",
> OFFSET(psnr), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
> +    {"ssim",  "Enables computing ssim along with vmaf.",
> OFFSET(ssim), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
> +    {"ms_ssim",  "Enables computing ms-ssim along with vmaf.",
> OFFSET(ms_ssim), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
>

See related comment above.


> +static int read_frame_8bit(float *ref_data, float *main_data, int stride,
> double *score, void *ctx){
> +
>

{ Should go on new line for function declarations.


> +    VMAFContext *s = (VMAFContext *)ctx;
>

.. = (VMAFContext *) ctx; << note space between ")" and "ctx"


> +    if (s->eof == 1) {
> +        s->eof++;
> +    }
> +    else if (s->eof == 2) {
> +        return s->eof;
> +    }
>

Umm, this isn't what I meant with my comment earlier. There's a race
condition here where s->eof is written to by a different thread but you
read it outside the critical section within s->lock protection:


> +    pthread_mutex_lock(&s->lock);
> +
> +    while (s->frame_set == 0) {
> +        pthread_cond_wait(&s->cond, &s->lock);
> +    }
>

So at this point, you have two variables (s->eof and s->frame_set) both
accessible from s->lock. That's fine, but the problem is that you're not
checking whether s->frame_set is 1 before exiting based on s->eof values.
My suggestion is something like this:

pthread_mutex_lock(&s->lock);
while (!s->frame_set && !s->eof) {
    pthread_cond_wait(&s->cond, &s->lock);
}
if (s->frame_set) {
    .. copy data into vmaf buffers
}

int res = !s->frame_set;
s->frame_set = 0;
pthread_cond_signal(&s->cond);
pthread_mutex_unlock(&s->lock);

return ret;

+static int read_frame_10bit(float *ref_data, float *main_data, int stride,
> double *score, void *ctx){
>

I'd also recommend to tempalte the read_frame_8/10bit() functions, that
makes maintenance easier because it reduces source code size. But that can
be done later...

+static void compute_vmaf_score(VMAFContext *s)
> +{
> +    void *func;
>

int (*read_frame)(int arg1, int arg2);


> +    if (strcmp(s->format, "yuv420p") || strcmp(s->format, "yuv422p") ||
> strcmp(s->format, "yuv444p")) {
> +        func = read_frame_8bit;
>

read_frame = read_frame_8bit;


> +    }
> +    else {
>

"} else {" all on same line.

+        func = read_frame_10bit;
>

read_frame = read_frame_10bit;


> +static av_cold int init(AVFilterContext *ctx)
> +{
>
[..]

> +    if (s->stats_file_str) {
> +        if (s->stats_version < 2 && s->stats_add_max) {
> +            av_log(ctx, AV_LOG_ERROR,
> +                   "stats_add_max was specified but stats_version < 2.\n"
> );
> +            return AVERROR(EINVAL);
> +        }
> +        if (!strcmp(s->stats_file_str, "-")) {
> +            s->stats_file = stdout;
> +        } else {
> +            s->stats_file = fopen(s->stats_file_str, "w");
> +            if (!s->stats_file) {
> +                int err = AVERROR(errno);
> +                char buf[128];
> +                av_strerror(err, buf, sizeof(buf));
> +                av_log(ctx, AV_LOG_ERROR, "Could not open stats file %s:
> %s\n",
> +                       s->stats_file_str, buf);
> +                return err;
> +            }
> +        }
> +    }
>

I think ATM all this code can be removed.


> +static av_cold void uninit(AVFilterContext *ctx)
>
[..]

> +    if (s->stats_file && s->stats_file != stdout)
> +        fclose(s->stats_file);
>

Can be removed.


> +    av_log(ctx, AV_LOG_INFO, "VMAF score: %f\n",s->vmaf_score);
> +
> +}
>

Remove empty line at the end.

Don't forget to pthread_mutex/cond_destroy() the cond and lock variables.

Ronald