Summary:	ASTERISK-15125: [patch] g726 to g726aal2 translation and cost-calculation are wrong but easily fixed.
Reporter:	Jon Considine (jonconsi)	Labels:
Date Opened:	2009-11-12 07:09:43.000-0600	Date Closed:	2011-06-07 14:00:19
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Codecs/codec_g726
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) codec_g726.c ( 1) codec_g726.c.diff ( 2) codec_g726.c.patch ( 3) ex_g726.h.diff ( 4) issue16230_1.6.1_20100124.diff
Description:	As detailed in Issue 15504, sometimes Asterisk incorrectly calculates that it is lower-cost to go from g726->g726AAL2->linear instead of directly from g726->linear. This exposed a bug in the g726->g726AAL2 (or vice-versa) translation, which is a simple nybble re-ordering. This bug gives rise to badly distorted and choppy audio. The fix given in 15504 was to delete the g726->g726AAL2 translation but this is unneccessary. ****** ADDITIONAL INFORMATION ****** There are two steps to completely fixing this issue. 1) Correct the g726-g726AAL2 translation. The translation fails because each frame should have the same input and output length of 80bytes. However, after re-packing the frame by swapping the nybbles the output data length is incremented by 160. 2) Correct the cost-calculation for g726AAL2 This cost-calculation is wrong because when PLC is enabled for g726 it is currently *only* enabled for g726 and not the g726AAL2 variant. Therefore, the g726AAL2->linear translation gets a lower cost and when added to the almost negligible cost of g726-g726AAL2 comes out lower than straight g726->linear with PLC added. This is not always the case, because of processor timing variation etc. However, with fix (1) above, it doesn't matter if it Asterisk does occasionally pick the g726->g726AAL2 translate (and on some systems this will never happen anyway) as the translation is fixed.
Comments:	By: Jason Parker (jparker) 2009-11-12 11:16:45.000-0600 You'll need to upload this as a patch. By: Sean Bright (seanbright) 2010-01-24 15:44:59.000-0600 Created a diff from OP's post and attached it.  Patch is against 1.6.1 as I couldn't get it to compile under trunk. By: Scott Johnson (globalnetinc) 2010-01-26 01:01:35.000-0600 do you have a patch for 1.6.2 By: Jon Considine (jonconsi) 2010-01-26 04:20:35.000-0600 I've uploaded a patch for 1.6.2. However, I do not have a 1.6.2 build environment, so if someone else could test it that would be much appreciated. I kept the 1.6.2 header names - that's the bit I'm not sure about. By: Sean Bright (seanbright) 2010-01-26 15:34:57.000-0600 jonconsi - unified diffs are preferred if you would like them reviewed.    diff -u file1 file2 By: Scott Johnson (globalnetinc) 2010-01-26 20:13:20.000-0600 does not patch correctly and does not compile on 1.6.2.1 codec_g726.c: In function âg726tolin_sampleâ: codec_g726.c:793: error: âg726_slin_exâ undeclared (first use in this function) codec_g726.c:793: error: (Each undeclared identifier is reported only once codec_g726.c:793: error: for each function it appears in.) codec_g726.c: In function âlintog726_sampleâ: codec_g726.c:807: error: âslin_g726_exâ undeclared (first use in this function) By: Jon Considine (jonconsi) 2010-01-27 08:27:20.000-0600 Uploaded revised unified diffs. This was built in the 1.6.2.1 tarball, but not yet tested with a g726 UA. By: Scott Johnson (globalnetinc) 2010-02-02 00:38:31.000-0600 Add the 2 diffs to 1.6.2.1 and g726 seems to work well. By: Jason Parker (jparker) 2010-02-02 11:56:24.000-0600 The XtoY_samples method has been intentionally changed, since it is not required.  You should continue using g726_samples, and slin8_samples (rather than g726toslin_samples and slintog726_samples).  If they need to be corrected somehow, that's fine, but please do not re-add those structures. By: Leif Madsen (lmadsen) 2010-03-23 10:52:34 Is the reporter still interested in moving this forward based on Qwell's comments? By: Paul Belanger (pabelanger) 2010-04-28 16:04:17 Ping! Like to see if the reporter is working on this. By: Jon Considine (jonconsi) 2010-05-04 04:02:46 Not currently - I'm working with an older build at the moment, hence my changes being out of step with the latest 1.6 build. By: Paul Belanger (pabelanger) 2010-06-25 09:04:53 Suspending for now, when you have time resubmit your patches.  Would be nice to see this in 1.8. --- Suspended due to lack of activity. Please request a bug marshal in #asterisk-bugs on the IRC network irc.freenode.net to reopen the issue should you have the additional information requested. Further information can be found at http://www.asterisk.org/developers/bug-guidelines