[Home]

Summary:ASTERISK-02918: [patch] Control codec priorities in IAX2
Reporter:Anthony Minessale (anthm)Labels:
Date Opened:2004-12-02 21:07:42.000-0600Date Closed:2008-01-15 15:20:45.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) iax_just_show_peer-aka-rev5_split.diff
( 1) iax_with_prefs_rev2.diff
( 2) iax_with_prefs_rev3.diff
( 3) iax_with_prefs_rev4.diff
( 4) iax_with_prefs_rev5.diff
( 5) iax_with_prefs_rev6.diff
( 6) iax_with_prefs_rev7.diff
( 7) iax_with_prefs_rev8.diff
( 8) iax_with_prefs_rev9.diff
( 9) iax_with_prefs.diff
Description:This is a 1st draft at making IAX2 support codec
prefs using the new codec API recently added to CVS

Basicaly it uses the data structure AST_FORMAT_LIST
now found in frame.c

static struct ast_format_list AST_FORMAT_LIST[] = {
   { 1, AST_FORMAT_G723_1 , "g723" , "G.723.1"},
   { 1, AST_FORMAT_GSM, "gsm" , "GSM"},
   { 1, AST_FORMAT_ULAW, "ulaw", "G.711 u-law" },
   { 1, AST_FORMAT_ALAW, "alaw", "G.711 A-law" },
   { 1, AST_FORMAT_G726, "g726", "G.726" },
   { 1, AST_FORMAT_ADPCM, "adpcm" , "ADPCM"},
   { 1, AST_FORMAT_SLINEAR, "slin",  "16 bit Signed Linear PCM"},
   { 1, AST_FORMAT_LPC10, "lpc10", "LPC10" },
   { 1, AST_FORMAT_G729A, "g729", "G.729A" },
   { 1, AST_FORMAT_SPEEX, "speex", "SpeeX" },
   { 1, AST_FORMAT_ILBC, "ilbc", "iLBC"},
   { 0, 0, "nothing", "undefined" },
   { 0, 0, "nothing", "undefined" },
   { 0, 0, "nothing", "undefined" },
   { 0, 0, "nothing", "undefined" },
   { 0, AST_FORMAT_MAX_AUDIO, "maxaudio", "Maximum audio format" },
   { 1, AST_FORMAT_JPEG, "jpeg", "JPEG image"},
   { 1, AST_FORMAT_PNG, "png", "PNG image"},
   { 1, AST_FORMAT_H261, "h261", "H.261 Video" },
   { 1, AST_FORMAT_H263, "h263", "H.263 Video" },
   { 0, 0, "nothing", "undefined" },
   { 0, 0, "nothing", "undefined" },
   { 0, 0, "nothing", "undefined" },
   { 0, 0, "nothing", "undefined" },
   { 0, AST_FORMAT_MAX_VIDEO, "maxvideo", "Maximum video format" },
};

The users/peers store a ast_codecs_pref object
which hold an array of 32 bytes each index in the array
is an order of preference from left to right and the
value of each index is 1 index value from AST_FORMAT_LIST[]
offset by 1 to allow 0 to mean undefined

for instance g726 is at the 5th index in AST_FORMAT_LIST
if you start counting at 1 so to store g726 as your first preference you store a 5 in your prefs obj
if you want alaw to come second you append a 4 this is mostly irrelivant to the programmer because there is an api to handle the adding and removing of codecs The important part is this patch which uses a technique where 56 is added to all the index of your prefs which cause a printable string using letters of the alpahbet to transmit your pref order.

so without understanding it fully one can solicit an specific codec order by sending the information element
CODEC_PREFS with an alpha string of capital letters
corresponding to the above AST_FORMAT_LIST

A = g723
B = gsm
C = ulaw
D = alaw
E = g726
F = adpcm
G = slin
H = lpc10
I = g729
J = speex
K = ilbc

so CODEC_ORDER=JEC will set the callers desired codec order
to speex,g726,ulaw making a very compact way to transmit
a desired codec prefrence and possibly obseleting some
other elements.

Since this is a protocol change it is important that the
community have some feedback in the process so please
post your opinions here.


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

Disclaimer on file
anthmct@yahoo.com
Comments:By: twisted (twisted) 2004-12-02 21:27:00.000-0600

Makes sense to me. I like it. Go go gadget codecs!

By: Anthony Minessale (anthm) 2004-12-02 21:45:06.000-0600

oh yeah it adds iax2 show peer <peername> cli command too for debugging

By: stevekstevek (stevekstevek) 2004-12-03 09:38:38.000-0600

Since I just implemented the old negotiation stuff in libiax2/iaxclient (patch is in CVS, waiting for the solaris patch), and I suppose I'd need to also implement this, let me see if I have this straight:

What happens presently, (as I understand it) is this:

1) The caller sends both format and capabilities when it makes a new call.
2) The remote side chooses a format, based on it's own capabilities, preferences, and the caller's capabilities and preferences.
3) The remote side ACCEPTs the call, and sends back the format to use.

So, this is what I implemented in iaxclient and libiax2.

In step2, asterisk seems to do this:

1) Use the caller's preferred format, if it supports it,
2) Choose amongst all capabilities, using a hard-coded order of preference.

In step2, in iaxclient, I implemented it as:
a) Use the caller's preferred format, if we support it,
b) Use _our_ preferred format, if they support it,
c) finally, look at the union of capabilities, and (the same) hard-coded order.


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

This patch, seems to In "step 2":
a) look at user-defined preferences, and call ast_codec_choose, looking only at our capabilities.
b) if the chosen codec isn't supported by the caller, call ast_codec_choose again with the union of the caller's capabilities, and our capabilities, but using the caller's preferences?
c) if that doesn't work, give up.

===

If I understand the process correctly, then here's my comments:

a) trying ast_codec_choose twice seems wasteful, why not just pass the union of our capabilities and the caller's capabilities to ast_codec_choose in the first place?

Can you correct my understanding of the process of negotiation if I have it wrong?  It is certainly nicer to have a more flexible method of codec negotiation, but I think we need to make sure the process is documented well enough that it should never be surprising what codec is chosen for a call.

By: Anthony Minessale (anthm) 2004-12-03 12:53:20.000-0600

What I have done so far is

1) caller is presented with host's codec list like a plate of cookies.
2) an attempt to take the codec closest to the top of callers list is made.
3) If the caller does not like any of the codecs, the host will choose from
  the combination of the caller and host's common codecs based on the host's
  prefrences.

If the caller does not present any prefs (older version with no patch)
then the host's preference is substituted where any prefs are in need.

Since we don't have an agreed way to handle this it is good that there
is an simple API now that we can use to quickly implement it any way we
see fit so let's figure it out.


Some questions up for debate are:

Who's prefs should get first consideration caller or host?
Shoud it perhaps be configurable? Maybe a host has the option
of passive prefs where it lets the caller decide or a more strict
mode where the host wants to reserve the right to make the decision.

preferences=strict
;prefrences=passive

Should we use some kind of algorythm to create a unified preference order
where we look at both lists and score them 1 to n in order of apperance
discarding any that do not exist in both lists then adding the score
and going with the lowest scoring codec

EXAMPLE

caller
1 g729
2 speex
3 ulaw

host
1 speex
2 ilbc
3 ulaw

When you take the common codecs and combine the score the lowest scoring
codec is speex so it it chosen.

3 speex
6 ulaw

This was all just off the top of my head so feedback is appriciated.

By: Andrey S Pankov (casper) 2004-12-03 13:10:18.000-0600

And what do SIP RFCs say about codec preference? The problem exists only when both codec sets are identical (or identical after codec sets merge accomplished), but codec order in them are different:

calling: speex; g729
called: g729, speex

In your example speex should be used regardless of any "preferences" option.

And what should we do when we have "strict" on both sides?

By: stevekstevek (stevekstevek) 2004-12-03 13:17:25.000-0600

I don't have any strong opinion about the actual mechanics, as long as it's well documented, and stable.  Also, of course, backwards compatibility is important (which should be OK, as long as whatever the receiver choose is in the caller's capabilities).

Please remember that chan_iax2 inside of asterisk isn't the only IAX2 implementation, though, so whatever we do here needs to be (hopefully easily) ported into libiax2, the IAXy, and any other IAX2 implementations that exist out there.

In the end, either side can definitively force a codec by saying it only supports one;  

Right now, the balance of codec choice seems to be on the caller's side (the receiver will accept the caller's format if it can).  Your proposal about adding numbers seems OK;  you could make the IE then be a list of 32 bytes, where each byte is the preference (weight) for the particular codec.   Maybe, if we don't have a reason to do something different, we could look at what SIP does, and work that way..  It might end up making things more transparent when you set up SIP->IAX->SIP calls, etc.

In the long run, IAX2's format stuff might need some extension, though, because there's the hard limit of 32 different formats..  But that's an asterisk limitation as well, and a lot of work to change.  

Also, I'm not sure how this mechanism works with video (I guess it doesn't affect it).

Anyway, assuming Mark lets me take the necessary bits from ast_codec_choose and bring them to libiax2 to keep parity, it's all cool with me.

By: Brian West (bkw918) 2004-12-03 13:20:22.000-0600

I can see kram's head explode at the mention of making something in IAX kinda SIP like. :P

bkw

By: stevekstevek (stevekstevek) 2004-12-03 13:24:17.000-0600

Heh.

I really don't /personally/ give a rat's ass what SIP does, but judging from what I saw at Astricon and read on the lists, it seems that compatibility is important to some people :)

By: Anthony Minessale (anthm) 2004-12-03 13:55:01.000-0600

To answer the question of both sides using strict I had imagined that
the strict mode is only applicable for the host role so it would play no part in the caller's rights rather it would allow the host to declare "I am NOT willing to let you pick the codec" versus "Ok go ahead an pick a codec I dont mind which one you use".  The idea behind this is one application may be in a limited bandwidth site and it may want to ensure the lowest cost codec is chosen where another application may have tons of bandwidth and is willing to let the caller choose his preference.

By: nikko (nikko) 2004-12-04 10:23:40.000-0600

Using letters limits scalability, you might use a numbered list instead

By: Brian West (bkw918) 2004-12-04 12:22:53.000-0600

It doesn't limit anything the codec namespace is only 32 long.

bkw

By: Anthony Minessale (anthm) 2004-12-04 15:46:04.000-0600

rev2 to keep up w CVS

By: Anthony Minessale (anthm) 2004-12-05 09:10:30.000-0600

Not only is namespace only 32 but audio codecs only live in the bottom half.
Also note the letters are lucky coincidence for humans to make some kind
of simplistic connection with what's going on.
A char can hold 0-128 as a matter of fact so the real limit is 128

The method of storing the indexes is not really the topic up for debate
as that has already been adopted into asterisk and chan_sip.  rather we
are discussing the way iax negotiates the codecs.

By: Anthony Minessale (anthm) 2004-12-14 18:00:44.000-0600

buelleur? ................. anyone? this patch already works has anyone played with it ?

By: Anthony Minessale (anthm) 2004-12-23 13:39:08.000-0600

Nobody cares about the hours i wasted mking this problem go away? I am shocked nobody is willing to even comment now the patch is broken cos it sat here festering >=0

By: Brian West (bkw918) 2004-12-26 17:16:59.000-0600

Lets get some input or let mark just say "THIS IS HOW ITS GONNA BE" :P

going once? going twice?  SPEAK UP PEOPLE

bkw

By: Olle Johansson (oej) 2004-12-27 15:49:40.000-0600

This seems very logical to me, since we are getting more and more IAX devices and softphones with various properties. Codec negotiating seems basic. There's no need to make it as complicated as the SDP offer/answer model though ;-)

By: krisk20 (krisk20) 2004-12-27 15:57:52.000-0600

I like the lowest scoring codec match.  Also, I think that the host should be given some weighted preference in actual codec selection.  I would think that it is assumed (in most/some scenarios) that the host would be "wiser" as to which codec should be used (at least more so than the caller)...

By: Anthony Minessale (anthm) 2004-12-29 11:23:50.000-0600

rev3 works with todays cvs though if this patch gets added I want IE id 42 =D

By: stevekstevek (stevekstevek) 2004-12-29 12:09:27.000-0600

IE 42 is already taken:

#define IAX_IE_CAUSECODE                        42              /* Hangup cause (u8) */

I have plans for 43-46:
#define IAX_IE_RR_JITTER                        43              /* Received jitter (as in RFC1889) u32 */
#define IAX_IE_RR_LOSS                          44              /* Received loss (high byte loss pct, low 24 bits loss count, as in rfc1889 */
#define IAX_IE_RR_PKTS                          45              /* Received frames (total frames received) u32 */
#define IAX_IE_RR_DELAY                         46              /* Max playout delay for received frames (in ms) u16 */

But I'm not picky.  I know 43 or 47 are no 42, but they're prime!

By: Anthony Minessale (anthm) 2004-12-30 10:15:18.000-0600

revision 4 to keep up with 12-29-2004 enc IAX patch

By: stevekstevek (stevekstevek) 2004-12-30 10:42:07.000-0600

Two more comments:

1) The change to iax2_parser makes it even less portable to libiax2;  Originally, iax2_parser was supposed to be a module that could compile by itself, It's gotten harder and harder to do that, and to try and keep the libiax2 version up-to-date (already, it's gotten cluttered with asterisk-isms).  It is possible to make dump_prefs work without needing to be linked to asterisk?

2) I haven't tried to compile this yet, but you added stuff to create_addr, but the patch doesn't seem to actually modify create_addr; at least I don't see it's signature changing in the patch.

Without seeing this, I don't know what the format of the IE is, so I can't really comment on it..

My comment on "12-03-04 13:17" still applies, though.  I'd really like to see the negotiation process, and the format of the IEs documented somewhere (unless someone else is going to add all this to libiax2...).

By: mtntop (mtntop) 2004-12-30 12:57:37.000-0600

I certainly don't see any problem with the lowest score model.

But in the case of switch to switch IAX connections, which 'host' would have preference if host preferencing is implemented, and how would this work in a multi-hop config? Perhaps the simple union with lowest score should always win?

By: Anthony Minessale (anthm) 2004-12-30 18:17:58.000-0600

rev5 adds gracious=true possibility to user so you can allow inbound callers to have first crack at picking the codec

in fact rev4 is missing some code lost in the recent changes to CVS so its useless.

By: dhetzel (dhetzel) 2004-12-30 19:10:06.000-0600

Please add support for G.722 (the flavor supported by Grandstream, of the
available flavors of G.722)

Please choose a coding method that does not so sharply constrain the number of possible codecs which can be coded.

Please choose a simple and obviously deterministic codec selection method, i.e. originator says "here are the codecs I am willing to deal with, in order of preference", responder says "here is the one from your list that I choose".

By: Brian West (bkw918) 2004-12-31 09:17:26.000-0600

adding g722 isn't not just as simple as adding it to the list.  The core of asterisk and rtp would have to know about it.

bkw

By: cmaj (cmaj) 2004-12-31 19:13:11.000-0600

Oops I added a patch three times my bad getting late.  But I think that the "show peer" command should be split off from this codec business -- they are separate issues and the command is more immediately useful w/o debate, I would think.  So thats the "just show peer" patch.  (Revised patch compiled clean but not tested.)

I agree with dhetzel that limiting the codecs to 26 is not moving the protocol forward.  Also, I disagree with some semantics in this patch, such as changing "user/peer" to "his/my" and the full text like "mine" is kind of odd.  Maybe it's like this elsewhere in the code base, I'm not sure.

edited on: 12-31-04 19:13

By: Mark Spencer (markster) 2004-12-31 19:52:11.000-0600

1) find_peer does not work with realtime peers.

2) You know better than this:

+ iaxs[callno]->prefs = user->prefs;
+ if(ast_test_flag(user, IAX_CODEC_FIRST_CHOICE)) {
+ iaxs[callno]->flags |= IAX_CODEC_FIRST_CHOICE;
+ }
+

see ast_copy_flags

3) It strikes me we should first narrow down the codec and then simply replace the call to ast_best_codec with something else which takes the correct preferences into account.  That would be the least intrusive and presumably least amount of code method.  I believe the G.726-32 is the G.722 the grandstream implements although they may have opposite indianness from us (we match cisco).  The selection of letters does not limit you to 26 since you can continue up the chain to higher and higher numbers up to 127 or possibly even 255 presumably.  Another option though would be to simply start at 1 and make it clear it's a raw format and not a string.  

4) The name "codec_first_choice" would be the first option name with underscores (lets not) and, as a boolean is confusing.  Does "codec_first_choice=yes" mean it's my choice or the other side's choice?

Other than that, conceptually i think we're on the right track.  Sorry I couldn't get it in tonight!

By: Brian West (bkw918) 2005-01-01 19:50:20.000-0600

I think that should be "iaxs[callno]->_flags" (so we know not to dink with it)

bkw

By: Anthony Minessale (anthm) 2005-01-03 16:55:04.000-0600

biting togune and adressing previous concers with a gritted smile.

By: Kevin P. Fleming (kpfleming) 2005-01-03 20:47:46.000-0600

I'll add my voice to the list of users that think this is a good idea. I haven't reviewed the code so I can't comment on the implementation, though.

By: Mark Spencer (markster) 2005-01-03 23:12:48.000-0600

cvs diff -u if possible please...  thanks!

By: Anthony Minessale (anthm) 2005-01-04 09:44:12.000-0600

doh, diff -u version of rev6 uploaded

By: stevekstevek (stevekstevek) 2005-01-04 11:32:50.000-0600

OK, so, BKW seems to ask for feedback everywhere he goes for this, and I've written feedback several times, but nobody seems to have answered any of my questions.  I went the extra mile here, and spend about a half-hour looking at this.

So, first, I'll answer my own questions:
1) I missed the create_addr change, because the whole signature didn't show up in the patch;  I get it now.  It's also broken, and needs the following:

@@ -2203,6 +2309,10 @@
       if (ofound)
               *ofound = found;
       if (!p && !found) {
+               if(pref_str) { /* use global iax prefs for unknown peer/user */
+                       ast_log(LOG_WARNING, "XXX: using global codec prefs\n");
+                       ast_codec_pref_shift(&prefs, pref_str, pref_size, 1);
+               }
               hp = ast_gethostbyname(peer, &ahp);
               if (hp) {
                       memcpy(&sin->sin_addr, hp->h_addr, sizeof(sin->sin_addr));

With this change, calling the demo transmits our global iax2 prefs, otherwise we would indicate no prefs if you make a call to a destination that is not a user/peer..  

2) I guess I can deal with the iax2-parser stuff, and just skip the other stuff, and just print the actual string..

3) This is kinda of picky, and just a stylistic concern, but I think ast_codec_pref_shift would be easier to understand like this:

void ast_codec_pref_shift(struct ast_codec_pref *pref, char *buf, size_t size,    
int right)
{
-       int x = 0, differential = 65, mem = 0;
+        int x = 0, differential = 'A', mem = 0;
       char *from = NULL, *to = NULL;

And the description is also confusing, because it doesn't shift anything by 65 bytes, it copies the string, and adds or subtracts 65 to the value of each of the string's elements..

/* Shift a codec preference list up or down 65 bytes so that it becomes an ASCII string */

(saying "shift ... bytes" makes one think this thing is like memmove(pos+65,pos, len))...  The real confusion comes from using the unit bytes to describe the change in values..

By: stevekstevek (stevekstevek) 2005-01-05 11:23:26.000-0600

OK, doesn't look like the cases of non-peer/non-user access were considered or tested, in either direction.

In addition to the change given above, for outgoing calls, incoming guest calls are also broken:

The testcase is basically the same one that the demo call to misery.digium.com would make:  Call into a server with this patch from a non-codec-pref IAX2 endpoint (like iaxclient).


In this case, the codec is chosen from the set of capabilities on the receiving side, without taking into account the capabilities of the call initiator.

What ends up happening in this case:

1) The caller comes in, and says "I want Ulaw, I only support Ulaw". (IAX2 NEW packet).
2) The server ignores this, and chooses GSM.  (IAX2 ACCEPT packet).
3) The caller rejects the call, because the server is broken, and chose a codec  we told it we don't support.
4) iaxclient segfaults (this is something i need to fix :)).

The fix for this one, is (this is a diff of a diff..):

+                                                      pref = iaxs[fr.callno]->prefs;
+
+                                              //format = iaxs[fr.callno]->peerformat & iaxs[fr.callno]->capability;
-+                                              format = ast_codec_choose(&pref, iaxs[fr.callno]->capability, 0);
++                                              format = ast_codec_choose(&pref, iaxs[fr.callno]->capability & iaxs[fr.callno]->peercapability, 0);
+                                              ast_codec_pref_string(&rpref, caller_pref_buf, sizeof(caller_pref_buf) - 1);
+                                              ast_codec_pref_string(&iaxs[fr.callno]->prefs, host_pref_buf, sizeof(host_pref_buf) - 1);
+

I can send the complete, updated patch with these two changes, if anyone cares. [it seems to be the thing to do to complain that nobody cares, eh?] "

By: Anthony Minessale (anthm) 2005-01-05 14:14:14.000-0600

added stevekstevek's revisions
changed codec_pref_shift to ast_codec_pref_convert per stevekstevek's suggestion

hopefully his next reply will contain less sarcasm

By: stevekstevek (stevekstevek) 2005-01-05 14:29:24.000-0600

Cool -- you even found another case of the same issue that I didn't..

And.. I'm thinking of a sarcastic way to say this, but... the patch can't compile, because you didn't include your changes to frame.c

P.S.  Please feel free to add sarcastic, or any comments, to bug 3236 bug 3203 or bug 2532

By: Anthony Minessale (anthm) 2005-01-05 15:50:45.000-0600

DoH! rev8 adds frame.? to the patch

LoL that was the most sarcastic non-sarcastic reply I have seen in quite a while I like it.

By: stevekstevek (stevekstevek) 2005-01-05 16:31:21.000-0600

OK, one more (maybe this is the last).

You added my instrumentation, which probably shouldn't be there -- (it was just for me to make sure I knew which way the code actually was going, when figuring out why prefs weren't working..):

+                       ast_log(LOG_WARNING, "XXX: using global codec prefs\n");

I don't think any message needs to be there at all.

I guess I'll wait 'till kram gives his thumbs up on this (and perhaps, until someone gives me the go-ahead to use this code in LGPL libiax2), before I port it over there..  iaxclient people probably might get pissed if I change the API again, though, so I guess I'll also have to try to figure out how to add this to the API without breaking the old one..

By: Anthony Minessale (anthm) 2005-01-05 17:14:02.000-0600

ok rev9 removes the debug line and the // comments i was using a reference for the old behaviour

By: Mark Spencer (markster) 2005-01-09 04:28:34.000-0600

Merged in CVS head, thanks!

By: Digium Subversion (svnbot) 2008-01-15 15:20:45.000-0600

Repository: asterisk
Revision: 4727

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

------------------------------------------------------------------------
r4727 | markster | 2008-01-15 15:20:44 -0600 (Tue, 15 Jan 2008) | 2 lines

Merge experimental codec preferences for IAX2 (bug ASTERISK-2918)

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

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