Summary:	ASTERISK-04467: [branch] setting the musicclass doesn't work as one might expect (hold)
Reporter:	Terry Wilson (twilson)	Labels:
Date Opened:	2005-06-23 09:43:41	Date Closed:	2006-07-19 16:23:46
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Resources/res_musiconhold
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:
Description:	Situation: SIP/1000 has a musicclass set as [jazz] and SIP/1001 has a musicclass set as [classical] (both set in sip.conf) and 1000 calls 1001. If 1000 puts 1001 on hold, it just goes to the default music on hold (even though you would expect it to be jazz).  If 1001 puts 1000 on hold, the musiconhold that is chosen is jazz(though you would expect it to be classical). All of this is w/o using SetMusicOnHold.  If I put a SetMusicOnHold(classical) before the Dial(SIP/1001), then I get the correct (classical) musiconhold if 1001 puts 1000 on hold, but still of course get the default MOH when 1000 puts 1001 on hold. I would have made this a tweak, but I can't seem to find a workaround to get musiconhold to select properly if the person who placed the call puts the calee on hold. (well, I can change the ast_moh_start(bridgepeer, NULL) to ast_moh_start(bridgepeer, p->musicclass), but that breaks it if the callee puts the caller on hold!)
Comments:	By: Kevin P. Fleming (kpfleming) 2005-06-23 13:46:48 I've been looking at this area of the code myself lately, and I don't particularly like the way it works... I think what will be required is for channels to start their own MOH, rather than having it started by other channels, so they can choose the class they want. By: Kevin P. Fleming (kpfleming) 2005-06-23 21:56:21 Your description of the problem is not quite right, but there is apparently a flaw here. In this situation (sip.conf): [1000] musicclass=jazz [1001] musicclass=classical Then, if 1000 puts 1001 on hold, 1001 should hear 'classical', since that is the musicclass assigned to 1001. If 1001 puts 1000 on hold, 1000 should hear 'jazz'. This would work regardless of which side initiates the call. Please retest and confirm exactly which way works and which way doesn't. Thanks! By: Terry Wilson (twilson) 2005-06-24 09:27:55 Acutally, just retested and is happening exactly as I described. [1000] musiconhold=jazz [1001] musiconhold=classical 1000 calls 1001 If 1000 presses hold, default If 1001 presses hold, jazz 1001 cllas 1000 If 1001 presses hold, default If 1000 presses hold, classical Also, I don't understand what value there is in assigning to a particular phone what MOH that they *hear* on that device.  It seems like there would be more use for a setting for what MOH that is generated when the *place* someone on hold.  At least, in my situation.  If you are hosting multiple organizations on one Asterisk box and want to be able to set custom music on hold for different organizations, if org 1 calls org 2 and the person from org1 places someone on hold, they would expect that their music on hold would play--not org 2's. What about outbound calls to the PSTN?  If you have more than one musicclass across different orgs, how do you set the musiconhold that the PSTN caller will here if the customer puts them on hold?  Even a SetMusicOnHold doesn't work here (at least I haven't been able to get it to work) because if you set it before doing the Dial out, you are creating a new channel that doesn't have anything set. By: Kevin P. Fleming (kpfleming) 2005-06-24 13:29:37 OK, we've had some discussion here about this, and it's actually working as designed, but the design doesn't make sense for a virtual PBX application. Here's what we are going to do: add a MUSICCLASS dialplan function, that can both read and write, and can also operate on the 'bridged' channel. With that, you can be very creative :-) For example, if you are going to deliver a call from a PRI channel to a SIP phone, and you want the PSTN caller to hear the SIP phone's chosen musicclass if they are put on hold by the SIP user, you can do this: [macro-setpeermoh] exten => s,1,Set(MUSICCLASS()=${MUSICCLASS(b)}) [incoming] exten => 1234,1,Dial(SIP/peer||M(macro-setpeermoh)) This will change the music class on the Zap channel to the class defined for the SIP peer, after the SIP peer answers the call. This will work even if you do a 'forked' dial, where you don't know which peer is going to answer the call. We really did consider changing the behavior of Asterisk itself to handle this, but it's too radical a change to make at this point. This solution should do what you need and be relatively easy to use. By: Terry Wilson (twilson) 2005-06-24 14:07:50 Ok.  Sounds like we could live with that.  Might get a little tricky w/ our setup (a fairly intricate res_perl based dialplan... think _X.,1,Perl() as a dialplan), but I'm sure we can figure out what goes where.  Thanks. By: Olle Johansson (oej) 2005-06-25 04:39:56 Ahh. Now I understand. I was looking at this earlier. We need to start musiconhold not in the channel that puts someone on hold, but when we receive a AST_CONTROL frame that puts us on hold. I thought it might be wrong, but could not find a reason to change it. Guess we found it now. Thanks Terry! (I am still on holiday, so I can't react quickly to this, sorry.) By: Kevin P. Fleming (kpfleming) 2005-07-11 23:26:26 This whole idea was a bust... since the macro can't modify the bridged channel when Dial() runs it, because the bridge has not been established yet. It's going to require some more thought, to make this behave in the way that some users are (legitimately) expecting it to behave. By: Olle Johansson (oej) 2005-07-20 13:35:22 According to Kevin, the current idea of an correct behaviour is to start the Musiconhold in the channel that receives AST_CONTROL_HOLD with the musicclass defined in the bridged channel - the one that puts our channel on hold... Another bug report implies a different look at this, where a channel that receives a hold from a phone should not affect the hold status of the bridged channel at all! See ASTERISK-4478 By: Olle Johansson (oej) 2005-07-20 13:38:03 ...so I guess we need to come to some sort of conclusion on how Asterisk handles HOLD and when we play musiconhold music and if so, which class we're using. By: Kevin P. Fleming (kpfleming) 2005-08-22 16:40:40 These changes will require invasive modification of most channel drivers, and as such are not a candidate for merging before 1.2 is released. By: opsys (opsys) 2005-12-30 17:41:22.000-0600 Housekeeping: Anyone what to continue with this or should we close?? By: Olle Johansson (oej) 2005-12-31 01:46:18.000-0600 This issue needs to be open, since it is clearly an error that needs a fix. By: Olle Johansson (oej) 2006-02-02 01:29:58.000-0600 This is still an issue waiting for AST_CONTROL frames with payload I guess. /Housekeeping By: Olle Johansson (oej) 2006-03-02 11:53:58.000-0600 This "post 1.2" patch is still waiting for control frames with payload... By: Serge Vecher (serge-v) 2006-05-10 10:21:41 Are these the control frames w/ payload this bug is waiting for? http://lists.digium.com/pipermail/svn-commits/2006-May/013434.html By: Kevin P. Fleming (kpfleming) 2006-05-10 10:43:02 Yes, I am working on these changes right now. By: Olle Johansson (oej) 2006-05-11 02:14:49 We made a decision on how to solve this at AstriDevCon Europe, so this will be solved in 1.4. By: Olle Johansson (oej) 2006-05-11 02:15:28 A bug reported on June 23 last year is not "new" any longer :-) By: Serge Vecher (serge-v) 2006-06-28 10:29:42 removing [pending architectural change] tag as russelb has confirmed that part was done. By: Russell Bryant (russell) 2006-07-16 10:35:04 I now have a branch open where I am working on changes related to this issue.  I will have these changes finalized and merged before the 1.4 release.  The branch is located here: svn co http://svn.digium.com/svn/asterisk/team/russell/hold_handling Now, for an explanation of what is being implemented ... There is no more musicclass option in any of the channel drivers.  Instead, there are two new musicclass options.  Some channel drivers have both, some just one, some none.  Check the sample configuration files to see which options a channel supports.  SIP supports both of these options. mohsuggest --- This MOH class is used when putting the other channel on hold.  If SIP/100 is putting SIP/101 on hold, SIP/100's mohsuggest setting is passed as the suggestion as to what class SIP/101 should listen to.  The only thing that can override the mohsuggest setting is the musicclass on the channel itself (SIP/101 in this example).  The musicclass on the channel itself is now *only* set inside of the dialplan using Set(CHANNEL(musicclass)=whatever). mohinterpret --- This MOH class specifies what MOH class to listen to when put on hold.  For the example of SIP/100 putting SIP/101 on hold, SIP/101 will hear the MOH class defined in this setting only if the channel's musicclass was not set in the dialplan, and SIP/100 had no mohsuggest setting.  IAX2 has an additional feature for this option.  If mohinterpret is set to "passthrough" the HOLD and UNHOLD control frames are passed through as IAX control messages.  The idea is to implement this feature in all of the channel drivers where it makes sense. And, of course, when none of these options are set, the default MOH class is used. On the implementation side of things, all of this is accomplished by no longer starting MOH on the bridged channel directly.  Instead, an AST_CONTROL_HOLD frame is queued up with the payload being the mohsuggest setting.  Also the handling of AST_CONTROL_HOLD/UNHOLD has been standardized to mean that this channel is being put on hold, or being retrieved from hold.  The channel drivers are now responsible for starting MOH on the channel themselves, or alternatively, passing it through as signalling. By: Russell Bryant (russell) 2006-07-16 15:14:22 By the way, this branch is ready for testing. The things I would like to do before merging this into the trunk are to implement HOLD signalling passthrough for chan_zap and potentially chan_sip, and also, of course, test it out. By: Russell Bryant (russell) 2006-07-16 20:42:08 I was finally able to test my changes out for myself and found out things are not working correctly.  I'll report back when I feel that the changes are working as they should.  Stay tuned ... By: Russell Bryant (russell) 2006-07-17 18:53:31 Alright, I have fixed some bugs in the code and have it working with some basic tests with SIP and Zap phones.  If anyone would like to try it out know, it would be much appreciated. oej: Here is something for you to start thinking about.  I have implemented mohinterpret=passthrough for both chan_iax2 and chan_zap.  However, I think we should get it in there for chan_sip as well. By: Serge Vecher (serge-v) 2006-07-19 16:22:45 branch commited to trunk in r37988/37999 by kpfleming. Thanks, russell, for job well done ;)