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, deps.windows: Update libdatachannel to v0.20.1 #204

Merged
merged 1 commit into from
Jan 27, 2024

Conversation

Sean-Der
Copy link
Contributor

@Sean-Der Sean-Der commented Aug 3, 2023

How Has This Been Tested?

I have a working implementation here

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.

@troman123
Copy link

troman123 commented Sep 7, 2023

@Sean-Der
When should PR the v0.19.0 libdatachannel update to obs-deps.
thanks.

@Sean-Der Sean-Der force-pushed the update-libdatachannel branch from 3661460 to 75e8d67 Compare September 7, 2023 13:32
@Sean-Der
Copy link
Contributor Author

Sean-Der commented Sep 7, 2023

@troman123 done!

@Sean-Der Sean-Der changed the title deps.ffmpeg, deps.windows: Update libdatachannel to v0.19.0-alpha.6 deps.ffmpeg, deps.windows: Update libdatachannel to v0.19.0 Sep 7, 2023
@Sean-Der
Copy link
Contributor Author

Sean-Der commented Sep 7, 2023

@troman123 This probably won't be merged for a while. OBS 30 stabilization is happening now, and since this doesn't fix any bugs it won't be merged.

@RytoEX will have the actual direction on that.

@RytoEX
Copy link
Member

RytoEX commented Sep 7, 2023

@troman123 This probably won't be merged for a while. OBS 30 stabilization is happening now, and since this doesn't fix any bugs it won't be merged.

@RytoEX will have the actual direction on that.

@Sean-Der has the right of it. We'll circle back around to non-bug-fix PRs sometime after OBS Studio 30 stabilizes.

@Sean-Der
Copy link
Contributor Author

Sean-Der commented Sep 7, 2023

@RytoEX No rush, but mind re-running eventually?

Looks like sourceforge is having issues? Lots of things failing to download with curl: (22) The requested URL returned error: 500

@troman123
Copy link

@Sean-Der thanks, OBS 30 stabilization has the highest priority.

@Sean-Der Sean-Der force-pushed the update-libdatachannel branch 2 times, most recently from 90cd6ab to 45c896b Compare January 8, 2024 01:15
@Sean-Der
Copy link
Contributor Author

Sean-Der commented Jan 8, 2024

@troman123 @RytoEX Would you mind running the tests?

This fixes obsproject/obs-studio#9846 now

@tytan652
Copy link
Contributor

tytan652 commented Jan 8, 2024

Nitpik, update the Pull Request name.

@tytan652
Copy link
Contributor

tytan652 commented Jan 8, 2024

Another nitpik (sorry), this PR does not fix directly obsproject/obs-studio#9846, the Flatpak manifest needs to be updated once a new obs-deps is released and put in use.

@Sean-Der Sean-Der changed the title deps.ffmpeg, deps.windows: Update libdatachannel to v0.19.0 deps.ffmpeg, deps.windows: Update libdatachannel to v0.19.5 Jan 8, 2024
@Sean-Der
Copy link
Contributor Author

Sean-Der commented Jan 8, 2024

Done! @tytan652 are you able to approve the workflow for tests?

@tytan652
Copy link
Contributor

tytan652 commented Jan 8, 2024

@tytan652 are you able to approve the workflow for tests?

No, I'm just a contributor on obs-deps.

Sean-Der added a commit to Sean-Der/libdatachannel that referenced this pull request Jan 11, 2024
After 7591b96 libdatachannel stopped working with OBS on macOS[0].
XCode only resolves one level of symlinks [1]. libdatachannel
is generating two levels.

This commit updates CMake to instead use ELF Headers instead of file
names to communicate version of macOS.

[0] https://stackoverflow.com/questions/29946961/xcode-copy-files-build-phase-and-symlinks
[1] obsproject/obs-deps#204 (comment)
Sean-Der added a commit to Sean-Der/libdatachannel that referenced this pull request Jan 11, 2024
After 7591b96 libdatachannel stopped working with OBS on macOS[0].
XCode only resolves one level of symlinks [1]. libdatachannel
is generating two levels.

This commit updates CMake to instead use ELF Headers instead of file
names to communicate version of macOS.

[0] https://stackoverflow.com/questions/29946961/xcode-copy-files-build-phase-and-symlinks
[1] obsproject/obs-deps#204 (comment)
Sean-Der added a commit to Sean-Der/libdatachannel that referenced this pull request Jan 11, 2024
After 7591b96 libdatachannel stopped working with OBS on macOS[0].
XCode only resolves one level of symlinks [1]. libdatachannel
is generating two levels.

This commit updates CMake to instead use ELF Headers instead of file
names to communicate version of macOS.

[0] https://stackoverflow.com/questions/29946961/xcode-copy-files-build-phase-and-symlinks
[1] obsproject/obs-deps#204 (comment)
Sean-Der added a commit to Sean-Der/libdatachannel that referenced this pull request Jan 11, 2024
After 7591b96 libdatachannel stopped working with OBS on macOS[0].
XCode only resolves one level of symlinks [1]. libdatachannel
is generating two levels.

This commit updates CMake to instead use ELF Headers instead of file
names to communicate version of macOS.

[0] https://stackoverflow.com/questions/29946961/xcode-copy-files-build-phase-and-symlinks
[1] obsproject/obs-deps#204 (comment)
Sean-Der added a commit to Sean-Der/libdatachannel that referenced this pull request Jan 11, 2024
After 7591b96 libdatachannel stopped working with OBS on macOS[0].
XCode only resolves one level of symlinks [1]. libdatachannel
is generating two levels.

This commit updates CMake to instead use ELF Headers instead of file
names to communicate version of macOS.

[0] https://stackoverflow.com/questions/29946961/xcode-copy-files-build-phase-and-symlinks
[1] obsproject/obs-deps#204 (comment)
Sean-Der added a commit to Sean-Der/libdatachannel that referenced this pull request Jan 13, 2024
After 7591b96 libdatachannel stopped working with OBS on macOS[0].
XCode only resolves one level of symlinks [1]. libdatachannel
is generating two levels.

This commit updates the shared object to only use only the major version
in the name.

[0] https://stackoverflow.com/questions/29946961/xcode-copy-files-build-phase-and-symlinks
[1] obsproject/obs-deps#204 (comment)
@Sean-Der Sean-Der force-pushed the update-libdatachannel branch from 45c896b to 42656ca Compare January 20, 2024 20:50
@Sean-Der Sean-Der changed the title deps.ffmpeg, deps.windows: Update libdatachannel to v0.19.5 deps.ffmpeg, deps.windows: Update libdatachannel to v0.20.0 Jan 20, 2024
@Sean-Der Sean-Der force-pushed the update-libdatachannel branch from 42656ca to 88af6c3 Compare January 20, 2024 20:52
@Sean-Der
Copy link
Contributor Author

@RytoEX This PR has been updated to be v0.20.0 instead

@Sean-Der
Copy link
Contributor Author

v0.20.0 marked some APIs we used deprecated. To fix those warnings apply this patch

--- a/plugins/obs-webrtc/whip-output.cpp
+++ b/plugins/obs-webrtc/whip-output.cpp
@@ -112,16 +112,14 @@ void WHIPOutput::ConfigureAudioTrack(std::string media_stream_id,
 
 	auto rtp_config = std::make_shared<rtc::RtpPacketizationConfig>(
 		ssrc, cname, audio_payload_type,
-		rtc::OpusRtpPacketizer::defaultClockRate);
+		rtc::OpusRtpPacketizer::DefaultClockRate);
 	auto packetizer = std::make_shared<rtc::OpusRtpPacketizer>(rtp_config);
 	audio_sr_reporter = std::make_shared<rtc::RtcpSrReporter>(rtp_config);
 	auto nack_responder = std::make_shared<rtc::RtcpNackResponder>();
 
