Use pkg-config to discover the location of the ffmpeg libs.
Needs RevisionPublic
Actions

Authored by Wren Turkal (wt) on Sep 30 2022, 11:06 AM.

Details

Reviewers

Ray Molenkamp (LazyDodo)
Brecht Van Lommel (brecht)
Bastien Montagne (mont29)
Campbell Barton (campbellbarton)

Summary

This will make it easier to build blender from source when
using system libs for building.

Diff Detail

Repository

rB Blender

Branch

use_pkgconfig_for_ffmpeg_libs (branched from master)

Build Status

Buildable 24041
Build 24041: arc lint + arc unit

Event Timeline

Wren Turkal (wt) requested review of this revision.Sep 30 2022, 11:06 AM

Wren Turkal (wt) created this revision.

Harbormaster completed remote builds in B24041: Diff 56321.Sep 30 2022, 11:06 AM

Is there anyone that can see if the pkg-config stuff works on Windows?

Move variable into non-pkg-config part of the discovery.

Harbormaster completed remote builds in B24059: Diff 56357.Oct 1 2022, 9:01 AM

In D16113#438801, @Wren Turkal (wt) wrote:

Is there anyone that can see if the pkg-config stuff works on Windows?

It doesn't, pkg-config is not a thing on windows with msvc (although a mingw/msys based build will have it), our windows support currently has it's own handling and doesn't rely on FindFFmpeg.cmake to find libs, also system libs aren't a thing on windows so that simplifies things a lot... what i'm saying is: don't worry about windows

Ray Molenkamp (LazyDodo) requested changes to this revision.Oct 1 2022, 4:51 PM

Ray Molenkamp (LazyDodo) added reviewers: Ray Molenkamp (LazyDodo), Brecht Van Lommel (brecht), Sybren A. Stüvel (sybren), Bastien Montagne (mont29).

Ray Molenkamp (LazyDodo) added inline comments.

CMakeLists.txt
27	We could discuss bumping our minimum required cmake version as a project, this however should not be part of a diff that just aims to make some ffmpeg tweaks

Can you describe which problem prompted you to make this change? This has worked for system libraries in the past, but maybe something changed or there's an issue with a specific setup?

build_files/cmake/Modules/FindFFmpeg.cmake
40	This seems like it will prefer pkgconfig over other detection, which may break building with precompiled libraries where we want it to be the other way around.

Brecht Van Lommel (brecht) added inline comments.Oct 1 2022, 5:28 PM

build_files/cmake/Modules/FindFFmpeg.cmake
40	`FFMPEG_ROOT_DIR` is also ignored here I guess, so even when using system libraries that may make it impossible to manually pick the ffmpeg to use.

Wren Turkal (wt) added inline comments.Oct 5 2022, 8:09 AM

CMakeLists.txt
27	A couple things just so we have them here: Cmake 3.12 was released in 2018 Audaspace already uses the cmake pkg-config support, so we might already need this or this change may not. And a couple questions: What is the right way to get this changed? Do you think we can just leave this out of this change?
build_files/cmake/Modules/FindFFmpeg.cmake
40	I'll change this around.

Don't ignore manually specified ffmpeg dir.

Harbormaster completed remote builds in B24116: Diff 56467.Oct 5 2022, 8:25 AM

In D16113#439046, @Brecht Van Lommel (brecht) wrote:

Can you describe which problem prompted you to make this change? This has worked for system libraries in the past, but maybe something changed or there's an issue with a specific setup?

Sure. I am trying to build Blender on Fedora Rawhide. The defaults location for ffmpeg libs on this system is /usr/lib64/ffmpeg/... However that directory isn't searched by the setup before the change. I can, of course manually set the FFMPEG_ROOT_DIR variable, but I thought it would be better to use pkg-config since that helps discover everything this module attempts to discover. Honestly, I wonder if at some point it would make sense to remove the non-pkg-config discovery that I left in for backward compat when pkg-config isn't available. I would be surprised if any mainstream linux distro doesn't ship the pkg-config support for ffmpeg.

