[Home]

Summary:ASTERISK-02893: [patch] when disallow=all and allow=all in [general], all other codec settings in peers do not workand no sound heard.
Reporter:Max Litnitskiy (litnimax)Labels:
Date Opened:2004-11-26 09:41:37.000-0600Date Closed:2008-01-15 15:15:41.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/CodecHandling
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_sip.c_nonformat.patch.txt
( 1) codec_fix_rev2.diff.txt
( 2) codec_fix_rev3.diff.txt
( 3) codec_fix.diff.txt
Description:When  you have
[general]
....
disallow=all
allow=all
....

[user]
disallow=all
allow=gsm (for example, nothing works)

In call attempt you can hear no sound on Asterisk CVS-HEAD-07/30/04-01:22:44 and
Nov 26 13:56:29 NOTICE[19935]: rtp.c:489 ast_rtp_read: Unknown RTP codec 72 received  on later builds.

if you remove disallow=all allow=all in  [general], all works. This is not a critical error, but somebody spent much time to figure it out.
Sorry to bother you with such a minor issue!

Comments:By: Brian West (bkw918) 2004-11-26 10:18:13.000-0600

You never do allow=all.  This isn't a bug really becuase you want to only allow what you want in the general section and leave the rest out... but never allow=all

bkw

edited on: 11-26-04 10:18

By: ewieling (ewieling) 2004-11-26 13:26:29.000-0600

The problem is actually that disallow= lines in peers are not honoured.

By: Brian West (bkw918) 2004-11-27 01:15:57.000-0600

Yep just tested if I disallow=all, disallow=ulaw allow=g729 its correct so we have a stray ulaw in here somewhere.

bkw

By: Brian West (bkw918) 2004-11-27 02:47:02.000-0600

EUREKA!!! I think I found it!

By: Max Litnitskiy (litnimax) 2004-11-27 05:32:48.000-0600

After months of saying in lists that nobody should do allow=all, many offten DO and waste much time. If you correct it, you'll save users time and nerves :)

By: khb (khb) 2004-11-27 10:57:39.000-0600

Has anyone thought about getting rid of this allow/disallow nonsense, and simply have an option

codecs=codec_a codec_b codec_c
where codec_x could also be "all"

if one really needs
allow=all
disallow=codec_y

it could be
codecs=all -codec_y

edited on: 11-27-04 11:01

By: khb (khb) 2004-11-28 16:27:58.000-0600

bkw:  when you say you discovered this extra ULAW(4) creeping up, are you talking about the prefcodec that gets assigned in sip_request() from pbx core?

Indeed it always seemed to add PCMU to the top of the offer list, and is indicated by 'Answering with root capability' or similar in the logs.

Do you all think that the offer/answer model as implemented is working ok?

By: Brian West (bkw918) 2004-11-28 16:35:49.000-0600

it has to be the core because chan_iax does the same thing. :P

bkw

By: Brian West (bkw918) 2004-11-29 10:32:01.000-0600

Again as always less is more... this should do the same thing with less code.

bkw

By: Anthony Minessale (anthm) 2004-11-29 16:50:47.000-0600

This patch 'frame_chan_sip.diff.txt' fixes bug 2938
and also refactors a ton of messy code dealiing
with codecs the end result is:

A) One can now get a faithful "all" when you allow=all
B) One can list several codecs in 1 allow line (allow=ulaw,g726)
C) Each peer and user has it's own codec preference order (the order you define them)
D) The directive 'allow=all' no longer breaks the preference logic
E) The zombie channel transfer bug (2938) is eliminated.
F) Dramatic re-factoring of several routines.

Disclaimer on file

anthmct@yahoo.com

By: khb (khb) 2004-11-30 00:18:14.000-0600

Anthm: Looking at these latestes patches only briefly, it does a lot of good things. I had done very similar things and will compare.
I have obsoleted the allow/disallow syntax (but still accepted), and only use codecs= [none] [all] <list>
and wish this would become the norm. What are the thoughts on that?
You maintain the linked preference list, I have removed it and simply use an array, which is much more efficient since we have only so many codecs (fixed set) to work with, the size of the vector is smaller than all the code to maintain the linked lists. Seemed extremely weird to use a list.
Q: how does your patch combine the peer preferences (1st priority I assume) with the channel preferences and the requested codec that gets passed from the PBX in sip_request()?

By: Brian West (bkw918) 2004-11-30 16:56:14.000-0600

khb we actually are redoing this patch without the link lists... we are almost done with it.  the reason allow/disallow must stay is to keep backwards compatibility.  The codecs are sent in the order using the same method the global prefs are done.

Example:

sip show peer 16 output has this
 Codecs       : 0x15 (ulaw|g726)
 Codec Order  : (g726|ulaw)

We're at 65.38.28.146 port 24678
Answering with preferred capability 0x10 (g726)
Answering with preferred capability 0x4 (ulaw)
Answering with non-codec capability 0x1 (telephone-event)
Reliably Transmitting (no NAT):
SIP/2.0 200 OK
v: SIP/2.0/UDP 65.38.28.147:5060;branch=z9hG4bK-b3c7c30d
f: Brian West <sip:16@65.38.28.146>;tag=8c383c236fa547ddo0
t: <sip:996@65.38.28.146>;tag=as2e2910fc
i: 51bb0e1b-3d53de45@65.38.28.147
CSeq: 102 INVITE
User-Agent: bkw.org/1.0
Allow: INVITE, ACK, CANCEL, OPTIONS, BYE, REFER
m: <sip:996@65.38.28.146>
c: application/sdp
l: 267

v=0
o=root 31245 31245 IN IP4 65.38.28.146
s=session
c=IN IP4 65.38.28.146
t=0 0
m=audio 24678 RTP/AVP 2 0 4 101
a=rtpmap:2 G726-32/8000
a=rtpmap:0 PCMU/8000
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-16
a=silenceSupp:off - - - -


We also fixed the "Answering with non-codec capability 0x1 (telephone-event)"

It used to say g723.1 as the non-codec capability but that was just in the verbose output.. very confusing when you're debugging ya know :P

bkw

edited on: 11-30-04 17:01

By: Brian West (bkw918) 2004-11-30 17:00:01.000-0600

Another example.  

allow=all in general.

[16]
type=friend
allow=g726,ulaw

 Codecs       : 0xf07ff (g723.1|gsm|ulaw|alaw|g726|adpcm|slinear|lpc10|g729|speex|ilbc|jpeg|png|h261|h263)
 Codec Order  : (g726|ulaw)

We're at 65.38.28.146 port 18978
Answering with preferred capability 0x10 (g726)
Answering with preferred capability 0x4 (ulaw)
Answering with capability 0x1 (g723.1)
Answering with capability 0x2 (gsm)
Answering with capability 0x8 (alaw)
Answering with capability 0x20 (adpcm)
Answering with capability 0x40 (slinear)
Answering with capability 0x80 (lpc10)
Answering with capability 0x100 (g729)
Answering with capability 0x200 (speex)
Answering with capability 0x400 (ilbc)
Answering with non-codec capability 0x1 (telephone-event)
Reliably Transmitting (no NAT):
SIP/2.0 200 OK
v: SIP/2.0/UDP 65.38.28.147:5060;branch=z9hG4bK-d12e6536
f: Brian West <sip:16@65.38.28.146>;tag=31cc15fa58186b22o0
t: <sip:996@65.38.28.146>;tag=as0b1476d1
i: 6a94b0da-a57d8dc2@65.38.28.147
CSeq: 102 INVITE
User-Agent: bkw.org/1.0
Allow: INVITE, ACK, CANCEL, OPTIONS, BYE, REFER
m: <sip:996@65.38.28.146>
c: application/sdp
l: 467

v=0
o=root 31288 31288 IN IP4 65.38.28.146
s=session
c=IN IP4 65.38.28.146
t=0 0
m=audio 18978 RTP/AVP 2 0 4 3 8 5 10 7 18 110 97 101
a=rtpmap:2 G726-32/8000
a=rtpmap:0 PCMU/8000
a=rtpmap:4 G723/8000
a=rtpmap:3 GSM/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:5 DVI4/8000
a=rtpmap:10 L16/8000
a=rtpmap:7 LPC/8000
a=rtpmap:18 G729/8000
a=rtpmap:110 speex/8000
a=rtpmap:97 iLBC/8000
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-16
a=silenceSupp:off - - - -

to 65.38.28.147:5060

By: khb (khb) 2004-11-30 19:35:30.000-0600

What you are doing there with the telephone-event isn't correct.
Attached is correct patch.

The payload type is 101.  but the events have individual names. see patch.

PS:  the two routines should actually go into frame.c of course where the other format type ones live.

edited on: 11-30-04 19:50

By: Brian West (bkw918) 2004-11-30 20:20:56.000-0600

Our patch is correct we only corrected the verbose output.

This is correct... it is Telephone event.. we were doing an ast_getformatbyname in the ast_verbose line.. but on the snprintf were doing it right.. we just made the verbose correct.

ast_verbose("Answering with non-codec capability 0x%x (%s)\n", x, ast_rtp_lookup_mime_subtype(0, x));


snprintf(costr, sizeof(costr), "a=rtpmap:%d %s/8000\r\n", codec, ast_rtp_lookup_mime_subtype(0, x));

edited on: 11-30-04 20:21

By: Brian West (bkw918) 2004-11-30 20:22:42.000-0600

ast_verbose("Answering with non-codec capability 0x%x(%s)\n", x, ast_getnonformatname(x));

That is wrong its not a format its part of rtp.c so we should use ast_rtp_lookup_mime_subtype.

bkw

By: Anthony Minessale (anthm) 2004-11-30 20:28:02.000-0600

codec_fix.diff.txt does a boatload please review.

By: connor (connor) 2004-11-30 21:15:46.000-0600

When/If this makes it to CVS, Will it be back-ported to 1.x ??  I sure hope it does, because this has been a major problem/bug for lots of people... drumkilla, comments?

By: khb (khb) 2004-11-30 21:42:10.000-0600

bkw: read the patch
it's not a format, that's why they are called non-codec capabilities, and therefore ...getnonformat... is called, which was lacking. ...getformat.. printed G723 because the routine didn't exist.
It should print DTMF and not telephone-event in exactly the same spirit as all the other formats were printed.  The value at that point is  '1' and not '101' and it refers to AST_RTP_DTMF.
At that point we were printing, e.g., ULAW, and not PCMU,  therefore we should be printing DTMF and not telephone-event.

By: Brian West (bkw918) 2004-11-30 21:54:40.000-0600

from rtp.c:
[101] = {0, AST_RTP_DTMF},

{{0, AST_RTP_DTMF}, "audio", "telephone-event"},

Granted what we print might not be right but its not 100% incorrect either.  frame.c yet again needs to gain another fetures/function for this.

bkw

By: khb (khb) 2004-11-30 22:31:38.000-0600

Anything is only right or wrong in a given framework, and in this case the frame was prior art.  The only other 'right' is what the standards bodies prescribe and in that case we have been doing many things wrong. I am all for standardizing.
For codecs, * is using various names in various places. ulaw in the configuration, g711 in the printout, PCMU in the SDP lists. What needs to be done is pull all of that out of each module and put it in one place. We should be refering to one codec the same way everywhere, and after that permit users to use some common aliases.
Yes, there is too much duplication/overlap, no clear separation between rtp.c and frame.c.

By: Brian West (bkw918) 2004-11-30 23:59:27.000-0600

Notes to me and tony.. show codecs and show translation are whacked now.

bkw

By: Anthony Minessale (anthm) 2004-12-01 12:37:39.000-0600

rev2 fixes "show codecs" and "show translation"

edited on: 12-01-04 12:38

By: Brian West (bkw918) 2004-12-02 19:44:05.000-0600

ok this patch some what is in CVS now with one small change that kram did.. but I think that change broke it.

Example:

Answering with preferred capability 0x100 (g729)
Answering with preferred capability 0x10 (g726)
Answering with preferred capability 0x4 (ulaw)
Answering with capability 0x1 (g723)
Answering with capability 0x2 (gsm)
Answering with capability 0x8 (alaw)
Answering with capability 0x20 (adpcm)
Answering with capability 0x40 (slin)
Answering with capability 0x80 (lpc10)
Answering with capability 0x200 (speex)
Answering with capability 0x400 (ilbc)

Ok now on this peer I have this:

disallow=all
allow=g726,ulaw

Codecs       : 0xf07ff (g723|gsm|ulaw|alaw|g726|adpcm|slin|lpc10|g729|speex|ilbc|jpeg|png|h261|h263)
 Codec Order  : (g729|g726|ulaw)


It should ONLY answer with the capabilities I said so right?

edited on: 12-02-04 19:44

By: Brian West (bkw918) 2004-12-02 21:02:13.000-0600

NOTES: ast_parse_allow_deny should be ast_parse_allow_disallow

once we all decide on the way IAX should do the pref order...

bkw

By: Mark Spencer (markster) 2004-12-03 18:47:49.000-0600

Merged in CVS except for IAX portion which will happen separately.  Also did the name change for allow_disallow.  Thanks!

By: Russell Bryant (russell) 2004-12-06 09:03:16.000-0600

fixed in 1.0 as well - will be in 1.0.3

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

Repository: asterisk
Revision: 4370

U   trunk/frame.c
U   trunk/include/asterisk/frame.h

------------------------------------------------------------------------
r4370 | markster | 2008-01-15 15:15:26 -0600 (Tue, 15 Jan 2008) | 2 lines

Merge updates to frame.h and frame.c (codec stuff from bug ASTERISK-2893, part 1)

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

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

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

Repository: asterisk
Revision: 4371

U   trunk/channels/chan_sip.c

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

Merge SIP portion of new codec work from bug ASTERISK-2893

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

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

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

Repository: asterisk
Revision: 4385

U   trunk/channels/chan_sip.c
U   trunk/frame.c
U   trunk/include/asterisk/frame.h

------------------------------------------------------------------------
r4385 | markster | 2008-01-15 15:15:40 -0600 (Tue, 15 Jan 2008) | 2 lines

Fix allow/disallow nomenclature (bug ASTERISK-2893, part deux)

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

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