From 9d447bddd6fe498aebf2e9beaecc86359f50167f Mon Sep 17 00:00:00 2001 From: Nicolas Hug Date: Fri, 14 Jun 2024 06:46:31 -0700 Subject: [PATCH] Make 'build against installed ffmpeg' the default build strategy (#27) Summary: This PR changes the "default" build strategy of torchcodec externally: - before: by default torchcodec is built against all of our FFmpeg versions available on S3. That's slow - after: by default torchcodec is built against the installed version of FFmpeg. That's fast and that's what most developers will want during development. We are removing the `BUILD_AGAINST_INSTALLED_FFMPEG` env variable and introducing instead `BUILD_AGAINST_ALL_FFMPEG_FROM_S3`. I think there's a pretty low risk that binaries get built without settting `BUILD_AGAINST_ALL_FFMPEG_FROM_S3=1`. We'll add a battery of sanity checks as we build the distribution packages to make sure that never happens. This is a follow-up to this review comment: https://www.internalfb.com/diff/D58527965?dst_version_fbid=262903956885891&transaction_fbid=981479793448363 Pull Request resolved: https://github.com/pytorch-labs/torchcodec/pull/27 Reviewed By: ahmadsharif1 Differential Revision: D58584992 Pulled By: NicolasHug --- .github/workflows/unit_test.yaml | 6 +-- setup.py | 11 ++--- src/torchcodec/decoders/_core/CMakeLists.txt | 40 +++++++++---------- .../decoders/_core/video_decoder_ops.py | 2 +- 4 files changed, 30 insertions(+), 29 deletions(-) diff --git a/.github/workflows/unit_test.yaml b/.github/workflows/unit_test.yaml index 4d45f94d..7fcfa7bf 100644 --- a/.github/workflows/unit_test.yaml +++ b/.github/workflows/unit_test.yaml @@ -45,7 +45,7 @@ jobs: fi # TODO: should we pass -DCMAKE_BUILD_TYPE=Debug here? That's what we # do for the C++ tests. - python -m pip install -e ".[dev]" --no-build-isolation -vvv + BUILD_AGAINST_ALL_FFMPEG_FROM_S3=1 python -m pip install -e ".[dev]" --no-build-isolation -vvv # list the built so files, for debugging. find src | grep ".so" @@ -87,9 +87,9 @@ jobs: ffmpeg -version - name: Build and run C++ tests run: | - export BUILD_AGAINST_INSTALLED_FFMPEG=1 + unset BUILD_AGAINST_ALL_FFMPEG_FROM_S3 - # Why we need BUILD_AGAINST_INSTALLED_FFMPEG and pkg-config: + # Why we're not setting BUILD_AGAINST_ALL_FFMPEG_FROM_S3 here: # The C++ tests decode x264 files which is not supported by our # pre-built non-GPL FFmpeg libraries. Unfortunately, that's what our # C++ tests currently link against by default, which would lead to diff --git a/setup.py b/setup.py index 891926d5..fdd5c96c 100644 --- a/setup.py +++ b/setup.py @@ -89,13 +89,14 @@ def get_ext_filename(self, fullname): return ext_filename -# If BUILD_AGAINST_INSTALLED_FFMPEG is set, we only build against the installed -# FFmpeg. We don't know what FFmpeg version that is, so we build -# `libtorchcodec.so` without any version suffix. -# If BUILD_AGAINST_INSTALLED_FFMPEG is not set then we want to build against all +# If BUILD_AGAINST_ALL_FFMPEG_FROM_S3 is set then we want to build against all # ffmpeg major version that are available on our S3 bucket. +# If BUILD_AGAINST_ALL_FFMPEG_FROM_S3 is not set, we only build against the +# installed FFmpeg. We don't know what FFmpeg version that is, so we build +# `libtorchcodec.so` without any version suffix. We could probably figure out +# the version number by invoking `pkg-config --modversion`. FFMPEG_MAJOR_VERSIONS = ( - ("",) if os.getenv("BUILD_AGAINST_INSTALLED_FFMPEG") is not None else (4, 5, 6, 7) + (4, 5, 6, 7) if os.getenv("BUILD_AGAINST_ALL_FFMPEG_FROM_S3") is not None else ("",) ) extensions = [ Extension( diff --git a/src/torchcodec/decoders/_core/CMakeLists.txt b/src/torchcodec/decoders/_core/CMakeLists.txt index 9372a8fe..786dd3e5 100644 --- a/src/torchcodec/decoders/_core/CMakeLists.txt +++ b/src/torchcodec/decoders/_core/CMakeLists.txt @@ -49,7 +49,26 @@ function(make_torchcodec_library library_name ffmpeg_target) ) endfunction() -if(DEFINED ENV{BUILD_AGAINST_INSTALLED_FFMPEG}) +if(DEFINED ENV{BUILD_AGAINST_ALL_FFMPEG_FROM_S3}) + message( + STATUS + "Building and dynamically linking libtorchcodec against our pre-built + non-GPL FFmpeg libraries. These libraries are only used at build time, + you still need a different FFmpeg to be installed for run time!" + ) + + # This will expose the ffmpeg4, ffmpeg5, ffmpeg6, and ffmpeg7 targets + include( + ${CMAKE_CURRENT_SOURCE_DIR}/fetch_and_expose_non_gpl_ffmpeg_libs.cmake + ) + + # The libtorchcodec names here must be kept in sync with the library names + # in setup.py. Grep for [ LIBTORCHCODEC_KEEP_IN_SYNC ] + make_torchcodec_library(libtorchcodec4 ffmpeg4) + make_torchcodec_library(libtorchcodec5 ffmpeg5) + make_torchcodec_library(libtorchcodec6 ffmpeg6) + make_torchcodec_library(libtorchcodec7 ffmpeg7) +else() message( STATUS "Building and dynamically linking libtorchcodec against the installed @@ -71,23 +90,4 @@ if(DEFINED ENV{BUILD_AGAINST_INSTALLED_FFMPEG}) # consistency with the other targets. This can slightly simplify the Python # loading and building code as well. make_torchcodec_library(libtorchcodec PkgConfig::LIBAV) -else() - message( - STATUS - "Building and dynamically linking libtorchcodec against our pre-built - non-GPL FFmpeg libraries. These libraries are only used at build time, - you still a different need FFmpeg to be installed for run time!" - ) - - # This will expose the ffmpeg4, ffmpeg5, ffmpeg6, and ffmpeg7 targets - include( - ${CMAKE_CURRENT_SOURCE_DIR}/fetch_and_expose_non_gpl_ffmpeg_libs.cmake - ) - - # The libtorchcodec names here must be kept in sync with the library names - # in setup.py. Grep for [ LIBTORCHCODEC_KEEP_IN_SYNC ] - make_torchcodec_library(libtorchcodec4 ffmpeg4) - make_torchcodec_library(libtorchcodec5 ffmpeg5) - make_torchcodec_library(libtorchcodec6 ffmpeg6) - make_torchcodec_library(libtorchcodec7 ffmpeg7) endif() diff --git a/src/torchcodec/decoders/_core/video_decoder_ops.py b/src/torchcodec/decoders/_core/video_decoder_ops.py index a718660c..7bf55278 100644 --- a/src/torchcodec/decoders/_core/video_decoder_ops.py +++ b/src/torchcodec/decoders/_core/video_decoder_ops.py @@ -17,7 +17,7 @@ def load_torchcodec_extension(): # On fbcode, _get_extension_path() is overridden and directly points to the # correct .so file, so this for-loop succeeds on the first iteration. - # grep for BUILD_AGAINST_INSTALLED_FFMPEG to explain the `""` part. + # grep for BUILD_AGAINST_ALL_FFMPEG_FROM_S3 to explain the `""` part. exceptions = [] for ffmpeg_major_version in (7, 6, 5, 4, ""): library_name = f"libtorchcodec{ffmpeg_major_version}"