-
Notifications
You must be signed in to change notification settings - Fork 106
Add TORCHCODEC_FFMPEG_DIR to specify FFmpeg dynamic libs search path. #1161
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
50efd51
85a34e5
61fe977
a81434e
628cfdf
1e520b7
1632ff0
cce75db
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 |
|---|---|---|
|
|
@@ -5,6 +5,8 @@ | |
| # LICENSE file in the root directory of this source tree. | ||
|
|
||
|
|
||
| import ctypes | ||
| import glob | ||
| import io | ||
| import json | ||
| import os | ||
|
|
@@ -22,13 +24,30 @@ | |
|
|
||
|
|
||
| expose_ffmpeg_dlls = nullcontext | ||
| if sys.platform == "win32" and hasattr(os, "add_dll_directory"): | ||
| if ffmpeg_dir := os.getenv("TORCHCODEC_FFMPEG_DIR"): | ||
| if hasattr(os, "add_dll_directory"): | ||
|
|
||
| def expose_ffmpeg_dlls(): # type: ignore[no-redef] # noqa: F811 | ||
| return os.add_dll_directory(str(ffmpeg_dir)) | ||
|
|
||
| else: | ||
| ffmpeg_library_glob = "*.dylib" if sys.platform == "darwin" else "*.so*" | ||
| ffmpeg_library_paths = glob.glob(os.path.join(ffmpeg_dir, ffmpeg_library_glob)) | ||
| if not ffmpeg_library_paths: | ||
| raise RuntimeError( | ||
| "TORCHCODEC_FFMPEG_DIR is set, but no FFmpeg shared libraries " | ||
| f"were found in {repr(ffmpeg_dir)}." | ||
| ) | ||
| for ffmpeg_library_path in ffmpeg_library_paths: | ||
| ctypes.CDLL(ffmpeg_library_path) | ||
|
Comment on lines
+41
to
+42
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. This is not ideal: this is loading every single .so file that is present in the If, for example, the user specifies We shouldn't load the .so files ourselves, we should continue leaving it up to Something along these lines: custom_path = os.environ.get('TORCHCODEC_FFMPEG_LIBRARIES')
if custom_path:
# Prepend so it takes priority, just like LD_LIBRARY_PATH
ld_path = os.environ.get('LD_LIBRARY_PATH', '')
os.environ['LD_LIBRARY_PATH'] = f"{custom_path}:{ld_path}" if ld_path else custom_path
# now call load_torchcodec_shared_libraries() as usual
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. That seems reasonable on Linux. However, as far as I know, on macOS,
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. Thanks for pointing this out @bcw222! Unfortunately this behavior on Linux and MacOS means we need to know the names of all the required .so files to avoid loading many unnecessary .so files (as @NicolasHug points out), and we load the .so files before These introduce complexity and potential for unexpected side effects. Based on our discussion, what we actually want is for So reconsidering this approach, we should be able to have FFmpeg libs be correctly detected by setting We can update this PR to document this in # Conda on Linux
LD_LIBRARY_PATH="$CONDA_PREFIX/lib:$LD_LIBRARY_PATH"
# Conda on macOS
DYLD_LIBRARY_PATH="$CONDA_PREFIX/lib:$DYLD_LIBRARY_PATH"# Conda on Windows Powershell
$env:PATH = "$env:CONDA_PREFIX\Library\bin;" + "$env:PATH"
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. Wait, are you sure? On Linux it should be
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. My apologies, those examples were very incorrect as I sent my message while drafting, I'll fix my previous comment. My understanding is that the
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.
@bcw222 our understanding is that on Windows, the
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. Indeed, but globally modifying
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. Given that the OS differences require us to:
Lets stick to the standard environmental variables, and add a section documenting how to use them! It can be similar to my edited comment above and list the various
Depending on your set up, could you call
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. You're right. So we have two solutions: with the original logic untouched, on Windows we could either modify |
||
|
|
||
| elif sys.platform == "win32" and hasattr(os, "add_dll_directory"): | ||
| # On windows we try to locate the FFmpeg DLLs and temporarily add them to | ||
| # the DLL search path. This seems to be needed on some users machine, but | ||
| # not on our CI. We don't know why. | ||
| if ffmpeg_path := shutil.which("ffmpeg"): | ||
|
|
||
| def expose_ffmpeg_dlls(): # noqa: F811 | ||
| def expose_ffmpeg_dlls(): # type: ignore[no-redef] # noqa: F811 | ||
| ffmpeg_dir = Path(ffmpeg_path).parent.absolute() | ||
| return os.add_dll_directory(str(ffmpeg_dir)) # that's the actual CM | ||
|
|
||
|
|
||
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.
reprseems overkill