Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
First experiments with ICE restarts #753
This PR tries to lay the foundation to add support for something that would be quite helpful: ICE restarts. These are typically needed whenever something in your network changes (e.g., you move from WiFi to mobile or a different WiFi) but want to keep the conversation going: in this case, an ICE restart needs to take place, as the peers need to exchange the new candidates they can be reached on. Of course, since Janus always stays where it is, this is only relevant for users. Beware that ICE restarts are not enough to cover the scenario I mentioned: in fact, if your signalling channel going down due to the network switch results in the PeerConnection being destroyed on Janus (what happens for instance with the WebSockets transport in Janus itself), then ICE restarts can't help. If you're wrapping the Janus API, you need to make sure that signalling can be kept alive and restored after a switch. How that happens and how to distinguish that from plain timeouts is up to you: this PR only covers media once you decide that happened.
Before I begin, let me anticipate that this is NOT ready and complete. While the core bits have been done, and ICE is something the core itself worries about, this is not a feature that is transparent to plugins, since they need to be aware a renegotiation is taking place (e.g., to increase the SDP version). This is especially true in case we also decide to allow offerer/answerer to switch roles during a renegotiation (more on this later).
I conceived this PR as a patch to #403 (reference counters) and not master as I want to accellerate the adoption of those as well. This may be a good opportunity for interested users to start messing with that too.
This patch only modifies EchoTest and Streaming plugins/demos to test the feature. The reason for this is simple: they're the simplest plugins to involve to test the different roles Janus can take. In the EchoTest, the user always offers and Janus always answers; in the Streaming plugin it's the other way around. For this reasons, they both allowed me to experiment a bit.
Besides, the patch modifies
That said, as anticipated one of the main questions I asked myself was: should we allow users to always originate a new offer to restart ICE, whether the handle supports that or not (see the Streaming plugin or VideoRoom subscribers, for instance), or should we stick to the approach the plugin uses, and let restarts be triggered by users in another way (e.g., a new plugin message or changes to existing ones)? I tried to address this in the PR by allowing both in the Streaming plugin. I'm personally in favour of the latter (keeping roles) as it simplifies things both on the server and the client side, as it will be clearer in the next examples.
This is basically the same thing as when we create the new PeerConnection in the demo, with an additional
Streaming (sending offer instead of answer)
As you can see, here we can't just set
To carry the new SDP, we simply send a
Again, the Admin API or webrtc-internals will show the changes.
Streaming (Janus sends updated offer)
So, exactly the same
Again, the Admin API or webrtc-internals will show the changes.
Please test and provide feedback, expecially on what we should support as a mechanism. Allowing users to offer may be helpful, but awkward on the user side, and not transparent at all to plugins, which need to be implemented to support both approaches even though they only use one (see examples above).
Should your tests on this preliminary effort work as expected, I'll proceed to extend support to other plugins as well for further experimentation.
Since I got no feedback at all, I went on by myself. Just worked to add this new ICE support to other plugins: right now only the SIP plugin is missing (as we have to take into account possible updates on the SIP side as well so I want to think better about this). I also changed the way the Streaming plugin works, which now only uses the server-originated offer of the examples I made before.
Summarizing below the behaviour for each plugin and how you can test it (again, getting Admin API info on the updated handle should display a new
Nothing changed from the original commit. Just add
To trigger an ICE restart, send a new
Whether you're the caller or the callee, you can force an ICE restart by sending a new offer, and attach it to a
To trigger an ICE restart, create a new offer with
Different behaviour in case you're updating a publisher or a subscriber.
For publishers, create a new offer with
For subscribers, just send a
To force an ICE restart, send a
Different behaviour in case you're recording or replaying something.
For recorders, create a new offer with
For playout, send a
Pretty meaningless considering all VoiceMail sessions last at max 10 seconds, but anyway... To force an ICE restart, create a new offer which is the same as the original one but with
Hope that now that almost all plugins are updated, I'll get some feedback on this. Please test!
Great work @lminiero
But i tested it in a different scenario and facing few issues !
Scenario: Switching steam in active peerconnection (Common use case for switching microphones/webcams or webcam to screenshare).
Changes in the client side :
Observations in Janus Server:
2> While processing new RTP getting below warning logs but sequence number is resetting (hopefully no issue with this)
3> Need to test recording stream with multiple ssrc's.
4> While sending to listeners mostly getting below srtp protect error and audio is always audible at listeners. (something we need to handle similar to switch stream)
[Sat Feb 11 16:15:44 2017] [ERR] [ice.c:janus_ice_send_thread:3567]  ... SRTP protect error... err_status_replay_old (len=100-->100, ts=2048199192, seq=4110)...
@chest3x not sure about what you were trying to do: I see you closed a recording there, but then tried to do an ICE restart for the second session? That's not what ICE restarts do: they allow you to restart the ICE state machine while you're on a recording, in this case, not to start a new recording. For that you need to do a second, new, negotiation (so brand new offer/answer).
Can you elaborate on what you tried to do? In any case, you probably found a bug we need to fix.
I realize now that the examples I wrote may have been misleading. When I wrote "create a new offer with iceRestart:true and attach it to a record request, pretty much as you'd do to start a new one":
I meant as an update to the recording you started: the name must be the same, you can't restart and change recording name. Maybe it would be a good idea to add a
This last commit adds ICE restart support to the SIP plugin too. To force an ICE restart, generate a new offer that forces an ICE restart and attach it to a new request called
On the plugin side it generates a SIP re-INVITE, which means the response is now handled pretty much as responses to plain INVITEs are. We should probably handle the distinction better, but in my simple tests it seemed to be working for now. There will be room for improvement, assuming you guys play with this and give me feedback of course.
The segmentation fault is fixed. However now, insted of a segfault, I am getting "Not a recording session, can't refresh" message. Maybe it is supposed to work that way, I don't know.
My behavior on client is:
It appears to me like, Janus is trying to use ICE restart after second attempt to record the video on the same session.
These are the last words from the server:
I am using the clean, non-modified version of Janus (ice-restart branch)
Probably a bug somewhere, as I noticed that too but the .nfo files are definitely there and playable. My guess is that when it reaches that code, the recording object has already been removed (it can happen after different triggers) and so it thinks it never existed while it really did.
I have tested this PR against VideoRoom plugin, and seems fine by me.
@lminiero, great work, I would love to see this merged, cheers