CasparCG / server

CasparCG Server is a Windows and Linux software used to play out professional graphics, audio and video to multiple outputs. It has been in 24/7 broadcast production since 2006. Ready-to-use downloads are available under the Releases tab https://casparcg.com.
GNU General Public License v3.0
914 stars 268 forks source link

Question: Bumping C++ version to c++17 #1478

Closed jpc0 closed 1 year ago

jpc0 commented 1 year ago

Description

Just a general question regarding whether there is any current blockers to bumping the language version to C++17. It is currently at C++14.

Solution suggestion

Move to C++17

Julusian commented 1 year ago

Is there a particular reason to move to c++17? Do you have a change planned that would benefit from it?

I'm not opposed to requiring it, there simply hasn't been a reason to

jpc0 commented 1 year ago

@Julusian Not necessarily. Was just digging into the ffmpeg producer again and couldn't use a structured bindings when looking throught the code.

Seems like the only blocker is a bunch of std::auto_ptr in the common project that seem to be able to be replaced with unique_ptr, that makes it build anyway, and from what I've found online if there isn't any compile errors it "should" be right. Someone who knows more about the differences between what auto_ptr provided vs unique_ptr might be able to comment in.

We should likely be changing those auto_ptr regardless since they are only in the single place in the codebase and are concidered obsolete...

If I have a compelling reason I'll post an actual issue to bump it.

Another question, if it were to be bumped, would the approach there to be set cxx_std_17 per target to target_compile_features which is the current cmake recommendation or use the older globals?

Would we also be disabling compiler extensions? Not sure if any of the current code makes use of them, it builds with them disabled but with the amount of UB in C++ I don't think that is necessarily a glowing recommendation.

Julusian commented 1 year ago

I'm not seeing any usages of std::auto_ptr in master. There might be some in the version of boost we are using though? I do expect that boost will need updating to support c++17, the version we are using is a bit old. As long as we still support older versions, I don't mind that being updated. We just need to retain compatability for the versions used in ubuntu 22.04.

Another question, if it were to be bumped, would the approach there to be set cxx_std_17 per target to target_compile_features which is the current cmake recommendation or use the older globals?

I don't know enough about cmake to have a preference either way. Probably best to go with their recommendations, but that depends on the version of cmake the new version requires.

Would we also be disabling compiler extensions?

What compile extensions is that?

dimitry-ishenko commented 1 year ago

C++17 has a few nice features that Caspar can benefit from:

While there may not be an immediate reason to switch, this can help with code update/maintenance in the future.

Just my 2 cents.

Julusian commented 1 year ago

yeah some of those things look like they would be nice. still, it requires someone to update the code to use them ;)

dimitry-ishenko commented 1 year ago

still, it requires someone to update the code to use them ;)

You have a very strong argument there. 😃 On the other hand, I don't think there's been any ABI changes since C++11, so bumping standard version to C++17 (or even C++20, if you ask me) shouldn't break any existing code... theoretically... 😵‍💫

jpc0 commented 1 year ago

I'm not seeing any usages of std::auto_ptr in master. There might be some in the version of boost we are using though? I do expect that boost will need updating to support c++17, the version we are using is a bit old. As long as we still support older versions, I don't mind that being updated. We just need to retain compatability for the versions used in ubuntu 22.04.

Another question, if it were to be bumped, would the approach there to be set cxx_std_17 per target to target_compile_features which is the current cmake recommendation or use the older globals?

I don't know enough about cmake to have a preference either way. Probably best to go with their recommendations, but that depends on the version of cmake the new version requires.

Would we also be disabling compiler extensions?

What compile extensions is that?

Very well may have been in boost however I did get build errors... Boost version 1.66 claims it's tested on C++17 so it shouldn't have any build errors. I'll double check

yeah some of those things look like they would be nice. still, it requires someone to update the code to use them ;)

This was my primary reason for only actually upgrading when a feature is needed. As I said I put in structured bindings to clarify while I was reading through the code however they aren't needed in the codebase.

If there is an actual feature that uses them it would be possible to bump the version on only the project in cmake, like just bumping the ffmpeg module to C++17 while keeping the rest C++14 as an interim measure.

jpc0 commented 1 year ago

Locale:

Added support of unique_ptr interface in addition to C++2003 auto_ptr - in order to support C++2017, now you can use BOOST_LOCALE_HIDE_AUTO_PTR definiton to remove auto_ptr from the interfaces and prevent deprecated watnings.

From boost 1.67.0 so seems like this was fixed in version 1.67 of boost.

jpc0 commented 1 year ago

Would we also be disabling compiler extensions?

What compile extensions is that?

Was referring to this property in cmake https://cmake.org/cmake/help/latest/prop_tgt/CXX_EXTENSIONS.html#prop_tgt:CXX_EXTENSIONS. I'm not sure any of the extensions are currently being used, it's a large code base though and there are a large amount of extensions depending on the compiler.

Everything builds with it set to off but as I mentioned earlier I don't feel very confident in that.

jpc0 commented 1 year ago

It does look like at least in the linux build that are manually set to off in a way note even old cmake recommended...

https://github.com/CasparCG/server/blob/6483b20e1368b7f8727ebc06331734bb3e791cfd/src/CMakeModules/Bootstrap_Linux.cmake#L80

Julusian commented 1 year ago

or even C++20, if you ask me

As long as it still works with the compiler on Ubuntu 20.04 I don't have a problem with this.

If there is an actual feature that uses them it would be possible to bump the version on only the project in cmake, like just bumping the ffmpeg module to C++17 while keeping the rest C++14 as an interim measure.

I think it would be better to have everything using the same, unless we have a compelling reason to keep one lower. Otherwise I fear it will discourage use of newer features as we don't want to risk breaking something by changing it

It does look like at least in the linux build that are manually set to off in a way note even old cmake recommended...

Haha, yeah I don't think we've had a cmake expert here for many years (if ever), so the cmake files are likely full of all kinds of smells. Just that linux and windows have entirely separate flows is pretty suspicious.
So any changes to tidy up the cmake is appreciated :)

jpc0 commented 1 year ago

I'm by no means an expert. Just know that manually adding that flag when there are two different ways to do it using cmake features definitely is an issue.

jpc0 commented 1 year ago

Regarding C++20 as far as I know support is still kind of spotty and as far as I know 20.04 does not support it, at least some of the debian builds using unstable debian doesn't support C++20 in the system compilers.

dimitry-ishenko commented 1 year ago

FWIW gcc-10 is available on Ubuntu 20.04: https://packages.ubuntu.com/focal/devel/gcc-10

and it supports most of the core C++20: https://en.cppreference.com/w/cpp/compiler_support#cpp20 as well most of the C++20 library features: https://en.cppreference.com/w/cpp/compiler_support#C.2B.2B20_library_features

Plus, hopefully people start upgrading to 22.04 or even 24.04, which is just around the corner 😉

dimitry-ishenko commented 1 year ago

Haha, yeah I don't think we've had a cmake expert here for many years (if ever), so the cmake files are likely full of all kinds of smells. Just that linux and windows have entirely separate flows is pretty suspicious. So any changes to tidy up the cmake is appreciated :)

Years ago I've done some CMake clean-ups but I don't think they've ever been merged into the main branch. I have given up on Caspar since then, but if there is interest I might put in some work on it when we start upgrading our playout machine (in the next few months hopefully).

Julusian commented 1 year ago

2.4.0 hasnt been released yet, so we could easily drop support for 20.04 if that would be beneficial. I'm not attached to keeping it, and am unlikely to ever attempt to run it on there myself.

On an unrelated note, but maybe kinda related to cmake things, I am considering whether the caspar linux builds should be distributed as a deb file, and properly rely on system libraries. The current linux builds did not run when I last tried one on 20.04, and the so file copying has been such a pain.
I'm not sure if/when I will get around to it though, and don't really have any experience on it. But hopefully can base it on what they have done for debian https://salsa.debian.org/multimedia-team/casparcg-server/-/tree/master/debian?

dimitry-ishenko commented 1 year ago

2.4.0 hasnt been released yet, so we could easily drop support for 20.04 if that would be beneficial.

I am considering whether the caspar linux builds should be distributed as a deb file

I am in favor of both.

and properly rely on system libraries.

I've been preaching that for years and had a fork of the server that built against system libs on Linux. But, none of my PRs ever got merged and have rotten into oblivion. 😅

Julusian commented 1 year ago

I've been preaching that for years and had a fork of the server that built against system libs on Linux. But, none of my PRs ever got merged and have rotten into oblivion. sweat_smile

It almost all is these days. Just cef, ffmpeg and boost are not taken from the system.

dimitry-ishenko commented 1 year ago

It almost all is these days. Just cef, ffmpeg and boost are not taken from the system.

oscpack, GLEW, TBB and OpenAL are the other ones.

Maybe we could recommend or rely on a ppa for this.

I like that. I already have one here: https://launchpad.net/~ppa-verse/+archive/ubuntu/cling where I host a few things. We can start with that for testing purposes and then make an official one for CasparCG.

There are already .deb packages for GLEW, TBB, OpenAL and Boost. Not sure if they are the right versions. We can separately package up CEF and FFmeg of whatever versions we need.

I am familiar with Debian workflow and can put in some time on that in about a month or so, as long as my patches don't end up bit-rotting again. 😸

Julusian commented 1 year ago

oscpack, GLEW, TBB and OpenAL are the other ones.

oh yes, oscpack is true. the rest are taken from the system, the files in https://github.com/CasparCG/server/tree/master/src/packages are windows only

We can start with that for testing purposes and then make an official one for CasparCG.

I'm not sure if we want/need a ppa/apt repository. We don't do releases particularly often, and even when we do, users wont want to be surprised by a new version being potentially auto-installed. I'm not decided either way yet though.

We can separately package up CEF and FFmeg of whatever versions we need.

I'm not entirely sure if it makes more sense to have CEF as part of the main deb. I think it will mostly depend on if we do go the ppa route. If not it would be easier for users to not have to worry about a second deb file, which will have quite strict version rules.

I am familiar with Debian workflow and can put in some time on that in about a month or so, as long as my patches don't end up bit-rotting again. smile_cat

That would be great. I've had a bit of a play and do feel rather lost. I have cpack generating a deb file with just the executable in it, and Im wondering if theres a way to do these install() rules without duplicating the staging rules we already have and will need to be able to run the build without installing it. But I have no idea what to google for this, so havent found anything helpful.
These rules are already a mess, defined completely differently for windows and linux so would be really nice to not have to add a third version of them. I'm going to put this down for now.

jpc0 commented 1 year ago

It almost all is these days. Just cef, ffmpeg and boost are not taken from the system.

oscpack, GLEW, TBB and OpenAL are the other ones.

I have a branch in my github where glew is replaced with glad, since glew is currenty not recommended and Caspar isn't running the latest version as is published on github vs what is on the sourceforge page.

It may need some work on the linux build side though since they are setting manual flags on all projects in cmake where glad is threoretically a C lib and that is breaking its build on linux... Could possibly just use glad as header only there...

dimitry-ishenko commented 1 year ago

I'm not sure if we want/need a ppa/apt repository. We don't do releases particularly often, and even when we do, users wont want to be surprised by a new version being potentially auto-installed. I'm not decided either way yet though.

Versioning is not a problem. We can add version number into the server's package name. Like Ubuntu does with eg gcc. There is generic gcc package, which depends on gcc-11 (on 22.04 at least) and then there are gcc-9, gcc-10, gcc-11 and gcc-12 packages if you need a specific version.

The beauty of having own PPA is you have full control of the release cycle and dependencies.

Also, people who are afraid of breaking things, should just pin their packages so they doesn't update.

I'm not entirely sure if it makes more sense to have CEF as part of the main deb. I think it will mostly depend on if we do go the ppa route. If not it would be easier for users to not have to worry about a second deb file, which will have quite strict version rules.

Monolithic .deb is not a good solution IMHO, as it goes against the Unix philosophy.

Say there is a bug or security fix in CEF or FFmpeg. Now you have to recompile the server package and put out a new .deb. To do that you have to bump its version. And then people may think there was a change in the server, when there wasn't. And then how do you tell people that they should grab new version?

Julusian commented 1 year ago

Also, people who are afraid of breaking things, should just pin their packages so they doesn't update.

True, We could just make that part of the readme/installation guide to recommend pinning it.

Monolithic .deb is not a good solution IMHO, as it goes against the Unix philosophy.

Say there is a bug or security fix in CEF or FFmpeg. Now you have to recompile the server package and put out a new .deb. To do that you have to bump its version. And then people may think there was a change in the server, when there wasn't. And then how do you tell people that they should grab new version?

For ffmpeg I think that separate makes sense, as it can be updated without issue. Many users will be happy enough with the distros default 4.4. Others will want a much newer version.

But for CEF, we are highly unlikely to want to publish a new version of it separately. In large because the CEF branches are only maintained for a few months. If we want any security patches beyond that we would need to backport them and compile CEF ourselves (doable, it just takes a couple of hours). Additionally, I'm not sure if CEF has a stable C api, so I don't know if it would even be possible to avoid that rebuild. There is some talk of a hash, so I guess it will refuse to run if mismatched, but no comments on whether that is guaranteed to be stable within a release branch. So out of caution, if we do have a separate CEF deb, I would want the casparcg deb to reference the exact version of it that it built against unless it can be proven unnecessary.

I have not thought about the scanner at all here.. Thats something else that should be done, and can definitely be its own package

Julusian commented 1 year ago

I have a branch in my github where glew is replaced with glad, since glew is currenty not recommended and Caspar isn't running the latest version as is published on github vs what is on the sourceforge page.

I don't have an opinion on glew vs glad vs something else. As long as it works on everything, and doesnt require committing any linux binary dependencies to the repository (ideally it should come from the os package manager).
For windows, we do have some dependencies committed with some replaced with ones on nuget. Some of those I published, so we can do more of that if needed

jpc0 commented 1 year ago

I don't have an opinion on glew vs glad vs something else. As long as it works on everything, and doesnt require committing any linux binary dependencies to the repository (ideally it should come from the os package manager). For windows, we do have some dependencies committed with some replaced with ones on nuget. Some of those I published, so we can do more of that if needed

I just built with glad to test whether GLEW wasn't causing https://github.com/CasparCG/server/issues/1450, it wasn't hence not pull request. I don't think GLEW needs to be replaced however glad can be a header only library in the STB fashion so it would end up not being a dep other than as a loader exactly as glew is not. It would however end up being built into the binary. Glad is also public domain... https://github.com/Dav1dde/glad I don't see a reason to change though, except for possibly switching to Vulcan but I'm not going to be the one writing all that code.

Something we might want to look into is bumping to glew 2.2.0 for windows, in linux it is the default from 22.04 but 20.04 is still using 2.1.0, not sure what changed since they didn't seem to do release notes for the version bump. https://github.com/nigels-com/glew

Julusian commented 1 year ago

Something we might want to look into is bumping to glew 2.2.0 for windows

There is a changelog hidden away there, but it appears they havent published the website https://github.com/nigels-com/glew/blob/master/doc/log.html#L101-L278

Yeah, might as well. It would be nice to get it out of the repository too, but finding maintained packages on nuget is a pain, I could create our own package for it instead though. It looks like we are currently on 2.1.0 based on when it was last updated here.

I did prototype using vcpkg instead of nuget for dependencies on windows a few months back, as packages there are much better maintained, but having to build ffmpeg from scratch makes it painful. I dont remember if I tried setting up binary caching or not. Or was it that the ffmpeg it produced was statically linked into the casparcg.exe that I didnt like?
I should probably pick this up again, and do it to avoid nuget for native libraries

Julusian commented 1 year ago

I don't see a reason to change though, except for possibly switching to Vulcan but I'm not going to be the one writing all that code.

Yeah, I don't think switching to vulkan or something will be trivial, and could be painful for some users as it will likely drop support for older gpus.

If anything were to be changed on that front, I would be very tempted by OpenCL instead, as we don't do much with geometry, and it would be simpler to do 10bit (or higher) colour and make non-rgb colorspaces easier

