Skip to content

Conversation

@bluebandit21
Copy link
Member

@bluebandit21 bluebandit21 commented Sep 3, 2021

WIP, not finished yet.

This aims to bump FFMPEG to 4.4, a fairly recent release.
FFMPEG 2.1.3, what we are using currently, is from 2014, making it rather old...
Updating FFMPEG now instead of waiting for it to break at some point in the future seems prudent.

MacOS currently builds ffmpeg from source; I have preserved this.
Linux and Windows both use pre-built libraries.
@nico-abram updated the Windows pre-built libraries (thank you!) but the Linux side has still not been touched.

Built using
./configure --disable-doc --disable-ffplay --disable-ffprobe --disable-ffmpeg --enable-asm --enable-yasm --disable-static --enable-shared --target-os=win64 --arch=x86_64 --toolchain=msvc

Should be LGPL licensed. Same flags without target-os and arch for
32bit. Built using MSVC 19.30 as described in
https://trac.ffmpeg.org/wiki/CompilationGuide/MSVC (Run vcvarsall then
launch msys2_shell.cmd and make sure link.exe is the MSVC one)
@bluebandit21
Copy link
Member Author

Just need to figure out the Linux pre-built library side for this to be ready.

I believe the issue preventing easily doing so was figuring out what distribution's pre-built FFMPEG to use, since I'm not sure where the original libraries even came from.

Alternatively, it might make more sense to build FFMPEG like we do for mac, since I'm not sure why the two are different in the first place.

@nico-abram
Copy link
Member

Are we actually building with ffmpeg on linux? I looked at the g++ and clang github actions logs for the latest develop commit and I can find no mention of "avcodec", "ffmpeg" or "MovieTexture_FFMpeg" within the build step logs. I can easily find these terms in the analogous mac and windows logs.

In this cmake file:

if(APPLE)
list(APPEND SMDATA_ARCH_MOVIE_TEXTURE_SRC "MovieTexture/MovieTexture_FFMpeg.cpp")
list(APPEND SMDATA_ARCH_MOVIE_TEXTURE_HPP "MovieTexture/MovieTexture_FFMpeg.h")
elseif(MSVC)
list(APPEND SMDATA_ARCH_MOVIE_TEXTURE_SRC "MovieTexture/MovieTexture_FFMpeg.cpp")
list(APPEND SMDATA_ARCH_MOVIE_TEXTURE_HPP "MovieTexture/MovieTexture_FFMpeg.h")
else() # Unix
if (${HAS_FFMPEG})
list(APPEND SMDATA_ARCH_MOVIE_TEXTURE_SRC "MovieTexture/MovieTexture_FFMpeg.cpp")
list(APPEND SMDATA_ARCH_MOVIE_TEXTURE_HPP "MovieTexture/MovieTexture_FFMpeg.h")
endif()

Only linux is conditionally not compiling the ffmpeg-dependant etterna C++ files. And grepping for "HAS_FFMPEG" in the entire repo brings up nothing at all, so we are not setting a default value for it nor enabling it in CI. That said, I can't see the actual compiler invocations in the build logs so I'm not sure if we are actually linking an ffmpeg version even if we're not compiling our files that depend on ffmpeg. Also, I can't find any precompiled binary artifacts in /extern/ffmpeg for linux (All of ./lib, ./windows and ./64bit seem to be windows files).

@bluebandit21
Copy link
Member Author

It's been quite a while since I looked at this, so unfortunately my memory is a bit hazy.
I'm fairly certain that we link in a pre-compiled FFMPEG build on Linux (although I don't know if we actually use it or not)
-- I remember the stopper that made me stop making progress on this while I was working on it so long ago was that I couldn't find "a" pre-compiled Linux build for FFMPEG to upgrade to.

Looking at the CMakeLists under extern/ffmpeg,
It does have the lines

if(UNIX)
    file(GLOB LIBS lib/*.a)
endif()

which means we are linking those static libraries on Linux builds.

That said, if we're not actually using that for anything, removing that entirely and just deleting the lib/ directory seems like a perfectly fine solution to me on the Linux side :)

@nico-abram
Copy link
Member

I wonder, does that mean it's linking these files then?
imagen

I'm scared to ask why these have "dll" in their filenames lol

@bluebandit21
Copy link
Member Author

I believe so.

As for the "dll" in file names, I don't want to know either 0_0

@nico-abram
Copy link
Member

There's an if(HAS_FFMPEG) for linux in this cmake script that includes the ffmpeg movieTexture which depends on ffmpeg:

else() # Unix
if (${HAS_FFMPEG})
list(APPEND SMDATA_ARCH_MOVIE_TEXTURE_SRC "MovieTexture/MovieTexture_FFMpeg.cpp")
list(APPEND SMDATA_ARCH_MOVIE_TEXTURE_HPP "MovieTexture/MovieTexture_FFMpeg.h")
endif()
endif()

if(APPLE)
  list(APPEND SMDATA_ARCH_MOVIE_TEXTURE_SRC "MovieTexture/MovieTexture_FFMpeg.cpp")
  list(APPEND SMDATA_ARCH_MOVIE_TEXTURE_HPP "MovieTexture/MovieTexture_FFMpeg.h")
  
elseif(MSVC)
  list(APPEND SMDATA_ARCH_MOVIE_TEXTURE_SRC "MovieTexture/MovieTexture_FFMpeg.cpp")
  list(APPEND SMDATA_ARCH_MOVIE_TEXTURE_HPP "MovieTexture/MovieTexture_FFMpeg.h")
  
else() # Unix
  if (${HAS_FFMPEG})
    list(APPEND SMDATA_ARCH_MOVIE_TEXTURE_SRC "MovieTexture/MovieTexture_FFMpeg.cpp")
    list(APPEND SMDATA_ARCH_MOVIE_TEXTURE_HPP "MovieTexture/MovieTexture_FFMpeg.h")
  endif()
  
endif()

I couldn't find anywhere that sets/defines that HAS_FFMPEG variable though. I assume there might be a way of setting/overriding it via the cmake command line?

@DashDashDashDashDash
Copy link
Contributor

Looking at the CMakeLists under extern/ffmpeg, It does have the lines

if(UNIX)
    file(GLOB LIBS lib/*.a)
endif()

which means we are linking those static libraries on Linux builds.

It seems like that isn't the case since

if(MSVC) # Only required on Windows
add_subdirectory(ffmpeg)
endif()

makes it so that only Windows even reaches that file.
However, even if I force it to load the ffmpeg subdir by removing the if(MSVC) entirely (and also forcing HAS_FFMPEG inside arch/CMakeLists.txt to be true), the files don't seem to be used, giving out build errors that indicates it's using the system's installed ffmpeg libraries.

I apologize if this isn't the correct way to go about debugging this issue, but I felt like the if(MSVC) line was important enough to comment on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants