[Home]

Summary:ASTERISK-03775: [patch] AST_FORMAT_SLINEAR has ambigious endianness.
Reporter:David Woodhouse (dwmw2)Labels:
Date Opened:2005-03-26 05:46:26.000-0600Date Closed:2008-01-15 15:30:50.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) endian-allnative-3.patch.txt
( 1) smoother_feed-fix.patch.txt
Description:In a lot of asterisk code, we assume that AST_FORMAT_SLINEAR is native-endian. In fact it varies -- the audio/L16 MIME time is big-endian, while most I/O is little-endian. So far we've got away with it mostly because we run on little-endian machines and don't seem to actually use slinear on the wire with RTP. But we should deal with it properly.

One easy option might be to hack every driver which deals with AST_FORMAT_SLINEAR, and make them all byte-swap to/from native endian on input/output. But that's going to be somewhat suboptimal on a BE machine in the relatively common case that we're passing audio frames directly from one LE channel to another -- we'd be gratuitously byteswapping it all twice in the process.

A second option is to treat 'wrong-endian slinear' as an entirely separate format, and provide a codec for converting it. The attached patch does that -- it gives us AST_FORMAT_SLINEAR_LE and AST_FORMAT_SLINEAR_BE, such that drivers can specify the endianness of the frames they produce or require. It also preserves the existing assumption that AST_FORMAT_SLINEAR is native-endian, by defining it as one or the other according to the host.

My concern about this second approach is that ast_dsp_process(), upon receipt of a wrong-endian buffer, is going to byteswap it to native-endian for itself (into a locally allocated buffer), and then potentially byteswap back again if writeback is necessary. Also, using a codec is less efficient in the case where we _do_ have to swap; it would be quicker just to do it inline.

A third option might be to distinguish between AST_FORMAT_SLINEAR_LE and AST_FORMAT_SLINEAR_BE as above, and to provide a function which byte-swaps frames _in-place_ to the required endianness. So asp_dsp_process() would call a function which checks that its input is native-endian, then the frame _remains_ native-endian as it's passed up the stack. Only when it hits something which requires a different endianness (i.e. an output driver) will it be swapped back. This way, we get fairly much optimal byteswapping but at the expense of special-casing the 'format conversion'. That's not extremely pretty, but should be limited to relatively few locations.

Some guidance would be appreciated.

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

Disclaimer on file.
Comments:By: Mark Spencer (markster) 2005-03-26 09:31:33.000-0600

I think we should leave the endianness as "native endianness" whenever we are within Asterisk and convert to network byte order when we transmit.  Since sending with RTP is not currently supported, that's probably not a big deal except for those running IAX in signed linear, but i guess they'll just have to upgrade :)
By: David Woodhouse (dwmw2) 2005-03-26 10:52:32.000-0600

Thus accepting the overhead of converting to native endianness and back again, even when we didn't actually want to do anything with it in between?

Wrong-endian slinear could be considered fairly similar to alaw and ulaw -- they're simple per-sample transforms. We don't convert ulaw and alaw to slinear when we pass it through; should we really do so for backwards-slinear?

Assuming that's really what you want, I'll do that and provide a patch.
By: Mark Spencer (markster) 2005-03-26 10:56:51.000-0600

There are a limited number of codec slots because codec masks are used very widely within Asterisk.  It would be unreasonable to take up 2 or 3 slots for linear, and many places within the code assume that linear is host-byte-order which makes sense.  On the other hand, it is extremely rare that someone communicates linear with the outside world, especially with wrong-endian byte order on both sides.  Until someone comes up with a situation where that is sufficiently commonplace, it would seem to me that our outside interfaces should be converting between byte orders (e.g. RTP and IAX) unless there is further debate/suggestion to the contrary.
By: David Woodhouse (dwmw2) 2005-03-26 17:23:48.000-0600

OK, how about this version then? I've tried to put the swapping in where we were already doing a memcpy, to keep the performance implications to a minimum.
By: David Woodhouse (dwmw2) 2005-03-26 19:11:12.000-0600

