Review Board 1.7.16


Bridging API for Conference Bridge purposes

Review Request #93 - Created Dec. 15, 2008 and submitted

Joshua Colp
team/file/bridging
Reviewers
asterisk-dev
russell
Asterisk
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.
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.
Review request changed
Updated (March 2, 2009, 4:49 a.m.)
Few new updates!
Posted (March 2, 2009, 11:23 p.m.)

   

  
/trunk/bridges/bridge_multiplexed.c (Diff revision 5)
 
 
I wonder if this should not be a static variable to allow for run-time config.
Imo this value will heavily depend on actual hardware the PBX will run 
  1. I agree that this should be configurable, but I'm going to push that to phase 2 since that is where it will actually make the most difference. We can then do tests to figure out the best solution for determining what the values should be (whether we can do it automatically) or making it configurable.
/trunk/bridges/bridge_multiplexed.c (Diff revision 5)
 
 
This should be static var too IMO.
  1. See above.
/trunk/bridges/bridge_multiplexed.c (Diff revision 5)
 
 
This is a dumb question maybe, but don't you think a pipe would be cheaper nudging mechanism than a signal?
  1. Good point, I will change it to use a pipe instead of a signal.
/trunk/bridges/bridge_multiplexed.c (Diff revision 5)
 
 
What is the interest to awake the thread BEFORE modifying  the channell array?

  1. This is to ensure that the multiplexed thread will be immediately servicing the channel upon return of the API call/that the thread will not be servicing the channel upon return of the API call.
Ship it!
Posted (March 5, 2009, 3:40 a.m.)
I think it's time we merge this code into trunk so that it gets more exposure and more testing.  It is self contained, so there is very low risk involved at this point.  We can continue working on it in trunk as we find things to improve.

Very nice work, Josh!
/trunk/apps/app_confbridge.c (Diff revision 5)
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
You have declared two instances of these two nameless enums.  You can just remove "option_flags" and "option_args".
  1. Done.
/trunk/apps/app_confbridge.c (Diff revision 5)
 
 
 
These could be unsigned
  1. Done.
/trunk/apps/app_confbridge.c (Diff revision 5)
 
 
 
superfluous return
  1. Done.
/trunk/apps/app_confbridge.c (Diff revision 5)
 
 
superfluous return
  1. Done.
/trunk/apps/app_confbridge.c (Diff revision 5)
 
 
 
superfluous return

(I'll stop marking these, now, you get the point.)  :-)
  1. Done.
/trunk/main/bridging.c (Diff revision 5)
 
 
 
missing space "ast_channel*"

Also, check for allocation failure here
  1. Done.
/trunk/main/bridging.c (Diff revision 5)
 
 
It's kind of interesting that usleep(1) is used everywhere in asterisk, when what we really mean is that we want to yield to other threads.

I think for new code, we should start using sched_yield() instead.  At some point, we should make fixing all of the other code a janitor project.
  1. Done in all the new code here.
/trunk/main/bridging.c (Diff revision 5)
 
 
 
 
 
 
 
 
 
 
I presume this function assumes the bridge is locked.  I would document that fact.
  1. Done.
/trunk/main/bridging.c (Diff revision 5)
 
 
I wish all of this code was wrapped at 90 columns, but we can deal with that later ...
  1. I'll make a note to modify my emacs configuration to make sure that at least all my future code personally adheres to this.
/trunk/main/bridging.c (Diff revision 5)
 
 
 
 
 
 
It seems like the handling of waiting should be done with the lock held to synchronize with threads that want to check this value.
  1. You are right in that bridge->waiting should be set before we release the lock to ensure anything waiting on waiting won't be allowed to execute until it has been set. The setting back to 0 though has to be done without the lock held because anything waiting on it has the lock held. I've made the change and think it should work fine.

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.