Summary:	ASTERISK-02771: [patch] correctly reinit a variable and other more substantial modifications to MARK2 echo canceller
Reporter:	kb1_kanobe2 (kb1_kanobe2)	Labels:
Date Opened:	2004-11-09 02:57:54.000-0600	Date Closed:	2011-06-07 14:04:56
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) mec2_const.h-fullyreformatted ( 1) mec2.h-fullyreformatted ( 2) mec2.h-pt1-fixmissingreinit.patch ( 3) mec2.h-pt2-refactorconvergencefactorcalculation.patch
Description:	This is a patch in two parts. The first part adds a missing reinitialisation of 'ec->Lu_i' after the echo can is restarted. Also, the special case where 'ec->max_y_tilde = 0' has accounted for to prevent triggering near constant false near-end speech detection when the channel is very quiet, such as on PRI facilities. The second part makes a stab at adding a couple of degrees of instrumentation to the MARK2 canceller to aid in tuning constants. As well the formatting has been somewhat normalised and various verbiage has been stirred in to assist the lay reader. ****** ADDITIONAL INFORMATION ****** I would like to hear some feedback on this as it may have "consequeces" for people who've already got their echo supression to behave - particularly the handling of 'ec->max_y_tilde = 0' Disclaimer on file.
Comments:	By: Mark Spencer (markster) 2004-11-09 09:02:07.000-0600 Looks like the Lu_i initialization is right.  However, the max_y_tilde change appears to incorrect.  If the transmitted signal is not greater than the received signal then (even if they're 0) there should not be training.  Am I missing something? By: kb1_kanobe2 (kb1_kanobe2) 2004-11-09 12:41:40.000-0600 In the light of a new day I think you're right about the max_y_tilde change. It seems that I tickled a mistaken reference to DEFAULT_ALPHA_ST_I that should instead be DEFAULT_ALPHA_YT_I when I was fiddling with the constants. It caused y_tilde_i to slowly, but steadily, grow unrecoverably more negative - hence causing max_y_tilde to tend to be 0. Take a look at the updated diff. By: Mark Spencer (markster) 2004-11-17 00:09:57.000-0600 I merged the actual fix, but not the instrumentation change.  Can we get it updated and without C++ style comments?  Also I want to be sure that without debugging enabled there MUST be a 0 performance penalty right? By: kb1_kanobe2 (kb1_kanobe2) 2004-11-17 02:09:22.000-0600 Looking at the patch that went into CVS I think I must have uploaded an updated fix just as you applied the changes - there was one additional variable that wasn't reinited and I don't see it in mec2.h Regardless, I will work on tidying up the instrumentation, comments and such and ensuring it's a compile time option that defaults to not included. By: Paul Cadach (pcadach) 2004-11-17 11:52:23.000-0600 Is it possible to have ioctl() to get echo canceller statistic and performance back to the user space (for example, ztdiag) to be represented as user-readable form (local end echo level, remote end echo level, FIR characteristics)? As I understand to not provide any penalties there is needs one function to get statistic back to zaptel from ECs internal structures. IMHO such type of diagnostic should be provided for all existing ECs... By: Olle Johansson (oej) 2004-12-12 16:00:12.000-0600 Where are we with this report? Any updates? Ready for closure? /Housekeeping By: kb1_kanobe2 (kb1_kanobe2) 2004-12-13 12:56:45.000-0600 I haven't been able to get back to this - too many competing issues. If you'd like to close it for now for bugtracker cleanliness then I'll reopen it and post the updated code when it becomes available. Thoughts? By: twisted (twisted) 2004-12-28 10:44:13.000-0600 closed per poster - will re-open when update available. By: kb1_kanobe2 (kb1_kanobe2) 2005-03-08 04:15:02.000-0600 Attached are drop-in replacements for mec2.h and mec2_const.h that have been reworked for ease of understanding, readability and documentation. I have been running this code without difficulty since mid-December, however as with all significant modifications your mileage may vary. I can only think of one change of note to the code itself - beginning line 303 the recalculation of Py_i has been refactored to conditionally avoid a potentially unneccessary calculation. There are comments and calculations here and there that were preserved from the original code - I trust these will mean something to someone as I have not been able to discern their origin. Similarly the yet-to-be converted floating point calculations have been removed entirely as they appeared to have little value without documentation. I had hoped to work on the performance stats more before resubmitting this code but have not had time and I like the idea of using an ioctl() interface as suggested by PCadach. Perhaps someone can pick this up and revise it further... By: Brian West (bkw918) 2005-03-17 21:38:03.000-0600 Any update?  Is  this ready? /b By: damin (damin) 2005-03-17 22:33:10.000-0600 I've applied this to my Development box and will report back in a couple of days if I notice anything funky. Compiled fine. By: Paul Cadach (pcadach) 2005-04-02 04:02:11.000-0600 Any news about this ticket is available? By: damin (damin) 2005-04-02 10:04:32.000-0600 So far so good. I haven't noticed any major issues w/rt to echo and even more important, my WIFE hasn't complained about anything. I can't notice any appreciable audio quality differences, and we've done about 800 calls using this code. If no one else is willing to test it, I'd advocate throwing it in to see if anyone notices. :) By: Paul Cadach (pcadach) 2005-04-07 08:12:04 kb1_kanobe2: Could you fix formatting a little? markster: Is it possible to come to CVS after formatting is updated? By: kb1_kanobe2 (kb1_kanobe2) 2005-04-07 12:04:54 PCadach, which formatting? I understood the replacement files dated 8 March to meet Marks stated requirements. The 9 November files are the earlier patches refered to at the beginning of this issue. However, point out my transgressions and I'll be glad to fix. :-) By: k3v (k3v) 2005-05-18 15:37:55 I've been running the drop-in headers for about a week, and I haven't heard one echo complaint from staff (I was getting them daily).  4000+ calls have gone though mec2.  Stable.  No problem.  2100hz detection working fine too. By: Mark Spencer (markster) 2005-05-18 15:55:53 Do we have a diff?  Why are these whole files? By: kb1_kanobe2 (kb1_kanobe2) 2005-05-18 21:22:04 I uploaded the drop-in replacements because of the degree of reordering, commenting and cleanup - see note 0023331. If you would prefer diffs against mec2.h from head I can purge all the files attached here and provide those instead. Comments? By: kb1_kanobe2 (kb1_kanobe2) 2005-05-18 21:24:55 k3v - did you change any constants in mec2_const.h? Particularly, did you adjust the MIN_UPDATE_THRESH_I value? By: kb1_kanobe2 (kb1_kanobe2) 2005-05-19 00:56:15 To help end the apparent confusion around this bug, I have purged the two patches against stable and the two whole-file replacements intended for cvs. There are now two diffs attached, relative to cvs-head. I trust this change will finally allow this bug to be closed. By: k3v (k3v) 2005-05-19 11:47:30 I didn't change anything.  Also, I just got off an 8-way conference call and I usually hear some echo from the number of analog members, and it was clean and clear. By: kb1_kanobe2 (kb1_kanobe2) 2005-05-19 14:36:43 k3v - ok, thanks. Correctly reseting that other variable between calls (note 0017117) must have been a contributing factor for your particular issue. Thanks for the feedback. By: Mark Spencer (markster) 2005-05-25 14:12:03 kb1: I'd like to discuss this patch with you before putting it in, but thanks for fixing up the formatting, although it is clear that this is a total rewrite! mark By: kb1_kanobe2 (kb1_kanobe2) 2005-05-26 02:30:27 Mark: I'm available at your convenience; we seem to keep mostly different schedules on irc. Perhaps drop me an email at kris.boutilier@scrd.bc.ca when you're around and I'll drop in to discuss. By: Tilghman Lesher (tilghman) 2005-06-26 13:43:50 Housekeeping request - 30 days of inactivity - can we get this one moved along? By: kb1_kanobe2 (kb1_kanobe2) 2005-06-29 19:12:16 Not really certain how I can assist moving this one. I could re-upload the drop in files in addition to the diffs - would that speed things up? By: Michael Jerris (mikej) 2005-07-26 22:08:02 can you please break this up into pieces, the fix first, and then the rework pieces. By: kb1_kanobe2 (kb1_kanobe2) 2005-08-05 06:09:05 This is now a patch in two phases to ease code review. The first consists of two material patches against the current mec2 echo canceller code. In 'mec2.h-pt1-fixmissingreinit.patch' I have broken out the patch that corrects the missing reinitiallisation of a variable before each use of an echo can instance. In 'mec2.h-pt2-refactorconvergencefactorcalculation.patch' I have refactored a couple of lines of code to eliminate a freqently unneeded calculation. The other two files provide mec2.h and mec2_const.h drop-in replacements for the files currently in cvs that included the two patches documented above, some rudimentary (and entirely optional) instrumentation for the echo can code and, as an added bonus, a complete reworking of the formatting and presentation to be more consistent with the coding style of Asterisk, per Marks request of last November. Finally, the layout has been tweaked and the comments have been beefed up, based on the information contained in the Texas Instruments whitepaper cited, to foster others to understand and experiment with the 'mec2 black box'. I'm at a bit of a loss how to present the rework and instrumentation as anything other than a whole file - diff is quite insistent about producing one single 567 line patch. I trust the intent is clear enough, that the new files are simpler to read and, now that the material changes have been broken out for inspection, that we can get this 9 month old contribution quietly slipped into cvs? :-) By: Matthew Fredrickson (mattf) 2005-08-23 18:22:51 Ok, this is what we'll do.  We'll add the reinit variable and the little optimization patch to the current mec2.h.  For the drop in replacements, we'll add another echo can option to zconfig.h so that we can get the rewritten versions more exposure "in the wild". By: k3v (k3v) 2005-08-23 20:08:10 I've been using this patch for some time now.  Prior, I heard echo complaints from in-house fairly steadily, and now not a single one for 3 months (except the caller hearing our headsets bleeding, grr).  On large meetme bridges, no echo, ever.  I submit this is a Good Thing for 1.2. By: Michael Jerris (mikej) 2005-08-24 00:22:30 mattf commited the fix portion of this as: Small fixes for mec2 echo can from ASTERISK-6752 diff -u -d -r1.4 -r1.5 --- mec2.h 9 Nov 2004 19:16:18 -0000 1.4 +++ mec2.h 24 Aug 2005 00:45:04 -0000 1.5 By: florian (florian) 2005-08-24 03:46:36 Any chance this can run on 1.0 at some level ? (We're looking into this now, but I'd appreciate comments from others) By: Matthew Fredrickson (mattf) 2005-08-24 14:47:47 Ok, I committed the minor fixes last night.  I'll put the big portion in today (i.e. this will be in 1.2 :-) ) By: Matthew Fredrickson (mattf) 2005-08-24 17:11:30 Finally, after nine long months it is in!  Thanks for your patience kb1. By: Digium Subversion (svnbot) 2008-06-07 10:46:22 Repository: dahdi Revision: 496 U   trunk/mec2.h ------------------------------------------------------------------------ r496 | markster | 2008-06-07 10:46:21 -0500 (Sat, 07 Jun 2008) | 2 lines Fix a couple of echo can issues (bug ASTERISK-2771) ------------------------------------------------------------------------ http://svn.digium.com/view/dahdi?view=rev&revision=496