[Home]

Summary:ASTERISK-03327: [patch] Send timestamps with trunk frame entries
Reporter:stevekstevek (stevekstevek)Labels:
Date Opened:2005-01-21 15:44:06.000-0600Date Closed:2008-01-15 15:27:41.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_iax2
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) bug3400audiodrop.dump
( 1) iax2_metamove.patch2.txt
( 2) iax2_trunktimestamps.patch2.txt
( 3) iax2-trunktimestamps.patch3.txt
( 4) iax2-trunktimestamps.patch4.txt
( 5) iax2-trunktimestamps.patch5.txt
Description:This patch introduces a backwards-compatible extension mechanism, and an extension to send timestamps for individual trunked frames, to IAX2 trunking.  

The reasons why this is necessary, and a discussion of the overhead can be found here http://lists.digium.com/pipermail/asterisk-dev/2005-January/008903.html

The way this works is:

1) An extension mechanism is defined, where a "trunk extension" frame is defined as a trunk entry where callno == 0
2) The extension is defined as:
+struct ast_iax2_meta_trunk_extra {
+       unsigned short zeros;
+       unsigned short len;
+       unsigned short type;
+       unsigned char data[0];

The timestamp extension is type 1, and then the data that follows is a list of timestamps.   Presently, we can only handle one of these timestamp extensions per trunk frame, although it turns out, it was easy enough to make the code handle it regardless of it's order within the trunk frame.

Extension type 2 should probably be video, because as far as I can see, video gets totally broken in the presence of trunking, because it will get thrown in, but then get unpacked as voice..

2) when sending trunked frames, if so configured, we keep track of the actual timestamps for the trunked frames.  When we send the trunk frame, we construct the timestamp extension, and tack it on to the end of the trunk frame.

3) On the receiving end, as we are unpacking trunked frames from a trunk frame, instead of calling schedule_delivery as we unpack, we put the trunked frames in a list.  If there's a timestamp extension we keep track of that too.  After we've unpacked everything, we go through the trunked frames, and, if there's a timestamp extension, we apply the timestamps, otherwise, we just call schedule_delivery.

Notes on the code:
1) There's actually two patches here:
   a) First patch is mechanical only:  I moved the handling of META stuff out of socket_read, which was already more than 1100 lines, and I figured I didn't want to be the guy that made it 1250.  It's a separate patch so that reading the next one is a bit easier.

   b) The implementation.  Sorry if a little extra housekeeping has shown up:
     i) I made a new function trunk_data_makeroom, because I needed to use the same stuff to add the timestamps in a different part of the file.
     ii) I changed a big conditional around all of send_trunk() to just be a shortcut exit at the beginning

Questions and such:
1) I haven't actually used this code in a trunk scenario yet, but I figured it looked like there were willing testers on asterisk-dev. So please, give it a try!

2) One think I'm not sure about is this section:
+                       if (iaxs[fr.callno]->bridgecallno) {
+                               forward_delivery(cur);
+                               iax_frame_free(cur); /* forward_delivery does not free the frame */
+                       } else

normally, we call forward_delivery with a frame on the stack, but in this case it's called with an iax_frdup2'ed frame;  I wouldn't be surprised if some "malloced" member is set wrong somewhere in this case?

Patches will follow:
iax2_metamove.patch2.txt: Move meta processing out of socket_read()
iax2_trunktimestamps.patch2.txt: (optionally) send, and (if present) parse trunk timestamps.




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

disclaimer on file!
Comments:By: rwjblue (rwjblue) 2005-01-21 21:43:04.000-0600

patches and compiles properly.  Seems to be backwards compatible with the three providers that we are setup with.  Need to test the new funtionality once others get it applied.

By: nick (nick) 2005-01-22 21:56:24.000-0600

This seems like a good idea and a good implementation...

By: Andrew Kohlsmith (akohlsmith) 2005-02-07 15:46:02.000-0600

Wow this breaks backward compatibility for me.  :-)
CVS HEAD 20050207+this patch talking to CVS HEAD 20050118 no patch

no audio once voice frames start getting transmitted.  turning trunk=off on the patched side doesn't help either.

By: Andrew Kohlsmith (akohlsmith) 2005-02-09 02:03:39.000-0600

More data:
I get no Zap audio when I use this patch with or without bug 2532's patchset, with or without trunktimestamps=yes or =no.

using switch-3.nufone.net (which has both 2532 and this patchset and trunktimestamps set to no) my audio dies about 3 seconds in.  I've attached a tcpdump packet dump of a GSM-coded call to echo@switch-3.nufone.net.  I get audio until about 3s in and then I hear nothing, although the UDP stream seems normal.

Now something interesting that I'd noted before was that "iax2 trunk debug" showed that I had about 5300 calls in the trunk, and climbed about a dozen or so every time I re-ran the command.  I think I just broke the asterisk server call density contest.  :-)

By: Andrew Kohlsmith (akohlsmith) 2005-02-09 02:41:05.000-0600

Just another note -- the one system I seem to be having so much trouble with this and the 2532 patches has BOTH a T100P+TDM430P in it.  Not sure if that's perhaps useful information or not...  Both jerjer and I had noticed a lot of HDLC abort errors in the console with this patch... maybe it's something specific to the T100P or something?

By: stevekstevek (stevekstevek) 2005-02-09 18:07:12.000-0600

Andrew,  

 Just to clarify:  you're building/installing on switch-3.nufone.net?

I haven't run this on a box with zap, but I can't see why this would cause any errors there, but 5300 calls in the trunk sounds bad, and whatever is making that happen (it could be a bug in this code, of course) may just be using a lot of CPU, and causing those problems?

I'll have to look at these traces tomorrow.  Mainly, I really wish Kram could take a few minutes to give me his thoughts on the direction these things are taking, before we spend a lot of time debugging them..   But, this patch at least, seems straightforward enough, and configurable, that tt's worth debugging on it's own.  (2532 has a lot more choices as to it's implementation).

By: stevekstevek (stevekstevek) 2005-02-09 18:33:47.000-0600

hmm, on second thought, since ethereal doesn't yet understand trunk frames at all, let alone these additional timestamps, maybe I ought to just instrument the code itself..  It's going to be quite a bitch to parse these frames like this..

The dump seems to show trunking happening only one way:  From you to switch-3, and not the other way 'round..

setting "trunktimestamps=no" on switch-3 won't have any effect in this case:  The "receiving" side of this is always on, that switch just affects whether the host will _send_ trunktimestamps.


[time passes].  OK, I put my binary decoder hat on, and decoded the first 7 trunk frames that were sent from you to switch-3.  They seem to be normal trunk frames, with no timestamps attached.

The first trunk frame contains three trunked frames, each with 33 bytes of data.
The rest seem to be just one trunked frame per trunk frame..

[To read this particular binary easier, look for "40 03 00 21" in there; that's the struct ast_iax2_meta_trunk_entry callno and length.  Then, go 33 bytes later, and you may see another one or not..

The trunktimestamps stuff would look like:

00 00 <- signifies this a a struct ast_iax2_meta_trunk_extra
xx xx <- 2 + 2*(ntimestamps)
00 01 <- signifies this will be timestamps

tt tt <- one of these for each timestamp..

By: Andrew Kohlsmith (akohlsmith) 2005-02-09 20:03:35.000-0600

clarifications:

I am calling switch-3 from my own box.  I have both a T100P and TDM430P in the box.  For those packet captures both cards' modules were loaded and chan_zap knew of them.

I am currently testing on a totally new box with the same T100P and TDM430P.  Same results.  I am, however, beginning to suspect that my T100P card is screwy, I'll be throwing a TE405P in there shortly.

You're right, switch-3 did not have trunk=yes set up.  It does now.

By: Andrew Kohlsmith (akohlsmith) 2005-02-09 22:28:49.000-0600

Ok I'm not sure if it is this patch or 2532 but I have to disable trunking altogether or I do not get two-way audio.  trunk=off and my audio's all good.  I'll provide some packet traces shortly, but I am not able to test this for several days now.

I'm running 2532+this patch but trunk=no and it all seems good.

By: stevekstevek (stevekstevek) 2005-03-04 18:55:13.000-0600

As discussed on a call with Mark (actually, this was over a week ago now), here's an updated patch:


iax2-trunktimestamps.patch3.txt (8,128 bytes) 03-04-05 18:48

This patch is greatly simplified, but is not backwards-compatible with endpoints which don't also have this patch.  [they'll probably die a fiery death or something].

Basically, we use the (presently unused) cmddata to switch from zero (present behavior) to 1 (new behavior).

When it is set to 1, then we still just add the same 2 bytes per trunked frame for timestamps, but they are sent with each trunked frame, instead of at the end.  This makes the construction and destruction much simpler.  Also, the actual format of the trunked frames becomes exactly <len><miniframe>, so instead of redeclaring individual members, I just wrote it that way.

Also, since we're using this format, it should be just as compatible to write <len><videoframe> or <len><any kind of IAX2 frame>, although this code won't handle those cases properly at the moment.

I still haven't tested this other than to see if it compiles, which it does.  

There's now just one patch, and it's simple enough to understand without the long explanation.

By: stevekstevek (stevekstevek) 2005-03-04 18:56:45.000-0600

Forgot to mention this:

The present code doesn't ever look at cmddata;  if we deploy this in -HEAD, it might make sense, if we don't add this to -stable, to at least add something to have it abort trunk processing if cmddata != 0.

By: stevekstevek (stevekstevek) 2005-03-07 16:28:24.000-0600

patch4 fixes a small (but fatal) bug when SUPERMINI (aka classic) trunked frames are used.

By: stevekstevek (stevekstevek) 2005-03-10 12:54:00.000-0600

patch5: Fixed a small bug:  wasn't doing htons() on the length of the frame when sending trunktimestamps.

By: Mark Spencer (markster) 2005-03-17 15:36:36.000-0600

Merged as part of 2532

By: Digium Subversion (svnbot) 2008-01-15 15:27:41.000-0600

Repository: asterisk
Revision: 5192

U   trunk/Makefile
U   trunk/channels/chan_h323.c
U   trunk/channels/chan_iax2.c
U   trunk/channels/chan_mgcp.c
U   trunk/channels/chan_sip.c
U   trunk/channels/chan_skinny.c
U   trunk/channels/iax2.h
U   trunk/codecs/codec_adpcm.c
U   trunk/codecs/codec_alaw.c
U   trunk/codecs/codec_g726.c
U   trunk/codecs/codec_gsm.c
U   trunk/codecs/codec_ilbc.c
U   trunk/codecs/codec_lpc10.c
U   trunk/codecs/codec_speex.c
U   trunk/codecs/codec_ulaw.c
U   trunk/configs/codecs.conf.sample
U   trunk/configs/iax.conf.sample
U   trunk/include/asterisk/channel.h
U   trunk/include/asterisk/translate.h
A   trunk/jitterbuf.c
A   trunk/jitterbuf.h
A   trunk/plc.c

------------------------------------------------------------------------
r5192 | markster | 2008-01-15 15:27:41 -0600 (Tue, 15 Jan 2008) | 2 lines

Add PLC and jitter buffer and iax2 meta trunk with timestamps (bug ASTERISK-2491, ASTERISK-3327)

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

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