SIP plugin: add audio/video stream with an update request(or reinvite) #2164
Conversation
| @@ -5785,10 +5804,6 @@ static void janus_sip_connect_sockets(janus_sip_session *session, struct sockadd | |||
| if(!session || (!audio_server_addr && !video_server_addr)) | |||
| return; | |||
|
|
|||
| if(session->media.updated) { | |||
There was a problem hiding this comment.
This is removed because session->media.updated is always false on this place.
There was a problem hiding this comment.
Not sure why you think it will always be false there? Anyway, since it only showed an additional log line, no harm in getting rid of it, I guess.
|
Thanks! I won't have time to look into this today, I'll try to do that next week 👍 |
There was a problem hiding this comment.
Found some time after lunch to have a quick look, and it seems fine, thanks! I added a couple of small notes, but nothing important: especially considering you tested it for quite some time, I guess this is good to go. Could I ask you to apply the same changes to the NoSIP plugin as well, though, since they share a lot of the same media code?
| if(video_added) | ||
| session->media.has_video = TRUE; /* FIXME Maybe we need a better way to signal this */ | ||
|
|
||
| if(audio_added || video_added) { |
There was a problem hiding this comment.
I think you can remove the three empty lines above, since they belong to the same group of code.
| @@ -5785,10 +5804,6 @@ static void janus_sip_connect_sockets(janus_sip_session *session, struct sockadd | |||
| if(!session || (!audio_server_addr && !video_server_addr)) | |||
| return; | |||
|
|
|||
| if(session->media.updated) { | |||
There was a problem hiding this comment.
Not sure why you think it will always be false there? Anyway, since it only showed an additional log line, no harm in getting rid of it, I guess.
|
One more note: I guess this won't work if the SIP session is using SDES-SRTP, since I think we only set that up at the beginning of the call, and I don't see crypto-related changes here. |
|
Merging. |
|
PS: looks like the NoSIP plugin already supports all that, and using the same additional |
|
Hello Lorenzo, Yes, I have implemented this based on the NoSIP plugin implementation. When you asked me about SDES-SRTP I realized that I didn't test it because we are not using it. |
Hi,
this pull request is about adding option in SIP plugin to add audio/video stream with update request (or SIP reinvite) if it wasn’t initially negotiated.
Of course, for SIP reinvite it will work only if you don’t use autoaccept_reinvites feature.
I have tested it with Kamailio and it works fine. We are using it in production for last 5 days and we didn’t notice any issue.
The main change is allocating local media ports on update request if audio or video stream is added.
Also, if update request is answer(not offer) session->media.updated is set to TRUE because we need to connect new media ports.
Let me know if you have any question, suggestion or anything else. Thanks.