[asterisk-dev] [Code Review] Modularized RTP stack support

[Russell Bryant]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/209/#review666
-----------------------------------------------------------

Ship it!


Excellent work, Josh!

At this point, I'm only finding nitpick items that really aren't that big of a deal.  Also, most of it is in pre-existing code.  I think this is ready to go.


/trunk/include/asterisk/rtp_engine.h
<http://reviewboard.digium.com/r/209/#comment1721>

    This is declared twice in this file.



/trunk/include/asterisk/rtp_engine.h
<http://reviewboard.digium.com/r/209/#comment1722>

    Fix capitalization of arguments here



/trunk/include/asterisk/rtp_engine.h
<http://reviewboard.digium.com/r/209/#comment1723>

    capitalization



/trunk/include/asterisk/stun.h
<http://reviewboard.digium.com/r/209/#comment1724>

    This should probably go in include/asterisk/_private.h with the rest of the _init() functions



/trunk/res/res_rtp_asterisk.c
<http://reviewboard.digium.com/r/209/#comment1725>

    Can you put these default values in a global constant?  They are referenced in a couple of places.



/trunk/res/res_rtp_asterisk.c
<http://reviewboard.digium.com/r/209/#comment1726>

    These min/max values could go into a global constant as well



/trunk/res/res_rtp_asterisk.c
<http://reviewboard.digium.com/r/209/#comment1727>

    nochecksums = ast_false(s) ? 1 : 0;


- Russell


On 2009-04-01 09:21:47, Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/209/
> -----------------------------------------------------------
> 
> (Updated 2009-04-01 09:21:47)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch provides a common API (known as the RTP engine API) so that RTP stacks can be easily plugged into Asterisk. Functionality wise this patch should be equal to the current capabilities of our in-core RTP stack. The API is documented in the rtp_engine.h header file and the in-core RTP stack has been broken out into a module called res_rtp_asterisk.
> 
> 
> Diffs
> -----
> 
>   /trunk/UPGRADE.txt 185780 
>   /trunk/apps/app_dial.c 185780 
>   /trunk/channels/chan_agent.c 185780 
>   /trunk/channels/chan_bridge.c 185780 
>   /trunk/channels/chan_gtalk.c 185780 
>   /trunk/channels/chan_h323.c 185780 
>   /trunk/channels/chan_jingle.c 185780 
>   /trunk/channels/chan_local.c 185780 
>   /trunk/channels/chan_mgcp.c 185780 
>   /trunk/channels/chan_sip.c 185780 
>   /trunk/channels/chan_skinny.c 185780 
>   /trunk/channels/chan_unistim.c 185780 
>   /trunk/configs/sip.conf.sample 185780 
>   /trunk/include/asterisk/rtp.h 185780 
>   /trunk/include/asterisk/rtp_engine.h PRE-CREATION 
>   /trunk/include/asterisk/stun.h PRE-CREATION 
>   /trunk/main/Makefile 185780 
>   /trunk/main/asterisk.c 185780 
>   /trunk/main/loader.c 185780 
>   /trunk/main/rtp.c 185780 
>   /trunk/main/rtp_engine.c PRE-CREATION 
>   /trunk/main/stun.c PRE-CREATION 
>   /trunk/res/res_rtp_asterisk.c PRE-CREATION 
> 
> Diff: http://reviewboard.digium.com/r/209/diff
> 
> 
> Testing
> -------
> 
> I've tested using the most complex channel driver that uses the RTP stack, chan_sip. I've confirmed calls of various scenarios work but would like further testing with additional channel drivers that I am unable to test.
> 
> 
> Thanks,
> 
> Joshua
> 
>