[asterisk-bugs] [Asterisk 0008126]: [patch] G.711 codec woes

[noreply at bugs.digium.com]

A NOTE has been added to this issue. 
====================================================================== 
http://bugs.digium.com/view.php?id=8126 
====================================================================== 
Reported By:                fossil
Assigned To:                murf
====================================================================== 
Project:                    Asterisk
Issue ID:                   8126
Category:                   Core/CodecInterface
Reproducibility:            always
Severity:                   minor
Priority:                   normal
Status:                     ready for testing
Asterisk Version:            SVN 
SVN Branch (only for SVN checkouts, not tarball releases):  1.2 
SVN Revision (number only!): 44743 
Disclaimer on File?:        Yes 
Request Review:              
====================================================================== 
Date Submitted:             10-09-2006 20:21 CDT
Last Modified:              08-20-2007 16:49 CDT
====================================================================== 
Summary:                    [patch] G.711 codec woes
Description: 
There is a *number* of problems in the a-law and u-law core transcoders
(most severe first):

1. a-Law decoder does not add the rounding error to the linear samples
output;
This results in a stable amplitude drop in the decoded signal overall, but
the negative phase portion of the signal is even more adversely affected:
the amplitude drop actually accumulates with consequtive transcodings (see
attached test patch). If the call encounters 127 tandem a-law transcodings
(a-alaw -> slin -> a-law -> slin -> ...), the entire negative portion will
be reduced to http://bugs.digium.com/view.php?id=#0.

2. Lookup table-driven slin->law coding rounds the negative values the
wrong way;
The breaks in linear value sequences do not happen where the table-driven
slin->law system expect them to. This results in certain negative linear
values to be encoded incorrectly (see attached test patch), which isn't
such a *big* problem, but a problem nonetheless.
There is no one-liner fix for this issue. To fix this, for example, we
could generate only half the slin->law table, for positive values only.
This table would contain half-cooked law bytes, so that the sign could be
added later to the values, along with the post-coding transform (NOT for
u-law and XOR 0x55 for a-law). In this case, AST_LIN2MU() would look
something like this:

inline unsigned char AST_LIN2MU(short sample)
{
  unsigned sign = ((unsigned)sample & 0x8000) >> 8;
  unsigned char law = __ast_lin2mu[(sample & 0x7fff) >> 2];
  return ~(law | sign);
}

3. slin->a-law and slin->u-law functions handle value -32768 incorrectly;
This is not really a problem when using a lookup table system because the
slot of -32768 is overwritten later, but for the sake of correctness...

4. alaw.c:linear2alaw() is less than optimal;

5. slin->law lookup table generation code is less than optimal;
There is no reason to enumerate all the possible values between -32768 and
32767 when most of the results are overwritten later.

====================================================================== 

---------------------------------------------------------------------- 
 fossil - 08-20-07 16:49  
---------------------------------------------------------------------- 
I honestly cannot believe anyone would willingly choose a broken but faster
implementation of something as trivial as G.711, but if you must go the
#ifdef way, all right then, let's at least merge the versions in a better
way (and I promise to try to contain my rants re: correctness vs. speed). I
will merge the code the way I think it should be and submit. If, however,
you insist on merging the way you did it, please at least apply the fix for
issue (1) to the original version.

In general, most of the issues raised here and fixed in the patch do not
impact the codec speed at all. The exception is issue (2), the fix for
which apparently slows down the slin->law translation. Taking this into
account, we can merge the two versions quite elegantly, with minimal #ifdef
sections.

I would turn on the tests in both versions, but in modified form -- type
and amount of output controlled by verbose/debug options. I never really
meant the tests to be used in their current form on production systems. The
tests should *theoretically* be used once -- to confirm correctness, and
then never again. Seeing coding tables warnings in the logs may encourage
users to switch to the new version ;-).

As for menuselect stuff, I cannot weigh in, do whatever you feel is right.
I would not turn on REDUCED_BRANCHING by default, however, because it ends
up being slower on newer systems with newer gcc (as indicated by my
system). 

Issue History 
Date Modified   Username       Field                    Change               
====================================================================== 
08-20-07 16:49  fossil         Note Added: 0069117                          
======================================================================