jpc0 commented 1 year ago

I would probably argue if there were to be a change on the GPU side to switch to an agnostic API, WebGPU or whatever that one google made. From what I know OpenGL development has stopped now that Vulkan has been released and I wouldn't be suprised if WebGPU makes some decent strides concidering creating cross platform code in it is pretty trivial.

For things I will probably look at in the near future it would probably be cleaning up some of the cmake stuff. From what I can tell if Ubuntu 20.04 is the minimum then we can bump cmake to 3.16 which will also give us access to their PCH api and we can get rid of the custom stuff that currently exists. Likewise getting rid of the ton of duplication between windows and linux builds would be great, possibly getting rid of all of it.

As mentioned in my other FFmpeg issue having some development documentation would be great. Architecture docs could be really useful. I haven't dug through the codebase a ton but what the actual code path from AMCP to where createproducer/consumer actually get's called seems to be behind more than one or two layers of abstraction, which is a necessary evil but an overview of that might help get new devs onboard.

Same as the still currently outstanding bug with ffmpeg where it had crashes when an incomplete or broken video get's played. That bug is entirely because there are 3 threads and 2 executors in there and it's all being syncronised with "fences" of sorts but exceptions are allowed to leak and are not getting caught.

If anything I would want to push C++20 just to get coroutine support in the future since it may actually make some of that code easier to abstract...

Julusian commented 1 year ago

we can bump cmake to 3.16 which will also give us access to their PCH api and we can get rid of the custom stuff that currently exists

did you notice my hack for pch on linux? https://github.com/CasparCG/server/blob/b0ecbeea48e9a9548199ed1d2cf0d241fa3695fa/src/CMakeModules/Bootstrap_Linux.cmake#L116-L118 ;)
Is it beneficial to be using them these days? having them disabled on linux hasn't bothered me at all.

For things I will probably look at in the near future it would probably be cleaning up some of the cmake stuff.

sounds good to me :)

If anything I would want to push C++20

gcc do still call their support experimental, so I'm not sure if we should go that far https://gcc.gnu.org/projects/cxx-status.html#cxx20

jpc0 commented 1 year ago

Apparently PCH speeds up build times? Maybe it's working since loading debug symbols the first time takes longer than building for me in VS...

Will try with them removed sometime.

jpc0 commented 1 year ago

gcc do still call their support experimental, so I'm not sure if we should go that far https://gcc.gnu.org/projects/cxx-status.html#cxx20

Was more tongue in cheek. I've found a lot of the C++20+ features that would be really nice have lackluster support

dimitry-ishenko commented 1 year ago

IMHO we should still bump C++ standard version to 20. MSCV has excellent support for it. And gcc is not too far off. By the time some gets around to using some of the more advanced C++20 features that are currently not implemented, they will most likely be implemented. But at least we won't have to talk about bumping from 17 to 20 in a few years from now.

dimitry-ishenko commented 1 year ago

Plus, if we set minimum Ubuntu version to 22.04 it's got gcc-11 as the default compiler and gcc-12 is available as well.

jpc0 commented 1 year ago

I think we should first prioritise fixing cleaning up the build system. Currently the manual compiler flags getting added actually cause some issues when trying to implement stuff.

Likewise would need to bump the cmake version up to at least 3.12 before we can even discuss C++20 since the current version, 3.10, doesn't have support for C++20.

dimitry-ishenko commented 1 year ago

We can do both. 😺 I will take a stab at it when I get a chance, but it won't be for a few weeks.

We are scheduling to upgrade our play-out box, and as part of it I will be able to spend some time on Caspar.

Julusian commented 1 year ago

After lookng into updating CEF, that requires c++17, so it is highly likely this will be a requirement soon

jpc0 commented 1 year ago

After lookng into updating CEF, that requires c++17, so it is highly likely this will be a requirement soon

I do believe the last time I tried that CasparCG does currently compile with C++17 but I could be mistaken.

Julusian commented 1 year ago

@jpc0 yes it compiles with it fine, and now the cmake files specify target_compile_features(accelerator PRIVATE cxx_std_17)