[asterisk-dev] [Code Review] Bridging API for Conference Bridge purposes

[Russell Bryant]

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



/trunk/apps/app_confbridge.c
<http://reviewboard.digium.com/r/93/#comment456>

    You should use ast_str_case_hash()



/trunk/apps/app_confbridge.c
<http://reviewboard.digium.com/r/93/#comment455>

    The coding guidelines say to keep code limited to 90 columns per line.  We haven't really enforced this much in the past, but personally, I would like to start doing so.  :-)



/trunk/apps/app_confbridge.c
<http://reviewboard.digium.com/r/93/#comment457>

    This is a common enough construct that it could probably be put into a function to ensure that nobody makes the mistake of not handling the locking properly



/trunk/apps/app_confbridge.c
<http://reviewboard.digium.com/r/93/#comment458>

    You can simplify testing multiple flags by doing ...
    
    if (!ast_test_flag(foo, FLAG1 | FLAG2))



/trunk/apps/app_confbridge.c
<http://reviewboard.digium.com/r/93/#comment459>

    I would go ahead and set bridge = NULL here.  It sometimes helps with debugging problems.



/trunk/apps/app_confbridge.c
<http://reviewboard.digium.com/r/93/#comment460>

    Having to lock the container isn't very common.  So, I would add a comment that indicates that the reason is so you prevent multiple callers from creating the same conference at the same time.



/trunk/apps/app_confbridge.c
<http://reviewboard.digium.com/r/93/#comment461>

    Please set the pointer to NULL after unref



/trunk/apps/app_confbridge.c
<http://reviewboard.digium.com/r/93/#comment462>

    \retval 0 success
    \retval -1 failure



/trunk/apps/app_confbridge.c
<http://reviewboard.digium.com/r/93/#comment463>

    Allocating and freeing an ast_channel is pretty expensive to do over and over again.  Could we optimize this so that the channel only gets created once for the life of the conference bridge?


- Russell


On 2008-12-15 21:34:46, Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/93/
> -----------------------------------------------------------
> 
> (Updated 2008-12-15 21:34:46)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch implements the new bridging API and brings with it a module for conference bridges. It does *not* replace existing internal bridging or features yet and will not cause any regressions when put in. It will essentially be introduced as a first test phase to work out any unforeseen critical issues. The bridging core itself is fully implemented besides the following: jitterbuffer support, native bridging, and interval hooks (hooks that are time based versus action based). If you would like an explanation of what the bridging API is made up of and how it works that can be found in the bridging.h header file.
> 
> 
> Diffs
> -----
> 
>   /trunk/Makefile 164597 
>   /trunk/apps/app_confbridge.c PRE-CREATION 
>   /trunk/bridges/Makefile PRE-CREATION 
>   /trunk/bridges/bridge_multiplexed.c PRE-CREATION 
>   /trunk/bridges/bridge_simple.c PRE-CREATION 
>   /trunk/bridges/bridge_softmix.c PRE-CREATION 
>   /trunk/channels/chan_bridge.c PRE-CREATION 
>   /trunk/include/asterisk/bridging.h PRE-CREATION 
>   /trunk/include/asterisk/channel.h 164597 
>   /trunk/main/Makefile 164597 
>   /trunk/main/bridging.c PRE-CREATION 
> 
> Diff: http://reviewboard.digium.com/r/93/diff
> 
> 
> Testing
> -------
> 
> Conference bridge testing using app_confbridge with features. Joining two channels with simple frame exchange and joining three channels to move it to a true conference bridge. IVR capability of app_confbridge was also tested.
> 
> 
> Thanks,
> 
> Joshua
> 
>