Skip to content

Conversation

@brad-richardson
Copy link
Contributor

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:

TODO:

Issues Fixed or Closed

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

@CLAassistant
Copy link

CLAassistant commented Nov 18, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@ReenigneArcher ReenigneArcher left a 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.

  1. Can we add the cbs stuff as a submodule? Seems weird if the files are just copied and pasted into here.
  2. 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.
  3. I'm not 100% sure if the version in our ffmpeg-prebuilt repo is ffmpeg5 or not, but I'd bet it is due to the published date.
  4. ffmpeg-prebuilt should 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.
  1. 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.

Comment on lines 3 to 5
#ifdef _WIN32
#define CURL_STATICLIB
#endif
Copy link
Member

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.

Copy link
Contributor

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.

@ReenigneArcher
Copy link
Member

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

@brad-richardson brad-richardson mentioned this pull request Nov 21, 2022
10 tasks
@brad-richardson
Copy link
Contributor Author

Closing in favor of #509

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants