[Home]

Summary:ASTERISK-16583: [patch] chan_sip fails to remove hold when receving a reINVITE without SDP
Reporter:frawd (frawd)Labels:
Date Opened:2010-08-17 11:05:35Date Closed:2010-11-08 15:04:03.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/Interoperability
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_sip_invite_without_sdp.patch
( 1) mediadirection-work-in-progress.diff.txt
( 2) no-sdp-unhold-1.6.2-fix1.diff
( 3) no-sdp-unhold-1.8-fix1.diff
( 4) no-sdp-unhold-trunk-fix1.diff
Description:Patch for issue ASTERISK-13552 never made it to trunk so all versions above 1.4 suffer this problem.

Here is an updated patch for 1.6.2.11.
Comments:By: frawd (frawd) 2010-08-17 11:25:31

ebroad, you're too fast! :-)
By: frawd (frawd) 2010-08-17 18:35:20

Sorry i hadn't seen issue ASTERISK-13496 which has exactly the same patch!

I've only seen that behavior in situations when the proxy wants to update something in the call setup. It places the call on hold to stop the stream and quickly sends an empty INVITE to force the UA (Asterisk) to renegotiate the media session from zero by letting it send the SDP 'offer' (in the '200 OK').

As the call was placed on hold by the other side, it means Asterisk didn't originally want that call to be on hold, so given the opportunity to send a new offer, it should remove it.

I don't understand why it wasn't committed yet, as it's not such a bad workaround for the time it might take to rethink important parts of SDP handling (as in ASTERISK-15145, ASTERISK-15204, ASTERISK-15258, and of course ASTERISK-16558).

It just fixes it!
By: frawd (frawd) 2010-08-18 04:47:42

For more information, RFC-3261 states:
"A UAS providing an offer in a 2xx (because the INVITE did not contain an offer) SHOULD construct the offer as if the UAS were making a brand new call [...]"

Asterisk does actually comply with this RFC as it isn't obligated to do it (SHOULD), but placing the call off-hold could be a nice practice.
By: David Woolley (davidw) 2010-08-24 05:13:53

This area is a lot more messy than it may at first seem.  Re-invite without SDP is not actually a request to remove hold, but rather a request for the receiving side to start the negotiation.  Hold should only actually be removed once both sides have exchanged SDP.

What the quote from the RFC is saying, is that the offer should not take account of the fact that a hold exists.  However, it is only an offer, and until the response is received (in the ACK) there has been no agreement to remove the hold.
By: frawd (frawd) 2010-08-24 06:31:46

Exactly, it is a request for the receiving side to start the negotiation (from zero). I don't see a case where we would want to start a new call with hold...

As you say, the RFC is indicates that the offer should not take account of the fact that a hold exists, so what do you mean by "agreement to remove the hold"?; What hold??
The goal of the patch is actually to clear the hold, because it is not supposed to exist in the first place! :-)

At this point, the choice would be between:
1 - Rewrite the SDP code to make it comply with the RFC.
2 - Add a simple workaround that might be ugly but works and has been working for a while in 1.4 branch, closes a couple of issues (maybe more), and does not break anything.

Oh, and 2 can be done while 1 is on its way!!

I would be very interested into helping for 1, but I have very few hours to spend and low C skills so without any help I won't be able to. Is there a SIP dev-group where this could be discussed?
By: David Woolley (davidw) 2010-08-24 06:57:46

Remote party initiates hold.

