On Fri, Dec 24, 2021 at 9:52 PM Kyle Swanson <k...@ylo.ph> wrote:
> 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.
>
>
Please read old reviews and follow them.
There is no exceptions.
> Thanks,
> Kyle
>
> On Wed, Jun 30, 2021 at 6:55 AM Moritz Barsnick <barsn...@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-requ...@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
