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

ffmpeg: enable libvmaf #88959

Closed
wants to merge 1 commit into from
Closed

Conversation

barrbrain
Copy link
Contributor

@barrbrain barrbrain commented Nov 7, 2021

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

The VMAF models are available since #88931 was merged, so we may enable libvmaf and replace the default model path. This is motivated by tools like Av1an which use the VMAF filter to evaluate and target video quality when encoding. Note that this does not expand the dependency tree of the ffmpeg formula as libvmaf is already a transitive dependency via aom.

@carlocab
Copy link
Member

carlocab commented Nov 8, 2021

I'm going to cherry pick your commit onto #88821 so that we only have to rev bump ffmpeg once.

carlocab pushed a commit to DevSysEngineer/homebrew-core that referenced this pull request Nov 8, 2021
Comment on lines +127 to +128
# Since libvmaf v2.0.0, `.pkl` model files have been deprecated in favor of `.json` model files.
inreplace f, "vmaf_v0.6.1.pkl", "vmaf_v0.6.1.json"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be fixed on FFmpeg master. Has this been reported upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will consult with the ffmpeg developers on whether they intend to make this configurable.
My understanding is that it is not practical to autodetect the model path, so this is left to packagers to resolve.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The related issues have been reported upstream but there appears not to be any related patches in the past year.
https://trac.ffmpeg.org/ticket/9094
https://trac.ffmpeg.org/ticket/9047
Netflix/vmaf#753
Netflix/vmaf#324

Other packagers have opted to replace the path at build time. I asked in #ffmpeg-devel and and they agreed that replacing the patch in the Homebrew formula is a reasonable solution for the time being.

@carlocab carlocab added the automerge-skip `brew pr-automerge` will skip this pull request label Nov 8, 2021
@carlocab
Copy link
Member

carlocab commented Nov 8, 2021

Merged in b8e351f. Thanks @barrbrain!

@github-actions github-actions bot added the outdated PR was locked due to age label Dec 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants