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

Makes UDPMux IPv4/IPv6 aware #431

Merged
merged 1 commit into from
Mar 2, 2022
Merged

Makes UDPMux IPv4/IPv6 aware #431

merged 1 commit into from
Mar 2, 2022

Conversation

Antonito
Copy link
Member

@Antonito Antonito commented Mar 2, 2022

UDPMux before only worked with UDP4 traffic.
UDP6 traffic would simply be ignored.

This commit implements 2 connections per ufrag. When requesting a
connection for a ufrag the user must specify if they want IPv4 or IPv6.

Relates to #518


This is basically a port of #424 to UDPMux

I tested it on an IPv6-only network, on which I was previously unable to connect to a SFU. It seems to work great so far.

@codecov
Copy link

codecov bot commented Mar 2, 2022

Codecov Report

Merging #431 (fafa43e) into master (427ac0f) will decrease coverage by 0.20%.
The diff coverage is 67.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     pion/webrtc#431      +/-   ##
==========================================
- Coverage   78.92%   78.71%   -0.21%     
==========================================
  Files          34       34              
  Lines        3886     3914      +28     
==========================================
+ Hits         3067     3081      +14     
- Misses        634      642       +8     
- Partials      185      191       +6     
Flag Coverage Δ
go 78.71% <67.27%> (-0.21%) ⬇️
wasm 24.96% <0.00%> (-0.18%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
udp_mux_universal.go 75.16% <0.00%> (ø)
udp_mux.go 77.00% <67.34%> (-6.73%) ⬇️
gather.go 67.33% <100.00%> (+0.40%) ⬆️
agent.go 81.93% <0.00%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 427ac0f...fafa43e. Read the comment docs.

udp_mux_universal.go Outdated Show resolved Hide resolved
@Sean-Der
Copy link
Member

Sean-Der commented Mar 2, 2022

@braginini I just added you to the Pion org. Would love your opinion on this! Since you have been improving this area as well.

@Sean-Der
Copy link
Member

Sean-Der commented Mar 2, 2022

@Antonito woah this is fantastic! Awesome work, really exciting that we got this fixed so quickly :)

This is an approve from me. Worth waiting for @davidzhao @braginini, but if they are busy my vote is API breakage + merge!

@Antonito
Copy link
Member Author

Antonito commented Mar 2, 2022

@Antonito woah this is fantastic! Awesome work, really exciting that we got this fixed so quickly :)

This is an approve from me. Worth waiting for @davidzhao @braginini, but if they are busy my vote is API breakage + merge!

Thanks a lot for the quick feedback! Really excited to get this landed, it's been an issue for a while on my SFU – I wasn't sure how to fix it, until I read your TCP PR 😁

I'll give it a couple hours and will break the API+merge if no new feedback :)

@Sean-Der
Copy link
Member

Sean-Der commented Mar 2, 2022

With @braginini approval I am all for it! Feel free to tag new ice/webrtc versions as well @Antonito !

UDPMux before only worked with UDP4 traffic.
UDP6 traffic would simply be ignored.

This commit implements 2 connections per ufrag. When requesting a
connection for a ufrag the user must specify if they want IPv4 or IPv6.

Relates to pion/webrtc#1915
@Antonito Antonito merged commit 45ff379 into master Mar 2, 2022
@Antonito Antonito deleted the udpmux-ip-version-aware branch March 2, 2022 18:02
Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

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

Excellent work @Antonito ! thank you for fixing this. @jech will be happy :)

@braginini
Copy link
Contributor

Great work @Antonito
This will actually help us at wiretrustee once we start working on ipv6 support :)

@jech
Copy link
Member

jech commented Mar 2, 2022 via email

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

Successfully merging this pull request may close these issues.

5 participants