-
Notifications
You must be signed in to change notification settings - Fork 1.8k
SetConfiguration now updates ICEGatherer's servers #3339
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
Conversation
04c1161 to
9e04d64
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3339 +/- ##
==========================================
+ Coverage 85.02% 85.12% +0.09%
==========================================
Files 80 80
Lines 9512 9532 +20
==========================================
+ Hits 8088 8114 +26
+ Misses 1001 998 -3
+ Partials 423 420 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9e04d64 to
1607cad
Compare
|
@JoTurk thanks for getting the pion/ice PR in so quickly. This is ready for review. |
JoTurk
left a comment
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.
Changes look good to me overall, and makes us closer to the browser behavior. thank you.
icegatherer.go
Outdated
| // UpdateServers updates the ICE servers and gather policy. | ||
| // If called before gathering starts, the new servers will be used for initial gathering. | ||
| // If called after gathering has started, the new servers will be used on the next ICE restart. | ||
| func (g *ICEGatherer) UpdateServers(servers []ICEServer, policy ICETransportPolicy) error { |
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 think this should be private.
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.
Done.
1607cad to
b31a179
Compare
JoTurk
left a comment
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.
Cool, also thank you for updating the spec comments.
Description
After using
SetConfigurationI found that the new ice servers have not been used. Turns out after creating a peer connection the ice gatherer is created with the current configuration, but it is never updated with new servers when the configuration changesThis pr should fix the issue and implement the spec properly. We just need to wait on pion/ice#866 to land.