[asterisk-dev] [Code Review] 1.4 implementation of fix for issue 12434

[Mark Michelson]

> On 2009-07-17 15:40:13, David Vossel wrote:
> > /branches/1.4/channels/chan_sip.c, line 6787
> > <https://reviewboard.asterisk.org/r/311/diff/1/?file=5942#file5942line6787>
> >
> >     is this change correct?!

Nope, that was a change I made at first when I couldn't make Asterisk give a reasonable answer to a video offer in the SDP. I made this change after looking at Jared Smith's patch on issue 15121. However, it turned out the reason Asterisk wasn't giving a valid answer was because I did not have the proper video codec allowed for the peer in sip.conf.

I'll upload a new diff.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/311/#review987
-----------------------------------------------------------


On 2009-07-15 16:40:05, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/311/
> -----------------------------------------------------------
> 
> (Updated 2009-07-15 16:40:05)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> In issue 12434, the reporter describes a situation in which audio and video is offered on the call, but because videosupport is disabled in sip.conf, Asterisk gives no response at all to the video offer. According to RFC 3264, all media offers should have a corresponding answer. For offers we do not intend to actually reply to with meaningful values, we should still reply with the port for the media stream set to 0.
> 
> In this patch, we take note of what types of media have been offered and save the information on the sip_pvt. The SDP in the response will take into account whether media was offered. If we are not otherwise going to answer a media offer, we will insert an appropriate m= line with the port set to 0.
> 
> It is important to note that this patch is pretty much a bandage being applied to a broken bone. The patch *only* helps for situations where video is offered but videosupport is disabled and when udptl_pt is disabled but T.38 is offered. Asterisk is not guaranteed to respond to every media offer. Notable cases are when multiple streams of the same type are offered. The 2 media stream limit is still present with this patch, too.
> 
> In trunk and the 1.6.X branches, things will be a bit different since Asterisk also supports text in SDPs as well. Also, since T.38 negotiation is different in those branches, further testing of T.38-specific code will be needed there. I will post a trunk version of these changes to reviewboard after I have written the code.
> 
> 
> This addresses bug 12434.
>     https://issues.asterisk.org/view.php?id=12434
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/channels/chan_sip.c 206706 
> 
> Diff: https://reviewboard.asterisk.org/r/311/diff
> 
> 
> Testing
> -------
> 
> I set up 2 sipp scenarios. One offered audio and video, and the other offered audio and T.38. I verified that Asterisk gave always gave an appropriate answer to the video and T.38 offers.
> 
> 
> Thanks,
> 
> Mark
> 
>