Summary:	ASTERISK-29373: res_rtp_asterisk: Flash events are duplicated
Reporter:	N A (InterLinked)	Labels:	patch
Date Opened:	2021-03-29 09:14:25	Date Closed:	2021-03-31 09:52:15
Priority:	Minor	Regression?
Status:	Closed/Complete	Components:	Resources/res_rtp_asterisk
Versions:	18.3.0	Frequency of Occurrence	Constant
Related Issues:
Environment:	Debian 10	Attachments:	( 0) ASTERISK-29373.diff ( 1) res_rtp_asterisk.patch
Description:	I noticed that for RFC2833 hook flash events, they were all getting duplicated. I found this is due to the way RFC2833 events work, where there is a start and end for each event. However, unlike DTMF events, for which there are beginning and end events, Asterisk has no notion of the beginning or end of a hook flash - there is only one hook flash (AST_CONTROL_FLASH). Thus, incoming RTP packets create duplicate flash events, which channel.c is not expecting. The attached patch fixes this bug by ensuring that only flash starts become AST_CONTROL_FLASH and flash end events are not duplicated, thus aligning what res_rtp_asterisk.c is doing with what channel.c expects.
Comments:	By: Asterisk Team (asteriskteam) 2021-03-29 09:14:27.178-0500 Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution. Please note that log messages and other files should not be sent to the Sangoma Asterisk Team unless explicitly asked for. All files should be placed on this issue in a sanitized fashion as needed. A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report. Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process]. Please note that once your issue enters an open state it has been accepted. As Asterisk is an open source project there is no guarantee or timeframe on when your issue will be looked into. If you need expedient resolution you will need to find and pay a suitable developer. Asking for an update on your issue will not yield any progress on it and will not result in a response. All updates are posted to the issue when they occur. Please note that by submitting data, code, or documentation to Sangoma through JIRA, you accept the Terms of Use present at [https://www.asterisk.org/terms-of-use/|https://www.asterisk.org/terms-of-use/]. By: N A (InterLinked) 2021-03-29 09:16:42.551-0500 Attached patch resolves described bug. By: Joshua C. Colp (jcolp) 2021-03-29 09:18:25.342-0500 Per comment on other issue, once license agreement is accepted then patches can be attached. By: N A (InterLinked) 2021-03-29 09:21:59.957-0500 Okay, sorry about the mixup. I only submitted that Friday and I'm not sure what the turnaround is for that. I submitted a patch on Friday for chan_sip.c as well and it let me attach a patch after I signed an agreement, so I assumed it was okay at that point. By: Joshua C. Colp (jcolp) 2021-03-29 09:24:27.853-0500 The system will allow you to do such a thing, but the patch is hidden and not attributed to the license agreement. An email goes out once the agreement is processed. By: N A (InterLinked) 2021-03-29 14:01:44.713-0500 Okay, I will watch for that. No issues with the other patches, but there seem to be all sorts of gotchas when trying to address this one. Trying to be direct about like dropping a frame led to Asterisk crashing, so that was a no go. It seems that if you try to prevent the flash event from going through, even by breaking out or returning null where applicable, it seems to auto fall through eventually to a DTMF_END event, with the result that a DTMF 'X' event passes through. In channel.c, one can try to prevent this by stifling DTMF if it happens to be 'X', and while this does prevent a LOG_DTMF event in the CLI, I noticed that if I'm at a dial tone like with Read() and I flash, this actually passes all the way through to the Read function as an "X". So the duplicate flash event is suppressed, but obviously, this is undesired. It seems impractical to try to go on a while goose chase and suppress this invalid DTMF everywhere, since DTMF seems to be written with the idea that if it isn't a flash event, it must be DTMF. Cutting the extraneous flash event out either doesn't work or leads to a crash. Architecturally, how appropriate would it be to introduce a AST_CONTROL_FLASH_END in addition to the existing AST_CONTROL_FLASH? (I looked for a dummy code like AST_CONTROL_USELESS, but it doesn't seem like there is such a thing). I just need to discard this and throw it away. This would simply be a dummy thing to eat up the end RTP packets. The way RTP is set up, it you ignore the end events entirely, then it's equivalent to a continuous DTMF that never ends, since it's waiting for the end. Which approach would be better, in your opinion? It seems that addressing this bug will require some kind of major change. By: George Joseph (gjoseph) 2021-03-29 14:44:35.260-0500 Waiting on license agreement to be approved. By: Joshua C. Colp (jcolp) 2021-03-29 15:17:26.749-0500 As I've said before, you're in uncharted seldom used noone has given it a thought territory. Flash is rarely, if ever, sent through Asterisk. I think though that introducing an AST_CONTROL_FLASH_END isn't really needed. It should be possible to change res_rtp_asterisk such that it only passes through a single flash event upon end - which is what the INFO implementation does. AST_CONTROL_FLASH is already effectively AST_CONTROL_FLASH_END. The create_dtmf_frame already shows how to return a null frame and do nothing, it should be as simple as adding an if (type == AST_FRAME_DTMF_BEGIN) into the if (rtp->resp == 'X') block that returns &ast_null_frame. By: N A (InterLinked) 2021-03-29 15:39:01.774-0500 Actually, I tried that already, and when I do that, Asterisk crashed. Not sure why (I didn't do a backtrace or anything), but it doesn't seem to like that. I have already spent 10-15 hours playing around and figuring out different ways to pre-empt the DTMF behavior and make it work in a logical way. The problem is in res_rtp_asterisk, but I could not get it to work in a satisfactory way just by trying to work with what is available. I tried over 50 combinations of things and they all had one downside or another. I ended up going ahead and adding an AST_CONTROL_FLASH_END to eat up the end event, and assign AST_CONTROL_FLASH to the start event and AST_CONTROL_FLASH_END to the end one (I don't think it really matters; this was an arbitrary choice). I had to modify several other files as well - I've been basically been searching the entire repo for existing similar things and mirroring them - mostly the hold and DTMF syntax. I found that when I use this second event, it works *perfectly*. No duplicate flash events, no erroneous DTMF. So, if this is not problematic in some way, this seems like the easiest way to go. I can't see how the FLASH_END event would  be useful in the future, but I guess if someone wanted that event in the future then it would be there. I have already patched the necessary files, including chan_iax2, with it programmed NOT to send the FLASH_END event (unlike the FLASH) event. I have patched 8 more files in addition to the 3 I patched earlier, and will probably need to patch a few more in order to get the AMI event working right. That's what I'm working on now. For some reason, manager show events does not show any of the DTMF events. I am not sure if this is yet some other bug or not. I set up an entirely new machine and Asterisk 18.3.0 for developing this feature today and its manager is also missing them - most of the events are there, but not the DTMF ones. So I'm wondering if my Flash event will show up even if I do it correctly now: Connected to Asterisk 18.3.0 currently running on mn3 (pid = 24716) mn3*CLI> manager show events Events:  --------------------  --------------------  --------------------  AGIExecEnd            AGIExecStart          AOC-D  AOC-E                 AOC-S                 AgentCalled  AgentComplete         AgentConnect          AgentDump  AgentLogin            AgentLogoff           AgentRingNoAnswer  Agents                AgentsComplete        Alarm  AlarmClear            AorDetail             AorList  AsyncAGIEnd           AsyncAGIExec          AsyncAGIStart  AttendedTransfer      AuthDetail            AuthList  AuthMethodNotAllowed  BlindTransfer         BridgeCreate  BridgeDestroy         BridgeEnter           BridgeLeave  BridgeVideoSourceUpd  CEL                   Cdr  ChallengeResponseFai  ChallengeSent         ChanSpyStart  ChanSpyStop           ChannelTalkingStart   ChannelTalkingStop  ConfbridgeEnd         ConfbridgeJoin        ConfbridgeLeave  ConfbridgeList        ConfbridgeMute        ConfbridgeRecord  ConfbridgeStart       ConfbridgeStopRecord  ConfbridgeTalking  ConfbridgeUnmute      ContactList           ContactStatus  ContactStatusDetail   CoreShowChannel       CoreShowChannelsComp  DAHDIChannel          DNDState              DeviceStateChange  DialBegin             DialEnd               DialState  EndpointDetail        EndpointList          ExtensionStatus  FAXSession            FAXSessionsComplete   FAXSessionsEntry  FAXStats              FAXStatus             FailedACL  FullyBooted           Hangup                HangupHandlerPop  HangupHandlerPush     HangupHandlerRun      HangupRequest  Hold                  IdentifyDetail        InvalidAccountID  InvalidPassword       InvalidTransport      Load  LoadAverageLimit      LocalBridge           LocalOptimizationBeg  LocalOptimizationEnd  MCID                  MWIGet  MWIGetComplete        MeetmeEnd             MeetmeJoin  MeetmeLeave           MeetmeMute            MeetmeTalkRequest  MeetmeTalking         MemoryLimit           MiniVoiceMail  MixMonitorMute        MixMonitorStart       MixMonitorStop  MonitorStart          MonitorStop           MusicOnHoldStart  MusicOnHoldStop       NewAccountCode        NewCallerid  NewConnectedLine      NewExten              Newchannel  Newstate              OriginateResponse     ParkedCall  ParkedCallGiveUp      ParkedCallSwap        ParkedCallTimeOut  PeerStatus            Pickup                PresenceStateChange  PresenceStatus        QueueCallerAbandon    QueueCallerJoin  QueueCallerLeave      QueueMemberAdded      QueueMemberPause  QueueMemberPenalty    QueueMemberRemoved    QueueMemberRinginuse  QueueMemberStatus     RTCPReceived          RTCPSent  ReceiveFAX            Registry              Reload  RequestBadFormat      RequestNotAllowed     RequestNotSupported  SIPQualifyPeerDone    SendFAX               SessionLimit  SessionTimeout        Shutdown              SoftHangupRequest  SpanAlarm             SpanAlarmClear        Status  StatusComplete        SuccessfulAuth        TransportDetail  UnParkedCall          UnexpectedAddress     Unhold  Unload   By: Joshua C. Colp (jcolp) 2021-03-29 15:49:07.965-0500 It's most likely the change you did was incorrect in res_rtp_asterisk or incomplete. For merging purposes I would strongly prefer that, as it touches far less things. You're free to attach your current approach once the license agreement is processed, but others may feel the same way if you put it up for code review for inclusion. As for why the simpler approach didn't work - without a backtrace it is anyone's guess. "manager show events" outputs based on the XML documentation. The XML documentation for the given events does not appear sufficient to place them in the XML documentation file, resulting in them not showing in "manager show events". I don't believe there's any issue filed for it. By: Joshua C. Colp (jcolp) 2021-03-29 16:03:01.208-0500 Something like this attached change is what I had in mind. By: N A (InterLinked) 2021-03-29 16:32:04.353-0500 Your patch is certainly different from mine. For the most part, I tried ignoring the end event, not the beginning event. I tried it once hte other way and the end event did not seem to work if the beginning the event was not there (i.e. the end became the beginning and then they were both ignored - so wrong the other way). I'll try your patch on my other system and see what happens. Assuming it works, then I'll cancel this particular patch - I don't want to rock the boat unnecessarily. As far as the manager event goes, even doing "manager show event <name>" does not work, even if the event is explicitly named. So it seems that the manager CLI does not allow for checking events individually if they do not show up in the summary list. Is there actually a way to use these events, or is this unintended? I can file a separate bug issue for that if that's what it is. By: Joshua C. Colp (jcolp) 2021-03-29 16:37:00.955-0500 I don't know what you mean by "use these events". "manager show events" and "manager show event" is purely informational, it has no impact on whether an event does or does not work. Best practices is to have events documented, but it is not enforced. By: N A (InterLinked) 2021-03-29 17:39:08.289-0500 Thanks, Joshua, clearly I need to read "AMI For Dummies" and figure out how it works! I was trying to find an easy way to view AMI events in the console so I can test when my Flash event works. Your patch works as far as I can tell when I tested it. I commented out my FLASH_END stuff and overwrote my res_rtp with the original and then patched it and I see one event each time. Would you be able to mark your patch as the resolution to this issue? Your patch fixes this issue perfectly. I will submit my patches for AMI support in a new feature issue since that is not related to this one. By: Joshua C. Colp (jcolp) 2021-03-29 17:50:14.609-0500 Until it goes up for review, is reviewed, and merged, then this issue will remain open. I may do that in the future. By: Friendly Automation (friendly-automation) 2021-03-31 09:52:16.711-0500 Change 15713 merged by Friendly Automation: res_rtp_asterisk: Only raise flash control frame on end. [https://gerrit.asterisk.org/c/asterisk/+/15713|https://gerrit.asterisk.org/c/asterisk/+/15713] By: Friendly Automation (friendly-automation) 2021-03-31 11:55:02.569-0500 Change 15721 merged by George Joseph: res_rtp_asterisk: Only raise flash control frame on end. [https://gerrit.asterisk.org/c/asterisk/+/15721|https://gerrit.asterisk.org/c/asterisk/+/15721] By: Friendly Automation (friendly-automation) 2021-03-31 11:55:20.931-0500 Change 15712 merged by George Joseph: res_rtp_asterisk: Only raise flash control frame on end. [https://gerrit.asterisk.org/c/asterisk/+/15712|https://gerrit.asterisk.org/c/asterisk/+/15712]