[Home]

Summary:ASTERISK-04195: [patch] [post 1.2] Bridge of 2 existing channels including Manager API
Reporter:Matt Florell (mflorell)Labels:
Date Opened:2005-05-16 16:49:12Date Closed:2011-06-07 14:05:03
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Resources/res_features
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) action_bridge-updated-10-12.txt
( 1) res_features_CVS.patch5
Description:The patches below(for releases 1.0.6, 1.0.7 and CVS-HEAD) allow the Bridging of two existing channels wither through the Manager API or through a function call. I use MASQ to dump the two existing channels into something I can manipulate then use ast_bridge_call to bridge the calls in a separate thread, using the same method that a call is consultatively transferred.

This code does sometimes cause a couple WARNINGs upon execution, but has never caused crashes or corruption of channels. Maybe a more experienced Asterisk coder can figure out how to streamline the patch somewhat.

Example Manager API Action send:

Action: Bridge
ChannelA: Zap/1-1
ChannelB: SIP/cc100-cd91

Comments:By: Tony Mountifield (softins) 2005-05-17 01:44:11

Glad you got it working, Matt! (TonyM, irc)

It's only a minor point, which I meant to mention earlier: I would use the labels Channel1 and Channel2 instead of A and B, to be consistent with the output of Link and Unlink events.

By: cmaj (cmaj) 2005-05-17 14:39:57

Just a few things:

-- I agree with changing ChannelA/B to 1/2, it's more consistent with the rest of the code.
-- For whatever reason, I was specifying a bad second channel, but then the first channel deadlocks.  You need to release the channel lock on the first channel if the second channel does not exist.
-- I was looking at action_redirect in manager.c, and I'm curious as to why you didn't put this code in manager.c ?  It might be a good idea to also use some of the same syntax in there for error messages -- like changing "channel not existant" to:

char buf[BUFSIZ];
snprintf(buf, sizeof(buf), "Channel does not exist: %s", name);
astman_send_error(s, m, buf);

By: Matt Florell (mflorell) 2005-05-17 15:49:06

As I get time to tweek this later this week I will change A/B to 1/2 and put in a better channel validator, although whether the first channel deadlocks does actually depend on what kind of channel it is. I am going to have to play around with that and see what works for all kinds of channels.

The reason I put it in res_features is that is where several related functions reside and since this is primarily a function to be used by other functions and it mostly involves the kind of stuff that is already in res_features(at least from a CVS-HEAD point of view) and because it doesn't really matter where you put manager functions in the asterisk code(they exist in several files throughout the Asterisk code) I stuck it in res_features to keep it simple and contained.

Thanks for the code snippit. I hope to have some new-updated patches posted here by the end of the week.

By: Olle Johansson (oej) 2005-05-17 16:21:28

Disclaimer on file?

Please observe that we never add new functions to stable, so concentrate on patches for cvs head. Patches for new fucntions in stable in the bugtracker are only for people that want to extend their software beyond the releases, but it is and will be unsupported...

I am sure you know, so that was for the record :-) Looks good when we have it working.

By: Olle Johansson (oej) 2005-05-17 16:25:49

The LOG_NOTICE in most cases should be LOG_DEBUG, possibly on a level 2 debug.

By: Olle Johansson (oej) 2005-05-17 16:28:58

* The formatting is wrong, you suddenly indent two tabs...
* You have first

if (!chan)
 [failure return]

--then later
if (chan)

To me the "if (chan)" is not necessary, since we fail if chan does not exist...

By: Olle Johansson (oej) 2005-05-17 16:30:17

In the .h file: "Deisgned" - should be "Designed". Please add full doxygen doc, including parameters. (See other .h files for inspiration!)

By: Matt Florell (mflorell) 2005-05-17 16:35:13

I think I faxed my disclaimer about a year ago, but just to be sure I faxed it again yesterday.

As for the Stable patches, I needed to have this feature for one of our stable servers and that is why I posted it. I will work on the CVS-HEAD version only going forward in this bug tracker.

I left the LOG in NOTICE because I wanted to test on a rather crowded server and the yellow NOTICE shows up nicely in the CLI. I will switch to LOG_DEBUG with a debug level of 2 check when I post the revision at the end of the week. That revision will also hopefully have the few other issues resolved in them.

Anything else look strange or improper? I really don't write/alter asterisk core code that much.

By: Matt Florell (mflorell) 2005-05-18 12:38:32

OK, I think I've taken care of all of the concerns listed and prettied the code up a bit. I've uploaded res_features_CVS.patch2 and features_h_CVS.patch2 with the new changes. Let me know how it looks.

By: Clod Patry (junky) 2005-05-18 12:50:47

i've just tested that patch.
this is my output after the bridge:

 == Manager 'junky' logged on from 127.0.0.1
May 18 13:47:33 NOTICE[31141]: res_features.c:253 action_bridge: Bridge: ChannelA SIP/154-cb1d is live and ready to Masq
May 18 13:47:33 NOTICE[31141]: res_features.c:277 action_bridge: Bridge ChannelA BridgeGoto/SIP/154-cb1d<ZOMBIE> Masq Successful to SIP/154-cb1d
May 18 13:47:33 NOTICE[31141]: res_features.c:287 action_bridge: Bridge: ChannelB SIP/159-6b1c is live and ready to Masq
May 18 13:47:34 NOTICE[31141]: res_features.c:311 action_bridge: Bridge ChannelB BridgeGoto/SIP/159-6b1c<ZOMBIE> Masq Successful to SIP/159-6b1c
May 18 13:47:34 NOTICE[31141]: res_features.c:322 action_bridge: ChannelA SIP/154-cb1d and ChannelB SIP/159-6b1c are compatible
   -- Playing 'beep' (language 'en')
 == Manager 'junky' logged off from 127.0.0.1
May 18 13:47:34 WARNING[31091]: app_meetme.c:1226 conf_run:     -- Stopped music on hold on BridgeGoto/SIP/154-cb1d<ZOMBIE>
Unable to write frame to channel: Resource temporarily unavailable
May 18 13:47:34 WARNING[31091]: app_meetme.c:1226 conf_run: Unable to write frame to channel: Resource temporarily unavailable
May 18 13:47:34 WARNING[31091]: app_meetme.c:1226 conf_run: Unable to write frame to channel: Resource temporarily unavailable
May 18 13:47:34 WARNING[31091]: app_meetme.c:1226 conf_run: Unable to write frame to channel: Resource temporarily unavailable
May 18 13:47:34 WARNING[31091]: app_meetme.c:1226 conf_run: Unable to write frame to channel: Resource temporarily unavailable
   -- Stopped music on hold on BridgeGoto/SIP/159-6b1c<ZOMBIE>
 == Spawn extension (test_sip, 100, 1) exited non-zero on 'BridgeGoto/SIP/154-cb1d<ZOMBIE>'
   -- Hungup 'Zap/pseudo-1324621234'
 == Spawn extension (test_sip, 2000, 1) exited non-zero on 'BridgeGoto/SIP/159-6b1c<ZOMBIE>'
asterisk*CLI> show ch
channel       channels      channeltypes
asterisk*CLI> show channels
       Channel  (Context    Extension    Pri )   State Appl.         Data
  SIP/159-6b1c  (default    s            1   )      Up Bridged Call  SIP/154-cb1d
  SIP/154-cb1d  (default    s            1   )      Up Bridged Call  SIP/159-6b1c
2 active channel(s)
0 active call(s)
asterisk*CLI>



but my 154 and my 159 has been hanguped, so i think it still miss something in here.

and if you could get the 2 files related to 1 file, that would be appreciated.



By: Matt Florell (mflorell) 2005-05-18 16:22:28

junky, We don't use moh(musiconhold) on any of our servers so we haven't tested it with that at all. We have tested Bridging calls from SIP, IAX2 and Zap channels and from calls that are in meetme rooms and it does seem to work in all cases. Could you try to patch2 version and see if it fares any better for you?

Also, I'm not sure what you mean by
  "and if you could get the 2 files related to 1 file, that would be appreciated."

By: Clod Patry (junky) 2005-05-18 17:39:04

ive tried patch2, same thing.
Any idea why the <ZOMBIE> appeared exactly?

i mean concat features_h_CVS.patch2 and  res_features_CVS.patch2 together. In that way i can download just one file and apply just one file instead of 2.

By: Clod Patry (junky) 2005-05-30 19:11:41

Any development here??

By: Matt Florell (mflorell) 2005-05-31 10:28:48

Haven't had a chance to get moh running on our test Asterisk server. We don't use it on any of our servers because it is buggy, picky about input files, generates zombie processes and is a processor and memory hog. I was not suprised it choked on this though, I have never been a fan of the "mpg123 is the only native way to do music-on-hold in Asterisk" philosophy that the major developers hold to because of all of the problems that it creates and has created in the last two years especially when implementing new features.

I will attempt to get it working by the end of the week on our test server so I can figure out why it chokes on this.

By: Matt Florell (mflorell) 2005-06-02 13:22:13

OK, I got mpg123 working on the test box, added a stop_moh to each channel before bridging and did a lot of testing from within meetme(single participant with moh on) and IAX2 and Zap and SIP channels and it always works.

The res_features_CVS.patch3 patch has the header file changes as well as the res_features changes in a single patch and applies cleanly against CVS_HEAD.

By: Michael Jerris (mikej) 2005-06-02 13:52:20

Can I kill off all the other patches on this bug, I assume the stable patches are no longer good with the current changes?

By: Matt Florell (mflorell) 2005-06-02 14:02:57

Yes, please delete them. What is the reason for the original reported of a bug and poster of the files not being able to delete files?

By: Michael Jerris (mikej) 2005-06-02 14:10:36

There has been chatter about changing that... Files deleted, Thanks

By: Michael Jerris (mikej) 2005-06-02 19:58:51

Ok so we need a full code review on this and some additional testers if possible please.

By: Clod Patry (junky) 2005-06-23 21:55:31

why do you set res variable if you return 1 anyways? there's no goal to set that variable if you're not returning it.


And about all that code:
+ if (option_debug)
+ ast_log(LOG_DEBUG, "Bridge: Channel2 %s is live and ready to Masq\n", chan2->name);
+ /* Stop music on hold */
+ ast_moh_stop(chan2);
+ ast_mutex_lock(&chan2->lock);
+ /* In order to Bridge the channel to another live channel, we have to
+   make a new channel, masquerade, put it to sleep while we do the same
+   with the other channel, then we can Bridge them together */
+ tmpchan2 = ast_channel_alloc(0);
+ if (tmpchan2) {
+ snprintf(tmpchan2->name, sizeof(tmpchan2->name), "BridgeGoto/%s", chan2->name);
+ ast_setstate(tmpchan2, chan2->_state);
+ /* Make formats okay */
+ tmpchan2->readformat = chan2->readformat;
+ tmpchan2->writeformat = chan2->writeformat;
+ /* Masquerade into temp channel */
+ ast_channel_masquerade(tmpchan2, chan2);
+ /* Grab the locks and get going */
+ ast_mutex_lock(&tmpchan2->lock);
+ ast_do_masquerade(tmpchan2);
+ ast_mutex_unlock(&tmpchan2->lock);
+ /* Send on our stolen channel to sleep as we wait to Bridge it */
+ if (ast_safe_sleep(tmpchan2, sleepms)) {
+ ast_log(LOG_WARNING, "Unable to send %s to sleep\n", tmpchan2->name);
+ ast_hangup(tmpchan2);
+ res = -1;
+ return 1;
+ }
+ if (option_debug)
+ ast_log(LOG_DEBUG, "Bridge Channel2 %s Masq Successful to %s\n", chan2->name, tmpchan2->name);
+ masqb=1;
+ } else {
+ res = -1;
+ if (option_debug)
+ ast_log(LOG_DEBUG, "Bridge Channel2 %s Masq Failed to %s\n", chan2->name, tmpchan2->name);
+ return 1;
+ }
+ ast_mutex_unlock(&chan2->lock);

it's exactly the same code as in chan1, no? why not create a function instead of duplicate all the code?

By: Paulo Mendes da Silva (kanelbullar) 2005-06-28 06:28:41

