[Home]

Summary:ASTERISK-05026: [branch] RTP Packetization
Reporter:Anthony Minessale (anthm)Labels:
Date Opened:2005-09-08 14:03:06Date Closed:2006-09-18 18:33:51
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/RTP
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 5162-head.diff
( 1) autoframing-stable.diff
( 2) codec-framing.diff
( 3) codec-framing-14.08.diff
( 4) frame.diff
( 5) packetization.diff
( 6) paketization-1.2.10.diff
( 7) README.framing
( 8) rtp_packetization.diff
( 9) rtp_packetization-r37291-2.diff
(10) rtp.packetization.2006-06-09.patch
(11) rtp-packetization-v2.diff
(12) trunk-40009-2.diff
(13) trunk-40837.diff
(14) trunk-41648.diff
(15) trunk-current.diff
(16) trunk-current-16.08.diff
Description:This patch refactors rtp.c to use a lookup table for various codecs to determine how much data is required for various timespec making it possible to choose a packet size based on the number of desired milliseconds.  The new method exposed 1 function to the public api

int ast_rtp_set_framems(struct ast_rtp *rtp, int ms);

By calling this function on an rtp session you can set the desired packet size in an expression of ms.

Included in the patch is an example of the new method implemented in chan_sip adding a packetization option to users and peers

packetization => <desired ms>



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

Disclaimer on file.
anthmct@yahoo.com
Comments:By: Clod Patry (junky) 2005-09-08 22:43:47

moc: can you explain why you deleted that patch? I guess you already talk about it with tony, right?

By: Anthony Minessale (anthm) 2005-09-09 09:46:58

Had to add a last-minute tweak it's back now.

By: Brian West (bkw918) 2005-09-09 10:36:09

http://www.packetizer.com/voip/diagnostics/bandcalc.html

By: dea (dea) 2005-11-03 12:17:20.000-0600

Is anyone else using/testing this?  I ported the changes to the ooh323c channel driver and have been very happy with the results.

We have some equipment that HAD to have 30ms ulaw and 729 packetization to
work around quality issues.

Before integrating this patch, call quality from that equipment to * was
good.  After the patch it was VERY good.

I remember seeing a discussion on the -dev list that this might not be
the correct approach, but don't recall seeing another solution

By: Serge Vecher (serge-v) 2005-11-03 12:53:28.000-0600

DEA, could you please post your changes to oo323c driver in a separate bug report noting that it is related to this one?

By: dea (dea) 2005-11-03 13:27:40.000-0600

Bug id 5588 added.

By: dea (dea) 2005-11-23 11:35:48.000-0600

The patch applies to 1.2.0 with a few rejects, most easily tracked and fixed.
It still functions as expected.

By: dea (dea) 2005-12-29 12:52:53.000-0600

rtp-packetization-v2 applies to SVN on 12/29/2005.

Disclaimer on file.



By: Olle Johansson (oej) 2006-01-04 05:48:32.000-0600

Any more tests, reviews, feedback? Are we ready to move forward?

We need input from Kevin or Mark on this as well.

By: dea (dea) 2006-01-04 10:24:52.000-0600

Olle-
 Thanks for looking.  I've been trying to spark a discussion
on the -dev list, as I recall that Mark or Kevin had an objection
to the architecture, but I don't recall the specifics.

 I think that the desire was to make the option per-codec,
allow=g729:30 for example, instead of per peer/user.  

 I'm using this patch on a low load server for MeetMe.  It works
as expected with Cisco SIP phones, and chan_ooh323.

By: Anthony Minessale (anthm) 2006-01-04 11:38:15.000-0600

FYI either way you do it in chan_sip the part in rtp.c is still the same to set the packetization the chan_sip modification was offered as an example.

Most likely your problem is the new board of egomaniacs having a problem with the fact that it is a patch posted by me and they are exhibiting thier usual childish behaviour when dealing with my patches as if it would somehow punish me.... yet all it does is make me want to never submit any more code.
The other possibility is they are hoping if they hold out long enough another unrelated patch will come along and break this one so they can make people waste thier time fixing it 40 times.

This patch to rtp.c not only works, it makes it way more useful and does not even change the default behaviour at all so go figure...

By: Mark Spencer (markster) 2006-01-04 12:23:01.000-0600

This functionality is definitely important, and I think that the work Tony has done is a fantastic start.  It removes a lot of duplicate code, but there's still more to be done:

1) This really has nothing to do with RTP per-se, since this could eventually be moved into IAX as well.  

2) Should packetization be selectable on a per-codec basis?

3) Should there be a set of global defaults within rtp.c?

4) Should the packetization be specified in samples rather than milliseconds since there are some codecs (e.g. LPC10) for which their natural packetization does not lie on an even millisecond boundry.

My initial impression is that we could assume that we could start that there is no reason to have separate packetization delays on a per-codec basis initially, so long as we make sure the syntax would support it eventually.  We could stick with ms for now (since that seems pretty standard), as is done in the patch, provided that we accept that one day we might have to accept a floating point number for that.  Finally I think we could push this into frame.c or something similar, and have a packetization standard target value in rtp.c

By: Anthony Minessale (anthm) 2006-01-04 14:01:51.000-0600

mark, thanks for noticing (at least someone reads my patches =D)

I would say you could move the lookup table and its functions or expose them from RTP.c so it was available to all (maybe a codec.c *shrug*) then add more functions as necessary down the road to set the value in other forms of expression (samples, float etc) since it is not a very big deal to do so

with that you could easily have it both ways and leave the per-peer patch in place but also add code to ast_parse_allow_disallow to perhaps fill in a table of each codec and it's packetization based on the input or something along those lines like an extra optionlal struct struct_packet_sizes arg that uses the index of what bit it is in the bitmask and the corresponding packet size based on the :x after the name in the case of allow=ulaw:30 ulaw is bit 2 so
sizes.speeds[2] would be 30 a null sizes struct would disable paying any attn to the numbers after the names </brainstorm>

struct ast_packet_sizes {
 int speeds[32];
};

By: Olle Johansson (oej) 2006-02-02 01:29:17.000-0600

markster/anthm: Who updates this patch according to your discussion?

/Housekeeping

By: Matt O'Gorman (mogorman) 2006-03-06 16:45:16.000-0600

marko you want to work on this?

By: dea (dea) 2006-03-28 14:31:03.000-0600

Is there any chance that this can be looked at before the feature freeze?

I know that the current architecture is not what is desired, but it works and
is a great feature when connecting to end points that do not use 20ms
packetization.

By: Denis Smirnov (mithraen) 2006-03-29 14:41:34.000-0600

Patch updated to head

By: klaus3000 (klaus3000) 2006-03-30 06:10:16.000-0600

Hi!

I've applied rtp-packetization-v2.diff to asterisk-1.2.2 and set the packet size to 60ms:

sip.conf:
[general]
packetization = 60

Nevertheless, Asterisk is still sending 20ms RTP packets. Is there something more I have to do to activate this feature?

btw: I'm using a SIP client which sends rtp every 60ms. Just after reception of the RTP packet, Asterisk sends 3 20ms at once. This looks like Asterisk sends RTP only after RTP reception - is this correct?

By: Denis Smirnov (mithraen) 2006-03-30 06:41:44.000-0600

Yes, it fixed with async rtp, added to trunk yesterday

By: wILMAR cAMPOS (willcampos) 2006-03-30 09:45:34.000-0600

Can someone please explain to me how to apply this patch, and how to have ooh323 or sip channel use packetization?

Thanks,

Wilmar

By: klaus3000 (klaus3000) 2006-03-30 11:57:04.000-0600

I have just installed todays SVN and rtp.packetization-2006-03-30.patch with

packetization = 60

in sip.conf. But it still does not work:
1. Asterisk still sends 20 ms packets
2. Asterisk RTP sending is still triggered by RTP reception (sending 3 packets at once)

19:52:42.669630 IP remote.16652   > asterisk.10006: UDP, length: 492
19:52:42.669674 IP asterisk.10006 > remote.16652:   UDP, length: 172
19:52:42.669682 IP asterisk.10006 > remote.16652:   UDP, length: 172
19:52:42.669688 IP asterisk.10006 > remote.16652:   UDP, length: 172
19:52:42.729318 IP remote.16652   > asterisk.10006: UDP, length: 492
19:52:42.729362 IP asterisk.10006 > remote.16652:   UDP, length: 172
19:52:42.729370 IP asterisk.10006 > remote.16652:   UDP, length: 172
19:52:42.729376 IP asterisk.10006 > remote.16652:   UDP, length: 172
19:52:42.789635 IP remote.16652   > asterisk.10006: UDP, length: 492
19:52:42.789678 IP asterisk.10006 > remote.16652:   UDP, length: 172
19:52:42.789686 IP asterisk.10006 > remote.16652:   UDP, length: 172
19:52:42.789692 IP asterisk.10006 > remote.16652:   UDP, length: 172

By: Serge Vecher (serge-v) 2006-03-30 13:53:42.000-0600

klaus3000: you have to start Asterisk with -I option in order to activate the internal timing feature (async rtp).

By: klaus3000 (klaus3000) 2006-03-30 15:41:33.000-0600

thanks. Is the -I option necessary only for async RTP or for both (async RTP + packetization option)?

btw: Could I have found this somewhere documented? Where would I have to look? asterisk -h does not report this option.

By: dea (dea) 2006-03-30 15:48:26.000-0600

-I is only for internal timing.

Where did you set the packetization = 60, global, peer or user?

What does sip show peer/user say?  The patch adds status information
to that output, which might help identify the source of your issue.

By: Olle Johansson (oej) 2006-03-30 18:22:44.000-0600

Merged updated patch in a branch /team/group/5162_rtp_packetization

By: klaus3000 (klaus3000) 2006-03-31 02:47:44.000-0600

[async RTP] although I started with -I there is no async RTP. Is this already on svn trunk?

[packetization] I've now configured a peer with packetization = 60. Incoming calls which match this peer will use 60ms. Incoming calls which will be handled by the default context still use 20ms. Thus, it seems as the packetization settings are only used for peers/users, but not for the default context/peer.
Further, "sip show peer foobar" shows Packetization: 60
But "sip show settings" does not show the packetization setting.

By: Yusuf Mayet (yusuf) 2006-04-18 09:59:50

Hi,

Marko said this 'packetization' might be added to IAX.  Is somebody working on it already, ,or is there some plan as to when this will be started,as i am very interested in this.  I am very eager to contribute/develop this packetization for IAX.

By: Serge Vecher (serge-v) 2006-04-18 10:39:30

yusuf: great to hear that you are willing to contribute to this issue -- could you please start off by investigating klaus3000's report of not being to set global packetization settings in chan_sip and produce a patch if necessary against /team/group/5162_rtp_packetization branch? Thanks!

By: dea (dea) 2006-04-18 18:48:52

I have confirmed Klaus3000's issue, at least I think I have.

If the SIP endpoint is configured as a peer/friend/user without
a packetization specified, then the global setting takes effect.

If the endpoint is not defined, such as a guest, then the global
setting does not take effect.

A number of options seem to be available-
1.  Initialize the sip_pvt->framems to the global setting (yuck)
2.  When a guest is found (not peer or user) set the framems (anything else)

chan_sip does not appear to support the concept of using a [guest],
so the above appear to be the only options (not that I am really
comfortable in the chan_sip code....)



By: Anthony Minessale (anthm) 2006-04-19 08:31:02

FYI,

Just to clarify and avoid confusion.  As the patch description states:
"Included in the patch is an example of the new method implemented in chan_sip adding a packetization option to users and peers"  

Thus asis, it does not set a global option from [general] but you can easily add it by copying how other settings do it (it's almost the same as how the peers do it but you need a static int somewhere to put the result instead of in the peer struct)

By: Serge Vecher (serge-v) 2006-06-06 16:33:44

is this dead :( ?

By: dea (dea) 2006-06-06 16:39:46

Seems to be, at least from the 'must include support for IAX' point of view.

I've had it running against an early SVN build for six months on both SIP
and ooH323 with zero issues.

By: Denis Smirnov (mithraen) 2006-06-07 04:32:33

I have this patch updated to last trunk, but not tested.

By: Denis Smirnov (mithraen) 2006-06-07 04:41:32

Patch updated for last trunk

By: dea (dea) 2006-06-07 11:04:45

The patch looks good after a quick read, except for a little bit of pollution
from the jitterbuffer work.

The patch adds ast_jb_conf to chan_sip.c, which is unrelated to this effort.

By: Nic Bellamy (nic_bellamy) 2006-06-08 21:20:30

After some messing about, I discovered that packetization for ALAW wasn't working - turns out RTP_CODEC_TABLE is missing an entry for ALAW.

I don't have a disclamer on file (yet, must get around to it... :-), so I won't post the change as a diff, but it's a simple matter of making a copy of the AST_FORMAT_ULAW entry and changing it to AST_FORMAT_ALAW.

Other than that, it seems to be working nicely (I have it backported to 1.2.x with the internal timing patch).

I'm about to try it out in a production environment, so might add another note with my experience later.



By: Denis Smirnov (mithraen) 2006-06-09 08:19:05

Thanks nic_bellamy
Patch updated to last trunk with patch for ALAW support.

By: Andrey S Pankov (casper) 2006-06-09 12:09:48

Denis: can you check please that line:
+ struct ast_jb_conf jbconf;
Where does it come from? Thanks!

By: Andrey S Pankov (casper) 2006-06-09 12:15:33

> Patch updated to last trunk with patch for ALAW support.
What about ADPCM format which was originally there?

By: Denis Smirnov (mithraen) 2006-06-10 09:14:10

It looks like my patch was mixed with some other patch...
Hm. I not sure that can fix it.

By: Sergey Basmanov (sb) 2006-06-11 14:35:58

I have a suggestion about this patch.
Can we use a little different way to setup packet size for each codec? Something like this:
;in peer/user/friend section, or global
disallow=all
allow=g729p30
allow=ulawp320
and so on..
By using this, we can setup packet size without any modication to channel driver. But this requires to change at least ast_parse_allow_disallow() and a way it stores codec prefs.

By: Andrey S Pankov (casper) 2006-06-11 16:49:31

>disallow=all
>allow=g729p30
>allow=ulawp320

g729:30
ulaw:320

Better like this?

As markster noticed above this patch is just a start, but the right way
is to have payload size configurable by payload type (codec).

By: Sergey Basmanov (sb) 2006-06-13 04:14:54

Oh, I missed that..
Well, I wonder how we can pass size information from ast_parse_allow_disallow() to rtp ?
It would be nice to be able to setup packet size without any modifications to channel drivers.
So, instead of rtp->framems, there should be some array of sizes for each codec.
Any ideas?
May be we can do this way:
as anthm noted, add:
struct ast_packet_sizes {
 int speeds[32];
};
into struct ast_rtp
also, add similar struct to ast_codec_prefs
then, store sizes into prefs, and when creating rtp, copy this information to rtp struct.
like: ast_rtp_set_framems(peer->prefs, rtp->packet_sizes);
But this requires modification of channel drivers.



By: Denis Smirnov (mithraen) 2006-06-14 06:21:35

DEA, can you help with fix it?

By: dea (dea) 2006-06-14 10:19:51

I wish that I could.  I have been testing and forward porting anthm's patch, but lack the deeper understanding of Asterisk's internals to attempt the
changes needed to frame.[ch]  I did start, and quickly realized that the result was going to be garbage.

The best I can offer is positive feedback on the RTP solution that anthm presented and the small maintenance tasks involved with trying to move it forward.

Oh, and I did add this support to ooh323c, which we have used internally for over six months.

By: Sergey Basmanov (sb) 2006-06-15 02:52:07

Can You please take a look at frame.diff ?
Am I going right way?
Next step - add struct ast_codec_pref prefs to struct ast_rtp
and modify packetization code to look for size for each codec in this field.
And rtp->prefs should be set by channel driver after config load.
Any suggestions?

By: Sergey Basmanov (sb) 2006-06-16 19:01:44

packetization.diff: this is how I think it can be implemented.
Code needs to be added to channel driver:
/* Set Frame packetization, use default if it doesn't exist. */
if (r->rtp) {
ast_rtp_set_codec_info(r->rtp, &r->prefs);
}
May be anyone take a look at this ?

By: dea (dea) 2006-07-07 16:38:33

Attached a port of this patch to SVN r37291.  The test branch oej opened is
very stale.  Tested with chan_sip and chan_ooh323 with no issues.

**edit**
I missed a recent update that added ALAW to the formats table.  Please
delete rtp_packetization-r37291.diff, rtp_packetization-r37291-2.diff
is the correct file.

[disclaimer on file]



By: Olle Johansson (oej) 2006-08-07 14:52:49

We should support the fmtp header for ilbc in this patch.

By: dea (dea) 2006-08-07 15:59:10

OK, either that is really simple or I'm clueless-

-               ast_build_string(a_buf, a_size, "a=fmtp:%d mode=20\r\n", rtp_code);
+               if ((p->framems == 20) || (p->framems == 30)) {
+                       ast_build_string(a_buf, a_size, "a=fmtp:%d mode=%d\r\n", rtp_code, p->framems);
+               } else {
+                       ast_build_string(a_buf, a_size, "a=fmtp:%d mode=20\r\n", rtp_code);
+               }


It compiles fine, but I don't have any iLBC capable endpoints.  If this is
correct, I roll another patch based on todays SVN.

[disclaimer on file]

By: Olle Johansson (oej) 2006-08-08 03:04:51

DEA: We already send fmtp indicating that we transmit 20ms, but we need to parse whatever we get from the other side.

By: dea (dea) 2006-08-08 10:56:36

OK, I'll dig a little deeper.

Answers to couple questions/points might help me along-

1.  The current code only deals with what we send, and makes
no attempt to match the incoming packetization.
2.  The current settings applies to all available codecs, and
my research of iLBC shows that only 20 and 30 are valid.  I would
think that at least a warning should be logged if the admin chooses
a packetization other than 20 or 30 and tries to use iLBC
3.  I did see that chan_sip sets mode=20, but with this patch mode=30
is also valid and I would guess desirable.

The big issue is the first one.  Is the desire that Asterisk
should attempt to match the offered packetization?

By: dea (dea) 2006-08-08 11:16:45

SB,
 If you are still working on this and subscribed to the -dev list, perhaps
we can discuss your progress there.  I think you are on the right track, but
the code will need more restructuring.

By: Sergey Basmanov (sb) 2006-08-08 12:32:40

DEA,
Unfortunately no, I'm not subscribed to -dev list.
After more detailed look at source I found that my code was completely wrong. But that was just idea. I'm very interested in this feature, so I'm ready to start working on it right now, but not sure if I really going right way. May be there is better way.

By: Sergey Basmanov (sb) 2006-08-10 09:17:09

Btw, what difference between mode=n and a=ptime:n ?
rfc2327:
a=ptime:<packet time>
      This gives the length of time in milliseconds represented by the
      media in a packet. This is probably only meaningful for audio
      data.  It should not be necessary to know ptime to decode RTP or
      vat audio, and it is intended as a recommendation for the
      encoding/packetisation of audio.  It is a media attribute, and is
      not dependent on charset.
Which one is better?
DEA, can You contact me by email? sb_at_9i_dot_ru



By: Andrey S Pankov (casper) 2006-08-10 09:46:31

Parameter "mode=n" is only valid for iLBC codec.

From http://www3.ietf.org/proceedings/03nov/I-D/draft-ietf-avt-rtp-ilbc-03.txt

  Parameter ptime can not be used for the purpose of specifying iLBC
  operating mode, due to fact that for the certain values it will be
  impossible to distinguish which mode is about to be used (e.g. when
  ptime=60, it would be impossible to distinguish if packet is
  carrying 2 frames of 30 ms or 3 frames of 20 ms etc.).

By: dea (dea) 2006-08-10 11:07:20

SB-  email sent.

The code missing to support iLBC is to parse the inbound SDP and
set determine if Asterisk needs to modify the codec framing/packetization
based on the fmtp: mode= value.

My understaning from the iLBC RFC is that if either side requests a
30ms packetization, then both sides will use 30.

By: dea (dea) 2006-08-10 22:05:51

I just posetd a proof of concept patch to the developers list that attempts to move the packetization down into frame.c, exposing it to chan_iax or any other non-RTP based channel.

I'm not posting it here yet, as it doesn't do anything interesting.  It
just lays the foundation, which might be horribly wrong.

By: dea (dea) 2006-08-11 15:42:37

Updated against SVN 39479.

This version removes the RTP_CODEC_TABLE and extends AST_FORMAT_LIST
to include the fields needed to select the proper framing.

Changes to chan_sip include having "sip show peer/user" to included
the framing settings per codec.

At this point other RTP based channels can be easily ported to this
framework, but will still work with the default of 20ms without any
changes.

If the patch passes an architectural review, I can update the remaining
RTP-based channel drivers.

I am starting on the chan_iax2, but cannot figure out what if any
support it gets from frame.c.  It looks like it handles framing directly,
so it might be a lot more effort to integrate.

By: Sergey Basmanov (sb) 2006-08-12 07:38:43

Same patch backported for stable 1.2.10 for those who want to try it, but don't have trunk version running.
Just one thing: I'm not sure if I filled AST_FORMAT_LIST with codec parameters correctly. It would be great if someone will look at it.



By: Sergey Basmanov (sb) 2006-08-14 03:37:57

Next step. Asterisk can match remote framing.
For example, 'allow=g729:a' means that asterisk should use framing as suggested by remote side, or default if not specified. I think current patch can be easy extended for this.
Another way - use peer/user config option. In this case, if remote is not sent us framing information, manually configured value will be used instead of default.
Any comments?

By: Roy Sigurd Karlsbakk (rkarlsba) 2006-08-14 06:30:59

autoframing would be wonderful. just configure the client and that's it. is this info set in the SDP anywhere, or would this need to be autodetected in counting frames?

By: Sergey Basmanov (sb) 2006-08-14 06:34:57

Patch for trunk with small fixes. Also, now framing works for incoming sip calls.
rkarlsba: if remote sends in SDP: a=ptime:N, we can read it and set framing for appropriate codec. I'm currently working on this.

By: Sergey Basmanov (sb) 2006-08-14 08:26:36

rkarlsba, good news :)
sip show peer office:
 Codecs       : 0x100 (g729)
 Codec Order  : (g729:20)

Aug 14 17:15:35 DEBUG[12397]: frame.c:1040 ast_codec_pref_setsize: Found codec in preferences, adding framing
Aug 14 17:15:35 DEBUG[12397]: chan_sip.c:3713 process_sdp: Setting framing for 256 to 40

Sent RTP packet to 62.118.164.xxx:18866 (type 18, seq 40166, ts 61440, len 40)
Got RTP packet from 62.118.164.xxx:18866 (type 18, seq 22252, ts 63216, len 40)

So, it works. At least for outgoing calls. Will try with incoming later today.
Code needs some cleanup, and then I port it for trunk and post here.

By: Anthony Minessale (anthm) 2006-08-14 09:16:32

I admire you guys.  This bug will be a year old in a few weeks I am very impressed you have the determination to keep this one alive with no help from the "developers".

By: Sergey Basmanov (sb) 2006-08-14 09:19:54

This patch is for stable (1.2.10). Sorry, I don't have trunk running.
In this patch I added option 'autoframing' in sip.conf
If autoframing=yes, framing is matched to remote framing, if not - manually configured or defaults. I will provide patch for trunk later today.
I tesed this on my production system with incoming&outgoing calls.

By: Sergey Basmanov (sb) 2006-08-14 09:28:40

anthm: thanks to DEA. He moved this issue from dead point. With his help I wrote this patch within night or two :)
Now I hope somebody will take a closer look at it. I hope my idea about implementation was correct.

