Fix763 #816
Fix763 #816
Conversation
A change was recently made to the SetLocal and SetRemoteDescription functions for PeerConnection that starts ICE gathering for the trickle method only when generating an answer. This means clients generating an offer to initiate signaling will stall indefinitely.
This commit uses synchronization primitives so that peers in the pion-to-pion-trickle example adhere to the trickle ICE specification https://tools.ietf.org/html/draft-ietf-mmusic-trickle-ice-02 To be more specific, the offering peer waits to send the answering peer remote ICE candidates until after they have sent the initial offer. The answering peer then waits to add any remote candidates until after it has processed the initial offer. Similarly, the offering peer waits to add remote candidates until it has processed the corresponding answer.
Codecov Report
@@ Coverage Diff @@
## master #816 +/- ##
==========================================
- Coverage 76.21% 76.02% -0.19%
==========================================
Files 59 59
Lines 4250 4246 -4
==========================================
- Hits 3239 3228 -11
- Misses 742 748 +6
- Partials 269 270 +1
Continue to review full report at Codecov.
|
|
Hmm, looks like travis is not happy here. There might have been more dependencies on the reverted change for examples |
Nevermind, it's passing now. I think it was the formatting issue of the commit message that i fixed last commit :) |
Thank you so much! I am not sure of what your other use cases are, but there might in the future be call for a unit test that is a bit more nuanced. Here's what the RFC referenced by the w3c draft states: https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-25#section-5.9
Thank you for taking a look at this so quickly, I'm really grateful for your understanding and feedback. Your responses have helped us a lot in integrating this library and evaluating it :) I'll be more cognizant of major/minor version upgrades in the future as well. Time spent assessing changes is not unusual for performing version updates of libraries. Out of curiosity, do you have changelogs or summaries of changes for minor version bumps? |
I personally don't use it, so not a lot of drive from me to work on it.
I will start doing that. I just haven't been doing it out of laziness/assuming no one is reading! |
| @@ -99,12 +110,14 @@ func main() { | |||
| } else if closeErr := resp.Body.Close(); closeErr != nil { | |||
| panic(closeErr) | |||
| } | |||
| answerwg.Add(1) //We have now sent the answer and can send candidates to the peer | |||
Shouldn't this be answerwg.Done()?
Hey @arlolra
Thanks for the review, we ended up dropping the WaitGroup because it didn't work in WASM https://github.com/pion/webrtc/blob/master/examples/pion-to-pion-trickle/answer/main.go
Hopefully this issue wasn't ported over!
Oh thanks for catching that arlo!
@Sean-Der does this fix make it work in WASM?
Yes! It now compiles for WASM with this change. I don't actually use the WASM implementation myself so don't know all the deep details sorry :/
Description
This patch reverts the breaking changes made to the core WebRTC API in #763 and modifies the pion-to-pion-trickle example to work properly with the WebRTC API as specified in https://w3c.github.io/webrtc-pc/ and following the specification of trickle ICE in https://tools.ietf.org/html/draft-ietf-mmusic-trickle-ice-02
The text was updated successfully, but these errors were encountered: