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

Add HTTP/2 and Brotli support #184

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

derrod
Copy link
Member

@derrod derrod commented May 22, 2023

Description

Adds brotli and nghttp2 libraries to curl for HTTP/2 and brotli compression support. Also switches curl to being a static library.

Motivation and Context

Currently all HTTP traffic OBS receives is uncompressed, which is wasting bandwidth. Additionally, we want to use HTTP/2 for multiplexing and asnychronous requests in the updater and possibly other parts of OBS.

How Has This Been Tested?

Built locally, but needs to be checked more thouroughly as updater changes are still in progress and we might need to build curl with a static VCRT for it (and thus probably twice in total), so the PR is a draft.

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Tweak (non-breaking change to improve existing functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@tytan652
Copy link
Contributor

Also switches curl to being a static library.

Unfortunately, OBS Studio is not the only binary relying on curl, some plugins do. So building statically is not a good thing, it will introduce duplication of the curl lib in various binaries.

@RytoEX
Copy link
Member

RytoEX commented May 22, 2023

Also switches curl to being a static library.

Unfortunately, OBS Studio is not the only binary relying on curl, some plugins do. So building statically is not a good thing, it will introduce duplication of the curl lib in various binaries.

Please be specific. What plugins rely on curl?

@tytan652
Copy link
Contributor

tytan652 commented May 22, 2023

What plugins rely on curl?

win-capture and rtmp-services, both use it through the file-updater.

@derrod
Copy link
Member Author

derrod commented May 22, 2023

Unfortunately, OBS Studio is not the only binary relying on curl, some plugins do. So building statically is not a good thing, it will introduce duplication of the curl lib in various binaries.

Honestly, not worth caring about. It's a few kilobytes or maybe a megabyte. But it's either that or compiling Curl and it's dependencies twice.

@derrod derrod force-pushed the curl-http2 branch 3 times, most recently from 2a75c82 to fb6f717 Compare May 24, 2023 10:07
@derrod
Copy link
Member Author

derrod commented Dec 11, 2023

Just to leave some notes on where this PR is at:

  • libcurl's cmake is annoying because it exports with absolute paths to dependent libraries (nghttp2/brotli)
    • Probably requires some patch to their cmake to fix
    • We'd probably have to write finders as well for use in OBS
  • Does compile and HTTP/2 works in OBS with minimal changes if you manually fix linking
  • static+shared is still annoying, libcurl itself would probably support both as separate targets but its dependencies might not, haven't investigated

@RytoEX
Copy link
Member

RytoEX commented Dec 12, 2023

Just to leave some notes on where this PR is at:
* libcurl's cmake is annoying because it exports with absolute paths to dependent libraries (nghttp2/brotli)
* Probably requires some patch to their cmake to fix
* We'd probably have to write finders as well for use in OBS
* Does compile and HTTP/2 works in OBS with minimal changes if you manually fix linking
* static+shared is still annoying, libcurl itself would probably support both as separate targets but its dependencies might not, haven't investigated

On libcurl, I would prefer that if it needs changes, that those changes are done on the curl repo. They release pretty frequently these days.

@derrod
Copy link
Member Author

derrod commented Dec 13, 2023

Based on some discussions off-thread we can probably use the dynamic CRT instead of static even for the updater, which makes things a little easier (see my standalone updater repo).

I don't even know where to start with the cmake changes or if upstream would even care about them.

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

Successfully merging this pull request may close these issues.

None yet

3 participants