Summary:	ASTERISK-04478: [patch] Manager Hold events are only generated for bridged channels
Reporter:	Paulo Mendes da Silva (kanelbullar)	Labels:
Date Opened:	2005-06-24 12:18:06	Date Closed:	2008-01-15 15:48:08.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) patch_HOLD_UNHOLD_0004594_1.txt ( 1) patch_HOLD_UNHOLD_0004594_2.txt
Description:	Manager Hold/Unhold events are only generated for bridged channels. These events should be generated in other situations as well. There are a few example scenarios that show this problem. These are easily reproducible with CVS HEAD. 1) A call is established between two sip phones. One of them puts the call on hold. At this point, a Hold event is correctly generated on the Manager API. The second phone also puts the call on hold. At this point, no Hold event is generated. We can't tell, using the Manager API, that the second call has been put on hold. If the first one is unheld, the second one remains on hold and we still won't be aware of that fact. 2) Two or more sip phones are connected to a meet me room, in a conference. One of them puts the call on hold. No Hold event is generated. We can't tell the call has been put on hold. A hold event should be generated in this case too. 3) A call is made to an extension that executes an AGI script that answers the call and plays some messages. The call is put on hold. No hold event is generated here either. It should also be generated, since we won't know the call has been put on hold. Hold events should be generated even if no bridge between the holding channel and another channel exists. Regular CTI protocols behave this way (CSTA, TAPI). That is, the Hold event is generated for each end of the call, regardless of there being an established connection, a held connection or whatever active call state. The same goes for Unhold events. ****** ADDITIONAL INFORMATION ****** After inspecting the chan_sip.c code, we noticed that Hold/Unhold events are only generated when there is a 'bridgepeer'. if ((bridgepeer=ast_bridged_channel(p->owner))) { /* We have a bridge */ /* Turn on/off music on hold if we are holding/unholding */ if (sin.sin_addr.s_addr && !sendonly) { ast_moh_stop(bridgepeer); /* Indicate UNHOLD status to the other channel */ ast_indicate(bridgepeer, AST_CONTROL_UNHOLD); append_history(p, "Unhold", req->data); if (callevents && ast_test_flag(p, SIP_CALL_ONHOLD)) { manager_event(EVENT_FLAG_CALL, "Unhold", "Channel: %s\r\n" "Uniqueid: %s\r\n", p->owner->name, p->owner->uniqueid); } ast_clear_flag(p, SIP_CALL_ONHOLD); This appears to follow the behavior of generating MOH streams. However, since generating the Hold/Unhold events is slightly different, perhaps it should be moved out of this 'if' test. We have made that experimental change in our labs and the scenario appeared to be correct for all the 3 cases that we mentioned in the description, that is, events were correctly genearated. Please consider making this change, as it would make the Manager API scenarios more suitable for CTI applications.
Comments:	By: Paulo Mendes da Silva (kanelbullar) 2005-06-24 12:21:08 This problem is related with issues ASTERISK-3423456 and ASTERISK-4264290 that dealt with generating Held events for SIP channels. By: Michael Jerris (mikej) 2005-07-12 18:11:03 Can you create a patch to provide the desired functionality in cvs head? By: Paulo Mendes da Silva (kanelbullar) 2005-07-15 12:06:14 Yes, I can create a patch. I will do it as soon as possible and place it here. By: Kevin P. Fleming (kpfleming) 2005-07-15 17:09:17 And it would ideally work for other channel types besides SIP as well :-) By: Paulo Mendes da Silva (kanelbullar) 2005-07-19 09:30:33 I've made a patch that corrects the problem. It is now possible to receive Hold and Unhold events for all the 3 situations that were described previously (two parties on hold, conference on hold, AGI on hold), for which there are no bridges, at the moment the event should be generated. In order to achieve this, I moved the code for event generation out of the bridged channel test that was shown in the additional information. Furthermore, I removed the "Indicate HOLD/UNHOLD status to the other channel", since a channel is really only on hold when we put it on hold and not when the remote one is put on hold. Following the suggestion that was made by kpfleming, I also implemented the generation of Hold and Unhold events for IAX2. The behavior will be similar. Events will be generated regardless of there being a bridged channel. By: Paulo Mendes da Silva (kanelbullar) 2005-07-19 09:31:20 I have faxed my disclaimer earlier today, so I believe you already have it there. By: Olle Johansson (oej) 2005-07-20 07:57:16 We need to make a decision on when to send AST_CONTROL_HOLD. If we're bridged, I believe we *should* tell the other channel that the other end of the call put it on hold... If we're not bridged, there's no reason to do so. Also, if the SIP channel *receives* AST_CONTROL_HOLD for a channel, should we set RTP to 0.0.0.0 to indicate hold to the phone - when there's no musiconhold set for that channel? By: Paulo Mendes da Silva (kanelbullar) 2005-07-20 12:57:27 Oej, my view of having a call on hold, from a CTI perspective, is considering both ends of the call as being separately on hold. Let's say we have an established call between A and B. A puts the call on hold, which temporarily disconnects A from B and generates a manager Hold event for A. B has made no action on the call, so, as far as CTI is concerned, B is still connected to the call, even if no audio stream is present. So, no manager Hold event is generated for B at this point. Later on, B puts the call on hold. At this point, a manager Hold event should be generated for B. If we had sent  AST_CONTROL_HOLD, when A put the call on hold, the SIP_CALL_ONHOLD flag would have been set, which would prevent us from being able to tell that B had not yet placed its end of the call on hold and from generating the correct manager Hold event. Since the AST_CONTROL_HOLD processing was only actually setting the SIP_CALL_ONHOLD flag, I decided to remove it. However, if you believe that we should keep the AST_CONTROL_HOLD indication and remove the SIP_CALL_ONHOLD setting inside its processing, I can change the patch that way. Perhaps that's the best option, in case there is need to implement some additional processing for the remote channel when the call is put on hold. What do you think? By: Olle Johansson (oej) 2005-07-28 10:20:22 Again, we need to discuss this amongst the developers. I believe it's wrong of chan_sip to send AST_CONTROL_HOLD to the bridged channel (yes, I've changed my mind :-) ) so I've removed that in another patch somewhere here in the bug tracker. Seems to me that for chan_sip, moving the manager events out of the bridged peer test makes sense. By: Michael Jerris (mikej) 2005-07-28 10:51:58 can we address this on the dev call today?  oej, can you attend? By: Paulo Mendes da Silva (kanelbullar) 2005-08-16 11:27:08 Since this bug is not waiting for us to provide any additional information and it is not assigned to anyone, I don't know which are the next steps I have to take in order to help the patch being included in release 1.2. Can you provide us some help on how to proceed? Is there anything else we need to do in order to have it included in release 1.2? I have tested it thoroughly and it works just fine for all the scenarios that I mentioned in the description. By: Michael Jerris (mikej) 2005-08-16 11:31:21 do you have a disclaimer? By: Paulo Mendes da Silva (kanelbullar) 2005-08-17 03:58:41 Yes, I do. I faxed it to Digium on July 19th 2005. By: Michael Jerris (mikej) 2005-08-17 08:25:34 waiting on review, and for oej and kpfleming to decide on the AST_CONTROL_HOLD related issues. By: Olle Johansson (oej) 2005-09-06 11:13:35 Can we update this patch to the current SIP channel, please? Seems like this patch is not affected by the MOH redesign. By: Paulo Mendes da Silva (kanelbullar) 2005-09-06 11:30:28 Added new patch version. By: Olle Johansson (oej) 2005-09-09 08:51:58 Recommend this for cvs By: Olle Johansson (oej) 2005-09-09 08:52:18 Can't review the IAX part, but the SIP part looks fine. By: Kevin P. Fleming (kpfleming) 2005-09-13 22:20:25 Committed to CVS HEAD, thanks! By: Digium Subversion (svnbot) 2008-01-15 15:48:08.000-0600 Repository: asterisk Revision: 6585 U   trunk/channels/chan_iax2.c U   trunk/channels/chan_sip.c ------------------------------------------------------------------------ r6585 | kpfleming | 2008-01-15 15:48:08 -0600 (Tue, 15 Jan 2008) | 2 lines ensure that Manager hold/unhold events are generated even for non-bridged channels (issue ASTERISK-4478) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=6585