Hi,

Never followed through on this vf_libvmaf patch from last June, and
I've had several people asking about its status lately. Rebased patch
attached. It's been a while, so I guess let's start the review again.
Would be nice if we could get this in before 5.0.

Thanks,
Kyle

On Wed, Jun 30, 2021 at 6:55 AM Moritz Barsnick <barsnick@gmx.net> wrote:
>
> 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".