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

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

[Kyle Swanson]

[-- Attachment #1: Type: text/plain, Size: 5491 bytes --]

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".

[-- Attachment #2: 0001-avfilter-vf_libvmaf-update-filter-for-libvmaf-v2.0.0.patch --]
[-- Type: application/octet-stream, Size: 30621 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Timo Rothenpieler]

[-- Attachment #1.1: Type: text/plain, Size: 745 bytes --]

On 24.12.2021 21:51, Kyle Swanson 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.
> 
> 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

The primary question remains.
Is dropping <2.0 support okay, when there's distros out there not having 
2.0 yet?

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4494 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Kyle Swanson]

Hi,

On Fri, Dec 24, 2021 at 12:54 PM Timo Rothenpieler
<timo@rothenpieler.org> wrote:
> > 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
>
> The primary question remains.
> Is dropping <2.0 support okay, when there's distros out there not having
> 2.0 yet?

I just checked and libvmaf v2.0.0 has been out for a little bit over a
year now. I don't know much about the various distro package
maintainers and the state of things, but I think it's OK to go ahead.

Thanks,
Kyle
_______________________________________________
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".

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

[Paul B Mahol]

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 <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".
> _______________________________________________
> 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".
>
_______________________________________________
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".

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

[Kyle Swanson]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]

Hi,

On Sat, Dec 25, 2021 at 1:24 AM Paul B Mahol <onemda@gmail.com> wrote:
>
> 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.

Thank you, I've taken all of Mortiz's comments. Updated patch attached.

Thanks,
Kyle

[-- Attachment #2: 0001-avfilter-vf_libvmaf-update-filter-for-libvmaf-v2.0.0.patch --]
[-- Type: application/octet-stream, Size: 30818 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Kyle Swanson]

Hi,

On Thu, Dec 30, 2021 at 5:03 PM Kyle Swanson <k@ylo.ph> wrote:
>
> Hi,
>
> On Sat, Dec 25, 2021 at 1:24 AM Paul B Mahol <onemda@gmail.com> wrote:
> >
> > 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.
>
> Thank you, I've taken all of Mortiz's comments. Updated patch attached.
>
> Thanks,
> Kyle

Unless there are any more reviews or questions, I will push soon.

Thanks,
Kyle
_______________________________________________
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".

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

[Andreas Rheinhardt]

Kyle Swanson:
> Hi,
> 
> On Sat, Dec 25, 2021 at 1:24 AM Paul B Mahol <onemda@gmail.com> wrote:
>>
>> 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.
> 
> Thank you, I've taken all of Mortiz's comments. Updated patch attached.
> 
> Thanks,
> Kyle
> 

> 
>  static const AVOption libvmaf_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 (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},
> +    {"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},
> -    {"enable_conf_interval",  "Enables confidence interval.",                           OFFSET(enable_conf_interval), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
>      { NULL }
>  };

You are removing lots of options; removing options is only permissible
during a major break and even then the options need to have been
deprecated before that.

- Andreas
_______________________________________________
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".

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

[Kyle Swanson]

[-- Attachment #1: Type: text/plain, Size: 3108 bytes --]

Hi,

On Sun, Jan 2, 2022 at 9:21 PM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
> >
> >  static const AVOption libvmaf_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 (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},
> > +    {"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},
> > -    {"enable_conf_interval",  "Enables confidence interval.",                           OFFSET(enable_conf_interval), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
> >      { NULL }
> >  };
>
> You are removing lots of options; removing options is only permissible
> during a major break and even then the options need to have been
> deprecated before that.

Good point, thanks. New patch attached. No more missing options,
everything that should be deprecated has been marked as deprecated and
appropriate fallback behavior implemented.

Thanks,
Kyle

[-- Attachment #2: 0001-avfilter-vf_libvmaf-update-filter-for-libvmaf-v2.0.0.patch --]
[-- Type: application/octet-stream, Size: 35214 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Kyle Swanson]

Hi,

On Mon, Jan 3, 2022 at 2:32 PM Kyle Swanson <k@ylo.ph> wrote:
>
> Hi,
>
> On Sun, Jan 2, 2022 at 9:21 PM Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
> > >
> > >  static const AVOption libvmaf_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 (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},
> > > +    {"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},
> > > -    {"enable_conf_interval",  "Enables confidence interval.",                           OFFSET(enable_conf_interval), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
> > >      { NULL }
> > >  };
> >
> > You are removing lots of options; removing options is only permissible
> > during a major break and even then the options need to have been
> > deprecated before that.
>
> Good point, thanks. New patch attached. No more missing options,
> everything that should be deprecated has been marked as deprecated and
> appropriate fallback behavior implemented.
>
> Thanks,
> Kyle

I think this patch should be ready to go. Any other comments let me
know, otherwise I will plan on merging in ~48 hrs.

Thanks,
Kyle
_______________________________________________
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".

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

[Andreas Rheinhardt]

Kyle Swanson:
> Hi,
> 
> On Sun, Jan 2, 2022 at 9:21 PM Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
>>>
>>>  static const AVOption libvmaf_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 (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},
>>> +    {"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},
>>> -    {"enable_conf_interval",  "Enables confidence interval.",                           OFFSET(enable_conf_interval), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
>>>      { NULL }
>>>  };
>>
>> You are removing lots of options; removing options is only permissible
>> during a major break and even then the options need to have been
>> deprecated before that.
> 
> Good point, thanks. New patch attached. No more missing options,
> everything that should be deprecated has been marked as deprecated and
> appropriate fallback behavior implemented.
> 

> +static AVDictionary **delimited_dict_parse(char *str, unsigned *cnt)
>  {
> -    LIBVMAFContext *s = (LIBVMAFContext *) ctx;
> -    compute_vmaf_score(s);
> -    if (!s->error) {
> -        av_log(ctx, AV_LOG_INFO, "VMAF score: %f\n",s->vmaf_score);
> -    } else {
> -        pthread_mutex_lock(&s->lock);
> -        pthread_cond_signal(&s->cond);
> -        pthread_mutex_unlock(&s->lock);
> +    int err = 0;
> +    if (!str)
> +        return NULL;
> +
> +    *cnt = 1;
> +    for (char *p = str; *p; p++) {
> +        if (*p == '|')
> +            (*cnt)++;
>      }
> -    pthread_exit(NULL);
> +
> +    AVDictionary **dict = av_calloc(*cnt, sizeof(*dict));
> +    if (!dict)
> +        goto fail;
> +
> +    char *str_copy = av_strdup(str);
> +    if (!str_copy)
> +        goto fail;
> +
> +    char *saveptr = NULL;
> +    for (unsigned i = 0; i < *cnt; i++) {
> +        char *s = av_strtok(i == 0 ? str_copy : NULL, "|", &saveptr);
> +        err = av_dict_parse_string(&dict[i], s, "=", ":", 0);
> +        if (err)
> +            goto fail;
> +    }
> +
> +    av_free(str_copy);
> +    return dict;
> +
> +fail:
> +    if (dict) {
> +        for (unsigned i = 0; i < *cnt; i++) {
> +            if (dict[i])
> +                av_dict_free(&dict[i]);
> +        }
> +        av_free(dict);
> +    }
> +
> +    av_free(str_copy);
> +    *cnt = 0;
>      return NULL;
>  }

1. FFmpeg uses the ancient C90 rule that only allows variable
declarations at the beginning of a block (before all statements). You
should actually have received a compiler warning because of the above
code (unless you use Clang, which claims to support this type of
warning, yet doesn't).
2. You jump over the initializion of str_copy if the dict allocation
fails and consequently you free an uninitialized string.

- Andreas
_______________________________________________
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".

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

[Kyle Swanson]

[-- Attachment #1: Type: text/plain, Size: 744 bytes --]

Hi,

On Mon, Jan 10, 2022 at 10:22 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
> 1. FFmpeg uses the ancient C90 rule that only allows variable
> declarations at the beginning of a block (before all statements). You
> should actually have received a compiler warning because of the above
> code (unless you use Clang, which claims to support this type of
> warning, yet doesn't).
> 2. You jump over the initializion of str_copy if the dict allocation
> fails and consequently you free an uninitialized string.

New patch attached. Fixes all of the GCC -Wdeclaration-after-statement
warnings. You're right, I didn't notice because I was building with
Clang. Andreas, I will just wait for your LGTM before merging.

Thanks,
Kyle

[-- Attachment #2: 0001-avfilter-vf_libvmaf-update-filter-for-libvmaf-v2.0.0.patch --]
[-- Type: application/octet-stream, Size: 35240 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Kyle Swanson]

[-- Attachment #1: Type: text/plain, Size: 1010 bytes --]

Hi,

On Thu, Jan 13, 2022 at 6:38 PM Kyle Swanson <k@ylo.ph> wrote:
>
> Hi,
>
> On Mon, Jan 10, 2022 at 10:22 AM Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
> > 1. FFmpeg uses the ancient C90 rule that only allows variable
> > declarations at the beginning of a block (before all statements). You
> > should actually have received a compiler warning because of the above
> > code (unless you use Clang, which claims to support this type of
> > warning, yet doesn't).
> > 2. You jump over the initializion of str_copy if the dict allocation
> > fails and consequently you free an uninitialized string.
>
> New patch attached. Fixes all of the GCC -Wdeclaration-after-statement
> warnings. You're right, I didn't notice because I was building with
> Clang. Andreas, I will just wait for your LGTM before merging.
>
> Thanks,
> Kyle

Updated patch attached. Thank you to mkver for the mini review on IRC.
The `delimited_dict_parse` multi-delimiter corner case should be
sorted now.

Thanks,
Kyle

[-- Attachment #2: 0001-avfilter-vf_libvmaf-update-filter-for-libvmaf-v2.0.0.patch --]
[-- Type: application/octet-stream, Size: 35777 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Kyle Swanson]

Hi,

On Wed, Jan 19, 2022 at 10:23 AM Kyle Swanson <k@ylo.ph> wrote:
>
> Hi,
>
> On Thu, Jan 13, 2022 at 6:38 PM Kyle Swanson <k@ylo.ph> wrote:
> >
> > Hi,
> >
> > On Mon, Jan 10, 2022 at 10:22 AM Andreas Rheinhardt
> > <andreas.rheinhardt@outlook.com> wrote:
> > > 1. FFmpeg uses the ancient C90 rule that only allows variable
> > > declarations at the beginning of a block (before all statements). You
> > > should actually have received a compiler warning because of the above
> > > code (unless you use Clang, which claims to support this type of
> > > warning, yet doesn't).
> > > 2. You jump over the initializion of str_copy if the dict allocation
> > > fails and consequently you free an uninitialized string.
> >
> > New patch attached. Fixes all of the GCC -Wdeclaration-after-statement
> > warnings. You're right, I didn't notice because I was building with
> > Clang. Andreas, I will just wait for your LGTM before merging.
> >
> > Thanks,
> > Kyle
>
> Updated patch attached. Thank you to mkver for the mini review on IRC.
> The `delimited_dict_parse` multi-delimiter corner case should be
> sorted now.
>
> Thanks,
> Kyle

Will push this soon.

Thanks,
Kyle
_______________________________________________
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".

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

[Kyle Swanson]

Hi,

On Thu, Jan 20, 2022 at 1:06 PM Kyle Swanson <k@ylo.ph> wrote:
>
> Hi,
>
> On Wed, Jan 19, 2022 at 10:23 AM Kyle Swanson <k@ylo.ph> wrote:
> >
> > Hi,
> >
> > On Thu, Jan 13, 2022 at 6:38 PM Kyle Swanson <k@ylo.ph> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Jan 10, 2022 at 10:22 AM Andreas Rheinhardt
> > > <andreas.rheinhardt@outlook.com> wrote:
> > > > 1. FFmpeg uses the ancient C90 rule that only allows variable
> > > > declarations at the beginning of a block (before all statements). You
> > > > should actually have received a compiler warning because of the above
> > > > code (unless you use Clang, which claims to support this type of
> > > > warning, yet doesn't).
> > > > 2. You jump over the initializion of str_copy if the dict allocation
> > > > fails and consequently you free an uninitialized string.
> > >
> > > New patch attached. Fixes all of the GCC -Wdeclaration-after-statement
> > > warnings. You're right, I didn't notice because I was building with
> > > Clang. Andreas, I will just wait for your LGTM before merging.
> > >
> > > Thanks,
> > > Kyle
> >
> > Updated patch attached. Thank you to mkver for the mini review on IRC.
> > The `delimited_dict_parse` multi-delimiter corner case should be
> > sorted now.
> >
> > Thanks,
> > Kyle
>
> Will push this soon.
>
> Thanks,
> Kyle

Pushed.

Thanks,
Kyle
_______________________________________________
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".