Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to set the value of libvmaf's n_threads option, deprecate .pkl model files #27

Merged
merged 4 commits into from
Feb 9, 2021
Merged

Conversation

CrypticSignal
Copy link
Contributor

@CrypticSignal CrypticSignal commented Feb 8, 2021

The main changes are as follows:

  • I have added the option of setting the value of libvmaf's n_threads option.

    • Since libvmaf v2.0.0, only 1 thread is used for VMAF calculation by default. See this. This means that, by default, VMAF calculation is not as fast as it can be for those with multi-core CPUs.

      Therefore, I have set the default value of n_threads to what Python's os.cpu_count() method returns. But users can set a different value by using the -nt/--n-threads argument.

  • I have edited the README to replace vmaf_v0.6.1.pkl with vmaf_v0.6.1.json as .pkl model files have been deprecated since libvmaf v2.0.0. I have also added links to FFmpeg binaries (for Linux, macOS and Windows) that use libvmaf >v2.0.0.
    These changes to the README fulfil the following goals in Issue Deprecate VMAF 1.x #19:

    • Update builds to include libvmaf 2.0 (depends on Homebrew and ffmpeg inclusion)

    • Deprecate usage of libvmaf < 2.0

@CrypticSignal CrypticSignal changed the title Add option to set the value of libvmaf's n_threads option Add option to set the value of libvmaf's n_threads option, deprecate .pkl model files Feb 8, 2021
@slhck
Copy link
Owner

slhck commented Feb 8, 2021

Thanks for providing the PR – that's much appreciated! I'll review it tomorrow.

@slhck
Copy link
Owner

slhck commented Feb 9, 2021

I only have a few minor changes to add, otherwise it's good to merge.

But: It does not work with ffmpeg installed via Homebrew, since that bundles libvmaf < 2.0, and so it won't work with the .pkl files that are shipped with Homebrew's libvmaf.

If I don't specify a model path at all, it'll try to load a model from /usr/local/share/model/vmaf_v0.6.1.pkl which does not exist or cannot be parsed anyway.

So, where does the average user who downloads the static builds, get the JSON model files?

Also, the ffmpeg documentation still seems outdated to me, as it mentions .pkl model files: http://ffmpeg.org/ffmpeg-filters.html#libvmaf

@slhck slhck merged commit 4218ad9 into slhck:master Feb 9, 2021
@CrypticSignal
Copy link
Contributor Author

CrypticSignal commented Feb 9, 2021

So, where does the average user who downloads the static builds, get the JSON model files?

I was thinking about submitting an additional PR later, with the model files added in a folder named vmaf_models. Their filesizes are in the kilobytes, so there's little effect on the time taken for users to clone your repository.

If I don't specify a model path at all, it'll try to load a model from /usr/local/share/model/vmaf_v0.6.1.pkl which does not exist or cannot be parsed anyway.

If the above is done, we can set the default path of the model file to be vmaf_models/vmaf_v0.6.1.json.

But: It does not work with ffmpeg installed via Homebrew, since that bundles libvmaf < 2.0, and so it won't work with the .pkl files that are shipped with Homebrew's libvmaf.

I'd mention in the README that the linked binaries should be used, as FFmpeg installed via Homebrew only supports libvmaf <v2.0.0. support for which has been deprecated (I would deprecate support for FFmpeg installed via Homebrew, for the reasons below):

  • Users can easily get FFmpeg with libvmaf >v2 support, as I have provided binaries, so no building/compiling is necessary for those who are new to this sort of stuff.
  • This will simplify the codebase, as the brew-related functions are no longer needed.

Support for Homebrew ffmpeg can be re-added if/when Homebrew ffmpeg gets libvmaf v2+ support.

Alternatively, we can keep Homebrew ffmpeg, but if Homebrew ffmpeg is detected, show a message saying that .pkl/.pkl.model files need to be used and the path to the .pkl model must be supplied using the -m argument.

If you're happy with all/some of the above, let me know and I can submit a PR, or multiple PRs:

  • Adding a folder named vmaf_models, which contains vmaf_v0.6.1.json, vmaf_4k_v0.6.1.json and perhaps vmaf_v0.6.1neg.json too (I'm not sure what the use case of the neg models are, I'm far from a VMAF expert).
  • Editing the README to mention that FFmpeg installed via Homebrew is not supported, and that you should use the linked binaries or build vmaf/ffmpeg yourself.
  • Make the necessary changes in __main__.py - setting the default model path, removing the brew-related functions (remove win_path_check too? It's not needed if a model included with this repository is used), etc.

Also, the ffmpeg documentation still seems outdated to me, as it mentions .pkl model files:

Yes, it is. The description for n_threads is incorrect too ("Set number of threads to be used when computing vmaf. Default value: 0, which makes use of all available logical processors."). The default since v2.0.0 is 1, which only uses 1 thread.

@slhck
Copy link
Owner

slhck commented Feb 9, 2021

Those are all very good suggestions, thanks!

I like Homebrew ffmpeg, since it integrates well with my workflow, and it supports nonfree libraries, but I realize it would be way easier to just go with the static builds.

Feel free to propose a PR with those changes. I appreciate your input.

We should turn also fix the Dockerfile. I can have a look at that after that set of changes.

@CrypticSignal
Copy link
Contributor Author

CrypticSignal commented Feb 9, 2021

I made an edit to my last comment:

Alternatively, we can keep Homebrew ffmpeg, but if Homebrew ffmpeg is detected, show a message saying that .pkl/.pkl.model files need to be used and the path to the .pkl model must be supplied using the -m argument.

Would you prefer this option? The message can be added under if has_brew() and ffmpeg_is_from_brew():.

Edit: After seeing get_brewed_model_path():, it seems like the user will not need to manually set the model path. In that case, let's keep Homebrew ffmpeg support?

Also, do you want the vmaf_models folder in the root of this repo or inside the ffmpeg_quality_metrics folder?

@slhck
Copy link
Owner

slhck commented Feb 9, 2021

Yes, in that sense, it's already backwards compatible with current Homebrew and newer static builds.

When bundling the model files, we need to consider that they must be shipped with a pip installation, and if the tool is installed with pip, that it can find the model files wherever it's installed. The easiest would be to place that folder inside the folder with the other Python files. I think it also needs some changes in setup.py to include them in the installation (have to verify that one, I'm just on a phone at the moment).

@slhck
Copy link
Owner

slhck commented Feb 9, 2021

See am example of such package data here: https://github.com/itu-p1203/itu-p1203/blob/master/setup.py#L40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants