Add option to build against installed FFmpeg and re-enable C++ tests (#…

…23)

Summary: Pull Request resolved: #23

Reviewed By: scotts, ahmadsharif1

Differential Revision: D58527965

Pulled By: NicolasHug

fbshipit-source-id: e0988ecdb25287562644840ae8c6929b1c9c3d97

.github/workflows/unit_test.yaml

@@ -61,17 +61,39 @@ jobs:
         run: |
           pytest test
 
-    # TODO: MAKE THESE TESTS WORK AGAIN!!!!
-    #   - name: Build and run C++ tests
-    #     run: |
-    #       # TODO: This is building all the .so a second time. We should try
-    #       # to avoid that. Maybe we should add a 'build test' option to the
-    #       # `pip install ...` step?
-    #       mkdir -p build
-    #       pushd build
-    #       TORCH_PATH=$(python -c "import pathlib, torch; print(pathlib.Path(torch.__path__[0]))")
-    #       Torch_DIR="${TORCH_PATH}/share/cmake/Torch"
-    #       cmake .. -DTorch_DIR=$Torch_DIR -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTS=ON -DCMAKE_VERBOSE_MAKEFILE=ON
-    #       cmake --build .
-    #       ctest
-    #       popd
+      - name: Build and run C++ tests
+        run: |
+          conda install pkg-config -c conda-forge
+          export BUILD_AGAINST_INSTALLED_FFMPEG=1
+
+          # Why we need BUILD_AGAINST_INSTALLED_FFMPEG and pkg-config:
+          # 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
+          # test failures. To remediate that, we have to tell the C++ tests to
+          # build against the FFmpeg that we installed from conda-forge (which
+          # is able to decode x264).
+          # Note: The Python tests are also decoding x264 files. The Python
+          # tests work fine because even though the library was built against
+          # our pre-built non-GPL at build time, at run time (during test
+          # execution), it's the installed FFmpeg version that is found.
+          #
+          # TODO: We should fix that and
+          # a) build the C++ tests against our pre-built non-GPL FFmpeg, like
+          #    for the Python package
+          # b) make sure that the C++ tests can dynamically load the installed
+          #    FFmpeg version.
+          # c) ideally, we should avoid re-building the entire libtorchcodec
+          #    library just for building the C++ tests. We should be able to
+          #    re-use the build that was done when we installed the Python
+          #    library and avoid recompilation.
+
+          build_tests_dir="${PWD}/build_tests"
+          mkdir $build_tests_dir
+          pushd $build_tests_dir
+          TORCH_PATH=$(python -c "import pathlib, torch; print(pathlib.Path(torch.__path__[0]))")
+          Torch_DIR="${TORCH_PATH}/share/cmake/Torch"
+          cmake .. -DTorch_DIR=$Torch_DIR -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTS=ON -DCMAKE_VERBOSE_MAKEFILE=ON
+          cmake --build .
+          ctest
+          popd

setup.py

@@ -1,3 +1,4 @@
+import os
 import subprocess
 from pathlib import Path
 
@@ -88,6 +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
+# ffmpeg major version that are available on our S3 bucket.
+FFMPEG_MAJOR_VERSIONS = (
+    ("",) if os.getenv("BUILD_AGAINST_INSTALLED_FFMPEG") is not None else (4, 5, 6, 7)
+)
 extensions = [
     Extension(
         # The names here must be kept in sync with the target names in the
@@ -104,6 +113,6 @@ def get_ext_filename(self, fullname):
         # CMakeLists.txt file.
         sources=[],
     )
-    for ffmpeg_major_version in (4, 5, 6, 7)
+    for ffmpeg_major_version in FFMPEG_MAJOR_VERSIONS
 ]
 setup(ext_modules=extensions, cmdclass={"build_ext": CMakeBuild})

src/torchcodec/decoders/_core/CMakeLists.txt

@@ -6,8 +6,6 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON)
 find_package(Torch REQUIRED)
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${TORCH_CXX_FLAGS}")
 
-include(${CMAKE_CURRENT_SOURCE_DIR}/fetch_and_expose_non_gpl_ffmpeg_libs.cmake)
-
 function(make_torchcodec_library library_name ffmpeg_target)
     set(
         sources
@@ -51,9 +49,45 @@ function(make_torchcodec_library library_name ffmpeg_target)
     )
 endfunction()
 
-# 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)
+if(DEFINED ENV{BUILD_AGAINST_INSTALLED_FFMPEG})
+    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
+    )
+
+    # TODO: To be pedantic, find what version of ffmpeg is installed and name
+    # the libtorchcodec library "libtorchcodec{ffmpeg_major_version}" for
+    # 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()

src/torchcodec/decoders/_core/video_decoder_ops.py

@@ -17,8 +17,9 @@ 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.
     exceptions = []
-    for ffmpeg_major_version in (7, 6, 5, 4):
+    for ffmpeg_major_version in (7, 6, 5, 4, ""):
         library_name = f"libtorchcodec{ffmpeg_major_version}"
         try:
             torch.ops.load_library(_get_extension_path(library_name))

test/decoders/CMakeLists.txt

@@ -33,14 +33,14 @@ target_include_directories(VideoDecoderOpsTest PRIVATE ../../)
 
 target_link_libraries(
   VideoDecoderTest
-  torchcodec
+  libtorchcodec
   GTest::gtest_main
   "${TORCH_LIBRARIES}"
 )
 
 target_link_libraries(
   VideoDecoderOpsTest
-  torchcodec
+  libtorchcodec
   GTest::gtest_main
   "${TORCH_LIBRARIES}"
 )