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

Joshua Colp jcolp at digium.com
Mon Mar 30 14:29:32 CDT 2009 


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > 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!

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/res/res_rtp_asterisk.c, line 2522
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3829#file3829line2522>
> >
> >     ARRAY_LEN()

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/res/res_rtp_asterisk.c, line 2513
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3829#file3829line2513>
> >
> >     ARRAY_LEN()

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/stun.c, lines 19-28
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3828#file3828line19>
> >
> >     Trim whitespace from this file

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, line 899
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3827#file3827line899>
> >
> >     Can you break this up over multiple lines?  The number of args here is getting pretty out of hand.  :-)

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, lines 392-393
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3827#file3827line392>
> >
> >     superfluous return

Fixed.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, lines 408-409
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3827#file3827line408>
> >
> >     superfluous return (and in a number of other places)

Fixed.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, lines 298-302
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3827#file3827line298>
> >
> >     Perhaps a LOG_ERROR message would be useful here to give people a hint that they need to load an RTP engine module?

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/include/asterisk/rtp_engine.h, lines 1-7
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3821#file3821line1>
> >
> >     Since this is derived from the original RTP infrastructure, you should probably also retain the copyright from rtp.h, as well.

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/include/asterisk/rtp_engine.h, lines 397-408
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3821#file3821line397>
> >
> >     trailing whitespace

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, lines 238-246
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3827#file3827line238>
> >
> >     You can simplify this entire block with:
> >     
> >     current_glue = AST_RWLIST_REMOVE(...);

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, lines 250-252
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3827#file3827line250>
> >
> >     ast_verb()

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, lines 225-227
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3827#file3827line225>
> >
> >     ast_verb()

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, lines 194-196
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3827#file3827line194>
> >
> >     ast_verb()

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, lines 168-170
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3827#file3827line168>
> >
> >     Use ast_verb()

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, line 600
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3827#file3827line600>
> >
> >     ARRAY_LEN()

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/res/res_rtp_asterisk.c, lines 2512-2516
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3829#file3829line2512>
> >
> >     Surely something in here can fail ...

Fixed.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, lines 49-54
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3827#file3827line49>
> >
> >     Can we fix this code to adhere to our naming convention?

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/include/asterisk/rtp_engine.h, line 464
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3821#file3821line464>
> >
> >     This is incredibly pedantic, but I like the \brief on a new line.  :-)

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, line 402
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3827#file3827line402>
> >
> >     can you change the naming of isAstFormat?

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, line 596
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3827#file3827line596>
> >
> >     change isAstFormat

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, line 613
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3827#file3827line613>
> >
> >     change isAstFormat

Done.


> On 2009-03-30 09:50:07, Russell Bryant wrote:
> > /trunk/main/rtp_engine.c, lines 728-743
> > <http://reviewboard.digium.com/r/209/diff/2/?file=3827#file3827line728>
> >
> >     This looks like a function that should be bumping up a ref count.

The only time this is called is to get the glue of a channel that is being bridged, which means the channel will have already bumped up the module reference count which would stop it from getting unregistered. That's why I didn't do reference counting on it.


- Joshua


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


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

More information about the asterisk-dev mailing list