Skip to content
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

Proceeding call state added #519

Merged
merged 3 commits into from
May 13, 2016
Merged

Conversation

stormbkk87
Copy link
Contributor

Added call state ‘proceeding’ from sofia nua_callstates. Since there’s
no early media, this allows the client interface to play a ringback
tone for the user.

Added call state ‘proceeding’ from sofia nua_callstates. Since there’s
no early media, this allows the client interface to play a ringback
tone for the user.
@lminiero
Copy link
Member

Cool, thanks for that! Indentation looks a bit weird, though, could you fix that?

Besides, could you sign our CLA as well? You need to authenticate via github and fill the form at the end of the page.

Thanks!

Early media for 183 session progress scenarios when SDP is available
before call accept.
@stormbkk87
Copy link
Contributor Author

Yeah indentation looks strange. Prob due to editor. I can fix. Also, I went a bit further and tried adding a scenario from sofia's website for early media under the nua_r_invite event. Works with my SIP server setup (Kamailio+Asterisk). The change covers the #1 and #2 use cases from the link below.

http://sofia-sip.sourceforge.net/refdocs/soa/soa_sdp_oa_use_cases.html

I've read on the SIP.js site that they don't like this solution for early media and that there's quite a complex scenario. As you'll see I include the SDP in a ringing event and the UAC can choose to set the description or wait until the SDP comes in the accept event. Not sure if this has side effects, your thoughts?

@lminiero
Copy link
Member

I'd rather keep it simple: some endpoints may not support early media and this could break things for them.

@saghul
Copy link
Contributor

saghul commented May 11, 2016

FWIW, I'm also -1 on attempting to support early media. It's a recipe for multiple headaches for very little to no benefit.

@stormbkk87
Copy link
Contributor Author

If you'll notice that is why I kept the 'accepted' event to include the sdp as well. So you could basically pick which one you started the audio session on and ignore the other. Use case #2.

Maybe make it an option in the cfg? And only send the 'ringing' event if the sdp is available. Let me know and I'll remove/add that code.

@saghul I agree that it may produce headaches to support the myriad of endpoints out there. However, I don't agree with the very little to no benefit. Not hearing a ringback is the number 1 complaint I get when someone doesn't configure their router correctly for mainstream SIP phones.

I'll probably end up going with the initial solution of the 'proceeding' event since I can start playing a ringback to the user sooner. Early media seems to come a few seconds later. Users want to hear the ringback asap. However, the extra development time to play a ringback through the earpiece is rough on iOS and Android to make it sound seamless.

@stormbkk87
Copy link
Contributor Author

Forgot to mention another case regarding early media outside of the ringback issue when calling mobile networks via SIP. If a mobile phone is off, an endpoint might play audio such as "Sorry, the subscriber is not acknowledged". There is no 'accepted' event triggered, SIP error code or hangup code sent until it hits a timeout which is set by the endpoint. So, in sofia sip use case #1 the user will hear a ringback for a minute or so when the real case is 'subscriber absent' or similar status. Therefore, giving the user the false impression that the endpoint phone was ringing.

I can just fork the early media stuff if you don't want to support it.

@lminiero
Copy link
Member

We can discuss the early media stuff as a separate PR, so that this one only adds the 'proceeding' state event, which is definitely safe and less "controversial" to have, and can be merged as is, I believe.

@saghul
Copy link
Contributor

saghul commented May 11, 2016

We can discuss the early media stuff as a separate PR, so that this one only adds the 'proceeding' state event, which is definitely safe and less "controversial" to have, and can be merged as is, I believe.

👍

@saghul
Copy link
Contributor

saghul commented May 11, 2016

Early media is where the rabbit hole starts. Renegotiation is known not to work well in WebRTC, and by supporting early media you must support it too.

About the ringback: 180 means you have to provide the ringback. I have never seen ringback come as inband audio in early media.

Last, this is not my project, but I wouldn't want to see the SIP plugin (which I dearly care about) become a monster: today is early media, tomorrow prack, sesion timers, etc.

Those are my 2 cents.

@stormbkk87
Copy link
Contributor Author

stormbkk87 commented May 11, 2016

:) Understood.

In my case, I get back a 183 with SDP from my endpoint which does have inband audio. Maybe my endpoint is more like use case #3 and I should send back a prack, etc. Not a SIP expert.

@saghul very nice SIP plugin by the way.

@lminiero
Copy link
Member

Thanks! I'll merge this right away.

@lminiero lminiero merged commit 0145412 into meetecho:master May 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants