Implement SPED (STUN Protocol for Embedding DTLS) - #876
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #876 +/- ##
==========================================
- Coverage 88.35% 87.97% -0.39%
==========================================
Files 46 47 +1
Lines 6279 6409 +130
==========================================
+ Hits 5548 5638 +90
- Misses 498 532 +34
- Partials 233 239 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
10ebcd0 to
53df31e
Compare
which will be used in pion#876 where the presence of some attributes depends on whether they were used in the original request.
which will be used in pion#876 where the presence of some attributes depends on whether they were used in the original request.
which will be used in pion#876 where the presence of some attributes depends on whether they were used in the original request.
which will be used in pion#876 where the presence of some attributes depends on whether they were used in the original request.
which will be used in pion#876 where the presence of some attributes depends on whether they were used in the original request.
which will be used in pion#876 where the presence of some attributes depends on whether they were used in the original request.
which will be used in pion#876 where the presence of some attributes depends on whether they were used in the original request.
which will be used in #876 where the presence of some attributes depends on whether they were used in the original request. (split from that to make review simpler, no functional changes)
0ab695f to
e9d8b98
Compare
1ea6639 to
c7f08eb
Compare
|
Thank you for this great feature! I have integrated this into the livekit SFU and made some connectivity checks, found 2 regression cases:
Both are fixed in our fork livekit@85afccd, livekit@5eec925. Will merge them back to upstream once this PR is merged if they make sense. |
|
@cnderrauber oops, forgot to respond. (1) the tl;dr is that you can send DTLS unembedded once ice is writable which for lite means you can send after receiving the first valid binding check. https://fippo.github.io/warp-snap-sped/draft-hancke-webrtc-sped.html#section-3.3.1 bullet (3) covers that (2) good catch! |
JoTurk
left a comment
There was a problem hiding this comment.
Sorry about ghosting this for too long, I have a few comments mainly about how Piggyback interacts with the agent.
| if a.piggyback.state == PiggybackingStateOff { | ||
| return a.connectionState != ConnectionStateConnected | ||
| } |
There was a problem hiding this comment.
shouldn't we queue / store packets before connected, wouldn't this affect early flight dtls packets?
There was a problem hiding this comment.
Yes, @zshang-oai found this too. This needs to go into the capture + flush part. I have a fix but keeping this open, this needs more 👀
5b98fcb to
e6f721c
Compare
|
updated! |
| defer a.piggyback.mu.Unlock() | ||
| if a.piggyback.state == PiggybackingStateOff && a.piggyback.connected { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Shouldn't this also include complete to fix the issue reported / fixed by livekit with late retransmits? @cnderrauber livekit@85afccd this maybe should also extend to address packets that were already queued.
There was a problem hiding this comment.
#876 (comment) -- was thinking of having that done as a f'up.
| a.piggyback.packets = append(a.piggyback.packets, packetWithCrc{packet, crc}) | ||
| } else if a.piggyback.state != PiggybackingStateOff { | ||
| a.piggyback.state = PiggybackingStatePending | ||
| } |
There was a problem hiding this comment.
This is triggered on dtls handshake right? from the comment "// A nil packet signals that the local DTLS handshake completed."
what happen if dtls handshake happens after complete? do we go back to pending or is it impossible because of something I'm missing?
There was a problem hiding this comment.
wait, is this a deja-vu? #876 (comment) 😂
There was a problem hiding this comment.
Sorry i misworded my comment, My concern here is different from my old comment, Is it possible for the state to be "complete" when (nil, true) gets called? and if that happens do we move back to pending?
There was a problem hiding this comment.
(Also from the glue this is called in completeStart which is called after full dtls handshake, this gets triggered after Finished in 1.2 and 1.3) so my concern is what happens if dtls handshake happens after sped handshake and is it possible?
There was a problem hiding this comment.
In other words what happens if pion receives and process the stun message before the pion/dtls' hanshake channel unlocks? do we enter complete => pending race?
(based on #914 )
Part of pion/webrtc#3335