Skip to content

Commit

Permalink
Fixed avresample dependency
Browse files Browse the repository at this point in the history
  • Loading branch information
@BrennoCaldato
BrennoCaldato committed Jun 20, 2021
1 parent e14adb7 commit 02f8936
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ mark_as_advanced(QT_VERSION_STR)
# Find FFmpeg libraries (used for video encoding / decoding)
find_package(FFmpeg REQUIRED COMPONENTS avcodec avformat avutil swscale)

set(all_comps avcodec avformat avutil swscale)
set(all_comps avcodec avformat avutil swscale avresample)
if(TARGET FFmpeg::swresample)
list(APPEND all_comps swresample)
else()
Expand Down

10 comments on commit 02f8936

@ferdnyc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BrennoCaldato

I keep seeing this change go by... avresample is long-deprecated in favor of libswresample, so FFmpegUtilities.h configures OpenShot to use that wherever possible, in which case libavresample isn't a dependency. What situations are you encountering where you need libavresample? Linking with any modern FFmpeg, it should be entirely superfluous.

@BrennoCaldato
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ferdnyc

My main desktop machine is still running an older version of ubuntu. Sorry about that, I imagined libavresample was deprecated in the newer ffmpeg versions but I needed to include it as a dependency for now.
I will make sure to remove this once we merge to develop

Still need to find time to upgrade my system though

@ferdnyc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BrennoCaldato Hmmm. Strange that the fallback logic isn't working, then. If your ffmpeg is old enough that swresample isn't present at all, then avresample should be picked up... unless there's something broken with the CMake find module. What version of ffmpeg, and what ubuntu, out of curiosity? I'll try to play around with it in a VM.

@ferdnyc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem might be that our version check logic is out-of-whack.

  • The CMake logic only links in avresample if swresample doesn't exist
  • The in-source gating in FFmpegUtilities.h only sets USE_SW if LIBAVFORMAT_VERSION_MAJOR > 57
  • swresample has existed since long before avformat 57 (which ~== FFmpeg 3.0).
  • swresample has only been formally deprecated since FFmpeg 4.0.

So, that's probably all wrong. Most likely you're using an FFmpeg where libavformat < 57, which prevents you from using swresample, but CMake tries not to include libavresample because swresample is available.

I think what I want to do is move the check logic out of FFmpegUtiltiies.h and into the CMakeLists.txt file (including the version test, to replace the simple "exists" check), then have CMake define USE_SW appropriately. That way it knows which library the code is going to be using (because it's telling it) and it'll only have to link with whichever one it chooses.

@BrennoCaldato
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ferdnyc

I'm on ubuntu 16.04. These are the ffmpeg and libav versions:

ffmpeg version 2.8.17-0ubuntu0.1

ii  libavcodec-dev:amd64            7:2.8.17-0ubuntu0.1 amd64        FFmpeg library with de/encoders for audio/video codecs - development files
ii  libavcodec-ffmpeg-extra56:amd64 7:2.8.17-0ubuntu0.1 amd64        FFmpeg library with additional de/encoders for audio/video codecs

@ferdnyc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BrennoCaldato Yeah, that's what I figured. You're not getting avresample because swresample exists. But the code isn't using swresample because it's pre-3.0 FFmpeg. Moving the logic to the CMake code will allow it to select the correct resampler target (FFmpeg::avresample) to link with. See #693.

@BrennoCaldato
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ferdnyc I will test the pull request.

Totally unrelated quick question: I am adding more audio effects to libopenshot but I'll probably need the juce DSP module to use FFT and other methods. This is currently not imported by libopenshot-audio and I tried to add it, but it didn't work very well. Do you have some advice for me when adding new modules to libopenshot-audio? Will try to get more info on the errors

@ferdnyc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#693 and #694 are merged, so the develop branch should now do the right thing WRT avresample/swresample on all ffmpeg versions.

Regarding the JUCE module, you'd probably want to do it the same way we generated the existing JuceAudioLibrary tree: By running their ProJucer application, and having it generate the necessary files. The OpenShotLibrary.jucer file in the root of the -audio repo is the config file for that application, that'd be the starting point.

If the module you need is juce_dsp, that should pretty much do it. ProJucer will handle the dependencies required, but at least juce_dsp is a non-gui module. juce_audio_processors, OTOH, brings in the whole of JUCE's GUI framework, but I don't think we need audio_processors anyway.

I'll start a branch in libopenshot-audio for enabling juce_dsp, we can work it from there.

@ferdnyc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha! According to ProJucer, juce_dsp requires a C++ standard greater than the currently-configured C++11.

I'd actually really like to move to C++14 or C++17, TBH, since both bring significant feature enhancements to the STL. But we'll have to see how compatible those later standards are with our current build environment(s).

Or we drag them kicking and screaming into the more recent past (never mind "the future"...), which is typically how it goes.

@ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented on 02f8936 Jun 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BrennoCaldato Check out OpenShot/libopenshot-audio#129, or the dsp-module branch in that repo. Should have what you need.

Please sign in to comment.