-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Update to ffmpeg5 #503
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
Update to ffmpeg5 #503
Conversation
ReenigneArcher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this PR!
In addition the specific line comments, I have a few more.
- Can we add the
cbsstuff as a submodule? Seems weird if the files are just copied and pasted into here. - The #402 PR you mentioned is just moving to ffmpeg5 for the AUR package. It's not ready to use and I don't really have time to play with it much. Ideally, all the ffmpeg5 updates could be moved to your PR. Macports build is already using ffmpeg5 I believe.
- I'm not 100% sure if the version in our
ffmpeg-prebuiltrepo is ffmpeg5 or not, but I'd bet it is due to the published date. ffmpeg-prebuiltshould probably have some attention given to it, to:
- Have a more consistent workflow/build process.
- Ensure the Linux version is sufficient for Sunshine needs.
- Actually use the Linux version.
- Add a macOS version, and use it as well.
- Can we statically link ffmpeg (and maybe all dependencies)? Users are going to have a tough time with a linux distro that doesn't include ffmpeg5 if we don't.
src/httpcommon.cpp
Outdated
| #ifdef _WIN32 | ||
| #define CURL_STATICLIB | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused why this is needed since your changes don't affect that. I guess it's an isolated build issue? But if it improves the build I'm okay with keeping it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This define is probably necessary, but as of the merging of #216, it's already added via cmake through ${CURL_STATIC_CFLAGS}: https://github.com/LizardByte/Sunshine/blob/nightly/CMakeLists.txt#L94
This should therefore be unnecessary on latest nightly.
|
By the way, I was also investigating moving ffmpeg (and potentially other dependency) building over to this repo (https://github.com/LizardByte/build-deps). See here: https://github.com/LizardByte/build-deps/blob/master/.github/workflows/build-ffmpeg.yml (just Linux so far, and don't know if it has everything required for Sunshine). If you're on our discord server, we can discuss some things there a little easier. https://app.lizardbyte.dev/discord |
4d9d037 to
e9d8806
Compare
|
Closing in favor of #509 |
Description
Attempt at moving at least Windows to ffmpeg5, most of these changes were pulled from #50. I additionally updated all the CBS headers to the latest from the ffmpeg5 branch.
Notes:
#define CURL_STATICLIB, not sure why that became a problem but found that solution here: https://stackoverflow.com/questions/54587853/c-curl-undefined-reference-to-imp-curl-easy-init-codeblocksTODO:
Issues Fixed or Closed
Type of Change
Checklist