Summary:	ASTERISK-04153: [patch] Speex VBR fix, VAD enable fix and VBR/ABR DTX enable fix
Reporter:	James Cowling (jcowling)	Labels:
Date Opened:	2005-05-12 20:13:15	Date Closed:	2008-01-15 15:38:58.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/CodecInterface
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) codec_speex_patch_new.txt ( 1) codec_speex_patch_NO_VBR_DTX.txt ( 2) codec_speex_patch.txt
Description:	(few small bugs but set this as 'major' since they completely prevent Speex VBR from working) Fixes the following bugs in codec_speex.c: * VBR (not ABR) was completely unintelligible owing to the wrong data type used in quality configuration - Speex VBR mustn't be very popular among Asterisk users :) * VAD may now be enabled for CBR streams - it was being set incorrectly when parsed from config file. * DTX may now me used for VBR and ABR, not just CBR.  See 'additional info' for clarification. Don't think this needs a disclaimer (just bug fixes) but let me know if required. ****** ADDITIONAL INFORMATION ****** There seems to be much confusion about the interaction between VAD/DTX and CBR/VBR/ABR.  For the record, my interpretation of these values is this: - VAD (voice activity detection) is *implicit* for VBR and ABR streams.  The setting only applies to CBR, not the other way around. - DTX (discrete transmission) is somewhat of an add-on for VAD, and thus needs to be specified where required, whether for VBR, ABR or CBR+VAD. (Feel free to try and convince me that DTX is also implicit for VBR and ABR - either way, enabling it for these streams does no harm.  I'm quite confident the above is correct however.)
Comments:	By: Michael Jerris (mikej) 2005-05-14 22:00:29 stevek, I know you have some understanding of VAD stuff is speex from app_conference.  Can you please review this. By: James Cowling (jcowling) 2005-05-30 22:06:46 Whoops, just noticed the VAD bug only prevents the VAD setting from being logged correctly, not the actual VAD setting itself :)  The change still holds however, and the VBR bug still stands. From Wikipedia re DTX: "Discontinuous transmission is an addition to VAD/VBR operation, that allows to stop transmitting completely when the background noise is stationary." ie. should be specified for ABR and VBR where requested, as my changes suggest. Just changed a very minor logging error I introduced and updated the patch file to the latest CVS version. By: James Cowling (jcowling) 2005-05-30 22:31:56 I've been doing some testing and found a very interesting bug: If DTX is enabled (in my case for ABR) using the patch, and a user hangs up while ringing another user, the other phone will keep ringing despite the fact that there is no active call.  When the phone is answered Asterisk seems to deadlock and the process must be killed. (the setup I was testing was SIP phone -> A* -> A* -> ISDN card -> PSTN phone) I'm at a total loss as to why this happens.  I've patched my own Asterisk setup for quite some time and it's only seemed to be a problem with more recent CVS versions (can't give you an exact date unfortunately). Unless someone else can shed some light on the subject my suggestion would be to abandon Speex VBR/ABR DTX in Asterisk for the timebeing.  I'll post a codec_speex_patch_NO_VBR_DTX.txt file to leave that option open. Can anyone offer any advice/feedback? By: Brian West (bkw918) 2005-06-01 10:24:48 Is this really a MAJOR bug?  I wouldn't think so. /b By: James Cowling (jcowling) 2005-06-01 20:09:53 See my "few small bugs but set this as 'major' since they completely prevent Speex VBR from working" comment... Just following "A bug which completely prevents Asterisk from operating in a method that it normally is expected to operate -- and particularly if it cannot be reasonably worked around -- is MAJOR. Significant protocol violations that are not simply policy decisions are MAJOR" guideline, in relation to my above comment.  Of course it's not a problem that is going to affect many people. Feel free to downgrade it. By: Michael Jerris (mikej) 2005-06-19 09:00:33 This is a feature as I do not beleive this has ever worked or been documented as working.  That being said, where do we stand on this?  Also, do you have a disclaimer? By: James Cowling (jcowling) 2005-06-19 20:25:37 DTX causes Asterisk to deadlock with the current CVS - it happened between the 12th and 14th of May but I haven't yet worked out why. I think it'd be best to leave the VBR DTX with me and apply the codec_speex_patch_NO_VBR_DTX.txt patch in the meantime.  That's just some straightforward bug fixes for VBR and logging. I'll open a VBR DTX feature later on if I can get it working with the current CVS. Is a disclaimer required for bug fixes?  Let me know and I'll send one in. By: Michael Jerris (mikej) 2005-06-19 20:42:44 Yes, disclaimers are required for bug fixes.  Thanks for all your hard work on this. By: James Cowling (jcowling) 2005-06-19 21:02:10 Just faxed in a disclaimer. No worries, James By: James Cowling (jcowling) 2005-06-20 23:57:33 Just found out the dead-lock when Speex DTX is implemented arose when ast_codec_get_samples() and speex_samples() was added to frame.c on the 15th of May.  Could take a little while to isolate the problem but I'll post a patch as a new minor bug when done (since it only deadlocks with my speex dtx patch). codec_speex_patch_NO_VBR_DTX.txt can be integrated any time.  codec_speex_patch_new.txt will have to wait till I've sorted the frame.c/rtp.c/chan_iax2.c business. By: Kevin P. Fleming (kpfleming) 2005-06-21 19:42:37 Committed the non-VBR/DTX version to CVS HEAD, thanks! By: Digium Subversion (svnbot) 2008-01-15 15:38:58.000-0600 Repository: asterisk Revision: 5964 U   trunk/codecs/codec_speex.c ------------------------------------------------------------------------ r5964 | kpfleming | 2008-01-15 15:38:57 -0600 (Tue, 15 Jan 2008) | 2 lines various speex fixes (bug ASTERISK-4153) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=5964