[Home]

Summary:ASTERISK-03777: [patch] Architectures without unaligned accesses unable to properly negotiate codecs
Reporter:mtaht (mtaht)Labels:
Date Opened:2005-03-26 09:56:15.000-0600Date Closed:2008-01-15 15:39:34.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) alignment_fix_and_endian_cleanup.txt
( 1) alignment-patch.txt
( 2) endian-bsdfix.patch.txt
( 3) 3867.stable.patch.txt
( 4) endian.h
Description:On iax2, no codec other ulaw works on a big endian Xscale iax2 -> little endian iax2 machine. Attempts to negotiate a codec fail with a "no such capability" error...

****** ADDITIONAL INFORMATION ******

turns out that the port to solaris needed to be generalized to just big endian... I note that a similar fix is probably needed in rtp.c, and there are probably faster ways to byte swap...

Index: iax2-parser.c
===================================================================
RCS file: /usr/cvsroot/asterisk/channels/iax2-parser.c,v
retrieving revision 1.38
diff -u -r1.38 iax2-parser.c
--- iax2-parser.c       17 Mar 2005 23:12:15 -0000      1.38
+++ iax2-parser.c       26 Mar 2005 15:39:06 -0000
@@ -21,6 +21,21 @@
#include <unistd.h>
#include <stdlib.h>
#include <stdio.h>
+#if defined( __OpenBSD__ )
+#  include <machine/types.h>
+#  include <sys/endian.h>
+#elif defined( __FreeBSD__ ) || defined( __NetBSD__ )
+#  include <sys/types.h>
+#  include <sys/endian.h>
+#elif defined( BSD ) && ( BSD >= 199103 ) || defined(__APPLE__)
+#  include <machine/endian.h>
+#elif defined( __sparc__ ) && defined( SOLARIS )
+#  define BIG_ENDIAN 4321
+#  define BYTE_ORDER BIG_ENDIAN
+#  define __BYTE_ORDER __BIG_ENDIAN
+#else
+#  include <endian.h>
+#endif
#include "iax2.h"
#include "iax2-parser.h"
#include "iax2-provision.h"
@@ -30,13 +45,13 @@
static int iframes = 0;
static int oframes = 0;

-#if defined(SOLARIS) && defined(__sparc__)
-static unsigned int get_uint32(unsigned char *p)
+#if __BYTE_ORDER == __BIG_ENDIAN
+static inline unsigned int get_uint32(unsigned char *p)
{
  return (p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3];
}

-static unsigned short get_uint16(unsigned char *p)
+static inline unsigned short get_uint16(unsigned char *p)
{
  return (p[0] << 8) | p[1] ;
}
Comments:By: Mark Spencer (markster) 2005-03-26 10:53:36.000-0600

Please attach a patch in diff -u format and a .txt extension and let us know when you have submitted your disclaimer.  For more information, please read the bug guidelines highlighted in yellow when you place a bug.  Thanks!
By: mtaht (mtaht) 2005-03-26 13:07:04.000-0600

I have now faxed the disclaimer, and read the guidelines.

I note that a more invasive patch would result in less code, by creating an
asterisk/endian.h with the relevant macros, and changing all the other places that are currently endian aware to use that header instead. Some other places have possibly incomplete logic (no apple/bsd support) like that in g726...

#ifdef __linux__
#include <endian.h>
#else
#ifdef SOLARIS
#include "solaris-compat/compat.h"
#else
#include <machine/endian.h>
#endif
#endif

could then just become

#include "asterisk/endian.h"

A quick grep for endian

./aesopt.h
./channels/adtranvofr.h
./channels/chan_skinny.c
./channels/iax2-parser.c
./codecs/codec_adpcm.c
./codecs/codec_alaw.c
./codecs/codec_a_mu.c
./codecs/codec_g726.c
./codecs/codec_ulaw.c
./db1-ast/btree/bt_open.c
./db1-ast/hash/hash.c
./db1-ast/include/db.h
./dns.c
./formats/format_g726.c
./formats/format_g729.c
./formats/format_gsm.c
./formats/format_h263.c
./formats/format_ilbc.c
./formats/format_jpeg.c
./formats/format_pcm_alaw.c
./formats/format_pcm.c
./formats/format_sln.c
./formats/format_vox.c
./formats/format_wav.c
./formats/format_wav_gsm.c
./include/asterisk/frame.h
./include/solaris-compat/compat.h
./md5.c
./rtp.c

So: do you just want the patches against rtp.c and iax2-parser.c or would something like this be preferable?

edited on: 03-26-05 13:24
By: slimey (slimey) 2005-03-26 14:34:40.000-0600

I think you're confusing the purpose of the get_uint16 / get_uint32 functions. They are needed because the Sparc processor can't deal with unalign reads of words (16bit or 32bit). At all the points where that function is called, you should
find that it is wrapped in ntohl or ntohs to sort endian-ness.

So, unless the Xscale also suffers from the unaligned read problem, I'm not sure why this fixes the problem for you - and it's more likely to cause performance problems on big endian architectures / OS combinations that don't have a problem with unaligned reads.
By: mtaht (mtaht) 2005-03-26 16:07:12.000-0600

Upon further review, I agree that I'm fixing an problem in the wrong place. The Xscale supports unaligned transfers, how fast they are I don't know.

The specific problem I was trying to solve and apparently did was that any attempt to negotiate an iax2 codec other than the default ulaw would end up in one of the calls like here  -

                                               ast_log(LOG_NOTICE, "Rejected call to %s, format 0x%x incompatible with our capability 0x%x.\n", ast_inet_ntoa(iabuf, sizeof(iabuf), sin.sin_addr), iaxs[fr.callno]->peerformat, iaxs[fr.callno]->capability);

To make matters more confusing, I went from a 02102005 pull to cvs head while exploring this bug last night.

Gimmie a day....

edited on: 03-27-05 13:12
By: David Woodhouse (dwmw2) 2005-03-26 16:51:33.000-0600

To get optimal loading of unaligned words with GCC on all architectures, you'd do something like this:

static uint32_t get_uint32(unsigned char *p)
{
 struct { uint32_t datum } __attribute__((packed)) *p2 = (void *)p;
 return p2->datum;
}

The packed structure means that GCC knows the word may be unaligned, and GCC also knows whether the machine is capable of just loading it, or whether we need to emit code to load it byte-by-byte. So it'll produce optimal code on all machines.

This function should probably be renamed to get_unaligned_uint32() and perhaps also moved into include/asterisk/unaligned.h. We can implement non-GCC versions there if needed.
By: David Woodhouse (dwmw2) 2005-03-26 17:45:53.000-0600

(disclaimer on file)

Try this. When we're using GCC it'll just tell GCC that the pointer may be unaligned, and the compiler when then emit optimal code -- if the machine can handle direct dereferences of unaligned pointers it'll do that, but if not it'll emit code to load it manually. When not using GCC we revert to the old hack of doing it manually on Solaris/SPARC and assuming everyone else can do it directly.
By: mtaht (mtaht) 2005-03-26 21:06:53.000-0600

With that patch asterisk on the xscale successfully negotiates iax gsm, ulaw, g726 & speex. It also transcodes gsm -> ulaw, and speex (v1.1.7) -> ulaw. I'll try a few more variants of this matrix today.