In D16113#439042, @Ray Molenkamp (LazyDodo) wrote:

In D16113#438801, @Wren Turkal (wt) wrote:

Is there anyone that can see if the pkg-config stuff works on Windows?

It doesn't, pkg-config is not a thing on windows with msvc (although a mingw/msys based build will have it), our windows support currently has it's own handling and doesn't rely on FindFFmpeg.cmake to find libs, also system libs aren't a thing on windows so that simplifies things a lot... what i'm saying is: don't worry about windows

Ack. Thx.

Wren Turkal (wt) marked 2 inline comments as done.Oct 5 2022, 8:43 AM

Wren Turkal (wt) added inline comments.

build_files/cmake/Modules/FindFFmpeg.cmake
40	This seems like it will prefer pkgconfig over other detection, which may break building with precompiled libraries where we want it to be the other way around. With the way I changed it, you would define FFMPEG_ROOT_DIR to use some other lib? Previously, I guess it would also try to find ffmpeg in /opt/lib/ffmpeg. After this change, that directory is not searched before the pkg-config discovery. This seems correct to me. I will grant that is can make a build that previously used a lib in /opt/lib/ffmpeg break if the FFMPEG_ROOT_DIR is not specified. That seems like it would be pretty rare. Is that a reasonable assumption? FWIW, any ffmpeg that was detected in dirs like /lib or /usr/lib (defaults already searched by find_path) before would continue to work either by the pkg-config discovery pointing there, or, if pkg-config support for ffmeg isn't there, would fall back to the old discovery mechanisms.

Ray Molenkamp (LazyDodo) added inline comments.Oct 5 2022, 3:47 PM

CMakeLists.txt
27	Cmake 3.12 was released in 2018 It may be, however my argument was you can't just bump the minimum cmake version for the whole project in a change that does some localized ffmpeg tweaks. These are 2 distinctly different changes and need to be done in separate commits. Audaspace already uses the cmake pkg-config support, so we might already need this or this change may not. Do you think we can just leave this out of this change? Cmake shipped with a version of the FindPkgConfig module as early as 3.0.2, what behaviour is the 3.12 module providing that 3.10 isn't that this change depends on? What is the right way to get this changed? Usually the project admins do a survey every couple of years to see what cmake versions disto's ship out of the box and what the oldest version shipped is (cough usually centos cough) and base our minimum version on that. With our centos7 support retiring in 2023 I'm expecting they'll have another look somewhere early 2023

Brecht Van Lommel (brecht) added inline comments.Oct 5 2022, 6:29 PM

build_files/cmake/Modules/FindFFmpeg.cmake
40	I will grant that is can make a build that previously used a lib in /opt/lib/ffmpeg break if the FFMPEG_ROOT_DIR is not specified. That seems like it would be pretty rare. Is that a reasonable assumption? This seems fine with the current code. For precompiled libraries we explicitly specify `FFMPEG_ROOT_DIR` so those should keep working.

Wren Turkal (wt) added inline comments.Oct 8 2022, 10:10 AM

build_files/cmake/Modules/FindFFmpeg.cmake
40	I believe this is working now. If FFMPEG_ROOT_DIR is set, the pkg-config logic will not be used. Are you satisfied with this logic?

Brecht Van Lommel (brecht) requested changes to this revision.Oct 12 2022, 5:57 PM

Brecht Van Lommel (brecht) edited reviewers, added: Campbell Barton (campbellbarton); removed: Sybren A. Stüvel (sybren).

Brecht Van Lommel (brecht) added inline comments.

CMakeLists.txt
27	To be clear, this change should be left out of the patch, assuming it still works with 3.10.
build_files/cmake/Modules/FindFFmpeg.cmake
40	It looks good to me.