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

[Mark Michelson]

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


This review is only partial and comes from taking a brief look through the code and doing some testing of app_confbridge. Some of these comments overlap with what has been recommended already, but I'm too lazy to edit this review. :)


/trunk/include/asterisk/bridging.h
<http://reviewboard.digium.com/r/93/#comment809>

    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.



/trunk/include/asterisk/bridging.h
<http://reviewboard.digium.com/r/93/#comment675>

    "created using ast_bridge_new"



/trunk/main/bridging.c
<http://reviewboard.digium.com/r/93/#comment676>

    I would like to see the channel's name (in addition to its address) printed here.



/trunk/main/bridging.c
<http://reviewboard.digium.com/r/93/#comment677>

    Spaces around the minus operators.



/trunk/main/bridging.c
<http://reviewboard.digium.com/r/93/#comment831>

    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.



/trunk/main/bridging.c
<http://reviewboard.digium.com/r/93/#comment678>

    You may wish to add a return here if newcapabilities is 0. it will save the computation needed by calling find_best_technology.



/trunk/main/bridging.c
<http://reviewboard.digium.com/r/93/#comment679>

    Any reason you don't call bridge_poke here?



/trunk/main/bridging.c
<http://reviewboard.digium.com/r/93/#comment763>

    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.



/trunk/main/bridging.c
<http://reviewboard.digium.com/r/93/#comment761>

    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.



/trunk/main/bridging.c
<http://reviewboard.digium.com/r/93/#comment792>

    Why not use the Dialling API here?



/trunk/main/bridging.c
<http://reviewboard.digium.com/r/93/#comment755>

    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.



/trunk/main/bridging.c
<http://reviewboard.digium.com/r/93/#comment806>

    This comment seems out of place since there is no locking occurring here.



/trunk/main/bridging.c
<http://reviewboard.digium.com/r/93/#comment805>

    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?



/trunk/main/bridging.c
<http://reviewboard.digium.com/r/93/#comment810>

    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.



/trunk/main/bridging.c
<http://reviewboard.digium.com/r/93/#comment802>

    Hard-coded DTMF? me no likey


- Mark


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