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

[ffmpeg] Add feature support for zlib, iconv, fdk-aac, mp3lame, opus, soxr, theora. #11277

Merged
merged 52 commits into from
Aug 18, 2020

Conversation

Sibras
Copy link
Contributor

@Sibras Sibras commented May 9, 2020

  • [zlib] Fix unistd.h being incorrectly required when imported from a project defining HAVE_UNISTD_H=0

  • [ffmpeg] Fix zlib feature. Zlib was not currently being used despite being marked as a dependency. Zlib was quietly failing due to a missing unistd.h. An update was also required to detect the correct debug lib.

  • [ffmpeg] Add additional features to enable:

    • iconv
    • fdk-aac
    • mp3lame
    • opus
    • soxr
    • theora
    • x265
    • vorbis
    • speex
    • snappy
    • wavpack
    • sdl2 (required for ffplay)
  • [ffmpeg] Fix various issues with FindFFmpeg.cmake:

    • correctly detect debug and release libs
    • add missing postproc lib
    • add optional feature dependency detection
  • [ogg] Enable output of pkg-config file (required for detection by ffmpeg and vorbis)

  • [speex] Enable output of pkg-config file

  • [ffmpeg] Set pkg-config to search for static configurations when building in static configuration

  • [ffmpeg] Fixes for detection of opencl, soxr, lame, iconv, fdk, x264, x265, openssl, wavpack on linux/static builds

  • [sdl2] Enable output of pkg-config file

  • [fdk-aac] Correct header installation to "include/fdk-aac" to match fdk's normal build scripts (required for any lib such as ffmpeg to get the correct header location)

  • [ffmpeg] Remove nvcodec's dependency on cuda

  • [pkgconfig] Add additional common system libraries.

  • [vcpkg_fixup_pkgconfig] Add to ffnvcodec, ogg, sdl2, speex, x265, wavpack, vorbis, opus

Fixes #7084, Fixes #9671, Fixes #7544

@msftclas
Copy link

msftclas commented May 9, 2020

CLA assistant check
All CLA requirements met.

@cenit
Copy link
Contributor

cenit commented May 9, 2020

A small subset of features is already under revision in #11151

@Sibras
Copy link
Contributor Author

Sibras commented May 10, 2020

I have seen #11151 but this is a superset of that which doesnt directly apply over the top of it without having to make some changes to #11151 (although minor ones). So since i had already done the work for lame and opus (the only 2 added by #11151) I left them in this patch as it makes it easier to add and test the others without dealing with conflicts.

As to which is used, it doesn't bother me. I just had this patch ready to go, all checks passing, so i thought id put it up.

@cenit
Copy link
Contributor

cenit commented May 10, 2020

Thank you for your work!

@Sibras
Copy link
Contributor Author

Sibras commented May 11, 2020

I also have commits that add support for vorbis, speex, snappy and wavpack features but ill wait for a review on these before pushing those

@cenit
Copy link
Contributor

cenit commented May 11, 2020

@Sibras when consuming ffmpeg, using cmake, is it necessary to modify FindFFMPEG.cmake in order to add missing dependencies, in case ffmpeg is built with some of your new features?
If you cannot try it on some OS, it’s enough to just put all features as default features temporarily, and see if CI is complaining. Maybe also try to turn ffmpeg as a default feature also in opencv, since that is a known port dependent on a proper FindFFMPEG.cmake

@Sibras
Copy link
Contributor Author

Sibras commented May 11, 2020

@cenit Most likely FindFFMPEG.cmake should be modified, However many of the existing features were already not added to FindFFMPEG.cmake (such as openssl, x264 etc.) before this so it was already broken. Ill have a look at fixing though

@cenit
Copy link
Contributor

cenit commented May 11, 2020

It’s not broken for default features for sure. I’d not be happy about merging even more features without support inside FindFFMPEG.cmake, but that’s just my feeling.

@Sibras
Copy link
Contributor Author

Sibras commented May 12, 2020

@cenit, OK ive looked over FindFFMPEG.cmake and it looks like it is actually broken even for defaults. It was incorrectly finding the wrong ffmpeg libs (only ever found debug) and was not searching for postproc.lib which is actually used by avfilter so you would still get link errors with static builds (linux and windows).

Ive fixed the above issues and added in handling for the previously missing dependencies + all the ones i have added. Ill update the pr later once i clean up the patches.

For reference just testing opencv is not enough as that just builds a lib. So for testing i created a 5 line program that pulled in ffmpeg using cmake and then tested it using linux+windows static build triplets. Not sure if something like that should/can be added to the CI to help test in the future.

@cenit
Copy link
Contributor

cenit commented May 12, 2020

@Sibras thanks for adding them. This kind of testing, while very important, is still out of scope for vcpkg.
To be honest, I also have it. And I see ffmpeg detecting debug and release libs and correctly linking it towards a very small simple program, but of course I can just be in a working corner case.
I hope to see your fixes soon :)

@Sibras Sibras force-pushed the master branch 2 times, most recently from 8649185 to aa7432f Compare May 12, 2020 12:34
@NancyLi1013 NancyLi1013 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label May 22, 2020
@Sibras
Copy link
Contributor Author

Sibras commented May 24, 2020

bump

@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY
Copy link
Contributor

Please resolve the file conflicts.

Thanks.

@Sibras
Copy link
Contributor Author

Sibras commented Aug 5, 2020

Only outstanding issues on linux are now:

  • avisynthplus: this port only supports dynamic linkage and so does not support linux
  • speex: Port currently does not support linux

scripts/cmake/vcpkg_common_definitions.cmake Show resolved Hide resolved
ports/ffmpeg/portfile.cmake Outdated Show resolved Hide resolved
ports/ffmpeg/FindFFMPEG.cmake.in Outdated Show resolved Hide resolved
@JackBoosY JackBoosY requested a review from PhoebeHui August 7, 2020 09:20
@PhoebeHui
Copy link
Contributor

PhoebeHui commented Aug 11, 2020

@Sibras, thanks for your hard work!

Test all features with latest changes:
x64-windows: pass
x64-linux: except the below 2 features, others passed.

x265: ERROR: libx265 not found
wavpack: ERROR: libwavpack not found

Double checked, the wavpack requires '-lm' library,

wavpack failure log:

