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

[Moritz Barsnick]

On Mon, Jul 03, 2017 at 22:08:12 +0530, Ashish Singh wrote:
> Added vmaf section in doc/filters.texi and Changelog.

Thanks for that.

> +This filter takes in input two input videos, the first input is

Input input? ;-) "This filter takes two input videos" should suffice.

> +Currently it requires Netflix's vmaf library (libvmaf) as a pre-requisite.
> +It can be enabled using --enable-libvmaf at ./configure ..

Extra dot there. Perhaps you also want to wrap "--enable-libvmaf" and
"./configure" in something like %code{}.

> +If no model path is not specified it uses default model.

Double negation. I think you missed to delete some word(s) when
rearranging the sentence.

> +On this example the input file @file{main.mpg} being processed is compared with the

"In this example". (Shouldn't the explanation come before the example?)

> +static const AVOption vmaf_options[] = {
> +    {"model_path",  "Set the model to be used for computing vmaf.",            OFFSET(model_path), AV_OPT_TYPE_STRING, {.str="/usr/local/share/model/vmaf_v0.6.1.pkl"}, 0, 1, FLAGS},
> +    {"log_path",  "Set the file path to be used to store logs.",            OFFSET(log_path), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 1, FLAGS},
> +    {"log_fmt",  "Set the format of the log (xml or json).",            OFFSET(log_fmt), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 1, FLAGS},
> +    {"disable_clip",  "Disables clip for computing vmaf.",            OFFSET(disable_clip), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
> +    {"disable avx",  "Disables avx for computing vmaf.",            OFFSET(disable_avx), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
> +    {"enable_transform",  "Enables transform for computing vmaf.",            OFFSET(enable_transform), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
> +    {"phone_model",  "Invokes the phone model that will generate higher VMAF scores.",            OFFSET(phone_model), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
> +    {"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},
> +    {"pool",  "Set the pool method to be used for computing vmaf.",            OFFSET(pool), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 1, FLAGS},
> +    { NULL }
> +};

You may want to document the options as well. (Yes, I am aware that
they are a bit self-documenting, but most users aren't aware.)

I realize "log_fmt" and "pool" are passed directly to libvmaf. I hope
that library reports errors in those correctly. Otherwise, enums would
be preferable (but would restrict ffmpeg to those it knows from
libvmaf).

You could also use that big load of whitespace before OFFSET() to
align the columns a bit. But that's cosmetic.

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

This and its 10 bit variant are so very similar, they should be
macro-fied.

Moritz