Skip to content

chore: Buffer remote candidates like local #77

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.

Sign up for GitHub

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

Merged
merged 25 commits into from
Jan 27, 2022
Merged

chore: Buffer remote candidates like local #77

Changes from 7 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
310eb82
chore: Buffer remote candidates like local
kylecarbs Jan 27, 2022
b874a1e
Use local description instead
kylecarbs Jan 27, 2022
411542a
Add logging for candidate flush
kylecarbs Jan 27, 2022
90e00db
Fix race with atomic bool
kylecarbs Jan 27, 2022
85e6def
Simplify locks
kylecarbs Jan 27, 2022
7f26a99
Add mutex to flush
kylecarbs Jan 27, 2022
a56560b
Reset buffer
kylecarbs Jan 27, 2022
58445d5
Remove leak dependency to limit confusion
kylecarbs Jan 27, 2022
6a07a99
Fix ordering
kylecarbs Jan 27, 2022
3272e13
Revert channel close
kylecarbs Jan 27, 2022
877ae59
Flush candidates after remote session description is set
kylecarbs Jan 27, 2022
1e6a923
Bump up count to ensure race is fixed
kylecarbs Jan 27, 2022
0c02c78
Use custom ICE dependency
kylecarbs Jan 27, 2022
0439e23
Fix data race
kylecarbs Jan 27, 2022
a8b5a07
Lower timeout to make for fast CI
kylecarbs Jan 27, 2022
d74f454
Add back mutex to prevent race
kylecarbs Jan 27, 2022
b613b6d
Improve debug logging
kylecarbs Jan 27, 2022
ba878ca
Lock on local description
kylecarbs Jan 27, 2022
ef41921
Flush local candidates uniquely
kylecarbs Jan 27, 2022
3515fe7
Fix race
kylecarbs Jan 27, 2022
fbe847c
Move mutex to prevent candidate send race
kylecarbs Jan 27, 2022
7321303
Move lock to handshake so no race can occur
kylecarbs Jan 27, 2022
bebc74d
Reduce timeout to improve test times
kylecarbs Jan 27, 2022
48078d2
Move unlock to defer
kylecarbs Jan 27, 2022
5021b95
Use flushed bool instead of checking remote
kylecarbs Jan 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 15 additions & 14 deletions peer/conn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ func newWithClientOrServer(servers []webrtc.ICEServer, client bool, opts *ConnOp
dcDisconnectChannel: make(chan struct{}),
dcFailedChannel: make(chan struct{}),
localCandidateChannel: make(chan webrtc.ICECandidateInit),
localCandidateBuffer: make([]webrtc.ICECandidateInit, 0),
localSessionDescriptionChannel: make(chan webrtc.SessionDescription),
pendingCandidates: make([]webrtc.ICECandidateInit, 0),
remoteSessionDescriptionChannel: make(chan webrtc.SessionDescription),
}
if client {
Expand Down Expand Up @@ -120,8 +120,8 @@ type Conn struct {
localSessionDescriptionChannel chan webrtc.SessionDescription
remoteSessionDescriptionChannel chan webrtc.SessionDescription

pendingCandidates []webrtc.ICECandidateInit
pendingCandidatesMutex sync.Mutex
localCandidateMutex sync.Mutex
localCandidateBuffer []webrtc.ICECandidateInit

pingChannelID uint16
pingEchoChannelID uint16
Expand All @@ -141,15 +141,14 @@ func (c *Conn) init() error {
if iceCandidate == nil {
return
}
// ICE Candidates on a remote peer are reset when an offer
// is received. We must wait until the offer<->answer has
// been negotiated to flush candidates.
c.pendingCandidatesMutex.Lock()
defer c.pendingCandidatesMutex.Unlock()
c.localCandidateMutex.Lock()
defer c.localCandidateMutex.Unlock()
if c.rtc.RemoteDescription() == nil {
c.pendingCandidates = append(c.pendingCandidates, iceCandidate.ToJSON())
c.opts.Logger.Debug(context.Background(), "adding local candidate to buffer")
c.localCandidateBuffer = append(c.localCandidateBuffer, iceCandidate.ToJSON())
return
}
c.opts.Logger.Debug(context.Background(), "adding local candidate")
select {
case <-c.closed:
break
Expand Down Expand Up @@ -307,16 +306,17 @@ func (c *Conn) negotiate() {
}
}

c.pendingCandidatesMutex.Lock()
defer c.pendingCandidatesMutex.Unlock()
for _, pendingCandidate := range c.pendingCandidates {
c.localCandidateMutex.Lock()
defer c.localCandidateMutex.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

I was surprised we didn't need this, but saw it was brought back! I thought there might be cases where ICE candidates could come in, in parallel to negotiate here.

for _, localCandidate := range c.localCandidateBuffer {
c.opts.Logger.Debug(context.Background(), "flushing local candidate")
select {
case <-c.closed:
return
case c.localCandidateChannel <- pendingCandidate:
case c.localCandidateChannel <- localCandidate:
}
}
c.pendingCandidates = make([]webrtc.ICECandidateInit, 0)
c.localCandidateBuffer = make([]webrtc.ICECandidateInit, 0)
c.opts.Logger.Debug(context.Background(), "flushed candidates")
}

Expand All @@ -328,6 +328,7 @@ func (c *Conn) LocalCandidate() <-chan webrtc.ICECandidateInit {

// AddRemoteCandidate adds a remote candidate to the RTC connection.
func (c *Conn) AddRemoteCandidate(i webrtc.ICECandidateInit) error {
c.opts.Logger.Debug(context.Background(), "adding remote candidate")
return c.rtc.AddICECandidate(i)
}

Expand Down