Skip to content
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

Make 'build against installed ffmpeg' the default build strategy #27

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/unit_test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
11 changes: 6 additions & 5 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
40 changes: 20 additions & 20 deletions src/torchcodec/decoders/_core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()
2 changes: 1 addition & 1 deletion src/torchcodec/decoders/_core/video_decoder_ops.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand Down
Loading