-
Notifications
You must be signed in to change notification settings - Fork 84
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?
Conversation
|
Hi @mscheltienne! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
| 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() |
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_PREFIX is only going to work for users building from source. For users installing the built wheels with e.g. pip install, the HOMEBREW_PREFIX will be hard-coded to opt/homebrew/opt/ffmpeg/lib which 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_PREFIX path specifically for this case where a user would use homebrew in a non-default location, in which case he will be able to build from source.
| endif() | ||
| set_target_properties(${library_name} PROPERTIES | ||
| INSTALL_RPATH "${HOMEBREW_FFMPEG_LIB}" | ||
| ) |
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.
On this:
It should not impact in any form the current resolution of the conda-installed libraries
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.
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.
I think you are right. From what I could figure out, it seems that on main, libtorchcodec_core*.dylib doesn't have any LC_RPATH entries and thus the resolution relies on torch's rpath entries (which include conda paths and thus torchcodec finds the conda FFmpeg.
With this change, libtorchcodec_core*.dylib now has one LC_RPATH which gets added to the rpath list and likely searched first. Thus, we have the following scenarios:
- Only
condaFFmpeg: no change in behavior, homebrew path is searched first but no lib found, falls back tocondapaths through the same path as before. - Only
homebrewFFmpeg, fails onmain, works on this PR. - Both installed, on
mainit would use thecondaFFmpeg, on this PR it would likely resolve thehomebrewFFmpeg first.
In this last scenario, torchcodec should still work and thus the change should be transparent to the user- the only way I can think of to retain control on which path to search first would be to add the conda path as well, but this is now a more invasive change, something along the lines of:
set_target_properties(${library_name} PROPERTIES
INSTALL_RPATH "@loader_path/../../..;${HOMEBREW_FFMPEG_LIB}"
)
Closes #570
This PR adds the homebrew FFmpeg path to
rpathat build. It should not impact in any form the current resolution of the conda-installed libraries, but will help users of plainpiporuvwith homebrew. I tested locally with the following:Resulting install shows:
And import works:
And locally, default tests passes with: