Summary:	ASTERISK-04207: [patch] allows for silence suppression
Reporter:	cmaj (cmaj)	Labels:
Date Opened:	2005-05-17 13:15:02	Date Closed:	2011-06-07 14:04:57
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) dsp_silence_suppression.diff2.txt
Description:	Maybe I'm missing the boat on this one, but calling ast_dsp_set_features was blowing away the silence suppression -- which is turned on by default when calling ast_dsp_new.  This itty bitty patch will start returning null frames when silence is detected, and it was tested on zap channels with "debug channel all" and some test calls. ****** ADDITIONAL INFORMATION ****** Disclaimer on file.
Comments:	By: Olle Johansson (oej) 2005-05-17 16:33:39 I don't know this function at all, but from reading the code it seems like you are not using the argument at all anymore, so if the base idea of this patch is right, you need to remove the argument... Is it the CALLING part that should set the values you replaced the parameter with instead of changing the function to always do the same thing? I might be totally off base, but just wanted to drop this syntax nitty-gritty note. ;-) By: cmaj (cmaj) 2005-05-17 19:57:53 You're right, it doesn't look right.  But the argument is still being used.  It's just being *enhanced* with the addition of silence suppression. The issue I had was that silence suppression is always turned on when you create a DSP (via ast_dsp_new), but then set_features lets you blow it away.  It seems to me like suppressing silence -- something that could/should save bandwidth -- is always a good idea.  And that's why it's set on by default when creating a DSP. By: Michael Jerris (mikej) 2005-05-17 20:09:57 Silence suppression has downsides as well such as clipping of audio.  While it is a nice feature, shouldn't it be optional instead of the new default? By: cmaj (cmaj) 2005-05-17 20:46:09 I guess I wasn't aware there were downsides.  Making it an option would be a very good idea in that case. Maybe making some new field in the frame struct to indicate that the DSP thinks it's silence could be helpful?  Then you could still pass the audio, if you wanted to, but checks on that field would give you more options up in the channel drivers.  Then you could put the "silencesuppression=yes" option in each of the channel configuration files.  Otherwise, I don't know where it could be toggled. By: cmaj (cmaj) 2005-05-18 08:38:33 First, bkw918, sorry about marking this major when it was minor, my bad. But MikeJ I have noticed little "pops" on the Zap-Zap bridged lines now with the suppression active.  So it's definitely having a slightly negative impact on quality.  I'm going to re-work this patch to disable silence suppression in ast_dsp_new, make it selectable from at least zapata.conf, and add some new mechanism to frames to mark them as silent. My goal for this is to aid in detection of dead analog lines (they just went silent) and also to help out in situations where bandwidth is limited and you need to decide what to throw away. By: Michael Jerris (mikej) 2005-05-18 08:45:22 CNG frames, like being used in 4222? By: cmaj (cmaj) 2005-05-18 09:31:17 Sure, when you turn on the option to suppress silence frames it would be good to convert them to CNG frames instead of just being null. I just did some more digging, and this stuff is already there in phone.conf and alsa.conf/oss.conf, so it is not without precedent.  The latter two let you define the threshold, which would also be useful. The only difference would be with a new "silencedetect=yes" option to toggle something in the frame.  Maybe an AST_OPTION... By: cmaj (cmaj) 2005-05-18 12:15:11 Well, one thing led to another, and I am currently adding an "energy" int to the frame struct.  Besides making this patch a whole lot bigger, it will help make the dsp a little more modular.  This way, instead of just adding silence detection and suppression, it will also add chaotic noise detection and suppression.  (I recall on the conference call last week a few moments of very loud screeching madness where some people had to throw off their headsets.) By: cmaj (cmaj) 2005-05-18 20:23:21 diff2 does not add chaotic noise suppression, but it does add the creation of CNG frames when silence suppression is enabled. By: Michael Jerris (mikej) 2005-06-23 06:35:24 kevin-  can you review this one please. By: Michael Jerris (mikej) 2005-07-25 21:57:21 This has been sitting for quite some time.  We need this updated for current cvs head and testing comments.  I am going to suspend this bug pending those updates.  Re-open when updates are ready.