manual ringing#3556
Conversation
|
I do agree that applications should have control on whether a ringing should be sent back automatically or not, since Janus is not the end user. Of course whatever change would need to be optional and defaulting to disabled for backwards compatibility. I'll review your PR in the next few days and get back to you. Thanks! |
lminiero
left a comment
lminiero
left a comment
There was a problem hiding this comment.
I looked at the patch and, apart from a few editorial notes, it seems fine to me. The only doubt I have is related to the send_ringing request: I was wondering it it may make sense to have a generic method for sending generic error codes back, but thinking about it it would probably be a mess, as it would complicate the checks on when you can send stuff, and we have dedicated messages for rejecting calls for instance.
|
I apologize for the codestyle mistakes. Should be fine now. |
No need to apologize at all, I myself forget about whatever style has been used throughout the code sometimes 🤣 |
| goto error; | ||
| } | ||
| if(session->stack->s_nh_i) | ||
| nua_respond(session->stack->s_nh_i, 180, sip_status_phrase(180), TAG_END()); |
There was a problem hiding this comment.
I have one small doubt: shouldn't this part also check whether automatic ringing is enabled, and/or whether or not we already sent a 180 back? Not sure what happens in the SIP world if you send a 180 twice or more.
There was a problem hiding this comment.
Yes, I had the same concern about state corruption if you do this multiple times. Frankly I'm no SIP expert but to my knowledge (based on quick google searching RFCs), it's specified that multiple progress messages may be sent freely:
A UAS MAY
send as many provisional responses as it likes. Each of these MUST
indicate the same dialog ID. However, these will not be delivered
reliably.
https://www.rfc-editor.org/rfc/rfc3261.html#section-13.3.1.1
For the first question - the same rationale as above, having automatic ringing doesn't mean someone wouldn't want to send multiple progresses later on - for what purpose I do not know, but I figured I Janus shouldn't be too opinionated in this regard.
There was a problem hiding this comment.
Also regarding your previous comment
The only doubt I have is related to the send_ringing request: I was wondering it it may make sense to have a generic method for sending generic error codes back, but thinking about it it would probably be a mess
tbh the main reason why I didn't add it is because I see no use for it. And again going back to me being a SIP novice, I wouldn't want to miss any validation that a generic API would require that I'm completely oblivious about lol.
There was a problem hiding this comment.
Makes sense, thanks for the feedback! I think this is good to merge, so I'll go ahead, and see if this is easy to backport to 0.x too. Thanks again for your contribution!
As a side and unrelated note, not sure if you had the chance to look at #3514: as a regular user of the SIP plugin, I'd be interested in feedback on that part (even though it may not be needed in your scenarios).
2 participants
Currently, SIP plugin sends ringing automatically when an incoming call is received. This doesn't allow us to have a real ringing indicator if our WebRTC side has a ringing notifier.
For example, we have certain setups where we asynchronously detect that the call cannot be handled on WebRTC and want to reject the incoming call altogether, and as such there was never any actual ringing.
SIP side, however, thinks the call was ringing and then rejected.
I'd like to have a way for janus not to send this event. Instead, when we receive confirmation that the call is actually ringing, we would then ask janus to actually emit the ringing event and continue normally.