-	auto opus_handler =
-		std::make_shared<rtc::OpusPacketizationHandler>(packetizer);
-	opus_handler->addToChain(audio_sr_reporter);
-	opus_handler->addToChain(nack_responder);
-	audio_track->setMediaHandler(opus_handler);
+	packetizer->addToChain(audio_sr_reporter);
+	packetizer->addToChain(nack_responder);
+	audio_track->setMediaHandler(packetizer);
 }
 
 void WHIPOutput::ConfigureVideoTrack(std::string media_stream_id,
@@ -148,11 +146,9 @@ void WHIPOutput::ConfigureVideoTrack(std::string media_stream_id,
 	video_sr_reporter = std::make_shared<rtc::RtcpSrReporter>(rtp_config);
 	auto nack_responder = std::make_shared<rtc::RtcpNackResponder>();
 
-	auto h264_handler =
-		std::make_shared<rtc::H264PacketizationHandler>(packetizer);
-	h264_handler->addToChain(video_sr_reporter);
-	h264_handler->addToChain(nack_responder);
-	video_track->setMediaHandler(h264_handler);
+	packetizer->addToChain(video_sr_reporter);
+	packetizer->addToChain(nack_responder);
+	video_track->setMediaHandler(packetizer);
 }

@tytan652
Copy link
Contributor

tytan652 commented Jan 20, 2024

Just for reference since obs-deps is not related to Linux builds.
Libdatachannel 0.20.0 is not buildable on Linux since it is not compatible with a stable release of usrsctp.

Edit: Upstream issue paullouisageneau/libdatachannel#1100

@RytoEX
Copy link
Member

RytoEX commented Jan 20, 2024

v0.20.0 marked some APIs we used deprecated. To fix those warnings apply this patch

--- a/plugins/obs-webrtc/whip-output.cpp
+++ b/plugins/obs-webrtc/whip-output.cpp
@@ -112,16 +112,14 @@ void WHIPOutput::ConfigureAudioTrack(std::string media_stream_id,
 
 	auto rtp_config = std::make_shared<rtc::RtpPacketizationConfig>(
 		ssrc, cname, audio_payload_type,
-		rtc::OpusRtpPacketizer::defaultClockRate);
+		rtc::OpusRtpPacketizer::DefaultClockRate);
 	auto packetizer = std::make_shared<rtc::OpusRtpPacketizer>(rtp_config);
 	audio_sr_reporter = std::make_shared<rtc::RtcpSrReporter>(rtp_config);
 	auto nack_responder = std::make_shared<rtc::RtcpNackResponder>();
 
-	auto opus_handler =
-		std::make_shared<rtc::OpusPacketizationHandler>(packetizer);
-	opus_handler->addToChain(audio_sr_reporter);
-	opus_handler->addToChain(nack_responder);
-	audio_track->setMediaHandler(opus_handler);
+	packetizer->addToChain(audio_sr_reporter);
+	packetizer->addToChain(nack_responder);
+	audio_track->setMediaHandler(packetizer);
 }
 
 void WHIPOutput::ConfigureVideoTrack(std::string media_stream_id,
@@ -148,11 +146,9 @@ void WHIPOutput::ConfigureVideoTrack(std::string media_stream_id,
 	video_sr_reporter = std::make_shared<rtc::RtcpSrReporter>(rtp_config);
 	auto nack_responder = std::make_shared<rtc::RtcpNackResponder>();
 
-	auto h264_handler =
-		std::make_shared<rtc::H264PacketizationHandler>(packetizer);
-	h264_handler->addToChain(video_sr_reporter);
-	h264_handler->addToChain(nack_responder);
-	video_track->setMediaHandler(h264_handler);
+	packetizer->addToChain(video_sr_reporter);
+	packetizer->addToChain(nack_responder);
+	video_track->setMediaHandler(packetizer);
 }

If those APIs are not available in v0.19 (which Linux builds would seem to be stuck on for now), then we cannot simply apply that patch.

@paullouisageneau
Copy link

Just for reference since obs-deps is not related to Linux builds. Libdatachannel 0.20.0 is not buildable on Linux since it is not compatible with a stable release of usrsctp.

Edit: Upstream issue paullouisageneau/libdatachannel#1100

Compatibility with usrsctp 0.9.5.0 is fixed in 0.20.1.

This updates to libdatachannel 0.20.1
@Sean-Der Sean-Der force-pushed the update-libdatachannel branch from 88af6c3 to 8221dc6 Compare January 20, 2024 23:29
@Sean-Der Sean-Der changed the title deps.ffmpeg, deps.windows: Update libdatachannel to v0.20.0 deps.ffmpeg, deps.windows: Update libdatachannel to v0.20.1 Jan 20, 2024
@Sean-Der
Copy link
Contributor Author

@RytoEX @tytan652 A v0.20.1 has been released that address the Linux issue!

Thank you @paullouisageneau

@Sean-Der
Copy link
Contributor Author

@RytoEX Did I do everything right/does that resolve the issue?

@RytoEX
Copy link
Member

RytoEX commented Jan 22, 2024

@RytoEX Did I do everything right/does that resolve the issue?

I typically don't review PRs on weekends, so I'm just now getting to this.

If those APIs are not available in v0.19 (which Linux builds would seem to be stuck on for now), then we cannot simply apply that patch.

As far as I can tell, we don't currently build this feature (WebRTC/WHIP on OBS Studio) on Linux (except for Flatpak) where these changes would be problematic due to outdated library versions in Ubuntu's packages repo that we cannot control. That being the case, this seems fine. However, changes in the patch provided does require the accompanying code changes to be made at the same time as the dependency update on obs-studio to avoid deprecation warnings or breaking the builds. That isn't unresolvable, but it is one more thing to keep track of when updating this.

The PR itself seems fine otherwise.

@RytoEX
Copy link
Member

RytoEX commented Jan 22, 2024

I've unlinked this PR from this Issue:

That Issue is specific to systems that use OpenSSL, which is not the case on Windows and macOS. That Issue can be fixed in the Flatpak builds by updating libdatachannel to v0.19.5+ there, which is done in obsproject/obs-studio#10118.

@Sean-Der
Copy link
Contributor Author

I have updated the description to include obsproject/obs-studio#9800 which will be fixed by this PR

Updating to v0.20.0 includes my commit that adds support for all certain types.

@RytoEX
Copy link
Member

RytoEX commented Jan 22, 2024

I have updated the description to include obsproject/obs-studio#9800 which will be fixed by this PR

Updating to v0.20.0 includes my commit that adds support for all certain types.

That would seem correct. Will take a look at this and the associated obs-studio patch when I can. I wish there was a way we could apply an ifdef on the obs-studio side so that we could patch obs-studio independently, but libdatachannel doesn't seem to provide version data in a way that would let us do that.

@Sean-Der
Copy link
Contributor Author

@paullouisageneau what do you think of adding some version system?

As I add more things to libdatachannel for WebRTC sources it will be a problem :/

@paullouisageneau
Copy link

paullouisageneau commented Jan 24, 2024

@paullouisageneau what do you think of adding some version system?

Yes sure, could you please open an issue with what would be needed? I guess defines for major, minor and build would work?

@Sean-Der Sean-Der requested a review from RytoEX January 26, 2024 04:10
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.

I've tested this with the suggested patch and it works fine. I am fine with this as-is, as I don't want to keep the linked macOS bug from being fixed. However, I would like to see a way to differentiate libdatachannel versions in the future so that we can land such patches on obs-studio independently with ifdefs for breaking changes, if needed.

@RytoEX RytoEX merged commit 4202b50 into obsproject:master Jan 27, 2024
21 checks passed
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.

OBS Crashes with this WHIP
6 participants