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

cmake: make external, shared ffmpeg default #10479

Merged
merged 3 commits into from Nov 27, 2016

Conversation

@wsnipex
Copy link
Member

commented Sep 17, 2016

Since our own ffmpeg is - as of v3.1 - essentially vanilla (-1 unimportant build related commit), lets use external, shared ffmpeg libs by default.
This should make the life of all linux distro kodi maintainers easier.

I have left out autotools on purpose as incentive for packagers to switch to cmake.
supersedes #10432

ping @fetzerch @notspiff @hudokkow @rbalint

@wsnipex

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2016

jenkins build this please

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 17, 2016

I would keep it as e have it now. Currently we don't carry any changes of ffmpeg in our repo but this can change tomorrow.

@wsnipex wsnipex force-pushed the wsnipex:ffmpeg branch from 7927814 to 6d7e9e8 Sep 17, 2016
@wsnipex

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2016

If really necessary the default can be changed, but we should really try to stay away from custom patches after all those years trying to get rid of them.

jenkins build this please

@wsnipex wsnipex added the WIP label Sep 17, 2016
Copy link
Member

left a comment

I've mostly looked at the cmake module, others know better if we want internal or external ffmpeg by default.

One additional thing I would like to get rid of is the ffmpeg link wrapper (https://github.com/xbmc/xbmc/blob/master/project/cmake/CMakeLists.txt#L391). Don't yet have a good idea how, but if you have one, let me know.

if(FFMPEG_PATH)
message(WARNING "Internal FFmpeg enabled, but FFMPEG_PATH given, ignoring")
set(ENV{PKG_CONFIG_PATH} "${FFMPEG_PATH}/lib/pkgconfig")
list(APPEND ${CMAKE_PREFIX_PATH} ${FFMPEG_PATH})

This comment has been minimized.

Copy link
@fetzerch

fetzerch Sep 17, 2016

Member

CMAKE_PREFIX_PATH doesn't need ${} here

AND PC_FFMPEG_libswscale_VERSION
AND PC_FFMPEG_libswresample_VERSION
AND PC_FFMPEG_libpostproc_VERSION)
set(FFMPEG_VERSION 3.1)

This comment has been minimized.

Copy link
@fetzerch

fetzerch Sep 17, 2016

Member

I would set a variable at the top where you define FFMPEG_PKGS, that way if we bump the version, the version variables are in once place.

FFMPEG_LIBSWRESAMPLE
FFMPEG_LIBPOSTPROC
FFMPEG_VERSION
FAIL_MESSAGE "FFmpeg 3.1 not found, please consider using -DENABLE_INTERNAL_FFMPEG=ON")

This comment has been minimized.

Copy link
@fetzerch

fetzerch Sep 17, 2016

Member

you can use the same var here as well

${FFMPEG_LIBSWSCALE} ${FFMPEG_LIBSWRESAMPLE}
${FFMPEG_LIBPOSTPROC})
set(FFMPEG_LDFLAGS ${PC_FFMPEG_LDFLAGS})
list(APPEND FFMPEG_DEFINITIONS -DFFMPEG_VER_SHA=\"${FFMPEG_VERSION}\")

This comment has been minimized.

Copy link
@fetzerch

fetzerch Sep 17, 2016

Member

are you sure that quotes need to be escaped here? I've done a similar change for lirc recently and there it was not needed

This comment has been minimized.

Copy link
@wsnipex

wsnipex Sep 18, 2016

Author Member

I'm pretty sure I put it like this for a reason, but don't remember why exactly. This var is only use internally for some loglines and seems to have worked as is for quite some time now.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 18, 2016

the default should be our fork. don't change this

@wsnipex wsnipex force-pushed the wsnipex:ffmpeg branch from 9238bfe to 32b9af0 Sep 18, 2016
@wsnipex

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2016

changed to fallback to internal if required version is not found.

@fetzerch to get rid of the link wrapper we'd have to build ffmpeg outside of the kodi project or introduce some magic to build it during config stage.

@wsnipex

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2016

@fetzerch wsnipex@e4d36ab is a POC on building ffmpeg during config stage and getting rid of the link wrapper. Not sure if I like it better.


# FFMPEG v3.1
set(REQUIRED_FFMPEG_VERSION 3.1)
set(FFMPEG_PKGS libavcodec>=57.48.101

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 18, 2016

Member

imo those checks are a bit overenginered. note that developers need to be able to build with any version of ffmpeg. if external ffmpeg is used we can expect from that persons that they know what they are doing. hence no version check required.

This comment has been minimized.

Copy link
@wsnipex

wsnipex Sep 18, 2016

Author Member

are you suggesting that we should just accept any libs thrown at us? If yes, we can scrap a whole lot of our build system and deal with the fallout every day.
A dev that insists on building with a lower version can patch this.
The same version checks exists in autotools for years and have been in cmake from the start as well.

This comment has been minimized.

Copy link
@wsnipex

wsnipex Sep 18, 2016

Author Member

oh and you call it over engineered, I call it architecture and good practice

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 18, 2016

Member

are you suggesting that we should just accept any libs thrown at us?

that is not the case here because we use our fork as default.

A dev that insists on building with a lower version can patch this.

I disagree. Development is a main use case that has to be considered. In general developers don't be experts on build system. hence this has to be kept as simple as possible.

@rbalint

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2016

Thank you for working on making our (distro packagers) life easier.
The smaller the patches are I have to carry the more time I can spend on other quality-related issues.

@fritsch

This comment has been minimized.

Copy link
Member

commented Sep 19, 2016

My take on this from a technical level (without politics):
As a developer I easily want to test newer / custom ffmpeg as in a lot of times, we work with upstream to either fix issues or develop new features or doing API transitions. Until now I just used --with-ffmpeg=/opt/ffmpeg and all was working for me. I'd like to also have an easy solution in the future in a documented way, so that I don't have to dig into whatever, but just can keep my development use case as I did before without too much hazzle.

As a Kodi-Supporter especially on the bugtracker and the forum I don't want to fix bugs, that I solved with upstream ffmpeg again and again. That means: Though we work on getting fixes included with upstream the very same day - it won't be always possible and we will be "shipping" a custom kodi ffmpeg version, which then still is mandatory for distributions as everything else means known (!) (cause we obviously fixed bugs) breakage. Same accounts for cherry-picks we do from upstream, e.g.: hevc-10 bit fixes for dxva, or VAAPI / VP9 enhancements, which might break compatibility with distributions as certain fields are not defined (good case as it breaks in the compiler) or certain code paths (e.g. format queries return NULL) -> runtime segfault not happening for us and so on.

There are also other things besides the "versioning", which is: missing options. If you use an ffmpeg without mjpeg, png encoder support, our Image handling won't work. If you don't enable ffmpeg's dts-hd decoder, we won't display metadata correctly anymore. So if you see the same version does not mean we run on this build, that's perhaps the biggest technical difference not yet named: versioning is not enough to make sure that everything is fine.

The message should not be: kodi is now "supporting" your distribution's specific ffmpeg version, cause we can't do that out of stated reasons.

Btw. ffmpeg has deprecated codec_info and when fernet and I are done with v17 we will transition ffmpeg to codecpar - this will break backwards compatibility to every ffmpeg version < 3.1. Distributions need to be aware of this. Same accounts for all the avdecode5 methods.

@wsnipex wsnipex referenced this pull request Oct 20, 2016
1 of 3 tasks complete
@wsnipex wsnipex force-pushed the wsnipex:ffmpeg branch from 32b9af0 to e071806 Oct 21, 2016
@wsnipex

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2016

jenkins build this please

@wsnipex

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2016

once more, jenkins build this please

@wsnipex

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2016

jenkins build this please ....

@wsnipex

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2016

jenkins build this please

@wsnipex

This comment has been minimized.

Copy link
Member Author

commented Oct 22, 2016

github issues, lets try again:
jenkins build this please

@wsnipex wsnipex force-pushed the wsnipex:ffmpeg branch from 1a71a3e to 51bacee Nov 23, 2016
@wsnipex

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2016

jenkins build this please

@wsnipex wsnipex force-pushed the wsnipex:ffmpeg branch 2 times, most recently from f4413f0 to 097cfe4 Nov 24, 2016
@wsnipex

This comment has been minimized.

Copy link
Member Author

commented Nov 24, 2016

jenkins build this please

@wsnipex wsnipex added v17 Krypton and removed WIP labels Nov 24, 2016
@wsnipex wsnipex force-pushed the wsnipex:ffmpeg branch 2 times, most recently from bb778cf to ff125c7 Nov 24, 2016
@wsnipex

This comment has been minimized.

Copy link
Member Author

commented Nov 24, 2016

@fetzerch this is ready from my side, can you please check again?

the following changed:

  • new option for devs: -DWITH_FFMPEG=/path/to/prefix which disables version checks
    nothing else needed to build with whatever (old) version you want
  • existing option FFMPEG_PATH retains version checks

jenkins build this please

Copy link
Member

left a comment

cmake wise it looks good to me, not yet tested

# usage: -DWITH_FFMPEG=/path/to/ffmpeg_install_prefix
if(WITH_FFMPEG)
set(FFMPEG_PATH ${WITH_FFMPEG})
message(STATUS "Warning: FFmpeg version checking disabled")

This comment has been minimized.

Copy link
@fetzerch

fetzerch Nov 24, 2016

Member

message(WARING)?

This comment has been minimized.

Copy link
@wsnipex

wsnipex Nov 24, 2016

Author Member

I had that first, but don't really like the cmake Warning. It looks too much like an error.

list(APPEND CMAKE_PREFIX_PATH ${FFMPEG_PATH})
endif()

# FFMPEG v3.1

This comment has been minimized.

Copy link
@fetzerch

fetzerch Nov 24, 2016

Member

remove the comment as we have the versions at the top?

option(FFMPEG_PATH "Path to external ffmpeg?" "")
option(ENABLE_INTERNAL_CROSSGUID "Enable internal crossguid?" ON)
option(ENABLE_OPENSSL "Enable OpenSSL?" ON)
option(ENABLE_SDL "Enable SDL?" OFF)
if(CORE_SYSTEM_NAME STREQUAL linux OR CORE_SYSTEM_NAME STREQUAL freebsd)
option(ENABLE_X11 "Enable X11 support?" ON)
option(ENABLE_AML "Enable AML?" OFF)
option(ENABLE_INTERNAL_FFMPEG "Enable internal ffmpeg?" OFF)

This comment has been minimized.

Copy link
@fetzerch

fetzerch Nov 24, 2016

Member

The module is quite difficult to read, maybe it's a good idea to have a comment in the module that explains the default behavior. Assuming I build on linux without depends installed, it would now try to find ffmpeg 3.1, and because I don't have that installed, it would build the kodi ffmpeg version for me, correct?

This comment has been minimized.

Copy link
@wsnipex

wsnipex Nov 24, 2016

Author Member

correct. Will add a comment

@MilhouseVH MilhouseVH referenced this pull request Nov 24, 2016
@wsnipex wsnipex force-pushed the wsnipex:ffmpeg branch from ff125c7 to e7a10c6 Nov 25, 2016
@wsnipex

This comment has been minimized.

Copy link
Member Author

commented Nov 25, 2016

addressed comments
one last time: jenkins build this please

@wsnipex

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2016

heads up: I'll merge this in 24h.

@wsnipex wsnipex merged commit 0b822d6 into xbmc:master Nov 27, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins4kodi You did a great job. Have a cookie.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.