-
Notifications
You must be signed in to change notification settings - Fork 785
Fix JsSIP 3.7.1 doesn't allow sending DTMF during EarlyMedia #689 #925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
lib/Dialog.js
Outdated
| this._route_set = message.getHeaders('record-route').reverse(); | ||
| } | ||
|
|
||
| const cseq = message.cseq ? parseInt(message.cseq, 10) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message.cseq is guaranteed to be present. sanityCheck.js used from UA.js ensures it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed this should not be needed at all as the cseq must be equal to the original request. Did you see any problem with the current approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I introduced the update of _local_seqnum / _remote_seqnum from message.cseq is that during testing with an IVR sending 183 Session Progress with early media, calls would consistently drop after ~11–12 seconds if this assignment was not done.
Even though the DTMF INFO requests were being delivered, the dialog state was not stable and the PBX terminated the session. Updating the dialog’s seqnum here prevented those drops and kept the call alive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth it investigating this because the responses of a request have the same seq num as the request itself, and the dialog's seq num is already set with the request one so.. this lines are fixing something that should be fixed somewhere else. We should get to the root of the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are local and remote sequence numbers set upon dialog construction:
https://github.com/versatica/JsSIP/blob/master/lib/Dialog.js#L61
https://github.com/versatica/JsSIP/blob/master/lib/Dialog.js#L81
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have moved the local CSeq update to RTCSession.js, where the early dialog is promoted to dialog.
Without it, calls with IVR and early media drops when the remote party answered aftert 12 / 13 sec max.
@jmillan please let me know if you think there’s a more appropriate place to handle this CSeq alignment, but based on testing this adjustment keeps the dialog stable without affecting the standard dialog update flow.
The patch introduces two changes:
Early dialog handling
When no confirmed dialog (this._dialog) is present, the code now attempts to send the request through the first available early dialog.
Added safety checks to ensure _earlyDialogs is not empty and that the selected dialog supports sendRequest.
CSeq update in Dialog.update()