By: Serge Vecher (serge-v) 2006-08-14 09:36:19

keep up the good work, guys. This issue may not get as much as attention as necessary due to an impending 1.4 release, but, hopefully, afterwards it will. Thanks!

By: Sergey Basmanov (sb) 2006-08-14 09:42:45

So, no chance this feature will be included in 1.4 ?

By: dea (dea) 2006-08-14 10:51:17

I appreciate the kudos Anthm, but you started this and it is too good a feature to let drop, so consider it a tribute to your genius ;-)

The RTP side of the house looks solid at this point and a trivial task to
add support for the RTP channels.  Sadly IAX is not any closer to being
supported.  I just do not see any place in chan_iax that can easily be made
aware of this feature.

In testing support for SIP and ooH323 I have confirmed that if I feed
Asterisk a payload other then 20ms, it happily retransmits it to IAX
endpoints.
Take this sample call, outbound and inbound from the SIP point of view-
    SIP(30ms) -> Asterisk -> IAX(30ms)
    IAX(20ms) -> Asterisk -> SIP(30ms)

The SIP endpoint is manually configured for 30ms, and Asterisk doesn't
attempt to alter it.  The IAX client, and old Firefly build, works fine
with 30ms payloads.  Audio from the IAX client is of course still in a
20ms payload, and Asterisk with this patch properly repackages the stream
into a 30ms payload.

So we have a solution that is missing two elements-
    1.  IAX endpoints supporting alternate audio packetization
    2.  Chan_IAX using a configured packetization and not a simple pass-through

Item 2 is important if Asterisk is acting as an endpoint, such as hosting
conferences.

SB is doing a wonderfull job extending the RTP features well past the
minimum enterprise requirement, but neither he nor I can figure out what
to do next in chan_iax2

By: Anthony Minessale (anthm) 2006-08-14 11:36:13

It *is* called *RTP* Packetization after all.  I would think adding IAX to the mix will just impede your progress. The IAX2 protocol has no notion of packetization afaik but they are the implementors of the protocol so they could easily extend it. BTW, You have other issues to condsider too like adding ptime to sdp so other ends who support it can take atvantage in a more professional manner.

Maybe IAX3 should use RTP for audio like everyone else does ;)

By: Sergey Basmanov (sb) 2006-08-14 11:49:56

anthm: ptime:n is already in sdp.
There is small issues in 'codec-framing-14.08.diff' which I already fixed in 'autoframing-stable.diff' Now same patch for trunk with autoframing is ready to be uploaded here (I just doing additional checking).

By: dea (dea) 2006-08-14 11:51:31

I was trying to address one of the early complaints, which was that the patch
did not address IAX.  I have no need for packetization in IAX, but if that
was the one major roadblock to getting this much needed enterprise feature
into the tree, I thought I would give it a try.

