[asterisk-dev] [Code Review] Pimp My SIP Media Improvements

[Mark Michelson]

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



/team/group/pimp_my_sip/channels/chan_gulp.c
<https://reviewboard.asterisk.org/r/2318/#comment14805>

    This is either a step backwards or the change to use session->endpoint->context wasn't merged up into the media branch when you posted your review. Either way, be on the lookout for this when you merge.



/team/group/pimp_my_sip/channels/chan_gulp.c
<https://reviewboard.asterisk.org/r/2318/#comment14806>

    To save some typing and line length, it may be worthwhile to save off a reference to session->media[AST_SIP_MEDIA_AUDIO].rtp.



/team/group/pimp_my_sip/configs/res_sip.conf.sample
<https://reviewboard.asterisk.org/r/2318/#comment14808>

    Is this an option to allow IPv6 for RTP to be used, or is this option meant to force the usage of IPv6 for RTP?



/team/group/pimp_my_sip/configs/res_sip.conf.sample
<https://reviewboard.asterisk.org/r/2318/#comment14807>

    This name makes a lot more sense than "autoframing" :)



/team/group/pimp_my_sip/res/res_sip_sdp_audio.c
<https://reviewboard.asterisk.org/r/2318/#comment14809>

    ast_sockaddr_resolve() allocates memory on the heap. Be sure to ast_free(addrs) when you are finished in this function. The easy way to do this would be to use RAII_VAR when declaring addrs
    
    RAII_VAR(struct ast_sockaddr *, addrs, NULL, ast_free_ptr);



/team/group/pimp_my_sip/res/res_sip_sdp_audio.c
<https://reviewboard.asterisk.org/r/2318/#comment14810>

    You don't have the same memory leak with this use of ast_sockaddr_resolve(), but the idea of RAII_VAR here still might make things useful. It can probably be used with other allocations as well, so you may not even need a cleanup label at all.



/team/group/pimp_my_sip/res/res_sip_sdp_audio.c
<https://reviewboard.asterisk.org/r/2318/#comment14811>

    Any particular reason you didn't use ast_copy_pj_str() here?



/team/group/pimp_my_sip/res/res_sip_session.c
<https://reviewboard.asterisk.org/r/2318/#comment14812>

    This bothers me.
    
    One annoying part about chan_sip is that a bunch of information on a sip_peer gets copied onto a sip_pvt. For the new SIP work, the session has a reference to the endpoint, so there should be no reason to copy endpoint data onto the session. Instead of having rtp_ipv6 on the session, just always get this value via the endpoint.



/team/group/pimp_my_sip/res/res_sip_session.c
<https://reviewboard.asterisk.org/r/2318/#comment14813>

    Does this stuff need to change if IPv6 is being used for the media?


- Mark


On Feb. 6, 2013, 11:57 a.m., jcolp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2318/
> -----------------------------------------------------------
> 
> (Updated Feb. 6, 2013, 11:57 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> These changes clean up media handling, move some more stuff into res_sip_sdp_audio, fixes a few bugs, and adds some additional features.
> 
> The act of negotiating an SDP media stream and actually applying the media stream are now separate operations.
> Hold/unhold works.
> RTP over IPv6 works.
> Use of the 'ptime' attribute works.
> Local Packet2Packet bridging works.
> Symmetric RTP can now be enabled per-endpoint.
> Reduced memory pool usage.
> Fixed bug where the RTP instance was never destroyed.
> 
> 
> Diffs
> -----
> 
>   /team/group/pimp_my_sip/channels/chan_gulp.c 380960 
>   /team/group/pimp_my_sip/configs/res_sip.conf.sample 380960 
>   /team/group/pimp_my_sip/include/asterisk/res_sip.h 380960 
>   /team/group/pimp_my_sip/include/asterisk/res_sip_session.h 380960 
>   /team/group/pimp_my_sip/res/res_sip/sip_configuration.c 380960 
>   /team/group/pimp_my_sip/res/res_sip_sdp_audio.c 380960 
>   /team/group/pimp_my_sip/res/res_sip_session.c 380960 
> 
> Diff: https://reviewboard.asterisk.org/r/2318/diff
> 
> 
> Testing
> -------
> 
> 1. Sent and received calls from a few different devices
> 2. Held/unheld a call
> 3. Attempted to set up incompatible calls (only configured for gsm, but offering ulaw only)
> 
> 
> Thanks,
> 
> jcolp
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130206/a2c0e7a0/attachment-0001.htm>