[asterisk-dev] [Code Review]: ICE, STUN, and TURN Support

Wed May 9 08:40:55 CDT 2012

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, lines 450-456
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line450>
> >
> >     Some ast<->pj type conversion functions might be useful for here and below, but each conversion is only used once so eh. It might be nice to have everything in one place. Completely your call, though. I'm just getting bored from not making any comments at this point.

Yeah, I pondered the same thing but after writing it and seeing how it was only used the one... I didn't think it was warranted.

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, lines 458-460
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line458>
> >
> >     I think the 1/2 should be COMPONENT_RTP/COMPONENT_RTCP.

You would be correct!

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, lines 531-532
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line531>
> >
> >     You could use ast_strndup here.

Changed.

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, lines 544-550
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line544>
> >
> >     See above about conversion functions. Also, the only places I see this called specify the type in the call--is there any reason we don't just pass the AST_ version to the function instead of the PJ_ and do away with conversion altogether?
> 
> Terry Wilson wrote:
>     Ah, missed the call to pj_ice_calc_foundation above. So perhaps just always passing things around as the AST_ version, then have a conversion function for when we need the PJ?

It's also used with the pj_ice_sess_add_cand call, thus why I pass in a PJ version. It's the most used in this function, which saves having another variable to store the result of a conversion or doing the conversion twice.

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, lines 564-574
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line564>
> >
> >     We have this same function in lots of different places-chan_sip, several res_calendar modules, etc. It might be time to add it to strings.h.

Agreed, I can do it as part of this if nobody has any qualms.

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, line 662
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line662>
> >
> >     Needs to be initialized before the copy according to netsock2.h.

Done.

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, line 984
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line984>
> >
> >     Good 'ol ARRAY_LEN not good enough for you now? Gotta go using the PJ one? Traitor. :-p

Hi!

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, lines 1268-1274
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line1268>
> >
> >     Does the magic number 0 for the RTP component rely on the component ID that we pass in when creating the session? If so, it might be good to use a #defined value based off of COMPONENT_RTP. Annoying that they make one zero-offset and the other not. :-/

Indeed, wrote a helper function that just takes the component and does the right thing.

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, line 663
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line663>
> >
> >     A red blob!

Fixed.

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, lines 1325-1330
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line1325>
> >
> >     Same question about the 0 here as above.

Fixed.

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, lines 1405-1409
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line1405>
> >
> >     Same

Fixed.

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, line 1519
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line1519>
> >
> >     Initialize.

Fixed.

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, line 1634
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line1634>
> >
> >     Initialize.

Fixed.

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, lines 1899-1905
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line1899>
> >
> >     Same 0 question as above.

Fixed.

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, lines 2744-2749
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line2744>
> >
> >     Same 0 question. By this time, I'm thinking that this block of code should be a helper function regardless of whether something should be done with the 0. :)

Fixed.

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, lines 3474-3479
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line3474>
> >
> >     Again.

Fixed.

> On May 3, 2012, 4:34 p.m., Terry Wilson wrote:
> > /trunk/res/res_rtp_asterisk.c, lines 3739-3745
> > <https://reviewboard.asterisk.org/r/1891/diff/1/?file=27606#file27606line3739>
> >
> >     Does pj_shutdown need to be called here?

Yes! Fixed.

- Joshua

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

On April 30, 2012, 1:50 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1891/
> -----------------------------------------------------------
> 
> (Updated April 30, 2012, 1:50 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch adds support for the following:
> 
> ICE attribute parsing and generation in chan_sip
> Usage of the ICE interface in chan_sip
> ICE support within res_rtp_asterisk
> STUN support within res_rtp_asterisk for getting the server reflexive address
> TURN support within res_rtp_asterisk for relaying traffic when needed
> Additional configuration options for the above
> 
> One area which could use some feedback is the pjproject integration. Should we try to use their build system as I have done to integrate things, or try to roll their stuff into ours further?
> 
> If you would like some background on ICE and the actual support here take a gander at the wiki page located at https://wiki.asterisk.org/wiki/display/~jcolp/ICE%2C+STUN%2C+and+TURN+Support - hopefully it proves useful to you.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 361952 
>   /trunk/configs/rtp.conf.sample 361952 
>   /trunk/include/asterisk/rtp_engine.h 361952 
>   /trunk/main/rtp_engine.c 361952 
>   /trunk/res/Makefile 361952 
>   /trunk/res/res_rtp_asterisk.c 361952 
> 
> Diff: https://reviewboard.asterisk.org/r/1891/diff
> 
> 
> Testing
> -------
> 
> Tested various scenarios using Bria and X-Lite behind multiple NATs, different NATs, different internet connections but locally reachable, etc. It works well.
> 
> 
> Thanks,
> 
> Joshua
> 
>

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