I think you are correct that there is just no clean way to make IAX use
even the concepts behind this patch.

On the bright side, SB has already written the code to add ptime: to the
SDP for SIP, and code to read and react to offered ptime values.  So one
(major) channel down.  From my reading of the H323 drivers, they will not
easliy become reactive, but chan_ooh323 already has support for 'announcing'
the packetization of the offered codecs.  SCCP appears to have an
feature in the protocl equivalent to ptime, so adding negotiable packetization
to chan_skinny should be fairly easy.

So it looks like this featue is ready for some hard-core testing.

By: Paul Cadach (pcadach) 2006-08-14 12:29:15

Attached difference with current SVN-HEAD and small README about usage of new feature.

By: Sergey Basmanov (sb) 2006-08-14 12:43:55

One small note: please check for correct values in AST_FORMAT_LIST in frame.c
I'm little lost in numbers :)

By: dea (dea) 2006-08-14 12:48:30

I'm planning to dig through the various codec RFCs to see if I can
find the min, max and increment values for each codec.

I do know that we have a few bad values in the current table, G729
has a max of 80ms I believe, while the code has 1200.  

If anyone happens to have a reference to the correct values, it would
make life easier, but I'm expecting that digging into the RFCs is going
to be required.

By: Sergey Basmanov (sb) 2006-08-15 04:08:03

I found some codecs parameters in cisco documentation.
Here is what I currently have:
Codec           Sample          Min     Max     Inc     Def
               size(bytes)     ms      ms      ms      ms
g729a           10              10      230     10      20
g7231r63        24              10      100     10      20
g711u           80              10      30      10      20
g711a           80              10      30      10      20
gsmfr           33              10      70      10      10
slin            160             10      70      10      20
g726r32         40              10      60      10      20
ilbc            ??              20      30      10      30
adpcm
g726aal2
speex

By: Sergey Basmanov (sb) 2006-08-15 06:09:57

Diff against trunk r39774.
Fixed some codecs parameters in AST_FORMAT_LIST (from cisco documetation)
Fixed possible division by zero in rtp.c (ast_rtp_write)
For codecs without inc_ms parameter specified, no attempt to create smoother will be made.

By: dea (dea) 2006-08-15 10:59:46

Refering to ast_codec_get_samples I pulled together values for these
codecs:
Codec        Sample  Min Max Inc Def
            size    ms  ms  ms  ms
            (bytes)
ilbc           50    20  30  10  30
adpcm         160    20  ??  20  20
g726aal2      160    20  ??  20  20
speex         160    20  ??  20  20

ilbc is odd in that it uses different encoding for 20 vs. 30ms.
Asterisk appears to use the values for 30ms(50 bytes) at the moment.
SpeeX also supports a number of different encoding options, making
it harder to set an initial size.

Lacking any better documentation, I'd suggest set ing adpcm, g726aal2
and speex to a max of 60ms.  We can update the values if we find more
information, but until then we can allow testing and get feedback.

By: Serge Vecher (serge-v) 2006-08-15 16:06:26

1. I think this line needs to be preceeded by if (option_debug) in 2 places in chan_sip.c:
+ast_log(LOG_DEBUG, "Setting framing for %d to %d\n", format, framing);
2. Also, the the 'fixme' for int max_ms needs to be addressed.

By: dea (dea) 2006-08-15 16:39:19

The fixme is not really a fixme.  The comment should be pulled as
the max_ms for 'most' codecs are established in their respecitve RFCs
and have little to nothing to do with RTP_MTU.

Now we could use RTP_MTU to establish Asterisk specific max_ms values,
but that won't help interoperability any.

If SB doesn't get a chance tonight, I'll submit an update addressing these
issues.

Oh, and there is small chan_sip cleanup mixed in here.  *cli>sip show user
was not using print_codec_to_cli while *cli>sip show peer was.  Should
that section be split out and submitted seperately?

For that matter should print_codec_to_cli be made common code?  I think
most VoIP channel drivers would benefit from it and eliminate some trivial
duplicated code.

By: Serge Vecher (serge-v) 2006-08-15 16:45:38