Oops; wrong patch. This fourth patch is different in that it lacks the typo which prevented chan_iax2.so from loading.
By: Mark Spencer (markster) 2005-03-26 20:17:31.000-0600

Is there a reason we're not using htons / ntohs since on many systems they're highly optimized?
By: David Woodhouse (dwmw2) 2005-03-27 03:24:39.000-0600

ntohs doesn't actually do anything on a big-endian host, and any compiler worth its salt should be able to optimise (x<<8)|(x>>8) anyway.

Since we're combining the byteswap with a memcpy whereever possible, it might be feasible to use MMX/SSE/Altivec for it if we really want to optimise the process. I suspect that's not necessary though; it's left as an exercise for the reader anyway.
By: Mark Spencer (markster) 2005-03-27 16:17:21.000-0600

When would we ever need to do anything on a big endian host?  In that case, we shouldn't be doing any manipulation since the native format will be the same as the network format...
By: David Woodhouse (dwmw2) 2005-03-27 16:30:05.000-0600

In the specific case of RTP, yes. But some hardware gives us little-endian samples which need swapping on a BE host.

Most of my testing and development at the moment is on a Fedora/PPC host, in fact, and the channel I'm testing with is chan_bluetooth, which can only give us little-endian samples.

Glibc doesn't optimise htons() manually, btw. It's just mask-and-shift. We trust the compiler to get it right.
By: David Woodhouse (dwmw2) 2005-03-29 05:31:07.000-0600

Updated patch to apply against today's CVS tree.
By: Kevin P. Fleming (kpfleming) 2005-04-03 18:04:41

Committed to CVS, thanks!
By: Russell Bryant (russell) 2005-04-05 02:17:47

fixed in 1.0 ... had to do a lot of it manually
By: David Woodhouse (dwmw2) 2005-04-05 07:20:22

Sorry, there was an error in my original patch. Attaching an update...
By: Russell Bryant (russell) 2005-04-05 12:15:22

d'oh!  I'm really surprised that we didn't catch that when we looked over it.

Fixed in CVS HEAD and 1.0
By: Digium Subversion (svnbot) 2008-01-15 15:30:17.000-0600

Repository: asterisk
Revision: 5373

U   trunk/channels/chan_iax2.c
U   trunk/channels/chan_phone.c
U   trunk/channels/iax2-parser.c
U   trunk/frame.c
U   trunk/include/asterisk/frame.h
U   trunk/rtp.c

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

handle AST_FORMAT_SLINEAR endianness properly on big-endian systems (bug ASTERISK-3775)

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

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

Repository: asterisk
Revision: 5399

U   branches/v1-0/CHANGES
U   branches/v1-0/channels/chan_iax2.c
U   branches/v1-0/channels/chan_phone.c
U   branches/v1-0/channels/iax2-parser.c
U   branches/v1-0/frame.c
U   branches/v1-0/include/asterisk/frame.h
U   branches/v1-0/rtp.c

------------------------------------------------------------------------
r5399 | russell | 2008-01-15 15:30:40 -0600 (Tue, 15 Jan 2008) | 2 lines

handle AST_FORMAT_SLINEAR endianness properly on big-endian systems (bug ASTERISK-3775)

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

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

Repository: asterisk
Revision: 5409

U   trunk/include/asterisk/frame.h

------------------------------------------------------------------------
r5409 | russell | 2008-01-15 15:30:49 -0600 (Tue, 15 Jan 2008) | 2 lines

don't define a functioning returning an int inside of a do{...}while(0) (bug ASTERISK-3775)

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

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

Repository: asterisk
Revision: 5410

U   branches/v1-0/include/asterisk/frame.h

------------------------------------------------------------------------
r5410 | russell | 2008-01-15 15:30:50 -0600 (Tue, 15 Jan 2008) | 2 lines

don't define a functioning returning an int inside of a do{...}while(0) (bug ASTERISK-3775)

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

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