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


Can you please add a note to UPGRADE.txt about requiring modifying modules.conf to load res_rtp_asterisk for RTP support for those not using autoload?

Thanks!


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

    Since this is derived from the original RTP infrastructure, you should probably also retain the copyright from rtp.h, as well.



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

    trailing whitespace



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

    This is incredibly pedantic, but I like the \brief on a new line.  :-)



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

    Can we fix this code to adhere to our naming convention?



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

    Use ast_verb()



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

    ast_verb()



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

    ast_verb()



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

    You can simplify this entire block with:
    
    current_glue = AST_RWLIST_REMOVE(...);



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

    ast_verb()



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

    Perhaps a LOG_ERROR message would be useful here to give people a hint that they need to load an RTP engine module?



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

    superfluous return



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

    can you change the naming of isAstFormat?



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

    superfluous return (and in a number of other places)



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

    change isAstFormat



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

    ARRAY_LEN()



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

    change isAstFormat



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

    This looks like a function that should be bumping up a ref count.



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

    Can you break this up over multiple lines?  The number of args here is getting pretty out of hand.  :-)



/trunk/main/stun.c
<http://reviewboard.digium.com/r/209/#comment1641>

    Trim whitespace from this file



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

    Surely something in here can fail ...



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

    ARRAY_LEN()



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

    ARRAY_LEN()


- Russell


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