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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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