- 
                Notifications
    You must be signed in to change notification settings 
- Fork 67
Expose TorchCodecConfig.cmake #938
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
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.
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).
        
          
                src/torchcodec/_core/ops.py
              
                Outdated
          
        
      | _pybind_ops: Optional[ModuleType] = None | ||
|  | ||
| variant = None | ||
| core_library_path = None | 
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.
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(ffmpeg_major_version "6") | ||
| elseif (${libavcodec_major_version} STREQUAL "61") | ||
| set(ffmpeg_major_version "7") | ||
| 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.
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
| else() | |
| message( | |
| STATUS | |
| "Building and dynamically linking libtorchcodec against the installed | |
| FFmpeg libraries. This require pkg-config to be installed. If you have | |
| installed FFmpeg from conda, make sure pkg-config is installed from | |
| conda as well." | |
| ) | |
| find_package(PkgConfig REQUIRED) | |
| pkg_check_modules(LIBAV REQUIRED IMPORTED_TARGET | |
| libavdevice | |
| libavfilter | |
| libavformat | |
| libavcodec | |
| libavutil | |
| libswresample | |
| libswscale | |
| ) | |
| # Split libavcodec's version string by '.' and convert it to a list | |
| string(REPLACE "." ";" libavcodec_version_list ${LIBAV_libavcodec_VERSION}) | |
| # Get the first element of the list, which is the major version | |
| list(GET libavcodec_version_list 0 libavcodec_major_version) | |
| if (${libavcodec_major_version} STREQUAL "58") | |
| set(ffmpeg_major_version "4") | |
| elseif (${libavcodec_major_version} STREQUAL "59") | |
| set(ffmpeg_major_version "5") | |
| elseif (${libavcodec_major_version} STREQUAL "60") | |
| set(ffmpeg_major_version "6") | |
| elseif (${libavcodec_major_version} STREQUAL "61") | |
| set(ffmpeg_major_version "7") | |
| elseif (${libavcodec_major_version} STREQUAL "62") | |
| set(ffmpeg_major_version "8") | |
| else() | |
| message( | |
| FATAL_ERROR | |
| "Unsupported libavcodec version: ${libavcodec_major_version}" | |
| ) | |
| 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.
Ok, I've missed this one. I will need to look into it.
| 
 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. | 
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 <dmitry.v.rogozhkin@gmail.com>
    Signed-off-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@gmail.com>
See: meta-pytorch/torchcodec#938 Signed-off-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@gmail.com>
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