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-0600 | Date Closed: | 2008-01-15 15:15:41.000-0600 |
Priority: | Minor | Regression? | No |
Status: | Closed/Complete | Components: | 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 |