[FFmpeg-devel] [PATCH] avfilter/vf_libvmaf: update filter for libvmaf v2.0.0

[Moritz Barsnick]

Hi,

> -enabled libvmaf           && require_pkg_config libvmaf "libvmaf >= 1.5.2" libvmaf.h compute_vmaf
> +enabled libvmaf           && require_pkg_config libvmaf "libvmaf >= 2.0.0" libvmaf.h vmaf_init

General question: Is it acceptable to drop support for libvmaf 1.x? I
saw that Fedora 33 is still on 1.x.

> -Obtain the VMAF (Video Multi-Method Assessment Fusion)
> -score between two input videos.
> +Calulate the VMAF (Video Multi-Method Assessment Fusion) score for a
> +reference/distorted pair of input videos.

These documentation improvements aren't related to the actual switch to
2.x, and should be in a separate commit.

>  @code{./configure --enable-libvmaf}.
> -If no model path is specified it uses the default model: @code{vmaf_v0.6.1.pkl}.

Unless they are consequences of the switch, of course.

> -    {"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 (csv, json or xml).",                       OFFSET(log_fmt), AV_OPT_TYPE_STRING, {.str=NULL}, 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},
> -    {"n_threads", "Set number of threads to be used when computing vmaf.",              OFFSET(n_threads), AV_OPT_TYPE_INT, {.i64=0}, 0, UINT_MAX, FLAGS},
> -    {"n_subsample", "Set interval for frame subsampling used when computing vmaf.",     OFFSET(n_subsample), AV_OPT_TYPE_INT, {.i64=1}, 1, UINT_MAX, FLAGS},
> -    {"enable_conf_interval",  "Enables confidence interval.",                           OFFSET(enable_conf_interval), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
> +    {"model", "Set the model to be used for computing vmaf.",                             OFFSET(model_cfg), AV_OPT_TYPE_STRING, {.str="version=vmaf_v0.6.1"}, 0, 1, FLAGS},
> +    {"feature", "Set the feature to be used for computing vmaf.",                         OFFSET(feature_cfg), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 1, FLAGS},
> +    {"log_path",  "Set the file path to be used to write log.",                           OFFSET(log_path), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 1, FLAGS},
> +    {"log_fmt",  "Set the format of the log (csv, json, xml, or sub).",                   OFFSET(log_fmt), AV_OPT_TYPE_STRING, {.str="xml"}, 0, 1, FLAGS},
> +    {"n_threads", "Set number of threads to be used when computing vmaf.",                OFFSET(n_threads), AV_OPT_TYPE_INT, {.i64=0}, 0, UINT_MAX, FLAGS},
> +    {"n_subsample", "Set interval for frame subsampling used when computing vmaf.",       OFFSET(n_subsample), AV_OPT_TYPE_INT, {.i64=1}, 1, UINT_MAX, FLAGS},


Some changes are easier to review, if you don't change all the
whitespace initially.

> +    if (err) return AVERROR(ENOMEM);

Line break.

> +    int err = 0;
> +    if (!str) return NULL;

Line break.

> +    if (!dict) goto fail;

Ditto, in several subsequent places.

> +    if (str_copy)
> +        av_free(str_copy);

No need to check for str_copy.

> +        e = NULL;
> +        while (e = av_dict_get(dict[i], "", e, AV_DICT_IGNORE_SUFFIX)) {

I believe the assigned value from "e = NULL" is never used.

>          .name         = "main",
>          .type         = AVMEDIA_TYPE_VIDEO,
> -    },{
> +    },
> +    {
>          .name         = "reference",

Unrelated style change.

Since it looks like a complete re-write, it doesn't look like support
for both old and new API seems feasible, right? Just wondering, I am
not the one to judge.

(And I cannot judge on the actual functionality.)

Oh, and you should probably bump at least libavfilter MICRO version -
not sure whether even MINOR is justified.

Cheers,
Moritz