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

deps.ffmpeg: Add libdatachannel #170

Merged
merged 3 commits into from
May 30, 2023
Merged

Conversation

DDRBoxman
Copy link
Member

@DDRBoxman DDRBoxman commented Mar 25, 2023

Description

Adds libdatachannel to the ffmpeg deps allowing us to use webrtc in OBS.

Libdatachannel requires srtp to be enabled in mbedtls so we enable that here.

libdatachannel is compiled in the ffmpeg deps to cut down on complexity and due to the fact that there are some lingering issues with mbedtls combined with libdatachannel with the msvc compiler.

Motivation and Context

This will allow us to add WHIP, WHEP, and eventually maybe even build out webrtc apis for plugins.

Prerequisite for this PR:
obsproject/obs-studio#7926

How Has This Been Tested?

Types of changes

  • New feature (non-breaking change which adds 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.

@pkviet
Copy link
Member

pkviet commented Apr 3, 2023

@DDRBoxman
Here's a patch which does the following:

  • update mbedtls to 3.4.0
  • update datachannel to the 0.19.0 tag
  • fix the cmake files generated at install to be useable by the ps1 scripts (the cmake file refer to .dll.a import libs which work when cross-compiling; but for msvc we need the .lib files generated at fixup)
    When the datachannel ps1 script is run with 0.19.0 tag the compilation works and the dll loads without crashing in obs.
    However the whip connection fails after a while; the whep player doesn't play anything.
    obs-deps-pkv_changes.patch
    This other patch is for the datachannel ps1:
    obs-deps-pkv2_changes.patch

@DDRBoxman DDRBoxman changed the title [WIP] Add libdatachannel Add libdatachannel Apr 5, 2023
@DDRBoxman DDRBoxman marked this pull request as ready for review April 5, 2023 04:17
@DDRBoxman DDRBoxman changed the title Add libdatachannel deps.ffmpeg Add libdatachannel Apr 5, 2023
@DDRBoxman DDRBoxman force-pushed the libdatachannel branch 2 times, most recently from 01247ed to e829f9b Compare April 7, 2023 01:51
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squash the second commit since it's just reverting changes in the first one. Separate the prefix from the rest of the commit message subject with a colon.

@PatTheMav do we want to split this into individual commits to update+patch mbedTLS, add libdatachannel (or update mbedTLS, add mbedTLS patch and libdatachannel)? I usually prefer component updates are at least their own commits. Mostly unsure how to treat the added patch.

deps.ffmpeg/60-mbedtls.zsh Outdated Show resolved Hide resolved
deps.ffmpeg/70-libdatachannel.zsh Outdated Show resolved Hide resolved
deps.ffmpeg/70-libdatachannel.zsh Outdated Show resolved Hide resolved
@Sean-Der Sean-Der force-pushed the libdatachannel branch 2 times, most recently from 9dd779d to 7c3e63a Compare May 17, 2023 02:15
@Sean-Der
Copy link
Contributor

@RytoEX mind reviewing again? I split the PR into three commits and addressed all other comments.

  • Upgrade mbedTLS
  • Enable DTLS-SRTP with patch in mbedTLS
  • Add libdatachannel

@RytoEX RytoEX changed the title deps.ffmpeg Add libdatachannel deps.ffmpeg: Add libdatachannel May 17, 2023
@Sean-Der
Copy link
Contributor

Sean-Der commented May 17, 2023

@RytoEX mind reviewing again? I enabled POSIX with mingw since libdatachannel depends on it for c++11 threading.

@RytoEX
Copy link
Member

RytoEX commented May 17, 2023

@RytoEX mind reviewing again? I enabled POSIX with mingw since libdatachannel depends on it for c++11 threading.

I've asked @PatTheMav to assist in review. From what I recall of previous conversations, there should have been a successful build without -posix before (@pkviet tested it), and I'm not keen on just switching to -posix for our entire cross-compile build without at least some investigation.

@PatTheMav
Copy link
Member

A bit of background on the whole thing:

  • x86_64-w64-mingw32-g++-posix is an implementation of C++11's native threading features using GCC's gthreads/pthreads - this automatically adds MingW-w64's pthreads library (libwinpthread-1.dll) as a dependency for any generated DLL.
  • We default to x86_64-w64-mingw32-g++-win32 (the default alias for x86_64-w64-mingw32-g++) because we do not wish to distribute any MingW-w64 DLLs and ensure that native Win32-API features are used.

There are some complications around this:

  • aom and libvpx have built-in shims that emulate native pthread support using Win32 threads (no MingW-w64 trickery necessary)
  • For mbedTLS we effectively disable thread support on Windows to avoid any issues while cross-compiling
  • srt was the most troublesome of the whole bunch - it either has a hard requirement for pthreads (no matter the platform) or can use C++11 threads instead (provided natively by MSVC's toolchain). When cross-compiling this creates an obvious issue:
    • The created DLL will either directly or indirectly require winpthread to be shipped with them
  • librist also has a pthread shim that could be used - I don't remember the exact reason but I recall there were issues with the shim's symbols clashing with other libraries when compiling FFmpeg, which is why we elected not to use it anymore
  • FFmpeg supports pthreads as well as Win32 threads, we chose each based on the following criteria:
    • When compiling FFmpeg with shared 3rd party libraries, we use Win32 threads - any possible pthreads-based symbols (shims or native) don't leak outside of the DLLs and won't interfere with FFmpeg
    • When compiling FFmpeg with static 3rd party libraries, we use pthreads as some of the 3rd party library carry pthreads as a private usage requirement anyway - FFmpeg will link against pthread and we have to force MingW-w64 to use static linking (and just like with srt we have to hide all DLLs in the filesystem to make the compiler actually use static linking)

Given that libdatachannel will not be used by FFmpeg (and is just compiled as part of it as a workaround for the time being), it should be ok to use the -posix variants to enable use of C++11 threads in it. As far as I can tell the compile flags enforce static linking of pthreads (just like with srt) and also creating a DLL so its own pthreads symbols don't clash with libobs own w32-pthreads variant.

@PatTheMav
Copy link
Member

You also need to upstream a patch to libdatachannel:

The mbedtls finder uses objdump to find the SONAME of the library - this doesn't work with toolchains: Instead the finder needs to use CMAKE_OBJDUMP (which should usually contain the objdump binary of the selected toolchain) and the toolchain files need to be updated to use the variant provided by MingW-w64.

@Sean-Der
Copy link
Contributor

@PatTheMav Opened a PR against libdatachannel paullouisageneau/libdatachannel#889

@Sean-Der Sean-Der force-pushed the libdatachannel branch 2 times, most recently from 2fbf795 to cbde754 Compare May 19, 2023 14:06
@Sean-Der
Copy link
Contributor

@PatTheMav the patch has been upstreamed and merged. Could I get another review please and the workflow approved. Will debug if anything fails!

@Sean-Der Sean-Der force-pushed the libdatachannel branch 2 times, most recently from 6fa8cb0 to 78eb61f Compare May 22, 2023 02:42
@pkviet
Copy link
Member

pkviet commented May 24, 2023

Regarding the libdatachannel commit that is checked out:

  • I noticed an issue when using master on windows; there's a consistent 2 sec at start of a whip stream where frames are missed and dropped due to encoder lag.
  • the issue was bisected and I picked the last good commit for windows, which is therefore used in that PR.
  • the issue only occurs when building the libdatachannel dll with mingw and cross-compiling. I don't see the issue if libdatachannel is compiled on MSVC.
  • the reason for the issue is unknown but seems related to threading( the 'bad' libdatachannel commits have to do with threading).

@RytoEX RytoEX mentioned this pull request May 24, 2023
6 tasks
@RytoEX
Copy link
Member

RytoEX commented May 24, 2023

Regarding the libdatachannel commit that is checked out:
* I noticed an issue when using master on windows; there's a consistent 2 sec at start of a whip stream where frames are missed and dropped due to encoder lag.
* the issue was bisected and I picked the last good commit for windows, which is therefore used in that PR.
* the issue only occurs when building the libdatachannel dll with mingw and cross-compiling. I don't see the issue if libdatachannel is compiled on MSVC.
* the reason for the issue is unknown but seems related to threading( the 'bad' libdatachannel commits have to do with threading).

If there are specific commits in libdatachannel that are causing a regression, we can forcefully patch them out or revert them if needed in the patch step of the dependency script. It would be ideal if the issue could be fixed upstream though.

If I understand the conversation correctly, it is these changes to libjuice that are seemingly causing this issue of 2 seconds of blocking at the start of streams:
paullouisageneau/libdatachannel@d3938a7
paullouisageneau/libjuice@8c23cc8...0d8c966

The regression is being tracked at paullouisageneau/libdatachannel#892.

Is that about the sum of things? Has someone tried bisecting the libjuice changes to confirm which commit there causes the issue, or are we working off of a best guess?

@Sean-Der
Copy link
Contributor

@RytoEX Can we for now not take those future commits, since we don't need them? I will work with upstream and get them resolved. I don't have access to a Windows PC for ~2 weeks (when I return to work)

Talking with @pkviet I believe this PR is ready to be merged. If we can land the deps then we can work concurrently again. I will go work on upstream issue, while other devs address the obs-studio PR itself.

thank you

DDRBoxman and others added 3 commits May 24, 2023 23:19
Adds support for SRTP Protection profiles to be negotiated during DTLS
handshake. Needed for libdatachannel/WebRTC support.
Allows POSIX toolchain to be selectively used by some targets
Co-authored-by: pkv <pkv@obsproject.com>
@pkviet
Copy link
Member

pkviet commented May 25, 2023

If I understand the conversation correctly, it is these changes to libjuice that are seemingly causing this issue of 2 seconds of blocking at the start of streams:
paullouisageneau/libdatachannel@d3938a7
paullouisageneau/libjuice@8c23cc8...0d8c966

The regression is being tracked at paullouisageneau/libdatachannel#892.

Is that about the sum of things? Has someone tried bisecting the libjuice changes to confirm which commit there causes the issue, or are we working off of a best guess?

@RytoEX yes you understand correctly.
The regression appears here:
paullouisageneau/libjuice@09ce20f
It's unclear if it's a bug on libjuice side or a bug in winpthread since an msvc build doesn't display the 'bug'.

For now, leaving out the commits related to thread naming, doesn't pose any issues.
Note that i checked out to the last good commit, instead of reverting and i patched the lone commit merged after the 'thread name' commits.
For some reason i don't get, git revert -m 2 on the merge commit for the 'thread name' branch didn't work.

Some things to explore in the future:

  • we could move the building of libdatachannel to msvc,
  • we could try linking statically to libdatachannel.

For now, the dlls generated in this pr work fine along with the whip pr. Although i'd rather prefer the issue with 'thread names' fixed eventually, this PR is imo in a mergeable state.

@Sean-Der
Copy link
Contributor

@PatTheMav do my changes look good? Anything else I should be doing?

@PatTheMav
Copy link
Member

I don't think my question regarding shared/static has been fully answered.

@pkviet
Copy link
Member

pkviet commented May 28, 2023

I don't think my question regarding shared/static has been fully answered.

@PatTheMav
Libdatachannel could be built as static since presumably it won't be used by another module.
I tried on windows + msvc to build mbedtls statically and then libdatachannel. I had issues though at the install step to create the right cmake target files.
Same issue if I cross compile on wsl + mingw.
Since the dll in the current pr works, i don't want to spend more time on investigating how to improve things with a fully static build, being neither the author of the PR nor that of the whip PR, although out of curiosity I did provide some help since the main authors can't easily develop or test on windows.
I would wait till the new FFmpeg msvc workflow is merged to seek for a more satisfactory solution. For the time being it's not ideal but works fine enough imo.

@PatTheMav
Copy link
Member

Sounds good to me then.

@pkviet
Copy link
Member

pkviet commented May 28, 2023

The libdatachannel situation would be the same as librist and SRT, which have the same mbedtls dependence and are shared builds.

Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feedback addressed. This seems good to go.

@RytoEX RytoEX merged commit 74babd0 into obsproject:master May 30, 2023
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.

5 participants