This also successfully cross-compiled (see  http://bugs.digium.com/bug_view_page.php?bug_id=0003868)


edited on: 03-26-05 22:45

edited on: 03-27-05 19:37
By: mtaht (mtaht) 2005-03-27 19:25:22.000-0600

new patch (alignment_fix_and_endian_cleanup.txt) supercedes alignment-patch.txt

The previous patch (alignment-patch.txt) did not change rtp.c to use the new put_unaligned_uint32 inline/macro. Also, the fallback macros were incorrectly named.

This patch also includes a bit of rework to simplify the endian include process, doing that via a new asterisk/endian.h header. I took the best of the 4 endian-detection schemes already in asterisk for that header, but this should be tested against solaris, i386 (windows?), and mac.

edited on: 03-27-05 19:31

edited on: 03-27-05 20:05
By: David Woodhouse (dwmw2) 2005-03-27 19:36:28.000-0600

I'd rather see the endian cleanup done in conjunction with the patch in bug 3865
By: mtaht (mtaht) 2005-03-27 19:49:12.000-0600

It struck me as simpler to clean up the endian include process first, and the current patches for bug 3865 apply cleanly over these.

I don't know which solution is the "right" way to do bug 3865, either. I only got into the position where I could test some heavier duty codecs yesterday....

edited on: 03-27-05 22:05
By: slimey (slimey) 2005-03-28 08:49:49.000-0600

I've just tested this patch on solaris/sparc and it works with IAX and SIP calls.

*CLI> show version
Asterisk CVS-HEAD-03/28/05-15:12:35 built by root@dev2 on a sun4u running SunOS
By: Brian West (bkw918) 2005-03-28 09:10:26.000-0600

PowerPC aka Mac is Big Endian... I recall having gsm working when I tested this on my ibook.

/b
By: David Woodhouse (dwmw2) 2005-03-28 09:21:37.000-0600

This bug isn't about endianness, despite the title (which I don't see a way to change). It's about whether the machine can do unaligned loads. Which PPC can, but ARM cannot (by default -- I believe you can tell the kernel to fix up alignment traps for userspace too).
By: mtaht (mtaht) 2005-03-28 16:37:07.000-0600

Yes it would be nice to change the title from being about endian to alignment.
By: Kevin P. Fleming (kpfleming) 2005-03-28 17:02:08.000-0600

Already changed five hours ago :-)
By: mtaht (mtaht) 2005-03-28 17:02:46.000-0600

Take a little nap, the world changes...
By: Mark Spencer (markster) 2005-03-28 22:56:48.000-0600

Added to CVS head, nicely done everyone that participated in this report!
By: Clod Patry (junky) 2005-03-31 19:59:13.000-0600

stustu wants to make a note.
By: staffanu (staffanu) 2005-03-31 20:29:55.000-0600

The committed patch breaks compilation on FreeBSD, since it requires __BYTE_ORDER, __BIG_ENDIAN, and __LITTLE_ENDIAN to be defined.  FreeBSD (both 4.x and 5.x) has the corresponding symbols with only one underscore defined in sys/endian.h.

Before the patch, the double underscore prefix variants were defined in frame.h, but now, the new endian.h file reports an error that they are not defined.
By: Brian West (bkw918) 2005-04-01 01:44:34.000-0600

OH FOR THE LOVE OF GOD... can we get autoconf?

/b
By: David Woodhouse (dwmw2) 2005-04-01 03:52:42.000-0600

Does endian-bsdfix.patch.txt fix it? I see no evidence of it dealing with symbols with only one underscore, but it's what used to be in frame.h.

I don't think we have to accept all the problems and complexity of autoconf just to fix this.
By: staffanu (staffanu) 2005-04-01 06:02:16.000-0600

Yes it does.  I probably should have mentioned that the symbols exist also without leading underscores at all.  Thanks!
By: Kevin P. Fleming (kpfleming) 2005-04-03 16:33:18

Committed endian-bsdfix.txt to CVS, thanks!
By: Michael Jerris (mikej) 2005-06-01 23:31:47

OK, to do this one in stable we need to include the endian.h from HEAD, and then modify the following:  

