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

[jcolp]

> On Feb. 6, 2013, 3:53 p.m., Mark Michelson wrote:
> > /team/group/pimp_my_sip/channels/chan_gulp.c, line 191
> > <https://reviewboard.asterisk.org/r/2318/diff/1/?file=33276#file33276line191>
> >
> >     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.

Noted.


> On Feb. 6, 2013, 3:53 p.m., Mark Michelson wrote:
> > /team/group/pimp_my_sip/channels/chan_gulp.c, line 228
> > <https://reviewboard.asterisk.org/r/2318/diff/1/?file=33276#file33276line228>
> >
> >     To save some typing and line length, it may be worthwhile to save off a reference to session->media[AST_SIP_MEDIA_AUDIO].rtp.

Meh, I don't think that's worthwhile (perhaps, yet).


> On Feb. 6, 2013, 3:53 p.m., Mark Michelson wrote:
> > /team/group/pimp_my_sip/configs/res_sip.conf.sample, line 22
> > <https://reviewboard.asterisk.org/r/2318/diff/1/?file=33277#file33277line22>
> >
> >     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?

It forces it on. Changed.


> On Feb. 6, 2013, 3:53 p.m., Mark Michelson wrote:
> > /team/group/pimp_my_sip/configs/res_sip.conf.sample, line 24
> > <https://reviewboard.asterisk.org/r/2318/diff/1/?file=33277#file33277line24>
> >
> >     This name makes a lot more sense than "autoframing" :)

Agreed.


> On Feb. 6, 2013, 3:53 p.m., Mark Michelson wrote:
> > /team/group/pimp_my_sip/res/res_sip_sdp_audio.c, lines 139-140
> > <https://reviewboard.asterisk.org/r/2318/diff/1/?file=33281#file33281line139>
> >
> >     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);

Changed.


> On Feb. 6, 2013, 3:53 p.m., Mark Michelson wrote:
> > /team/group/pimp_my_sip/res/res_sip_sdp_audio.c, line 367
> > <https://reviewboard.asterisk.org/r/2318/diff/1/?file=33281#file33281line367>
> >
> >     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.

Changed.


> On Feb. 6, 2013, 3:53 p.m., Mark Michelson wrote:
> > /team/group/pimp_my_sip/res/res_sip_sdp_audio.c, line 399
> > <https://reviewboard.asterisk.org/r/2318/diff/1/?file=33281#file33281line399>
> >
> >     Any particular reason you didn't use ast_copy_pj_str() here?

This code originated from before we had it, changed.


> On Feb. 6, 2013, 3:53 p.m., Mark Michelson wrote:
> > /team/group/pimp_my_sip/res/res_sip_session.c, line 500
> > <https://reviewboard.asterisk.org/r/2318/diff/1/?file=33282#file33282line500>
> >
> >     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.

Changed.


> On Feb. 6, 2013, 3:53 p.m., Mark Michelson wrote:
> > /team/group/pimp_my_sip/res/res_sip_session.c, lines 1099-1101
> > <https://reviewboard.asterisk.org/r/2318/diff/1/?file=33282#file33282line1099>
> >
> >     Does this stuff need to change if IPv6 is being used for the media?

Changed.


- jcolp


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


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/44cdecbf/attachment-0001.htm>