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

Fix build with FFmpeg 7, C++17, and external libresample and kissfft #53

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

emilazy
Copy link

@emilazy emilazy commented Aug 3, 2024

I started just doing a quick FFmpeg 7 patch for Nixpkgs but I might have gone overboard.

I removed support for ancient versions of FFmpeg as it didn’t feel worth it to maintain backwards compatibility.

This does also make an external libresample and kissfft implementation with .pc files a hard requirement; I can change it to fall back to the vendored source if you’d prefer.

@emilazy emilazy force-pushed the push-tzzznzslwvyu branch from 2c7fb9d to 754d5df Compare August 3, 2024 15:49
@emilazy
Copy link
Author

emilazy commented Aug 3, 2024

Note that the Travis build will probably fail because, uh… well, for one, is Travis CI on GitHub even still a thing? But also Debian’s libresample package is a bit broken currently; cf. minorninth/libresample#8.

Use GNUInstallDirs, CTest, and FindPkgConfig.
@emilazy emilazy force-pushed the push-tzzznzslwvyu branch from 754d5df to 86ed2b8 Compare August 3, 2024 16:12
@emilazy emilazy force-pushed the push-tzzznzslwvyu branch from 86ed2b8 to 62f197a Compare August 21, 2024 18:42
@f0k
Copy link
Collaborator

f0k commented Nov 28, 2024

Hey @emilazy, sorry for the late reaction! I finally looked at the commits. Thank you for the work, let's see if we can handle all of it in a single PR! My comments:

  • ab430ea: While we don't necessarily need to support ancient ffmpeg versions, I wouldn't want to depend on ffmpeg 7 alone. What versions of ffmpeg does your patch support? For comparison, the current version of torchaudio (2.5.0) supports ffmpeg 4.4+, 5, and 6. Ubuntu 22.04 LTS ships ffmpeg 4.4.2, and Ubuntu 20.04 LTS is even at 3.4.2 (full list).
  • 75efe27: Looks good, but can you quickly comment whether this breaks compatibility with something older?
  • 8abe638: Also looks good. Does this use CTest just to avoid manually defining the BUILD_TEST/BUILD_TESTING option or is there some side benefit? Requiring CMake 3.17 means it will work on Ubuntu 22.04 LTS, but not on Ubuntu 20.04 LTS (that ships CMake 3.10.2). Do you remember what feature of 3.17 you use? Sorry for always comparing against Ubuntu, it's just a very popular distro.
  • e4a0d0b / 62f197a: Makes sense! But are both libraries widely available across distros and operating systems, or will this be a pain on Windows, or for static builds? Otherwise it might be useful to keep them in as a fallback...

is Travis CI on GitHub even still a thing?

It would definitely make sense to switch to GitHub Actions!

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