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

obs-webrtc: Add Link Header support #10524

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sean-Der
Copy link
Contributor

Description

WHIP/WHEP allows ICE Servers to be specified via Link Headers

Motivation and Context

Some WebRTC deployments are behind a NAT. Companies/developers place their TURN server on the edge, and use that to access the TURN servers.

I have had 3 companies reach out to me about this, so implemented it.

How Has This Been Tested?

Ran broadcast-box and only listened on 127.0.0.1 and confirmed that traffic could be routed through pion/turn

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.

@iameli
Copy link

iameli commented Apr 12, 2024

I just tested this against OBS Studio and it seems to be working flawlessly.

@WizardCM WizardCM added the New Feature New feature or plugin label Apr 13, 2024
@WizardCM WizardCM changed the title obs-webrtc: Add Link Header suppor obs-webrtc: Add Link Header support Apr 14, 2024
@Sean-Der Sean-Der force-pushed the webrtc-turn branch 2 times, most recently from 654f876 to 4fe9a05 Compare April 16, 2024 16:04
@Sean-Der Sean-Der marked this pull request as ready for review May 14, 2024 20:04
@Sean-Der
Copy link
Contributor Author

@tt2468 Could I get a review of this one now please?

@Sean-Der Sean-Der force-pushed the webrtc-turn branch 2 times, most recently from 6edce9e to b986f28 Compare May 24, 2024 16:36
@Sean-Der
Copy link
Contributor Author

I have created a test application that confirms this is working. You can get it here https://github.com/Sean-Der/whip-turn-test

The only thing you need to change is the IP address of the host you are running the server on here

OBS without this patch running against this server behaves like this. It shows WHIP connected and starting pushing media.

Running WHIP server at http://localhost:8085
ICE Connection State has changed: checking
ICE Connection State has changed: connected
Getting incoming media audio/opus
Getting incoming media video/H264

Without this change the server prints the following. OBS doesn't have TURN support so is unable to connect

Running WHIP server at http://localhost:8085
ICE Connection State has changed: checking

Some users today who are using WHIP will see OBS unable to connect and have no idea why

Copy link
Member

@DDRBoxman DDRBoxman left a comment

Choose a reason for hiding this comment

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

Just a few comments, mostly around adding comments so this is easier to make sense of later.

plugins/obs-webrtc/whip-output.cpp Show resolved Hide resolved
plugins/obs-webrtc/whip-output.cpp Show resolved Hide resolved
plugins/obs-webrtc/whip-output.cpp Show resolved Hide resolved
plugins/obs-webrtc/whip-output.cpp Outdated Show resolved Hide resolved
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.

Posed a question, but feedback is not blocking.

plugins/obs-webrtc/whip-output.cpp Outdated Show resolved Hide resolved
@Sean-Der Sean-Der force-pushed the webrtc-turn branch 2 times, most recently from 92ca232 to 8192535 Compare June 3, 2024 03:38
WHIP/WHEP allows ICE Servers to be specified via Link Headers[0]

[0] https://www.ietf.org/archive/id/draft-ietf-wish-whip-13.html#name-stun-turn-server-configurat

Co-authored-by: Takeru Ohta <phjgt308@gmail.com>
@Sean-Der
Copy link
Contributor Author

Sean-Der commented Jun 3, 2024

Thank you so much @DDRBoxman @RytoEX I really appreciate the reviews :)

Copy link
Member

@DDRBoxman DDRBoxman left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Feature New feature or plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants