Update (current|pending)RemoteDescription SDP after addIceCandidate#3413
Update (current|pending)RemoteDescription SDP after addIceCandidate#3413
Conversation
fippo
force-pushed
the
addicecandidate-update-remotedescription
branch
from
a992540 to
85ba51d
Compare
|
@JoTurk 🤔 any idea what causes the test failures? They are either totally uncorrelated or I need to move the test to the go layer... |
|
@fippo there is a race in attribute. |
| if err := pc.pendingRemoteDescription.UpdateWithCandidate(candidate); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| if pc.currentRemoteDescription != nil { | ||
| if err := pc.currentRemoteDescription.UpdateWithCandidate(candidate); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
This updates the candidates' attributes while startRTP is reading them. which cases the test to fail with race detected.
There was a problem hiding this comment.
but why does startRtp look at the candidates in the SDP instead of getting them from the ICE agent? Also why does anything RTP not consider ICE a virtual socket? 🤔
There was a problem hiding this comment.
Sorry i missed this so this is partly done for historic reasons. during early calls (we'll change this with ice v5, where we'll move more ownership to ice), also check the mux in this repo, especially at the close handling, currently we have multiple semi ownership for sockets.
There is a plan to move most of the ownership and write to one place, also so we can do low level socket optimizations and similar stuff.
fippo
force-pushed
the
addicecandidate-update-remotedescription
branch
from
85ba51d to
5691236
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3413 +/- ##
==========================================
- Coverage 85.52% 85.47% -0.06%
==========================================
Files 81 81
Lines 9819 9854 +35
==========================================
+ Hits 8398 8423 +25
- Misses 1002 1006 +4
- Partials 419 425 +6
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:
|
2 participants
as described in
https://w3c.github.io/webrtc-pc/#dom-peerconnection-addicecandidate
(noticed while pondering pion/ice#912 (comment))