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

[Joshua Colp]

> On 2009-01-29 19:06:07, Mark Michelson wrote:
> > /trunk/include/asterisk/bridging.h, line 196
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2257#file2257line196>
> >
> >     You should define the MAXIMUM_FEATURE_DTMF_STRING (or whatever it is called) in this file and use that define here in this structure definition for the dtmf buffer.

Done.


> On 2009-01-29 19:06:07, Mark Michelson wrote:
> > /trunk/main/bridging.c, lines 185-186
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2260#file2260line185>
> >
> >     Spaces around the minus operators.

Done.


> On 2009-01-29 19:06:07, Mark Michelson wrote:
> > /trunk/include/asterisk/bridging.h, line 377
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2257#file2257line377>
> >
> >     "created using ast_bridge_new"

Done.


> On 2009-01-29 19:06:07, Mark Michelson wrote:
> > /trunk/main/bridging.c, line 158
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2260#file2260line158>
> >
> >     I would like to see the channel's name (in addition to its address) printed here.

Done.


> On 2009-01-29 19:06:07, Mark Michelson wrote:
> > /trunk/main/bridging.c, line 554
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2260#file2260line554>
> >
> >     You may wish to add a return here if newcapabilities is 0. it will save the computation needed by calling find_best_technology.

Done.


> On 2009-01-29 19:06:07, Mark Michelson wrote:
> > /trunk/main/bridging.c, line 572
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2260#file2260line572>
> >
> >     Any reason you don't call bridge_poke here?

No, the code just wasn't updated when bridge_poke was added. Now it is! Done.


> 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

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!


> On 2009-01-29 19:06:07, Mark Michelson wrote:
> > /trunk/main/bridging.c, lines 1274-1279
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2260#file2260line1274>
> >
> >     It seems like somewhere in this portion of this function, you need to indicate AST_CONTROL_HOLD to the transferee so that he/she hears MOH while the the transferer is dialing and talking to the transfer target. You may also wish to put the transferee into autoservice, too.

This is true. I think this might entail making an API call that allows you to do non-mixed frame queueing to all bridge participants. That would take care of the case of multiple people in the bridge. As for the autoservice comment this isn't needed in the bridging API since the bridge will continue to service the channel despite one executing a feature.


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

Partially done. bridge_softmix will no longer load if a timing module is not available.


> On 2009-01-29 19:06:07, Mark Michelson wrote:
> > /trunk/main/bridging.c, line 702
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2260#file2260line702>
> >
> >     This "500" here is equivalent to the old featuredigittimeout of 500 ms, which has since been changed to something more sensible.
> >     
> >     I'm of the opinion that featuredigittimeout is a silly thing to configure, so hardcoding it in here like this is acceptable. I think, though, that a more sensible timeout should be used here.

Done.


> On 2009-01-29 19:06:07, Mark Michelson wrote:
> > /trunk/main/bridging.c, lines 705-708
> > <http://reviewboard.digium.com/r/93/diff/2/?file=2260#file2260line705>
> >
> >     This debug statement isn't completely accurate here. A timeout only occurred if ast_waitfordigit returns 0. A return less than 0 would actually be indicative of something else, such as attempting to gather a digit from a zombie channel or if the channel you are waiting for hangs up.

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