Review Board 1.7.16

Add ability to set an alternate source for RTP media. Helps in SIP glare situations.

Review Request #252 - Created May 19, 2009 and submitted

Mark Michelson
For a description of the problem, see this:

This patch implements a way to let the RTP stack know that there may be a potential alternate source for media. The alterations to RTP are:

* new function ast_rtp_set_altpeer to set what the alternate media source may be.
* new sockaddr_in structures inside the ast_rtp and ast_rtcp structures to hold these alternate sources.
* ast_rtp_read and ast_rtcp_read have been updated to expect media from these alternate sources.

The alterations to chan_sip are:

* new function get_ip_and_port_from_sdp to get the remote IP address and port for audio/video streams from the SDP
* When we are going to respond to a REINVITE with a 491, we call get_ip_and_port_from_sdp, followed by ast_rtp_set_altpeer so that the
  RTP stack will not react incorrectly when it receives media from this alternate source.

Please see the "Testing Done" section for some code review details.
I tested by setting up phones and Asterisk boxes as shown in the first diagram in the link I printed in the "Description."

I found that in REINVITE glare situations, I always successfully had two-way audio. There was a slight catch, though. Usually there would be about a 0.5-1 second gap between when the callee answered the phone and when the caller was able to hear the callee's audio. I have not yet been able to track this odd behavior down. So, in addition to making sure that what I have presented here looks reasonable, if any reviewers might be able to point out what potentially is causing the short delay upon answering the calls, please speak up.
Review request changed
Updated (May 20, 2009, 5:48 a.m.)
Addressed Vadim's idea regarding the efficiency of get_ip_and_port_from_sdp. Re-tested and found that everything still appears to be working.
Ship it!
Posted (May 22, 2009, 6:31 a.m.)
I think vadim's point about another parameter for RTCP is something still good to consider for the sake of API stability.  It is reasonable to assume that we may need that parameter in the future, so it wouldn't hurt to have it there.

However, I would only worry about it for the trunk version of the patch, if at all.
/branches/1.4/include/asterisk/rtp.h (Diff revision 2)
Add \since 1.6.3 to the docs here
  1. Hmm, the plan was to add this to all branches, not just trunk. In fact, this review request was made against 1.4.
    Would \since 1.4.26 make more sense?
  2. Yeah, \since 1.4.26, sorry about that. runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to