$ grep -r "endian.h" *
aesopt.h:#include "asterisk/endian.h"
channels/chan_alsa.c:#include "asterisk/endian.h"
channels/chan_oss.c:#include "asterisk/endian.h"
dns.c:#include "asterisk/endian.h"
formats/format_g726.c:#include "asterisk/endian.h"
formats/format_g729.c:#include "asterisk/endian.h"
formats/format_gsm.c:#include "asterisk/endian.h"
formats/format_h263.c:#include "asterisk/endian.h"
formats/format_ilbc.c:#include "asterisk/endian.h"
formats/format_jpeg.c:#include "asterisk/endian.h"
formats/format_pcm.c:#include "asterisk/endian.h"
formats/format_pcm_alaw.c:#include "asterisk/endian.h"
formats/format_sln.c:#include "asterisk/endian.h"
formats/format_vox.c:#include "asterisk/endian.h"
formats/format_wav.c:#include "asterisk/endian.h"
formats/format_wav_gsm.c:#include "asterisk/endian.h"
include/asterisk/frame.h:#include "asterisk/endian.h"
md5.c:#include "asterisk/endian.h"
By: Michael Jerris (mikej) 2005-06-15 22:26:08

uploaded as of yet untested patch for stable, and the endian.h from head (include/asterisk dir).
By: Russell Bryant (russell) 2005-06-24 18:13:48

I have merged endian.h into 1.0
By: Digium Subversion (svnbot) 2008-01-15 15:29:12.000-0600

Repository: asterisk
Revision: 5295

U   trunk/aesopt.h
U   trunk/channels/chan_alsa.c
U   trunk/channels/chan_oss.c
U   trunk/channels/iax2-parser.c
U   trunk/dns.c
U   trunk/formats/format_g726.c
U   trunk/formats/format_g729.c
U   trunk/formats/format_gsm.c
U   trunk/formats/format_h263.c
U   trunk/formats/format_ilbc.c
U   trunk/formats/format_jpeg.c
U   trunk/formats/format_pcm.c
U   trunk/formats/format_pcm_alaw.c
U   trunk/formats/format_sln.c
U   trunk/formats/format_vox.c
U   trunk/formats/format_wav.c
U   trunk/formats/format_wav_gsm.c
A   trunk/include/asterisk/endian.h
U   trunk/include/asterisk/frame.h
A   trunk/include/asterisk/unaligned.h
U   trunk/md5.c
U   trunk/rtp.c

------------------------------------------------------------------------
r5295 | markster | 2008-01-15 15:29:11 -0600 (Tue, 15 Jan 2008) | 2 lines

Simplify endianness and fix for unaligned reads (bug ASTERISK-3777)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=5295
By: Digium Subversion (svnbot) 2008-01-15 15:30:13.000-0600

Repository: asterisk
Revision: 5368

U   trunk/include/asterisk/endian.h

------------------------------------------------------------------------
r5368 | kpfleming | 2008-01-15 15:30:13 -0600 (Tue, 15 Jan 2008) | 2 lines

remaining endian.h fixes for FreeBSD (bug ASTERISK-3777)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=5368
By: Digium Subversion (svnbot) 2008-01-15 15:39:34.000-0600

Repository: asterisk
Revision: 6005

U   branches/v1-0/aesopt.h
U   branches/v1-0/channels/chan_alsa.c
U   branches/v1-0/channels/chan_oss.c
U   branches/v1-0/dns.c
U   branches/v1-0/formats/format_g726.c
U   branches/v1-0/formats/format_g729.c
U   branches/v1-0/formats/format_gsm.c
U   branches/v1-0/formats/format_h263.c
U   branches/v1-0/formats/format_ilbc.c
U   branches/v1-0/formats/format_jpeg.c
U   branches/v1-0/formats/format_pcm.c
U   branches/v1-0/formats/format_pcm_alaw.c
U   branches/v1-0/formats/format_sln.c
U   branches/v1-0/formats/format_vox.c
U   branches/v1-0/formats/format_wav.c
U   branches/v1-0/formats/format_wav_gsm.c
U   branches/v1-0/include/asterisk/endian.h
U   branches/v1-0/include/asterisk/frame.h
U   branches/v1-0/md5.c

------------------------------------------------------------------------
r6005 | russell | 2008-01-15 15:39:34 -0600 (Tue, 15 Jan 2008) | 2 lines

merge endian.h (bug ASTERISK-3777)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6005