-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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 WHEP Source #10353
base: master
Are you sure you want to change the base?
obs-webrtc: Add WHEP Source #10353
Conversation
|
||
rtc::Description answer(response, "answer"); | ||
try { | ||
peer_connection->setRemoteDescription(answer); |
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.
Hey @Sean-Der, thank you for getting these changes going - we're excited to see more support of WHIP/WHEP in OBS. We're hoping to see full spec support (maybe too much to fit in this PR) where Link
headers are configured with the peer_connection
to handle STUN/TURN configs. Unfortunately as it currently stands, lack of STUN/TURN handling is a blocker for our customers being able to use OBS with WebRTC (either WHIP or WHEP).
If it can't fit into this PR, we have resources available to coordinate with you to make that change in a future PR.
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 am working on that right now @0xcadams, I am going to try to get it done this week :)
When you get a chance mind reviewing the roadmap? Would love to hear if I have any other gaps I am missing.
I would be so grateful if I could get some dev support! Can you join the Real-time Broadcasting Discord that is where most coordination is taking place. I can split up/start the work right now!
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 do not think this PR should increase its scope further. Please keep its scope as-is unless you are fixing bugs.
videoMedia.addH264Codec(96); | ||
video_track = peer_connection->addTrack(videoMedia); | ||
|
||
auto video_depacketizer = std::make_shared<rtc::H264RtpDepacketizer>(); |
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.
Let me share an issue related to H264RtpDepacketizer
and a possible solution to it.
It turned out, in the following scenario, H264RtpDepacketizer
could crash during processing incoming packets:
- Connect OBS WHEP client to a SFU server.
- Publish a stream from Chrome (to the above waiting WHEP client via SFU).
- The WHEP client receives video packets having empty payload.
- I don't know why but Chrome seems to sometimes send such empty payload packets
H264RtpDepacketizer
crashes, and the following log message is emitted bylibdatachannel
rtc::impl::DtlsTransport::doRecv@598: DTLS recv: MbedTLS error: SSL - Internal error (eg, unexpected failure in lower-level module)
I'm not sure this is the best solution, but I created the following patch to filter out such empty payload video packets before H264RtpDepacketizer
processes those packets.
diff --git a/plugins/obs-webrtc/whep-source.cpp b/plugins/obs-webrtc/whep-source.cpp
index 2f6d5ebf2..026af0a0b 100644
--- a/plugins/obs-webrtc/whep-source.cpp
+++ b/plugins/obs-webrtc/whep-source.cpp
@@ -348,6 +348,38 @@ void WHEPSource::OnFrameVideo(rtc::binary msg, rtc::FrameInfo frame_info)
}
}
+class RTC_CPP_EXPORT VideoDepacketizer : public rtc::H264RtpDepacketizer {
+public:
+ VideoDepacketizer() = default;
+ virtual ~VideoDepacketizer() = default;
+
+ void incoming(rtc::message_vector &messages, const rtc::message_callback &send) override{
+ messages.erase(std::remove_if(messages.begin(), messages.end(),
+ [&](rtc::message_ptr message) {
+ if (message->type == rtc::Message::Control) {
+ return false;
+ }
+
+ if (message->size() < sizeof(rtc::RtpHeader)) {
+ return false;
+ }
+
+ auto pkt = message;
+ auto pktParsed = reinterpret_cast<const rtc::RtpHeader *>(pkt->data());
+ auto headerSize =
+ sizeof(rtc::RtpHeader) + pktParsed->csrcCount() + pktParsed->getExtensionHeaderSize();
+ if (message->size() <= headerSize) {
+ // Discard empty payload packet to prevent error in the following `rtc::H264RtpDepacketizer::incoming()` call.
+ return true;
+ }
+
+ return false;
+ }),
+ messages.end());
+ rtc::H264RtpDepacketizer::incoming(messages, send);
+ }
+};
+
void WHEPSource::SetupPeerConnection()
{
peer_connection = std::make_shared<rtc::PeerConnection>();
@@ -395,7 +427,7 @@ void WHEPSource::SetupPeerConnection()
videoMedia.addH264Codec(96);
video_track = peer_connection->addTrack(videoMedia);
- auto video_depacketizer = std::make_shared<rtc::H264RtpDepacketizer>();
+ auto video_depacketizer = std::make_shared<VideoDepacketizer>();
auto video_session = std::make_shared<rtc::RtcpReceivingSession>();
video_session->addToChain(video_depacketizer);
video_track->setMediaHandler(video_depacketizer);
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.
@sile Nice catch! Do you want me to submit bug fix to libdatachannel, or do you want to make PR?
I would love to have you involved/make the PR! We have so much work left, so I could use the help :)
If you don't have the time I can do it though!
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.
Thank you!
I can make the PR for libdatachannel
💪
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.
Created: paullouisageneau/libdatachannel#1140
5b2af9c
to
c55d178
Compare
@Sean-Der I encountered an issue with the latest commit where the video stream seems to pause for a while when the WHEP source receives a keyframe from the peer. diff --git a/plugins/obs-webrtc/whep-source.cpp b/plugins/obs-webrtc/whep-source.cpp
index 983c8b935..baf10fe03 100644
--- a/plugins/obs-webrtc/whep-source.cpp
+++ b/plugins/obs-webrtc/whep-source.cpp
@@ -294,7 +294,6 @@ void WHEPSource::OnFrameVideo(rtc::binary msg, rtc::FrameInfo frame_info)
if (naluType == naluTypeSPS || naluType == naluTypePPS) {
std::move(msg.begin(), msg.end(),
std::back_inserter(sps_and_pps));
- return;
} else if (this->sps_and_pps.size() != 0) {
std::copy(this->sps_and_pps.begin(), this->sps_and_pps.end(),
std::back_inserter(msg));
@@ -381,7 +380,7 @@ void WHEPSource::SetupPeerConnection()
auto audio_depacketizer = std::make_shared<rtc::OpusRtpDepacketizer>();
auto audio_session = std::make_shared<rtc::RtcpReceivingSession>();
- audio_session->addToChain(audio_depacketizer);
+ audio_depacketizer->addToChain(audio_session);
audio_track->setMediaHandler(audio_depacketizer);
audio_track->onFrame([&](rtc::binary msg, rtc::FrameInfo frame_info) {
this->OnFrameAudio(msg, frame_info);
@@ -394,7 +393,7 @@ void WHEPSource::SetupPeerConnection()
auto video_depacketizer = std::make_shared<rtc::H264RtpDepacketizer>();
auto video_session = std::make_shared<rtc::RtcpReceivingSession>();
- video_session->addToChain(video_depacketizer);
+ video_depacketizer->addToChain(video_session);
video_track->setMediaHandler(video_depacketizer);
video_track->onFrame([&](rtc::binary msg, rtc::FrameInfo frame_info) {
this->OnFrameVideo(msg, frame_info); |
@sile oops :/ I must have regressed that! I will rebase and re-land your changes sorry about that :( |
40de15b
to
0b508c0
Compare
Move logic that will be used by WHIP/WHEP into a helper file
Co-authored-by: Kevin Wang <[email protected]
I've been testing this branch as a part of a workflow where OBS broadcasts via WHIP and then pulls a processed stream via WHEP which can then be pulled into another video editing app via NDI. The workflow has been working great! The only small issue I've found thus far is with missing HTTP headers on the offer request and I pushed a PR with a fix Sean-Der#11. I did notice that the UI requires the Bearer Token field to be non-empty for a WHEP Source (I think this is enforced here). I don't think this is a problem per se, but I did find the behavior surprising because I can configure a WHIP Service with an empty Bearer Token field. Might be worth considering allowing an empty Bearer Token field in both a WHIP Service/WHEP Source just to be consistent. Would love to see this in the next OBS release - happy to help with anything that would help with moving this PR forward! |
Description
This PR adds a WHEP (WebRTC) source. Users will be provided an option and can input a URL + StreamKey. It will then pull the audio + video as a new source. I have attached screenshots to show the new menu option and configuration.
This PR moves much of the logic in the WHIP output to a shared file. Would it be better to split that generalization into its own commit?
Motivation and Context
This change solves these problems for streamers
Podcasting/Conferencing inside of OBS Using WebRTC streamers don't have to leave OBS anymore to build co-streaming. Users can publish their webcam via WHIP, and then pull the via WHEP the users they want to stream with. The WHEP sources can also be generated from non-OBS tools. You could pull a WHEP source that is generated by a guest that is just using the browser.
The quality of these broadcasts will be much higher then screen recording conferencing software. You won't have multiple rounds of encode/decode/composite that add generational loss.
Easier to build multi-computer streaming setups Users can play their game and capture on one computer. Then stream to another computer in their network build the final stream. The low latency of WebRTC means streamers will have less delay between gameplay and commentary. The P2P model of WebRTC also makes this easier.
How Has This Been Tested?
I have tested it against Broadcast Box on Linux+Mac
Types of changes
Checklist: