[asterisk-dev] [Code Review] 3439: chan_sip: Support a=rtcp attribute in SDP

[Corey Farrell]

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



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/3439/#comment21391>

    SDP doesn't appear to allow whitespace before ":".  I suggest compare to to "rtcp:", to prevent "rtcp-wrong-attr:" and similar.  You could also just use "rtcp:%30u" for sscanf.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/3439/#comment21397>

    I will not get in the way if this change does not support parsing the optional address of the rtcp attribute.
    
    For any rtcp attribute where the address is specified, we need to either parse it or reject the whole attribute and return FALSE.



/trunk/include/asterisk/rtp_engine.h
<https://reviewboard.asterisk.org/r/3439/#comment21394>

    RFC 3605 allows an address to be set with the attribute.  I think this is worth giving a thought since we're making an ABI/API change and we might actually want remote_rtcp_address_set.
    
    I feel this finding applies even if we don't support parsing the address part from chan_sip right now.



/trunk/main/rtp_engine.c
<https://reviewboard.asterisk.org/r/3439/#comment21396>

    Nothing checks the return which is always 0, can just return void like other setter functions.


- Corey Farrell


On April 11, 2014, 9:46 a.m., Olle E Johansson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3439/
> -----------------------------------------------------------
> 
> (Updated April 11, 2014, 9:46 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> the A=rtcp attribute in SDP points out a different port than the mediaport+1 to receive RTCP on. This patch adds a new api to rtpengine and res_rtp_asterisk and updates chan_sip to use it.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_rtp_asterisk.c 412166 
>   /trunk/main/rtp_engine.c 412166 
>   /trunk/include/asterisk/rtp_engine.h 412166 
>   /trunk/channels/chan_sip.c 412166 
> 
> Diff: https://reviewboard.asterisk.org/r/3439/diff/
> 
> 
> Testing
> -------
> 
> A massive amount of testing with a test tool for interoperability.
> 
> 
> Thanks,
> 
> Olle E Johansson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140416/0e478937/attachment.html>