|Summary:||ASTERISK-18804: WaitExten(...,m(MOH)) doesn't play the correct audio when Set(CHANNEL(musicclass)=...) is used|
|Reporter:||Filip Jenicek (phill)||Labels:|
|Date Opened:||2011-11-02 02:50:56||Date Closed:||2011-12-06 15:48:16.000-0600|
|Status:||Closed/Complete||Components:||Core/PBX PBX/General Resources/res_musiconhold|
|Description:||It seems to me that there is a bug in the WaitExten command. WaitExten(...,m(moh)) doesn't play the correct audio when Set(CHANNEL(musicclass)=...) is used.|
Steps to reproduce:
exten => 1,n,Set(CHANNEL(musicclass)=rockandroll)
exten => s,n,WaitExten(5,m(silence))
I would expect to hear music on hold of class "silence"
Class "rockandroll" is played.
This issue is caused by:
1. Function local_ast_moh_start in res_musiconhold.c specifies the priorities of how music class is determined.
/* The following is the order of preference for which class to use:
* 1) The channels explicitly set musicclass, which should *only* be
* set by a call to Set(CHANNEL(musicclass)=whatever) in the dialplan.
* 2) The mclass argument. If a channel is calling ast_moh_start() as the
* result of receiving a HOLD control frame, this should be the
* payload that came with the frame.
* 3) The interpclass argument. This would be from the mohinterpret
* option from channel drivers. This is the same as the old musicclass
* 4) The default class.
It seems to me, that that (1) should be moved after (2). This would fix the issue, however I'm not certain that it won't break something else.
2. Another piece of code which might fix the issue is function pbx_builtin_waitexten in main/pbx.c where one could use a simple patch to force the desired behaviour.
This issue is probably related to:
|Comments:||By: Walter Doekes (wdoekes) 2011-11-02 04:41:18.371-0500|
Why do you call this a bug if it's documented behaviour?
Seems to me like:
(a) You're assuming it works differently,
(b) but the only documentation you can find says that it works like it should.
If you want to change documented behaviour you should probably call it an RFE and I would suggest you discuss this on the asterisk-users list first.
P.S.1. Don't post inline patches.
P.S.2. If I read the patch right, your WaitExten(..,m(myclass)) now implicitly does a Set(CHANNEL(musiconhold)=myclass). That's *very* wrong.
By: Filip Jenicek (phill) 2011-11-02 06:38:24.749-0500
The documentation says something different than how the application behaves. There is no information about priorities at [http://www.asterisk.org/docs/asterisk/trunk/applications/waitexten]
However, my common sense tells me, that the documentation looks right, but the implementation is wrong. I'd expect the command WaitExten to override the channel settings.
Regarding the second PS - Yes it's indeed wrong, but as a hot-fix it deals with the issue without affecting any other functionality.
My point was to report the bug and identify places in code which might be the cause
By: Jonathan Rose (jrose) 2011-12-06 14:51:07.478-0600
No, it shouldn't override channel settings. That would imply that the next time music on hold is invoked that it would use the setting set by WaitExten. My gut would have led me to believe that this should have taken priority too, but it was clearly documented otherwise, so eh... changing it now would mess with existing configurations that were made set up according to the way we documented it. Besides, setting the music on hold class channel variable isn't something you are really supposed to do unless you want to take manual control of the music on hold anyway. You can always clear the variable before entering scenarios like this anyway.
I'm clipping out your inline patch by the way. Posting inline patches like that means we absolutely can not use them since they aren't tied to a license agreement. They are also hard on the eyes, so just post your diff as an attachment.
Well, the priority is documented with this being the standard way of doing it. The proper way to deal with this seems to be not to set the music class as a channel variable. Either way, this needs to be documented in the WaitExten app.