-
Notifications
You must be signed in to change notification settings - Fork 86
Fix Homebrew FFmpeg discovery on macOS #1152
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,6 +53,21 @@ function(make_torchcodec_sublibrary | |
| # Avoid adding the "lib" prefix which we already add explicitly. | ||
| set_target_properties(${library_name} PROPERTIES PREFIX "") | ||
|
|
||
| # On macOS, add Homebrew's FFmpeg library path to the rpath so that users | ||
| # with Homebrew-installed FFmpeg can use torchcodec without setting | ||
| # DYLD_LIBRARY_PATH. See https://github.com/pytorch/torchcodec/issues/570 | ||
| if(APPLE) | ||
| if(DEFINED ENV{HOMEBREW_PREFIX}) | ||
| set(HOMEBREW_FFMPEG_LIB "$ENV{HOMEBREW_PREFIX}/opt/ffmpeg/lib") | ||
| else() | ||
| # Default Homebrew location on Apple Silicon | ||
| set(HOMEBREW_FFMPEG_LIB "/opt/homebrew/opt/ffmpeg/lib") | ||
| endif() | ||
| set_target_properties(${library_name} PROPERTIES | ||
| INSTALL_RPATH "${HOMEBREW_FFMPEG_LIB}" | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On this:
I think the change will impact the resolution of conda-installed ffmpeg, because (IIUC) the rpath will now take precedence for the path resolution order. So, if FFmpeg is installed in both the conda env and with homebrew, it's the homebrew FFmpeg that is going to be resolved first.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you are right. From what I could figure out, it seems that on With this change,
In this last scenario, |
||
| endif() | ||
|
|
||
| target_link_libraries( | ||
| ${library_name} | ||
| PUBLIC | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting that the handling of a custom
HOMEBREW_PREFIXis only going to work for users building from source. For users installing the built wheels with e.g.pip install, theHOMEBREW_PREFIXwill be hard-coded toopt/homebrew/opt/ffmpeg/libwhich may or may not be the current location of the user's homebrew.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, can you think of a better alternative? I included the
HOMEBREW_PREFIXpath specifically for this case where a user would usehomebrewin a non-default location, in which case he will be able to build from source.