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

[Joshua Colp]

> On 2009-01-29 19:06:07, Mark Michelson wrote:
> > /trunk/main/bridging.c, lines 336-344
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2260#file2260line336>
> >
> >     I did some testing and found a case where this loop continues forever.
> >     
> >     I had two users using app_confbridge. A third user attempted to enter. Because I had no timing source defined, the call to bridge_softmix's thread callback would immediately return -1 because ast_timer_open would fail.
> >     
> >     Instead of trying to fix this myself, I'm leaving it for you, because there are several points at which this problem might be fixed, and you know best where it should be. In my opinion, smart_bridge_operation should never pick bridge_softmix to use if there is no timing source available, and app_confbridge should not be usable if there is no timing source.
> >     
> >     Also, while I'm looking at this code here, res is set but never used.
> 
>  wrote:
>     Partially done. bridge_softmix will no longer load if a timing module is not available.

This has been completed. A new API call (ast_bridge_check) has been added that sees if it is possible to fulfill a specific set of capabilities. app_confbridge uses this to see if it can go to a multimix bridge in the future.


> On 2009-01-29 19:06:07, Mark Michelson wrote:
> > /trunk/main/bridging.c, lines 1199-1213
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2260#file2260line1199>
> >
> >     Why not use the Dialling API here?

The dialing API does not have the capability that is used here. It will need to have an API call added to essentially steal the dialed channel before it has answered. That is what happens with this code.


> On 2009-01-29 19:06:07, Mark Michelson wrote:
> > /trunk/main/bridging.c, line 1216
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2260#file2260line1216>
> >
> >     I believe Russell may have already commented on this, but I am strongly in favor of separating the bridge feature execution code from the actual bridge operation API.
> >     
> >     In addition to just being more logical in general, this will also remove the dependency on chan_local from a core piece of code.

Done. The built in features are now pluggable via a separate module. It is totally optional, the built in features will just fail to be enabled if it is not loaded.


> On 2009-01-29 19:06:07, Mark Michelson wrote:
> > /trunk/main/bridging.c, lines 1309-1310
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2260#file2260line1309>
> >
> >     Hard-coded DTMF? me no likey
> 
>  wrote:
>     Quite. I think I might go down the route of having unique feature structures that are able to be passed in that allows the developer to change the DTMF digits, plus potentially also have a default read from a configuration file... hrm!

Done. This is now configurable via a structure passed in to the enable API call on both the blind and attended transfer ones.


> On 2009-01-29 19:06:07, Mark Michelson wrote:
> > /trunk/main/bridging.c, line 1254
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2260#file2260line1254>
> >
> >     This comment seems out of place since there is no locking occurring here.

Fixed.


> On 2009-01-29 19:06:07, Mark Michelson wrote:
> > /trunk/main/bridging.c, line 1255
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2260#file2260line1255>
> >
> >     I know the trend in Asterisk code is to keep code compact, but I think this is going overboard. May I humbly request that this is broken up for the sake of readability?

Done.


- Joshua


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


On 2008-12-31 15:45:07, Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/93/
> -----------------------------------------------------------
> 
> (Updated 2008-12-31 15:45:07)
> 
> 
> Review request for Asterisk Developers and Russell Bryant.
> 
> 
> 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
> 
>