Remote party does re-invite, but without intent to remove hold (e.g.) it is changing the state of a different media stream (I know one of the current bugs is that it doesn't track these states for each stream).  It might also do a re-invite as a keep alive (that is one of the uses of re-invite).

For some reason (maybe it is a Cisco), it doesn't send SDP on the request.

As Asterisk didn't want the hold, it should send a=sendrecv, but it should not remove the hold until, it gets back a=sendrecv, as, in the cases above, there was no change in the hold state and the response will contain the same a=sendonly, as before.

Note that it is perfectly legitimate to start a call in a=recvonly state.  I don't think Asterisk can do this, but the wording in the RFC doesn't bar a call from being in a hold from the very beginning.  I believe what the RFC is saying is simply that one should not take account of previously received SDP from the remote party, at this level of the protocol.

Treating an SDP-less re-invite as unhold, can result in a hold immediately followed by an unhold, and, in practical terms, cause a glitch in the music on hold.

(I've actually have code that tries to do this right (with the existing single media stream problem), but it is intertwined with other changes.  I remember the remote party's desired hold state, and only change it on receiving SDP from them, but make a distinction between first offer and response, sending my wants on the first offer and including the remote party's request in a response.

In our case, we also want to correctly report holds initiated by Asterisk, to the held party, so we also have to factor in the local media direction attribute requirements.  Doing so is essential for Transfer() to work with some switches, and is also what you should do if you are going to send music on hold; if nothing else, in the latter role, it causes the user interface on the phone to correctly indicate the call is being held from the other end of the wire.

As that is a feature change, it would have to be separated out before submitting a patch.)
By: David Woolley (davidw) 2010-08-24 07:08:29

I would add that there is a shortage of flag bits, so the extra state memory needed forced the addition of a new flags word, which is likely to complicate the porting of our patch from 1.6.1.0 to a supported version.
By: David Woolley (davidw) 2010-08-24 08:58:29

I've made an attempt to separate out our media direction attribute handling changes, but:

- these are based on a patched 1.6.1.0; in particular it already contained the patch that is the subject of this issue;
- I haven't verified that I've got all the relevant changes;
- I don't currently have the resources to forward port this to 1.6.2.
- It contains code to allow Asterisk to be active in initiating holds, which should be considered a feature change, although I should have eliminated all places where this could be triggered from the configuration or environment.  Again I don't currently have the resources to decompose the new feature from the bug fix part.
- one known flaw in the outgoing hold logic is that it doesn't set an appropriate comment in the re-Invite, so it claims to be an external bridge request.
- it doesn't attempt to addrsess the problem of having different hold states for different media streams.


By: frawd (frawd) 2010-08-24 09:11:38

I'll certainly take a look at your patch, because I don't understand the explanation very well. Still I think I get more or less what the objective is.


A few comments about the above:

Sending an INVITE without an offer is quite common, I've seen this with Cisco, Nortel and Huawei proxies. I've looked into it and it's part of some method called "third party call control" (3pcc), it complies with all RFCs involved and even has a best practice RFC (RFC-3725).

It is indeed legitimate to start a call in a=recvonly, the only thing is that I have yet to see a real-world application that would need to do that.

Also in the few cases I've seen for this particular problem, the hold-unhold thing happens in less than 5ms, I'm not even sure that music on hold had time to send a single 20ms packet.
By: David Woolley (davidw) 2010-08-24 09:15:31

I could be wrong, but I'd expect the MOH stream to be stopped and reinitialised, not just paused.
By: David Woolley (davidw) 2010-08-24 09:23:25

We tried to get by with just the proposed hack, but our outgoing hold feature addition locked up in a=inactive state, and we had to do things properly.  Also, I suspect there are cases where a rapid, bogus, unhold/hold sequence could confuse AMI processing.
By: Matthew Nicholson (mnicholson) 2010-09-15 11:44:52

I am going to implement the current 1.4 behavior in the other supported branches to close this issue. davidw, please open a new issue with your purposed feature additions.
By: David Woolley (davidw) 2010-09-15 11:46:42

Which bit do you consider a feature?  I thought I'd removed any new features.
By: frawd (frawd) 2010-09-15 11:51:12

The current 1.4 behavior is in the patch I uploaded chan_sip_invite_without_sdp.patch

Davidw, haven't had time to test your patch. Why don't you put it on the review board so it gets reviewed/tested/committed at some point?
By: Matthew Nicholson (mnicholson) 2010-09-15 12:16:46

davidw,

My apologies, I misunderstood your earlier comment.
By: Matthew Nicholson (mnicholson) 2010-09-15 17:46:15

davidw,

I looked at your media direction patch, it looks good although it does need a few adjustments.  Also I think it should be on issue 14385 instead of this one.  I think I am going to close this one by porting the code from 1.4 into the latest asterisk versions.
By: Matthew Nicholson (mnicholson) 2010-09-15 17:52:05

Please test with the no-sdp-unhold patch I have uploaded.  It is a slightly modified version of the existing patch structured to be more similar to what is in asterisk 1.4.
By: Matthew Nicholson (mnicholson) 2010-10-12 10:21:24

Any updates on this?
By: frawd (frawd) 2010-10-18 03:18:25

Sorry I didn't have time to test, but your patch is functionally equivalent to the first one i posted which i have been using in production for the past 2 months.

I guess you can commit this.
By: Digium Subversion (svnbot) 2010-11-08 14:54:11.000-0600

Repository: asterisk
Revision: 294242

U   branches/1.6.2/channels/chan_sip.c

------------------------------------------------------------------------
r294242 | mnicholson | 2010-11-08 14:50:21 -0600 (Mon, 08 Nov 2010) | 8 lines

Go off hold when we get an empty reinvite telling us to.

(closes issue 0014448)
Reported by: frawd

(closes issue ASTERISK-16583)
Reported by: frawd

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=294242
By: Digium Subversion (svnbot) 2010-11-08 14:56:31.000-0600

Repository: asterisk
Revision: 294243

_U  branches/1.8/
U   branches/1.8/channels/chan_sip.c

------------------------------------------------------------------------
r294243 | mnicholson | 2010-11-08 14:56:31 -0600 (Mon, 08 Nov 2010) | 15 lines

Merged revisions 294242 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.6.2

........
 r294242 | mnicholson | 2010-11-08 14:50:21 -0600 (Mon, 08 Nov 2010) | 8 lines
 
 Go off hold when we get an empty reinvite telling us to.
 
 (closes issue 0014448)
 Reported by: frawd
 
 (closes issue ASTERISK-16583)
 Reported by: frawd
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=294243
By: Digium Subversion (svnbot) 2010-11-08 15:04:02.000-0600

Repository: asterisk
Revision: 294244

_U  trunk/
U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r294244 | mnicholson | 2010-11-08 15:04:02 -0600 (Mon, 08 Nov 2010) | 22 lines

Merged revisions 294243 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.8

................
 r294243 | mnicholson | 2010-11-08 14:56:30 -0600 (Mon, 08 Nov 2010) | 15 lines
 
 Merged revisions 294242 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.6.2
 
 ........
   r294242 | mnicholson | 2010-11-08 14:50:21 -0600 (Mon, 08 Nov 2010) | 8 lines
   
   Go off hold when we get an empty reinvite telling us to.
   
   (closes issue 0014448)
   Reported by: frawd
   
   (closes issue ASTERISK-16583)
   Reported by: frawd
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=294244