Always build libtorchcodec{ffmpeg_major}.so instead of libtorchcodec.…

…so (#28)

Summary:
When `BUILD_AGAINST_INSTALLED_FFMPEG` is set, we build against a single FFmpeg version, and create `libtorchcodec.so`. With this PR, we now create `libtorchcodec{installed_ffmpeg_major_version}.so` instead.

Pros:
- we know from the .so name which ffmpeg version we're using.
- the names of the `.so` files are consistent whether `BUILD_AGAINST_INSTALLED_FFMPEG` was set or not.

Cons:
- The Cmake implementation is NASTY
- ~~The logic of finding the installed FFmpeg version must be duplicated across Python and Cmake. This is prone to de-synchronizations and bugs.~~

~~IMHO the cons outweigh the pros by a lot. I'd be in favour of not landing this, and removing the associated TODOs.~~

EDIT in V2: The Python side of the implementation is now much cleaner due to #34

This PR addresses this comment: https://www.internalfb.com/diff/D58527965?dst_version_fbid=262903956885891&transaction_fbid=1932366733863261

Pull Request resolved: #28

Reviewed By: ahmadsharif1

Differential Revision: D58585136

Pulled By: NicolasHug

fbshipit-source-id: 77abe7d3440e53e23cb5dd6867c885427bf65f9f

src/torchcodec/decoders/_core/CMakeLists.txt

@@ -83,9 +83,30 @@ else()
         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)
+    # 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")
+    else()
+        message(
+            FATAL_ERROR
+            "Unsupported libavcodec version: ${libavcodec_major_version}"
+        )
+    endif()
+
+    set(libtorchcodec_target_name libtorchcodec${ffmpeg_major_version})
+    # Make libtorchcodec_target_name available in the parent's scope, for the
+    # test's CMakeLists.txt
+    set(libtorchcodec_target_name ${libtorchcodec_target_name} PARENT_SCOPE)
+
+    make_torchcodec_library(${libtorchcodec_target_name} PkgConfig::LIBAV)
 endif()

src/torchcodec/decoders/_core/video_decoder_ops.py

@@ -17,9 +17,8 @@ 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_ALL_FFMPEG_FROM_S3 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
-  libtorchcodec
+  ${libtorchcodec_target_name}
   GTest::gtest_main
   "${TORCH_LIBRARIES}"
 )
 
 target_link_libraries(
   VideoDecoderOpsTest
-  libtorchcodec
+  ${libtorchcodec_target_name}
   GTest::gtest_main
   "${TORCH_LIBRARIES}"
 )