Expose TorchCodecConfig.cmake #938
Conversation
meta-cla
bot
added
the
CLA Signed
There was a problem hiding this comment.
Thanks @dvrogozh , I left some questions and some comments below. It looks good overal, but I hope we can simplify and unify the logic with the one that already exists within our own cmake files.
On the #include path changes, I will have to pre-import your PR internall to verify that this isn't breaking our internal build (and to try to find internal workarounds if needed).
| except Exception: | ||
| pass | ||
|
|
||
| cmake_prefix_path = _osp.join(_osp.dirname(__file__), "share", "cmake") |
There was a problem hiding this comment.
Let's add a comment to explain what this is used for, and also let's use the more modern pathlib tools instead of os.path.
| variant = ffmpeg_major_version | ||
| core_library_path = decoder_library_path |
There was a problem hiding this comment.
Let's return them instead of having them as global.
Also, ffmpeg_major_version is a good name, so we can keep using it for the variable that we expose as torchcodec.ffmpeg_major_version
| _pybind_ops: Optional[ModuleType] = None | ||
|
|
||
| variant = None | ||
| core_library_path = None |
There was a problem hiding this comment.
Can you help me understand why in
we only need to load the libtorchcodec_core?.so file, and not the other .so like libtorchcodec_pybind_ops?.so and libtorchcodec_custom_ops?.so
| set(f4_library_file_names | ||
| libavutil.so.56 | ||
| libavcodec.so.58 | ||
| libavformat.so.58 | ||
| libavdevice.so.58 | ||
| libavfilter.so.7 | ||
| libswscale.so.5 | ||
| libswresample.so.3 | ||
| ) | ||
| set(f5_library_file_names | ||
| libavutil.so.57 | ||
| libavcodec.so.59 | ||
| libavformat.so.59 | ||
| libavdevice.so.59 | ||
| libavfilter.so.8 | ||
| libswscale.so.6 | ||
| libswresample.so.4 | ||
| ) | ||
| set(f6_library_file_names | ||
| libavutil.so.58 | ||
| libavcodec.so.60 | ||
| libavformat.so.60 | ||
| libavdevice.so.60 | ||
| libavfilter.so.9 | ||
| libswscale.so.7 | ||
| libswresample.so.4 | ||
| ) | ||
| set(f7_library_file_names | ||
| libavutil.so.59 | ||
| libavcodec.so.61 | ||
| libavformat.so.61 | ||
| libavdevice.so.61 | ||
| libavfilter.so.10 | ||
| libswscale.so.8 | ||
| libswresample.so.5 | ||
| ) |
There was a problem hiding this comment.
Let's try to unify that with those that are already in https://github.com/meta-pytorch/torchcodec/blob/main/src/torchcodec/_core/fetch_and_expose_non_gpl_ffmpeg_libs.cmake. We can create a new ffmpeg_versions.cmake file if that helps refactoring.
| set(ffmpeg_major_version "6") | ||
| elseif (${libavcodec_major_version} STREQUAL "61") | ||
| set(ffmpeg_major_version "7") | ||
| endif() |
There was a problem hiding this comment.
Same here, the logic in this whole branch should ideally be unified with
torchcodec/src/torchcodec/_core/CMakeLists.txt
Lines 276 to 315 in 75a3325
| if (UNIX AND NOT APPLE) | ||
| set(libdir "${prefix}/lib") | ||
| else() | ||
| message("Skipping ${target} on non-Linux platform") | ||
| return() |
There was a problem hiding this comment.
Let's just assert this is UNIX AND NOT APPLE at the top of this file. It's OK to only support linux for now.
| set(target "torchcodec::ffmpeg${ffmpeg_major_version}") | ||
| set(prefix "TORCHCODEC_FFMPEG${ffmpeg_major_version}_INSTALL_PREFIX") | ||
| if (NOT DEFINED ENV{${prefix}}) | ||
| message("Skipping ${target} as ${prefix} is not defined") |
There was a problem hiding this comment.
This should be an error I think?
| foreach(lib IN LISTS libs) | ||
| find_library(_LIB_PATH "${lib}" PATHS "${libdir}" NO_DEFAULT_PATH) | ||
| if (NOT _LIB_PATH) | ||
| message("Skipping ${target} as ${lib} is missing") |
There was a problem hiding this comment.
Here as well, we'll want to error
| # Removing _LIB_PATH from cache otherwise it won't be updated | ||
| # on the next call to find_library(). | ||
| unset(_LIB_PATH CACHE) | ||
| endforeach() |
There was a problem hiding this comment.
Actually, I might not completely understand what this branch does, but can we it seems to do very similar logic to what already exists in
torchcodec/src/torchcodec/_core/CMakeLists.txt
Lines 271 to 275 in 75a3325
and in
add_ffmpeg_target(4 "${f4_library_file_names}")
add_ffmpeg_target(5 "${f5_library_file_names}")
add_ffmpeg_target(6 "${f6_library_file_names}")
add_ffmpeg_target(7 "${f7_library_file_names}")
Can we unify these?
This commit exposes torchcodec core library to be used by third
party modules on the C++ level. The primary purpose is to allow
non-CUDA device interfaces out-of-tree implementations. There
are the following major changes:
* Exposed TorchCodecConfig.cmake which defines torchcodec
targets to be linked with
* Provided Python level APIs to faciliate out-of-tree device
interfaces work with torchcodec:
* `torchcodec.cmake_prefix_path` - path which points to
`TorchCodecConfig.cmake` configuration
* `torchcodec.variant` - variant of the torchcodec library
which was loaded, i.e. N in libtorchcodec_core{N}.so
(currently ffmpeg_major_version)
* `torchcodec.core_library_path` - full path of the loaded
torchcodec core library
* `src/torchcodec/_core/` dropped from include paths to allow
using of the core library out-of-tree
`TorchCodecConfig.cmake` has 2 working modes:
* By default config works by checking available version of
FFmpeg libraries via `pkg-config` and configures corresponding
(single) version of torchcodec
* Altenatively, if `TORCHCODEC_FFMPEG{N}_INSTALL_PREFIX` is set
(`N=4,5,6,7` - version of FFmpeg), then config defines
torchcodec target corresponding to the specified FFmpeg version.
Note that multiple prefixes can be specified at the same time
allowing to build against few torchcodec versions at once.
Config will define `TORCHCODEC_VARIANTS` variable with value
corresponding to FFmpeg major versions of available torchcodec
core libraries. Further, config will also define `torchcodec::ffmpeg${N}`
and `torchcodec::core${N}` targets where `N` takes values from
`TORCHCODEC_VARIANTS`.
Signed-off-by: Dmitry Rogozhkin <[email protected]>
I've rebased the PR and resolved conflicts, but I will take a look and start address review comments later today or early next week. Please, let me know how |
|
@NicolasHug has imported this pull request. If you are a Meta employee, you can view this in D85451979. |
|
Can confirm the header |
Thank you! Here is includes PR: #1002. |
2 participants
This commit exposes torchcodec core library to be used by third party modules on the C++ level. The primary purpose is to allow non-CUDA device interfaces out-of-tree implementations. There are the following major changes:
Exposed TorchCodecConfig.cmake which defines torchcodec targets to be linked with
Provided Python level APIs to faciliate out-of-tree device interfaces work with torchcodec:
torchcodec.cmake_prefix_path- path which points toTorchCodecConfig.cmakeconfigurationtorchcodec.variant- variant of the torchcodec library which was loaded, i.e. N in libtorchcodec_core{N}.so (currently ffmpeg_major_version)torchcodec.core_library_path- full path of the loaded torchcodec core librarysrc/torchcodec/_core/dropped from include paths to allow using of the core library out-of-treeTorchCodecConfig.cmakehas 2 working modes:pkg-configand configures corresponding (single) version of torchcodecTORCHCODEC_FFMPEG{N}_INSTALL_PREFIXis set (N=4,5,6,7- version of FFmpeg), then config defines torchcodec target corresponding to the specified FFmpeg version. Note that multiple prefixes can be specified at the same time allowing to build against few torchcodec versions at once.Config will define
TORCHCODEC_VARIANTSvariable with value corresponding to FFmpeg major versions of available torchcodec core libraries. Further, config will also definetorchcodec::ffmpeg${N}andtorchcodec::core${N}targets whereNtakes values fromTORCHCODEC_VARIANTS.See the following repository for an actual out-of-tree device interface torchcodec plugin:
I suggest to pay attention on these:
CC: @scotts, @NicolasHug