Summary:	ASTERISK-05216: [patch] Choppy dial tone on Linksys routers / FPU usage in indications.c
Reporter:	inducer (inducer)	Labels:
Date Opened:	2005-10-02 23:23:19	Date Closed:	2008-01-15 15:51:17.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) asterisk-1.0.7-no-fpu-indications.patch ( 1) asterisk-cvs-no-fpu-indications.patch
Description:	Here's a patch that switches tone generation in indications.c over from using actual floating point code to using a pregenerated sine table. This patch allows my poor FPU-less Linksys router to successfully play a dialtone with almost zero CPU usage. (If you care about the details: Generating the 8000-entry sine table at startup takes a bit more than a second on that machine, which is a MIPS using an FPU emulator. Since each sample out of playtones_generator() takes two sine evaluations and we're sampling at 8000Hz, the Linksys is too slow by about a factor of two.) This might even save some cycles on bigger setups. The patch applies cleanly and works on 1.0.x with x>=7, on 1.2.x it needs to be told where asterisk.h is, but otherwise seems to work fine.
Comments:	By: Russell Bryant (russell) 2005-10-04 15:27:56 Since this isn't necessarily a bug, it won't be included in the 1.0 tree.  Please update the patch to be against CVS HEAD. Something to consider is that Asterisk uses a define called LOW_MEMORY.  Some people might not want the extra 16k used by this.  However, most people probably already build Asterisk with LOW_MEMORY defined for a Linksys, or other embedded platform, but would obviously benefit from this.  Maybe it could be another define that is on by default.  I don't know ... I just wanted to throw it out there for discussion. By: Kevin P. Fleming (kpfleming) 2005-10-04 18:45:42 It's a tough call whether LOW_MEMORY should be used for this or not; I think I'd rather see an option in indications.conf to control whether the pre-generation is done or not. By: Russell Bryant (russell) 2005-10-04 18:55:30 Good call.  What do you think about the default value could be based on whether LOW_MEMORY is defined or not? By: Kevin P. Fleming (kpfleming) 2005-10-04 19:01:33 Yeah, the default could be 'on' when LOW_MEMORY is defined, but I'd rather not... it would be hard to describe in the sample config file :-) By: inducer (inducer) 2005-10-04 19:10:36 Updated patch for CVS. By: Kevin P. Fleming (kpfleming) 2005-10-04 19:15:13 We will still need a disclaimer from you before we can effectively review and merge your work :-) By: inducer (inducer) 2005-10-04 19:16:00 The way I see it, this only becomes a problem when there's plenty of CPU/FPU horsepower, but really *no* memory. (Remember, this is 16k per instance--not per connection, per thread, or anything else.) Is there anybody out there whose memory is that tight? As you noted, machines with LOW_MEMORY defined are typically the ones most likely to want the pregeneration, thus using more memory. Hmm. By: Russell Bryant (russell) 2005-10-04 21:48:44 kpfleming:  I was thinking more along the lines of having this default to 'off' if LOW_MEMORY is defined, but whatever.  :) Every time the discussion of LOW_MEMORY comes up, someone always says, "but it's only XXX bytes!".  I know, I know, ... but a lot of these optimizations add up to be significant, and that is the whole point of having it. I just had another thought ... what if the sine table was pre-generated and we just throw it into a header file instead of having it generated at run time? By: Mark Spencer (markster) 2005-10-13 16:40:28 Have you considered using the method that zaptel uses for calculating the coeffients and operating? By: Mark Spencer (markster) 2005-10-15 23:12:43 Fixed in CVS head using the zaptel.c method By: Digium Subversion (svnbot) 2008-01-15 15:51:17.000-0600 Repository: asterisk Revision: 6796 U   trunk/indications.c ------------------------------------------------------------------------ r6796 | markster | 2008-01-15 15:51:16 -0600 (Tue, 15 Jan 2008) | 2 lines Make indications work more efficiently and without heavy floating point (bug ASTERISK-5216) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=6796