[asterisk-dev] [Code Review] Add ability to set an alternate source for RTP media. Helps in SIP glare situations.

[Mark Michelson]

> On 2009-05-19 19:26:45, vadim wrote:
> > /branches/1.4/main/rtp.c, line 2070
> > <http://reviewboard.digium.com/r/252/diff/1/?file=5273#file5273line2070>
> >
> >     If i remember correctly there is an SDP to attribute specify rctp port port explicitly.
> >     So it would be wise to add an rtcpport parametr to this function and not simply assume that is is rtpport + 1
> >     
> >

I believe you are referring to RFC 3605. After looking at that document, not only can the RTCP port be specified in an a=rtcp line, but also a separate IP address may be specified for RTCP. You are correct, we should be looking for this attribute line in an SDP in case RTCP should be sent to a separate destination.

However, I have never seen this attribute used before, and this isn't the main focus of this patch. If we want to add support for parsing rtcp attribute lines in an SDP, I think it should be done as a separate patch. For now, I am simply copying the logic used currently when parsing an SDP.


> On 2009-05-19 19:26:45, vadim wrote:
> > /branches/1.4/channels/chan_sip.c, line 5118
> > <http://reviewboard.digium.com/r/252/diff/1/?file=5271#file5271line5118>
> >
> >     I think
> >     int get_ip_and_port_from_sdp(struct sip_request *req, const enum media_type media, struct sockaddr_in *sin);
> >     
> >     Will be more efficient

I will make that change.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/252/#review787
-----------------------------------------------------------


On 2009-05-19 15:00:31, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/252/
> -----------------------------------------------------------
> 
> (Updated 2009-05-19 15:00:31)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> For a description of the problem, see this: http://lists.digium.com/pipermail/asterisk-dev/2009-May/038348.html
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/channels/chan_sip.c 194872 
>   /branches/1.4/include/asterisk/rtp.h 194872 
>   /branches/1.4/main/rtp.c 194872 
> 
> Diff: http://reviewboard.digium.com/r/252/diff
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> Mark
> 
>