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

[Joshua Colp]

> On 2009-03-31 15:36:15, Russell Bryant wrote:
> > /trunk/channels/chan_sip.c, lines 21302-21325
> > <http://reviewboard.digium.com/r/209/diff/3/?file=3866#file3866line21302>
> >
> >     It looks like you converted a bunch of tabs to spaces in this section.

Fixed.


> On 2009-03-31 15:36:15, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, lines 629-632
> > <http://reviewboard.digium.com/r/209/diff/3/?file=3877#file3877line629>
> >
> >     This can be simplified as just:
> >     
> >        codecs->pref = *prefs;

Done.


> On 2009-03-31 15:36:15, Russell Bryant wrote:
> > /trunk/channels/chan_sip.c, line 7997
> > <http://reviewboard.digium.com/r/209/diff/3/?file=3866#file3866line7997>
> >
> >     missing space after if

Fixed.


> On 2009-03-31 15:36:15, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, line 299
> > <http://reviewboard.digium.com/r/209/diff/3/?file=3877#file3877line299>
> >
> >     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.

Fixed.


> On 2009-03-31 15:36:15, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, lines 41-48
> > <http://reviewboard.digium.com/r/209/diff/3/?file=3877#file3877line41>
> >
> >     These comments could use a ! to be doxygenified.

Fixed.


> On 2009-03-31 15:36:15, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, lines 520-524
> > <http://reviewboard.digium.com/r/209/diff/3/?file=3877#file3877line520>
> >
> >     Change the capitalization of this AstFormat stuff

Fixed everywhere.


> On 2009-03-31 15:36:15, Russell Bryant wrote:
> > /trunk/include/asterisk/rtp_engine.h, lines 423-443
> > <http://reviewboard.digium.com/r/209/diff/3/?file=3871#file3871line423>
> >
> >     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).

Done.


> On 2009-03-31 15:36:15, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, lines 588-623
> > <http://reviewboard.digium.com/r/209/diff/3/?file=3877#file3877line588>
> >
> >     This function could be simplified using an ast_str instead of a raw char buffer.

Done.


> On 2009-03-31 15:36:15, Russell Bryant wrote:
> > /trunk/include/asterisk/rtp_engine.h, lines 294-356
> > <http://reviewboard.digium.com/r/209/diff/3/?file=3871#file3871line294>
> >
> >     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.

As discussed out of band this is not a blocker for getting this out there, but I have made a note to look at doing this for both the RTP engine API and the bridging API.


- Joshua


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


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
> 
>