/home/phoebe/11277/vcpkg/installed/x64-linux/debug/lib/../lib/libwavpack.a(pack_utils.c.o): In function `WavpackSetConfiguration64':
/home/phoebe/11277/vcpkg/buildtrees/wavpack/src/0eba4c0c9f-ecbdfe4a39.clean/src/pack_utils.c:305: undefined reference to `floor'
/home/phoebe/11277/vcpkg/buildtrees/wavpack/src/0eba4c0c9f-ecbdfe4a39.clean/src/pack_utils.c:311: undefined reference to `floor'
/home/phoebe/11277/vcpkg/installed/x64-linux/debug/lib/../lib/libwavpack.a(pack.c.o): In function `pack_init':
/home/phoebe/11277/vcpkg/buildtrees/wavpack/src/0eba4c0c9f-ecbdfe4a39.clean/src/pack.c:59: undefined reference to `floor'
/home/phoebe/11277/vcpkg/installed/x64-linux/debug/lib/../lib/libwavpack.a(pack_dns.c.o): In function `dynamic_noise_shaping':
/home/phoebe/11277/vcpkg/buildtrees/wavpack/src/0eba4c0c9f-ecbdfe4a39.clean/src/pack_dns.c:137: undefined reference to `floor'
/home/phoebe/11277/vcpkg/buildtrees/wavpack/src/0eba4c0c9f-ecbdfe4a39.clean/src/pack_dns.c:140: undefined reference to `floor'
/home/phoebe/11277/vcpkg/installed/x64-linux/debug/lib/../lib/libwavpack.a(pack_dns.c.o):/home/phoebe/11277/vcpkg/buildtrees/wavpack/src/0eba4c0c9f-ecbdfe4a39.clean/src/pack_dns.c:189: more undefined references to `floor' follow

x265 failure log

gcc -Wl,--as-needed -Wl,-z,noexecstack -pthread -o /tmp/ffconf.b3WzRGjY/test /tmp/ffconf.b3WzRGjY/test.o -lx265 -lm -ldl -lstdc++ -lgcc_s -lgcc -lrt
/home/phoebe/11277/vcpkg/installed/x64-linux/lib/../lib/libx265.a(threadpool.cpp.o): In function `x265::ThreadPool::create(int, int, unsigned long)':
threadpool.cpp:(.text+0x21c): undefined reference to `numa_available'
threadpool.cpp:(.text+0x321): undefined reference to `numa_allocate_nodemask'
/home/phoebe/11277/vcpkg/installed/x64-linux/lib/../lib/libx265.a(threadpool.cpp.o): In function `x265::ThreadPool::setThreadNodeAffinity(void*)':
threadpool.cpp:(.text+0x585): undefined reference to `numa_available'
threadpool.cpp:(.text+0x591): undefined reference to `numa_run_on_node_mask'
threadpool.cpp:(.text+0x599): undefined reference to `numa_set_interleave_mask'
/home/phoebe/11277/vcpkg/installed/x64-linux/lib/../lib/libx265.a(threadpool.cpp.o): In function `x265::ThreadPool::getNumaNodeCount()':
threadpool.cpp:(.text+0x835): undefined reference to `numa_available'
threadpool.cpp:(.text+0x843): undefined reference to `numa_max_node'
/home/phoebe/11277/vcpkg/installed/x64-linux/lib/../lib/libx265.a(threadpool.cpp.o): In function `x265::ThreadPool::allocThreadPools(x265_param*, int&, bool)':
threadpool.cpp:(.text+0x9bb): undefined reference to `numa_available'
threadpool.cpp:(.text+0xc34): undefined reference to `numa_allocate_cpumask'
threadpool.cpp:(.text+0xc5c): undefined reference to `numa_bitmask_weight'
threadpool.cpp:(.text+0xc73): undefined reference to `numa_node_to_cpus'
threadpool.cpp:(.text+0xca9): undefined reference to `numa_bitmask_free'
threadpool.cpp:(.text+0x12b6): undefined reference to `numa_available'
threadpool.cpp:(.text+0x131c): undefined reference to `numa_allocate_cpumask'
threadpool.cpp:(.text+0x1330): undefined reference to `numa_bitmask_free'
/home/phoebe/11277/vcpkg/installed/x64-linux/lib/../lib/libx265.a(threadpool.cpp.o): In function `x265::ThreadPool::~ThreadPool()':
threadpool.cpp:(.text+0x566): undefined reference to `numa_bitmask_free'
/home/phoebe/11277/vcpkg/installed/x64-linux/lib/../lib/libx265.a(threadpool.cpp.o): In function `x265::ThreadPool::setThreadNodeAffinity(void*)':
threadpool.cpp:(.text+0x59f): undefined reference to `numa_set_localalloc'
collect2: error: ld returned 1 exit status
ERROR: libx265 not found

@Sibras
Copy link
Contributor Author

Sibras commented Aug 11, 2020

@Sibras, thanks for your hard work!

Test all features with latest changes:
x64-windows: pass
x64-linux: except the below 2 features, others passed.

x265: ERROR: libx265 not found
wavpack: ERROR: libwavpack not found

Wavpack is fixed now. I couldnt reproduce the x265 error on my machine, I have libnuma-dev installed but its not picking it up. So i think I fixed the x265 error but I cant reproduce it to test.

@PhoebeHui
Copy link
Contributor

@Sibras, after I installed libnuma-dev, the feature 'x265' passed on linux. the feature 'wavpack' also test pass on linux.

Summary the feature tests status:
x64-linux: feature 'avisynthplus' and 'speex' doesn't support on linux. Others features test pass.
x64-windows: feature 'x264' failed, currently the issue in dependency port x264 has been fixed on master, however I suggest to fix the feature in sperate PR.

Currently no issues to block this PR, @strega-nil, could you please help review and merge this PR?

@PhoebeHui PhoebeHui added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Aug 14, 2020
@strega-nil
Copy link
Contributor

@Sibras could you merge with master?

@BillyONeal BillyONeal self-assigned this Aug 18, 2020
@BillyONeal BillyONeal merged commit 2722695 into microsoft:master Aug 18, 2020
@BillyONeal
Copy link
Member

Thanks for your contribution!

remz1337 pushed a commit to remz1337/vcpkg that referenced this pull request Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet