[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/#review658
-----------------------------------------------------------



/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/209/#comment1687>

    missing space after if



/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/209/#comment1688>

    It looks like you converted a bunch of tabs to spaces in this section.



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

    I know it's a lot of functions, but full API documentation for each interface callback would be really helpful as an implementation guide for future RTP engine implementations.



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

    In all of the API documentation, there is a new tag that Jeff started adding when documenting changes between versions.  Please add the \since tag (\since 1.6.3).



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

    Excellent job on documentation of the RTP engine API calls!



/trunk/main/rtp_engine.c
<http://reviewboard.digium.com/r/209/#comment1692>

    These comments could use a ! to be doxygenified.



/trunk/main/rtp_engine.c
<http://reviewboard.digium.com/r/209/#comment1693>

    This ast_free() should be an ao2_ref(instance, -1).  Also, note that your destructor will be called after this, so make sure that ast_module_unref() isn't done twice due to the line above.



/trunk/main/rtp_engine.c
<http://reviewboard.digium.com/r/209/#comment1694>

    Change the capitalization of this AstFormat stuff



/trunk/main/rtp_engine.c
<http://reviewboard.digium.com/r/209/#comment1695>

    This function could be simplified using an ast_str instead of a raw char buffer.



/trunk/main/rtp_engine.c
<http://reviewboard.digium.com/r/209/#comment1697>

    This can be simplified as just:
    
       codecs->pref = *prefs;


- Russell


On 2009-03-30 14:45:07, Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/209/
> -----------------------------------------------------------
> 
> (Updated 2009-03-30 14:45:07)
> 
> 
> 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/main/rtp_engine.c PRE-CREATION 
>   /trunk/channels/chan_skinny.c 185119 
>   /trunk/channels/chan_unistim.c 185119 
>   /trunk/configs/sip.conf.sample 185119 
>   /trunk/include/asterisk/rtp.h 185119 
>   /trunk/include/asterisk/rtp_engine.h PRE-CREATION 
>   /trunk/include/asterisk/stun.h PRE-CREATION 
>   /trunk/main/Makefile 185119 
>   /trunk/main/asterisk.c 185119 
>   /trunk/main/loader.c 185119 
>   /trunk/main/rtp.c 185119 
>   /trunk/UPGRADE.txt 185119 
>   /trunk/apps/app_dial.c 185119 
>   /trunk/channels/chan_agent.c 185119 
>   /trunk/channels/chan_bridge.c 185119 
>   /trunk/channels/chan_gtalk.c 185119 
>   /trunk/channels/chan_h323.c 185119 
>   /trunk/channels/chan_jingle.c 185119 
>   /trunk/channels/chan_local.c 185119 
>   /trunk/channels/chan_mgcp.c 185119 
>   /trunk/channels/chan_sip.c 185119 
>   /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
> 
>