Should this feature be available in CVS HEAD? I tried to use it, as it appears to be very useful, but it doesn't seem to be available as of today.

By: Clod Patry (junky) 2005-06-28 08:00:02

mflorell: what do you think about my last comment? Does it makes sense?

By: Matt Florell (mflorell) 2005-06-28 09:27:47

junky: Yes your comments did make sense. Keep in mind that I am not an expert C programmer. I was really just piecing this patch together from lots of other code in other parts of the Asterisk codebase. The reason that I set a res variable while always returning 1 was mostly for testing while I developing, also I noticed many other places in the codebase where this occurs.

As for creating another sub-function to eliminate the code duplication, maybe I will do that after a code manager actually takes a look at the code to tell me if I am going i the right direction. I don't want to fragment the code even more just to find out that Mark doesn't like the methodology and I have to rewrite it all again.

I wrote this patch because despite over a years worth of requests for this functionality, none of the developers thought it was a necessary feature. So I took about a week to research the previous unsuccessful patches and dove into the code around the manager and channel handling. This patch works for my needs on my production systems and doesn't cause a crash. But I have not tested fully any impacts to CDR records or other applications which might be affected by this code.

It would be nice to have this feature integrated into CVS, but I've followed four other efforts to get code into CVS over the last 2 years, and only one made it in, and that took 6 months so I'm not holding my breath.

By: Clod Patry (junky) 2005-06-28 11:40:38

The first step would to eliminate the duplicate code, no? Merging the duplicate code in a function would be much more faster to change. In example, like you said, if Mark doesn't like it, you'll have to change it twice, while, with function, you'll have to change it just once.

By: Clod Patry (junky) 2005-07-05 22:39:43

Any development here mflorell?

By: Matt Florell (mflorell) 2005-07-06 08:51:39

Nope, just waiting for a bug marshall to notice this posting. Haven't had time to do code optimization, but that shouldn't stop someone from looking at the patch. I should have some time next week to look into the optimizations suggested.

By: Paulo Mendes da Silva (kanelbullar) 2005-07-06 10:08:28

I have tested the patch with several SIP clients (X-Lite, Grandstream Budgetone and Windows Messenger) and it appeared to work fine. The expected channels were dropped and the expected channels were bridged together.

By: Michael Jerris (mikej) 2005-07-13 14:44:41

Ok, this has been sitting for a week or so now.. it is tested, but we are waiting on the original poster to do updates to the code as requested.  Not sure what you are waiting for a bug marshall to notice this for?  Is there somthing that we need to do prior to you doing code cleanup?

By: Matt Florell (mflorell) 2005-07-13 17:01:04

I had some time yesterday to work on this but couldn't get fresh copy of CVS_HEAD to stop spewing WARNINGs when a manager connection was made so that just about wasted my afternoon. Now I'm just trying to finish up several other things before I go on vacation next week.

Other than removing the code duplication and the unnecessary res declarations I assume nothing else needs to be done, but because I'm not really a C programmer, that will take me a few hours to do and test before I get it right. If I can't get to it tomorrow, I should have some time the last week in July to work on it.

What I really wanted was for someone(with authority to put code into CVS) to OK the method of how this patch works. I've had one other patch that I personally wrote and two others that I helped with that would compile and run just fine but weren't ever accepted into CVS for methodology reasons.

MikeJ: are you one of the people with authority that can approve the methods of how this patch works?

By: Clod Patry (junky) 2005-07-13 19:43:37

mflorell: noone will commit that stuff until we're sure it works fine. That's why we have a dev branch.
I've tried that patch some weeks ago, and i've got no success.
I've give a suggestion about merging 2 identical part in one function and you didn't make it.
Does that work with MOH now?

By: Matt Florell (mflorell) 2005-07-13 23:33:01

junky: It works fine with channels that are in MOH. I tested it with Zap/IAX/SIP channels using MOH and MOH in a meetme room and it worked in all situations that I have tested. That was the patch 3 that I posted 6 weeks ago. You have posted since then, did you test it? does it not work for you? If it does not work please let me know how exactly you are using MOH and which CVS version/date you are using.

As for code optimizations, I just haven't had a block of time to do this, I hope to have a few hours to work on this tomorrow, but the functionality of the code will not be afffected at all by optimizing the code, so if you are having problems with the patche please let me know ASAP so I can work on that while I'm in the code tomorrow.

By: Matt Florell (mflorell) 2005-07-14 12:36:05

res_features_CVS.patch4 - has code optimizations:
- the MASQing portion in separate function
- variables that are not actively used were deleted.

Tested with CVS_HEAD 2005-07-14 16:54:53 UTC

Tested with MoH(MusicOnHold) from both a parked call and within a meetme room. Also, tested with multiple SIP/IAX/Zap channel conversation combinations.

Just to clarify, here is a sample of the correct Manager Action that you need to send to use this feature:

Action: Bridge
Channel1: Zap/1-1
Channel2: SIP/cc100-cd91



By: Matt Florell (mflorell) 2005-07-25 16:15:31

patch4 works with CVS_HEAD 2005-07-25. Waiting for review

Could someone delete the old res_features_CVS.patch3 file please?

By: Matt Florell (mflorell) 2005-08-23 12:10:56

res_features_CVS.patch5 works with CVS-HEAD 2005-08-23 16:48:05 UTC

changed to work with new res_features changes.

Tested 2005-08-23 with SIP/Zap/IAX2 channels and meetme/moh

someone please delete patch4

By: twisted (twisted) 2005-09-07 11:29:18

Haha.. I just wrote my own version of this a few days ago.  Guess i'll learn eventually to check the bugtracker first.  The only difference between your patch and mine is that I don't spawn the bridge thread, I actually call ast_bridge_call() outright.  I'm wondering if the thread will solve my issues with audio delays caused by other features playing sounds into the channel(s).

After figuring out all of the stuff you have on my own, I must say, good work, this is definately a tough one to make happen correctly :)

By: Matt Florell (mflorell) 2005-09-07 11:41:31

twisted- thanks for the compliment, this one kicked my ass for about a month ,off and on, until I finally figured it out. The whole process definitely seems much easier than it ends up being. With the patch there is a slight delay at the time of Bridging(), but we have tested it across multiple channel types, meetme conferences and MoH and it works every time.

We've been using this(as a Manager API Action) in production for a few months now with no problems. I just keep updating the patch to keep up with CVS_HEAD and hope that someone will notice it and eventually get it into HEAD.

By: twisted (twisted) 2005-09-08 17:00:55

I've found that putting the channels to sleep is not necessary - it only creates a confusing delay to the user.  I'll be posting my diff against res_features.c in a little bit for your inspection.

By: Michael Jerris (mikej) 2005-09-15 04:16:53

twisted-awaiting your diff.

By: twisted (twisted) 2005-09-15 13:43:00

no you're not. I have a strong feeling that no matter how good the diff is, this won't ever make it in, seeing how long this one has just kinda mulled around, and it's working.  

besides, i have a LOT of cleanup to do before I can post a valid diff.

By: Clod Patry (junky) 2005-09-17 13:14:13

Can we consider changrab a solution for this current bug?
I've played a lot with changrab and it works fine for me.

By: Matt Florell (mflorell) 2005-09-18 06:55:58

The only changrab app I've been able to find is on pbxfreeware.org not on the bugtracker meaning it will probably never be incorporated into Asterisk.

Also, changrab does not do the same thing that this patch does(grabbing two channels that are already in other connections and bridging them together). From what I've seen in the changrab code, the channel that calls the changrab function has to be free and not in another connection.

Another thing missing in changrab that is in this bridge patch is a manager API function.

So, no changrab does not duplicate the functionality of the bridge patch and is not a solution for this bug.



By: Clod Patry (junky) 2005-09-18 09:17:47

sure you can "grabbing two channels that are already in other connections and bridging them together". And yes you can bridge it via the manager, like you can bridge via the CLI, you can call the cli command via the manager, no?

By: Matt Florell (mflorell) 2005-09-18 09:29:39

junky: could you please post an example using changrab and Manager API commands of how you would bridge channels SIP/101-ab2a and SIP/201-ba3b together if they are in these conversations:

SIP/101-ab2a <=> SIP/103-8d4e
SIP/201-ba3b <=> Zap/2-1


I'm assuming this is the changrab code you are referring to:
http://www.pbxfreeware.org/app_changrab.c

By: Clod Patry (junky) 2005-09-18 13:45:16

sure,
polux*CLI> show channels
Channel              Location             State   Application(Data)
SIP/12-5a31          (None)               Up      Bridged Call(SIP/100-ceb7)
SIP/100-ceb7         12@polycom:1         Up      Dial(SIP/12)
SIP/10-e0bd          (None)               Up      Bridged Call(SIP/100-107b)
SIP/100-107b         10@polycom:1         Up      Dial(SIP/10)
4 active channels
2 active calls

which makes my SIP/100-107b bridged to SIP/10-e0bd and SIP/100-ceb7 bridge to SIP/12-5a31.

then i do a:
changrab -bB SIP/100-107b SIP/12-5a31

polux*CLI> show channels
Channel              Location             State   Application(Data)
SIP/100-ceb7         s@default:1          Up      Redirected Call(SIP/10-e0bd)
SIP/10-e0bd          s@default:1          Up      Bridged Call(SIP/100-ceb7)
2 active channels
0 active calls
polux*CLI>

(the 0 calls will have to be fixed)

now if im talking with my extension 10, im talking with my extension (the one originally from the exten 12).

Just apply that logic on ur example.



By: Matt Florell (mflorell) 2005-09-19 10:37:06

Thanks for the example I'll try it this week when I get some time.

Since the app is on pbxfreeware I am assuming that it will never be rolled into Asterisk CVS_HEAD.

I will still keep my patch and this bug up to date until this feature is either incorporated into Asterisk or rejected by the code keepers.

By: gnudialer gnudialer (gnudialer) 2005-09-20 23:32:45

Please consider this my vote (do I get one? :) ) for adding this feature to CVS HEAD.

It has so many uses, and the patch works really really great.

I've made many calls on it, so I know it works under many different circumstances.  No Matt did not ask me to say this.  In fact, we're kinda sorta competitors in a way.

If you think about it, coupled with existing features, it gives developers of third party applications total control over who gets connected to who and when and where.  The way I see it, it will cut down on questions like "How can I do this? How can I do that?"

One more note: I don't know who was right about the sleeptime issue or whatever, but what I do know for sure is that reducing it to 50 works fine.

Thank you for your time, and yes I realized I used 9874565213245215445 too many adverbs and pronouns in this post.

By: twisted (twisted) 2005-09-30 15:51:22

res_features.c-manager-bridge.txt is my method of doing this.  I also included the ability to send the courtesy tone via option "Tone: (yes|no)" in the event.

By: twisted (twisted) 2005-09-30 15:52:59

please note that the comments aren't correct, but I haven't the time to correct those yet...  I put it out here for code/functionality review - i'll correct the comments later if this has any possible chance of going anywhere.

By: twisted (twisted) 2005-10-12 17:46:48

Fixed comments, updated to current. (action_bridge-update-10-12.txt)

By: Serge Vecher (serge-v) 2005-11-12 11:28:51.000-0600

Looks like this bug needs some attention: we have positive reviews here ...

By: Denis Voitenko (denis) 2005-11-17 19:08:23.000-0600

The latest patch fails on CVS-HEAD 11/15/05

patch -p0 < action-bridge.txt
patching file res_features.c
patch: **** malformed patch at line 27: ast_channel_alloc(0);

By: gnudialer gnudialer (gnudialer) 2005-11-22 16:33:08.000-0600

patch works for me on CVS HEAD 11/22/05 (when i checked it out)

By: xiribitata (xiribitata) 2005-11-23 11:16:09.000-0600

I've tested this feature with the latest patch (action_bridge-updated-10-12) and it seems that it isn't sending any kind of response to the manager api (neither positive or negative). It is only sending the Bridge event, but this is an event, it doesn't have the action id of the request previously made, so i can't map it with the request.

Shouldn't it be sending a positive or negative response to the request as well as the event?

By: Matt O'Gorman (mogorman) 2006-01-09 12:55:03.000-0600

all current work is being done on ASTERISK-5689