Summary:	ASTERISK-16153: [patch] RTP directmedia is broken in some cases
Reporter:	Steve Davies (one47)	Labels:
Date Opened:	2010-05-26 09:45:03	Date Closed:	2011-04-28 08:55:24
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Channels/chan_sip/CodecHandling
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) post-patch.pcap ( 1) pre-patch.pcap ( 2) sip_codecs ( 3) sip_codecs_simplified ( 4) sip_codecs_simplified3 ( 5) sip_codecs_simplified4
Description:	It is "well known" that setting up directmedia paths between SIP devices with different preferred-codec lists can be problematic. The solution has previously been to either disable directmedia, or use the codec-negotiation patch which is available for Asterisk 1.2 and 1.4. PROBLEM: In its simplest form, the problem is (I believe) as follows: When RTP tries to set up a direct media path, it does so by setting each party separately, and giving each party free reign over their codec choice. Thus under some circumstances the 2 remote devices can make 2 choices which are different. If the 2 parties disagree on the negotiated codec, you generally get silence in both directions. SOLUTION: Make rtp.c just a bit more clever about what codecs each party is offered - In fact, do not allow a choice, just tell them what is acceptable. EXAMPLE: Device A (g722|alaw) calls via asterisk to device B (alaw) - Initially a transcoded path is setup between the 2 parties. directmedia is enabled, and they have a common codec (alaw). At present, during negotiation, RTP tells A to use (alaw) and B to use (g722|alaw) - chan_sip.c then notes those codec requests, ignores them and tells A to use (g722|alaw) and B to use (alaw) Of course this results in A using g722, and B using alaw - Resulting in silence. The patch (to be attached) changes: main/rtp.c to make it choose a single audio and video codec where possible before passing the directmedia request on to the channels. channels/chan_sip.c to make it use the passed in codec offering from RTP as long as it is doing a directmedia reinvite - At any other time, old behaviour remains, allowing better codec choices where possible. ****** ADDITIONAL INFORMATION ****** Patch tested with audio on: Yealink T22/T28 (g722|alaw) Cisco SPA502G (alaw only) Snom 360 (alaw only) Patch tested with audio and video on: Yealink VP2009 (alaw + h263) Polycom VVX1500 (alaw|g722 + h263)
Comments:	By: Leif Madsen (lmadsen) 2010-05-27 09:04:45 Can you also provide the SIP trace showing this happening? By: Steve Davies (one47) 2010-05-27 10:01:35 2 pcap traces uploaded to demonstrate the change. Looking at these traces, I may have broken some re-INVITE de-dupe code as there seem to be several extra re-INVITE packets that are identical. I'll check to see if I can identify what is happening there separately. By: Steve Davies (one47) 2010-05-27 11:42:12 *** Please change the target of this bug to be Channels/SIP *** Just found the cause of the multiple INVITES, and as a result I have posted a simplified patch. The cause of the extra invites was rtp.c trying to to the "right thing" in a different way, which clashed with my new mechanism. I removed the new mechanism, and taught chan_sip.c to use the existing one more correctly. ie. Use the RTP specified codecs instead of the sip preferred ones to create a native bridge. The downside of the new mechanism is that rtp.c MAY sometimes request some redundant re-INVITEs. This should be harmless, and the upside is that it is MUCH more simple. (I will re-do the larger verison of the code and propose it for 1.8 at a later date.) By: vadim (vadim) 2010-05-27 12:25:48 I wonder how difficult is to enchance this patch with 'fmtp' forwarding functionality. By: Paul Belanger (pabelanger) 2010-05-27 13:19:45 Minor CODING-GUIDELINES, missing braces around last else statement. By: Steve Davies (one47) 2010-05-27 16:05:58 Updated as per my corrected understanding of the CODING-GUIDELINES :) By: Steve Davies (one47) 2010-05-27 16:06:59 Re-uploaded as a source code item for licence - Please delete  sip_codecs_simplified2 By: Leif Madsen (lmadsen) 2010-06-08 09:34:58 Marking this as Ready for Testing for now unless someone feels that is not the correct course of action. By: Bruce McAlister (asgaroth) 2010-10-15 08:13:18 This patch fails to apply on asterisk 1.6.2.13 for me By: Private Name (falves11) 2010-10-16 03:51:55 I need a patch for 1.6.2 urgently. This issue affects the performance of my network since years ago. Congratulations to the author. By: Steve Davies (one47) 2010-10-16 11:20:50 The existing patch applies fine, and compiles on 1.6.2.13 for me. Just to be nice, I have re-cut the patch so that the line numbers match and the warnings go away when running patch. By: Private Name (falves11) 2010-10-22 13:54:58 The patch works fine in 1.62 and I think it makes my ASR go up. So let's bring it into the main code. By: Digium Subversion (svnbot) 2010-11-01 09:58:04 Repository: asterisk Revision: 293493 U   branches/1.8/channels/chan_sip.c ------------------------------------------------------------------------ r293493 | twilson | 2010-11-01 09:58:03 -0500 (Mon, 01 Nov 2010) | 14 lines Only offer codecs both sides support for directmedia When using directmedia, Asterisk needs to limit the codecs offered to just the ones that both sides recognize, otherwise they may end up sending audio that the other side doesn't understand. (closes issue ASTERISK-16153) Reported by: one47 Patches:      sip_codecs_simplified4 uploaded by one47 (license 23) Tested by: one47, falves11 Review: https://reviewboard.asterisk.org/r/967/ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=293493 By: Digium Subversion (svnbot) 2011-01-17 10:38:23.000-0600 Repository: asterisk Revision: 302048 _U  trunk/ U   trunk/channels/chan_sip.c ------------------------------------------------------------------------ r302048 | twilson | 2011-01-17 10:38:23 -0600 (Mon, 17 Jan 2011) | 21 lines Merged revisions 293493 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.8 ........  r293493 | twilson | 2010-11-01 09:58:00 -0500 (Mon, 01 Nov 2010) | 14 lines    Only offer codecs both sides support for directmedia    When using directmedia, Asterisk needs to limit the codecs offered to just  the ones that both sides recognize, otherwise they may end up sending audio  that the other side doesn't understand.    (closes issue ASTERISK-16153)  Reported by: one47  Patches:        sip_codecs_simplified4 uploaded by one47 (license 23)  Tested by: one47, falves11    Review: https://reviewboard.asterisk.org/r/967/ ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=302048