DEA: yes, please feel free to open a new report for minor fixes not related to RTP packetization itself; this way they get addressed and merged faster and the patch here is less confusing to the reviewer.

By: Sergey Basmanov (sb) 2006-08-16 04:19:39

Small fixes to avoid possible division by zero.
Also, addressed issue when some UAs (SPA1001) sending SDP like this:
m=audio 16402 RTP/AVP 18 100 101
a=rtpmap:18 G729a/8000
a=rtpmap:100 NSE/8000
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-15
a=ptime:30
a=sendrecv
This is because SPA1001 have one 'Payload size' option for all codecs.
Issue have place only when 'autoframing=yes'
Corrected g723.1 parameters in AST_FORMAT_LIST.
DEA, if You have correct parameters for rest of codecs in AST_FORMAT_LIST, please add them?

By: Sergey Basmanov (sb) 2006-08-16 04:29:24

vechers, I put 'fixme' in comment just because I had no idea what maximum value should be used for codecs. Now it seems to me that we can simply take maximum values from cisco documentation.
Also, 'if (option_debug)' added as suggested.

By: dea (dea) 2006-08-16 11:13:19

Updated to SVN 40009.  Remove the chan_sip show_user fix as it is now in trunk.

The AST_FORMAT_LIST is populated with values for all audio codecs.  Several
codecs are limited in their range of framing options-

ILBC was and still is limited to 30ms.  Prior to this patch the code
assumed 30ms, and this patch does not attempt to change that.

SpeeX will need a lot of testing and review.  Support for vbr will require
additional code before establishing the smoother(I think).

GSM is limited to 20ms increments to avoid a rounding error creating a
smoother 10% too small.

By: Sergey Basmanov (sb) 2006-08-16 11:24:14

Oh, there is small mistake in chan_sip:
- if (framing && found_rtpmap_codecs[last_rtpmap_codec]) {
+ if (framing && last_rtpmap_codec) {
I missed this while moved code from stable.
DEA, please, update this in Your code.

By: dea (dea) 2006-08-16 11:53:11

trunk-40009-2.diff has that correction.

If a manager will please delete trunk-40009.diff

By: dea (dea) 2006-08-22 13:28:45

Updated to SVN 40837 and the new directory structure.

By: dea (dea) 2006-08-31 12:49:59

I've been looking at the remaining RTP-based channel drivers to see if they
could be made to leverage this functionality (easily).

A couple of 'requirements' need to be meet for this to be practical/easy.
  1.  Use ast_parse_allow_disallow
  2.  Per endpoint configuration
  3.  Support for more than a single codec

Currently only chan_h323 meets this list, and it would likely require
additional changes to notify the OpenH323 stack of the desired framing.
Chan_jingle (and chan_gtalk, which is not in the version I have been basing
this research on) appears to be a possible candidate, but I don't know if
the common endpoints would support alternate packetization.

Chan_skinny and chan_mgcp do not meet any of the requirements.  Implimenting
the first one might not be too bad, but the remaining changes would need
a developer familiar with those technologies and endpoints to test with.

I'd be happy to make a pass at a patch to address these issues, but I will
not be able to test the results.  If any of the developers familiar with
these channels are interested and can spare the cycles to test the results,
please let me know

By: Serge Vecher (serge-v) 2006-08-31 12:55:23

DEA: could you please email the dev mailing list with those questions, to make sure that respective developers see it? I know PCadach is working on chan_h323, Qwell - chan_skinny, not sure about chan_mgcp.

Thanks,

By: dea (dea) 2006-08-31 16:14:19

I sent the same note to -dev right after adding the bug-note.
Discussion about the remaining RTP channels will continue there, with
updates posted here as they happen.

By: dea (dea) 2006-09-01 17:02:04

Updated to 41648 and added packetization to chan_skinny

By: Anthony Minessale (anthm) 2006-09-08 13:41:14

Happy Birthday to this bug!!! It's one year old today!!!!

We may have flying cars before this issue is resolved. =D

By: Brian West (bkw918) 2006-09-08 16:15:44

Happy Birthday to YOU!!!

YIPEEEEEEEEEEEEEEEEEEE

Lets see if she makes it to 2 yrs old!

/b

By: Sergey Basmanov (sb) 2006-09-18 10:05:06

Are we going to celebrate 2nd birthday?
Can somebody post here any reasons why this can not be included into 1.4 ?

By: Matt O'Gorman (mogorman) 2006-09-18 18:33:39

Committed revision 43243.
thanks for all those who helped.