Summary:	ASTERISK-06420: [patch] app_waitforsilence is broken
Reporter:	davetroy (davetroy)	Labels:
Date Opened:	2006-02-25 09:27:52.000-0600	Date Closed:	2006-09-03 15:24:48
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_waitforsilence
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) app_waitforsilence.c ( 1) app_waitforsilence.patch ( 2) app_waitforsilence.patch2
Description:	app_waitforsilence was at some point modified to return channel variable WAITSTATUS with values SILENCE and TIMEOUT, and to return after the specified time interval regardless of whether silence was detected or not. This defeats the purpose of the app, which is to wait indefinitely until silence is detected. While it's useful to know whether the app exited with silence or not, and it is useful to have a mechanism for exiting the silence-waiting loop (avoiding an infinite loop, for instance), in its current form the app is broken and unintuitive.  In the best case in its current form, you would have to put this app into a dialplan loop and call it multiple times to get the desired functionality, which requires lots of extra overhead and variable checks. I have reworked the app to behave properly again and added a 'timeout' parameter, which will achieve the desired results without having to resort to multiple calls to the app (why have all that overhead?).  Also reworked several bits of code to be more compact and use more descriptive variable names.  And the previous modifications (WAITSTATUS chan var, exit after timeout idea) have been cleaned up and incorporated. So now, both modes of use can be employed without major changes by either group of previous users.
Comments:	By: davetroy (davetroy) 2006-02-25 09:46:19.000-0600 Also changed silence threshold from 64 to 128, which seems to work a lot better for this application. By: davetroy (davetroy) 2006-02-25 10:42:51.000-0600 Tweaked patch to treat "no audio" condition (possible on direct digital channels, esp asterisk->asterisk) as an "infinite silence" exit condition.  Previously it would return -1 and hangup the channel. Use patch2 file instead. By: Andrey S Pankov (casper) 2006-03-01 22:10:49.000-0600 silencethreshold=128 really works better! :) By: Andrey S Pankov (casper) 2006-03-02 15:41:01.000-0600 It seems like treating of "no audio" condition is not good... the application will return in 40ms of no audio, right? IMO, that should be fixed to return after silencereqd ms. By: davetroy (davetroy) 2006-03-02 15:54:34.000-0600 Well, I'm making the assumption that "no audio" is a terminal/error-like condition and will make it impossible to get frames from ast_waitfor.  Note we also have to deal with the possibility of doing ast_waitfor twice in case of pseudochannels. I suppose I could invoke each ast_waitfor with a timeout of silencereqd/2. That get the equivalent of 'silencereqd' silence, which is all that loop's looking for.  Thoughts? By: Andrey S Pankov (casper) 2006-03-03 15:35:56.000-0600 I'm not sure if calling ast_waitfor twice is still needed or it was something "magic" in obscure times of pre-0.x. :) Any chance you ask this on asterisk-dev? Don't you think WaitForSilence will be obsoleted in favor of AMD? Or duplicating functionality is not "mauvais ton" unless markster doesn't have an eye on it? (That's about cdr_csv vs. cdr_custom committed recently... never mind... Personnaly me, I like and use WaitForSilence) By: Olle Johansson (oej) 2006-03-29 19:15:08.000-0600 We need one patch to fix a broken app in 1.2 and another patch for improving functionality in trunk. By: Andrey S Pankov (casper) 2006-03-29 19:59:55.000-0600 The problem is in what to consider improvements... :) For 1.2 one may just carefully revert 5888 and update README.variables. Or apply app_waitforsilence.patch (I didn't review the code, sorry). Anyway second patch is not ready for SVN yet. I'd like the patch to be commited... let's ask russel? By: davetroy (davetroy) 2006-03-30 11:05:29.000-0600 I have not had a chance to carefully review the AMD/answering machine detection stuff but it sounds interesting and as though it will have some overlap with current uses for WaitForSilence.  However, I can see all kinds of situations in complex IVRs, etc, where WaitForSilence is useful and AMD would not be. Anyway, I think that the patch I have submitted is a big improvement over the original app (which I submitted) and should be applied to 1.2.  It also fixes broken behavior introduced between the original and now.  I have not made any other changes to WaitForSilence in the interim and have been using my patch in production for over a month now. By: Andrey S Pankov (casper) 2006-03-30 15:00:33.000-0600 davetroy: please read carefully what oej writes. :) There will be no improvements commited to 1.2, fixes only. Can you propose/provide a _fix_ please? The 2nd patch is not ready for trunk because of 40ms problem I mentionned in note 0041953. There is app_machinedetect.c somewhere which had already this "noaudio" code. Still there is a question about dual ast_waitfor which exists only in the very old asrerisk code. Other improvement is to signal hangup condition with WAITSTATUS=HANGUP. I'd propose to merge both AMD and WaitForSilence even preserving WaitForSilence name for backward compatibility to not duplicate code. It would be nice to review app_amd, app_waiforsilence and ast_app_getvoice, ast_play_and_record, ast_play_and_prepend functionality to have more generic ways to treat silence/non-silence conditions. By: davetroy (davetroy) 2006-04-13 13:01:38 Sorry, I have not had any time the last few days to look at this.  Between the release of astmanproxy 1.2 and my real work, this has been on the back burner. I will try to work on this some over the next week.  What app currently implements best practice on this no audio condition? I intend to review app_amd, app_waiforsilence, ast_app_getvoice, ast_play_and_record, and ast_play_and_prepend.  What else?  Where can I find app_machinedetect.c? By: Andrey S Pankov (casper) 2006-04-13 13:35:36 > Where can I find app_machinedetect.c? http://www.voip-info.org/wiki/index.php?page=Asterisk%20cmd%20MachineDetect%20(addon) Please look at the discussion in http://bugs.digium.com/view.php?id=6911 By: Andrey S Pankov (casper) 2006-05-04 17:16:41 Let's fix silencethreshold in 1.2 and trunk first at least... Patch uploaded. Disclaimer is on file. By: Andrey S Pankov (casper) 2006-05-06 15:23:09 vechers: please mark this as "pending stable" for Russell to commit. Russell: please do not close the bug after commit, thanks! By: Joshua C. Colp (jcolp) 2006-05-22 16:29:12 The threshold change is now in 1.2 and trunk. By: Serge Vecher (serge-v) 2006-05-22 16:53:04 what else needs to be done here? By: Andrey S Pankov (casper) 2006-05-23 20:44:24 WaitForSilence should be "unbroken"... :) By: Serge Vecher (serge-v) 2006-06-05 20:06:32 dave: are you still working on this? Need to move quickly in order for this to be part of 1.4 beta .. By: davetroy (davetroy) 2006-06-07 10:00:25 I'm on travel and won't have any time to spend on this until this weekend.  I think, though, that the original fix that I contributed when I opened this bug is appropriate for the 1.4 branch.  The fixes that others have made to 1.2 are also appropriate.  The only question that needs to be answered now is whether the patched version that I submitted does everything in accordance with current best practices (namely the dual ast_waitfor).  If there is a current best practice on that, let's make that change and get this in 1.4 beta. I will try to review this some this weekend. By: Serge Vecher (serge-v) 2006-06-07 10:03:38 davetroy: sounds good -- the dev team is in rapid development mode now, so if you get a chance to pop-in on the asterisk-dev channel; I think that will be the best way to make sure that all issues are addressed at the patch is ready for commit. By: John Walliker (jrwalliker) 2006-06-12 05:17:24 Just a comment on the "no audio" condition. I have noticed that pressing the microphone mute button on a snom 190 phone stops audio frames from being sent until the mic is unmuted again.  This presumably counts as a "no audio" condition which is not an "infinite silence". John By: davetroy (davetroy) 2006-06-29 15:28:32 In this revision (app_waitforsilence.c 6-29-06) I significantly reworked the logic to account for digital silence and generally cleaned up the code. I don't have an instance of trunk setup to test with at the moment, so if some of you could please test this and let me know that it seems to work 1) as before with standard channels, 2) with digital silence (as in the snom muting mentioned by jrwalliker), I would appreciate it. Assuming this is all kosher then it should be ready to go.  Crossing fingers. For what it's worth, I think AMD suffers from the same digital silence issue.  Once we are sure waitforsilence is ok it may be a good idea to open an bug report on it as well. By: Serge Vecher (serge-v) 2006-06-29 15:40:08 davetroy: most of the new features that were supposed to go into 1.4 beta already did and I've been marking 'feature issues' as [post 1.4] for two weeks now. I don't know if kpfleming has agreed to get this into 1.4, so at this point, if you need this in 1.4 beta, please contact him by email or on asterisk-dev. Thanks. By: davetroy (davetroy) 2006-06-29 15:44:18 OK, I will email Kevin.  The version that is in trunk as of today is definitely broken, though, and what I'm supplying here doesn't add any new features but *should* a) fix the current design flaw that was introduced, b) eliminate the digital silence problem. By: davetroy (davetroy) 2006-06-30 07:50:32 Kevin this AM: If it's a bug fix it will go in; there is no restriction on when bug fixes can be merged. -- Kevin P. Fleming Senior Software Engineer Digium, Inc. By: davetroy (davetroy) 2006-06-30 11:01:39 Tested against trunk; silence detection, timeout and loops work as they should and this should be good to go for 1.4 beta. By: Mike Wade (mwade) 2006-08-21 17:52:23 This patch breaks when waiting for silence and someone hang's up.  It spins until the timeout...  It needs a break; if ast_read() fails. By: BJ Weschke (bweschke) 2006-09-03 15:24:47 Committed to /trunk in rev 41915. Thanks Dave!