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

[Terry Wilson]

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


I mostly only found piddly unimportant stuff.


/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11259>

    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.



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11257>

    I think the 1/2 should be COMPONENT_RTP/COMPONENT_RTCP.



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11258>

    You could use ast_strndup here.



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11260>

    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?



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11262>

    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.



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11277>

    Needs to be initialized before the copy according to netsock2.h.



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11255>

    A red blob!



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11264>

    Good 'ol ARRAY_LEN not good enough for you now? Gotta go using the PJ one? Traitor. :-p



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11268>

    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. :-/



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11269>

    Same question about the 0 here as above.



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11270>

    Same



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11271>

    Initialize.



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11272>

    Initialize.



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11273>

    Same 0 question as above.



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11274>

    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. :)



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11275>

    Again.



/trunk/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/1891/#comment11276>

    Does pj_shutdown need to be called here?


- Terry


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/20120503/fd9b7f32/attachment-0001.htm>