[Home]

Summary:ASTERISK-03875: [patch] H.323 driver improvements
Reporter:Chih-Wei Huang (cwhuang)Labels:
Date Opened:2005-04-06 03:31:54Date Closed:2011-06-07 14:10:44
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_h323
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) ast-h323-dead3a.diff
( 1) h323.diff
( 2) openh323-my.diff
( 3) pwlib-my.diff
( 4) crash_debug
( 5) deadlock
Description:I spent some time to fix some problems in the H.323 driver, include

* Compiling problem. Now it works with PWLib 1.8.1 / OpenH323 1.15.1 and later.
* Incoming call problem, especially works with a gatekeeper.
* Outgoing call problem with a gatekeeper.
* RTP problem on a host with multiple network interfaces.
* Early media problem.

The patch is available at

http://www.citron.com.tw/~cwhuang/asterisk-h323.html
Comments:By: Paul Cadach (pcadach) 2005-04-06 03:43:53

Some questions about the patch:
1) Is it disclaimed?
2) Is next is not do same functionality by Asterisk's RTP stack and OpenH323 stack too:
@@ -566,6 +566,7 @@
ast_mutex_lock(&p->lock);
if (p->rtp && (p->dtmfmode & H323_DTMF_RFC2833)) {
ast_rtp_senddigit(p->rtp, digit);
+ h323_send_tone(p->cd.call_token, digit);
3) You shouldn't force to build h323/libchanh323.a because it's optional (until you built h323/libchanh323.a you will not have chan_h323 to build too).
By: jerjer (jerjer) 2005-04-06 13:48:25

why are you modifing channels.c ?
By: Paul Cadach (pcadach) 2005-04-06 13:53:23

Just because this is a bug. Why to lookup best format then pass capabilities mask to requester()? Internal channel's requester() function knows about channel's capabilities, and want to know preferred format for connection being created. IMHO it shouldn't interfere with existing code.
By: Mark Spencer (markster) 2005-04-06 13:57:02

The channel.c portion is not correct.  We *should* pass the capability to the requestor, not what it's going to have to translate to.  If H.323 wants to support any format it should register "-1" as its capability.
By: Paul Cadach (pcadach) 2005-04-06 14:03:36

So, why we deduce preferred format if it is not used anywhere? Mark, could you reach me at IRC? I have some questions...
By: Paul Cadach (pcadach) 2005-04-06 14:18:43

Ok, looks like oh323_request needs to have its own call to ast_translator_best_choice() to save functionality at channel.c
By: nick (nick) 2005-04-06 21:25:18

Not major.
By: Chih-Wei Huang (cwhuang) 2005-04-06 21:27:52

To PCadach:
1) Yes.
2) This is a small change that I forgot to mention. I found DTMF sent from SIP channal can't be passed to H.323 channel. This is just a workaround. I think the DTMF handling of H.323 driver is not good enough. But if we want to handle more DTMF type that Openh323 stack supports, there are more works to do. Currently, you can ignore this workaround if you think it's unimportant.
3) I didn't force build libchanh323.a. Did I? Current Makefile touch a file everytime I make, that's stupid. I just create a depencency between chan_h323.c and stuff in h323 directory so that if files in h323 subdirectory change, chan_h323.so will be re-linked. (This is a standard way used by Makefile of Asterisk for each subdirectory. You may verify that.) Besides, I am curious why there is no configure script or similar in Asterisk. That's an easy way for a user to decide which portion to be compiled. If the answer is nobody had contributed one, I would like to try.

edited on: 04-06-05 22:24
By: Chih-Wei Huang (cwhuang) 2005-04-06 21:29:32

To JerJer and markster:
The reason I changed channel.c is already explained in Asterisk-dev list. See
http://lists.digium.com/pipermail/asterisk-dev/2005-March/010641.html
In short, I agree with PCadach's point of view.

edited on: 04-06-05 22:00
By: Chih-Wei Huang (cwhuang) 2005-04-06 21:41:29

To nick:
What is a major bug? As explained in the guideline:
"A bug which completely prevents Asterisk from operating in a method that it normally is expected to operate"
The CVS-HEAD is broken. It cannot make call, it cannot receive call, nor it can receive RTP stream. Worsely, even it cannot be compiled (if you don't use the specify version). Don't you think these are MAJOR problems?
Anyway, no matter they are major or minor, I eagerly hope they will be fixed soon.
By: Paul Cadach (pcadach) 2005-04-06 23:47:19

About ast_translator_best_choice(). If you look for its source at translator.c, capabilities is updated too to real set of capabilities which Asterisk (and its codec translator) currently supports. Driver just prepares list of capabilities which allowed by Asterisk's manager, and ast_request updates it to _real_ capability list could be handled in current environment.

DTMF sending should be studied a bit more because OpenH323 have 4 methods to send user indications while chan_h323 specifies 2 only...

In Makefile you have:
+h323/libchanh323.a: FORCE
+ @[ -d h323 ] && $(MAKE) -C h323 libchanh323.a
+
which is not dependency but forces to build h323/libchanh323.a if it is not available.

You correctly noticed about touching chan_h323.c every time libchanh323.a is updated but dependancy for it is already available:
chan_h323.so: chan_h323.o h323/libchanh323.a
$(CC) $(SOLINK) -o $@ $< h323/libchanh323.a $(CHANH323LIB) -L$(PWLIBDIR)/lib $(PTLIB) -L$(OPENH323DIR)/lib $(H323LIB) -L/usr/lib -lcrypto -lssl -lexpat

To fix many build problems together with OpenH323 my opinion is to have additional makefile (or replacing for existing one) in h323/ which is OpenH323-compliant and on make just generates additional text file ("sub-Makefile") with current comiling and linking options. This should fix many problems related to different options used for compiling ast_h323.cxx and OpenH323. For example, I uses optnoshared option to build OpenH323 so needs to update makefiles to link static libraries rather than dynamic, and when I disables builing of OpenH323 together with SASL, Expat, etc. I needs to update makefiles too. Why to not do that automagically, by Makefile which uses OpenH323 techniques to build libchanh323.a and exports required variables into additional file which would included by channels/Makefile to have build/linking options for chan_h323.so? I had modeled it some time ago but not so pretty as I want.
By: Chih-Wei Huang (cwhuang) 2005-04-07 02:00:21

To PCadach:
For the Makefile problem, your opinion is not correct. My changes just mean if the target h323/libchanh323.a had to be built, do
make -C h323 libchanh323.a.
This target will be built *only if* the target chan_h323.so will be built, while the later will be built *only if* the file h323/libchanh323.a *exist*. (since the line
CHANNEL_LIBS+=$(shell [ -f h323/libchanh323.a ] && echo chan_h323.so)
In conclusion, if people don't build h323/libchanh323.a first,
chan_h323.so and stuff in h323 directory will not be built at all.
It won't force to build h323/libchanh323.a. It is just a dependency. You may verify that by hand.
I have to say this technique is not invented by me. There are several examples of this kind of dependencies in the Makefile in the root directory of Asterisk. I just copied and modified from that. You can take a look.

BTW, I agree with you that the Makefile in h323 subdirectory should be re-worked to be Openh323-compliant. But I still think we  need an auto configure script so that a user can specify --enable-h323, --enable-sip and so on.

edited on: 04-07-05 02:02
By: Chih-Wei Huang (cwhuang) 2005-04-07 02:21:37

About ast_translator_best_choice(). Yes, I see what you said.
But how about the original problem? That is, how to know the preferred format on connection being created in a channel's requester(), if we don't pass the preferred format from ast_request()? Furthur, how to solve the early media problem I mentioned before? See
http://lists.digium.com/pipermail/asterisk-dev/2005-March/010597.html
and
http://lists.digium.com/pipermail/asterisk-dev/2005-March/010636.html
By: jerjer (jerjer) 2005-04-07 12:29:53

cwhuang, are you volunteering to always be around to manage autoconf for the next 100 years?
By: Paul Cadach (pcadach) 2005-04-07 14:10:06

cwhuang: Check ASTERISK-3889- it have patches for OpenH323-compatible build process as we discussed.
By: wgfreewill (wgfreewill) 2005-04-15 01:38:35

Hey guys, news from the test lab.

Current cvs wont connect calls to Nortel Succession carrier softswitch, Cisco Call Manager fails as well. I am going to test AS5850 tomorrow.

To get calls going now i use -D"2005/04/01 1:00" asterisk with PCadach h323-dead-cvs2.diff

Also DTMF and Ulaw negotiation is broken with Nortel in all versions.
By: Paul Cadach (pcadach) 2005-04-26 13:18:33

Attached patch is just for review. Some description of changes was made:
1) chan_h323's private information returned by find_call() currently pre-locked (and function name changed to find_call_locked());
2) codec manipulation is moved from per-endpoint to per-connection (should eliminate possible race conditions when codec set is changed within call negotiation);
3) patch by cwhuang is applied (partially, mostly for correct IP address recognition);
4) slight re-work of call initialization/shutdown to minimize a time when global OpenH323 locks is held;
TODO:
1) verify work with gatekeeper (current version isn't tested to work with GK);
2) make codec priorities works;
3) provide correct unload/reload with re-registration on GK.

edited on: 04-27-05 13:57
By: Paul Cadach (pcadach) 2005-04-27 14:15:27

Attached ast-h323-dead3.diff fixes most core/deadlock issues and tested under moderate load (about 10 simultaneous calls) together with PWLib 1.9.0 and OpenH323 1.17.1 (lastest versions recommended by JerJer) built for static linking (make optnoshared), which helps a much for debugging but brings other side-effects (see below). It isn't tested to work with Gatekeeper!!! (but probably could).

Patches for OpenH323/PWLib (probably) is required to make work much stable:
1) openh323 - don't hold global connection lock when new connection is creating (probably it could make some side effects), stop H.245 reply timers BEFORE negotiator is being locked (otherwise it could and raises deadlocks on timeouts), and unregister dynamically-registered GSM-library when destructor is called (otherwise it causes cores on Asterisk shutdown).
2) pwlib - make valgrind happy (declaration local variable then write() it unitialized, missed assiging value to isDynamic member, assign value to PX_firstTimeStart member of PProcess).

A side-effect found to make shared library with PWLib/OpenH323 compiled for static linking (i.e. without -fPIC flag). PWLib/OpenH323 have static instances which initializes on first usage and to cleanup registers destructors for call by atexit(), so you can't really unload module until totally shutdown Asterisk (when atexit() parts is called). To avoid problems caused by sequential calls to load_module()/unload_module() without re-initializing data segment (because module isn't really unloaded) there is slightly more cleanups to be sure all internal structures is get to initial state at unload_module().
By: Paul Cadach (pcadach) 2005-04-29 09:10:51

ast-h323-dead3a.diff is ast-h323-dead.diff for the current CVS-HEAD.
By: Paul Cadach (pcadach) 2005-05-03 07:27:08

Just want to notify about note from OpenH323 maintainer:
------------- cut here -------------
Paul,

  Thanks for these patches. After reviewing them I have applied them to
the latest CVS.

...

   Craig
------------- cut here -------------
By: Pash (opa__) 2005-05-21 00:48:57

JerJer: Is this patch out of mainstream?
By: Paul Cadach (pcadach) 2005-05-21 00:52:07

Could you note which patch do you mean?
By: Pash (opa__) 2005-05-21 00:53:47

PCadach: with this patch i has problems with g729 w VAD on Planet gateways:

May 21 12:32:54 VERBOSE[17576] logger.c:   2:18.240                H245:81c72e0      h323pdu.cxx(541)   H245    Sending PDU [ip$192.168.111.107
:37771/ip$192.168.111.109:1039] :
May 21 12:32:54 VERBOSE[17576] logger.c:   request openLogicalChannel {
May 21 12:32:54 VERBOSE[17576] logger.c:     forwardLogicalChannelNumber = 101
May 21 12:32:54 VERBOSE[17576] logger.c:     forwardLogicalChannelParameters = {
May 21 12:32:54 VERBOSE[17576] logger.c:       dataType = audioData g729 2
May 21 12:32:54 VERBOSE[17576] logger.c:       multiplexParameters = h2250LogicalChannelParameters {
May 21 12:32:54 VERBOSE[17576] logger.c:         sessionID = 1
May 21 12:32:54 VERBOSE[17576] logger.c:         mediaGuaranteedDelivery = FALSE
May 21 12:32:54 VERBOSE[17576] logger.c:         mediaControlChannel = unicastAddress iPAddress {
May 21 12:32:54 VERBOSE[17576] logger.c:           network =  4 octets {
May 21 12:32:54 VERBOSE[17576] logger.c:             c0 a8 6f 6b                                        ..ok
May 21 12:32:54 VERBOSE[17576] logger.c:           }
May 21 12:32:54 VERBOSE[17576] logger.c:           tsapIdentifier = 11197
May 21 12:32:54 VERBOSE[17576] logger.c:         }
May 21 12:32:54 VERBOSE[17576] logger.c:         silenceSuppression = FALSE
May 21 12:32:54 VERBOSE[17576] logger.c:       }
May 21 12:32:54 VERBOSE[17576] logger.c:     }
May 21 12:32:54 VERBOSE[17576] logger.c:   }

May 21 12:32:54 VERBOSE[17576] logger.c:   2:18.553                H245:81c72e0      h323pdu.cxx(541)   H245    Receiving PDU [ip$192.168.111.1
07:37771/ip$192.168.111.109:1039] :
May 21 12:32:54 VERBOSE[17576] logger.c:   request openLogicalChannel {
May 21 12:32:54 VERBOSE[17576] logger.c:     forwardLogicalChannelNumber = 61
May 21 12:32:54 VERBOSE[17576] logger.c:     forwardLogicalChannelParameters = {
May 21 12:32:54 VERBOSE[17576] logger.c:       dataType = audioData g729 2
May 21 12:32:54 VERBOSE[17576] logger.c:       multiplexParameters = h2250LogicalChannelParameters {
May 21 12:32:54 VERBOSE[17576] logger.c:         sessionID = 1
May 21 12:32:54 VERBOSE[17576] logger.c:         mediaControlChannel = unicastAddress iPAddress {
May 21 12:32:54 VERBOSE[17576] logger.c:           network =  4 octets {
May 21 12:32:54 VERBOSE[17576] logger.c:             c0 a8 6f 6d                                        ..om
May 21 12:32:54 VERBOSE[17576] logger.c:           }
May 21 12:32:54 VERBOSE[17576] logger.c:           tsapIdentifier = 30011
May 21 12:32:54 VERBOSE[17576] logger.c:         }
May 21 12:32:54 VERBOSE[17576] logger.c:       }
May 21 12:32:54 VERBOSE[17576] logger.c:     }
May 21 12:32:54 VERBOSE[17576] logger.c:   }

May 21 12:32:54 VERBOSE[17576] logger.c:   2:18.558                H245:81c72e0      h323pdu.cxx(541)   H245    Sending PDU [ip$192.168.111.107
:37771/ip$192.168.111.109:1039] :
May 21 12:32:54 VERBOSE[17576] logger.c:   response openLogicalChannelAck {
May 21 12:32:54 VERBOSE[17576] logger.c:     forwardLogicalChannelNumber = 61
May 21 12:32:54 VERBOSE[17576] logger.c:     forwardMultiplexAckParameters = h2250LogicalChannelAckParameters {
May 21 12:32:54 VERBOSE[17576] logger.c:       sessionID = 1
May 21 12:32:54 VERBOSE[17576] logger.c:       mediaChannel = unicastAddress iPAddress {
May 21 12:32:54 VERBOSE[17576] logger.c:         network =  4 octets {
May 21 12:32:54 VERBOSE[17576] logger.c:           c0 a8 6f 6b                                        ..ok
May 21 12:32:54 VERBOSE[17576] logger.c:         }
May 21 12:32:54 VERBOSE[17576] logger.c:         tsapIdentifier = 11196
May 21 12:32:54 VERBOSE[17576] logger.c:       }
May 21 12:32:54 VERBOSE[17576] logger.c:       mediaControlChannel = unicastAddress iPAddress {
May 21 12:32:54 VERBOSE[17576] logger.c:         network =  4 octets {
May 21 12:32:54 VERBOSE[17576] logger.c:           c0 a8 6f 6b                                        ..ok
May 21 12:32:54 VERBOSE[17576] logger.c:         }
May 21 12:32:54 VERBOSE[17576] logger.c:         tsapIdentifier = 11197
May 21 12:32:54 VERBOSE[17576] logger.c:       }
May 21 12:32:54 VERBOSE[17576] logger.c:       flowControlToZero = FALSE
May 21 12:32:54 VERBOSE[17576] logger.c:     }
May 21 12:32:54 VERBOSE[17576] logger.c:   }

May 21 12:32:55 VERBOSE[17576] logger.c:   2:18.624                H245:81c72e0      h323pdu.cxx(541)   H245    Receiving PDU [ip$192.168.111.1
07:37771/ip$192.168.111.109:1039] :
May 21 12:32:55 VERBOSE[17576] logger.c:   response openLogicalChannelAck {
May 21 12:32:55 VERBOSE[17576] logger.c:     forwardLogicalChannelNumber = 101
May 21 12:32:55 VERBOSE[17576] logger.c:     forwardMultiplexAckParameters = h2250LogicalChannelAckParameters {
May 21 12:32:55 VERBOSE[17576] logger.c:       sessionID = 1
May 21 12:32:55 VERBOSE[17576] logger.c:       mediaChannel = unicastAddress iPAddress {
May 21 12:32:55 VERBOSE[17576] logger.c:         network =  4 octets {
May 21 12:32:55 VERBOSE[17576] logger.c:           c0 a8 6f 6d                                        ..om
May 21 12:32:55 VERBOSE[17576] logger.c:         }
May 21 12:32:55 VERBOSE[17576] logger.c:         tsapIdentifier = 30010
May 21 12:32:55 VERBOSE[17576] logger.c:       }
May 21 12:32:55 VERBOSE[17576] logger.c:       mediaControlChannel = unicastAddress iPAddress {
May 21 12:32:55 VERBOSE[17576] logger.c:         network =  4 octets {
May 21 12:32:55 VERBOSE[17576] logger.c:           c0 a8 6f 6d                                        ..om
May 21 12:32:55 VERBOSE[17576] logger.c:         }
May 21 12:32:55 VERBOSE[17576] logger.c:         tsapIdentifier = 30011
May 21 12:32:55 VERBOSE[17576] logger.c:       }
May 21 12:32:55 VERBOSE[17576] logger.c:       flowControlToZero = TRUE
May 21 12:32:55 VERBOSE[17576] logger.c:     }
May 21 12:32:55 VERBOSE[17576] logger.c:   }
May 21 12:32:55 VERBOSE[17576] logger.c:   2:18.626                H245:81c72e0      h323neg.cxx(897)   H245    Received open channel ack: T-10
1, state=AwaitingEstablishment


so my no-vad request was ignored. and i will have problems with no JB,with no PLC.

Is this right?
By: Paul Cadach (pcadach) 2005-05-21 00:58:16

Try to configure your gateway to not use VAD. Asterisk is still non-VAD/TDX aware.
By: Pash (opa__) 2005-05-21 00:59:56

ast-h323-dead3a.diff
By: Paul Cadach (pcadach) 2005-05-21 01:02:04

BTW, most parts of dead3a already applied to the CVS-HEAD.
By: Pash (opa__) 2005-05-26 02:08:45

A real ussue found:
with dead3a I can't call Dial(H323/0027@1.2.3.4)
it want for Dial(H323/0027@peer_name) and declaration of this peer in h323.conf

=======

It's i was stupid: set default port 1721 and wonders why it cant connect to 1720....
:)
sorry


By: wgfreewill (wgfreewill) 2005-05-31 09:52:59

seg faults on dial,

lots of cisco and nortel devices tested.

see crash_debug
By: wgfreewill (wgfreewill) 2005-05-31 10:01:07

Just so we're all on the same page. I have also tried the h323.diff on the openh323 code and some make optnoshared.

Debian/Sarge stock packages, custom kernel

Linux xenon 2.6.8 #1 SMP Mon Apr 11 12:34:57 EDT 2005 i686 GNU/Linux

PWLIBDIR=/usr/src/pwlib
export PWLIBDIR
OPENH323DIR=/usr/src/openh323
export OPENH323DIR
LD_LIBRARY_PATH=$PWLIBDIR/lib:$OPENH323DIR/lib
export LD_LIBRARY_PATH


pwlib-v1_9_0-src-tar.gz
openh323-v1_17_0-src-tar.gz

/usr/src/pwlib
make clean opt
/usr/src/openh323
make clean opt

rm -r -f asterisk

cvs co asterisk

/usr/src/asterisk/channels/h323
make all
/usr/src/asterisk
make all
make install
By: Paul Cadach (pcadach) 2005-05-31 10:36:36

As pointed in channels/h323/README, you should use OpenH323 1.17.1 instead of 1.17.0.
By: Paul Cadach (pcadach) 2005-05-31 10:39:33

wgfreewill: Could you open new ticket for crashes on dial? And, attach debug/verbose output with 'h.323 debug', 'h.323 trace 5' options.
By: Pash (opa__) 2005-06-10 04:08:14

check ASTERISK-4389. here are same.


By: Pash (opa__) 2005-06-17 01:59:37

a deadlock occurs casualy. ( openh323-my.diff pwlib-my.diff   ast-h323-dead3a.diff are used on CVS 2th May)

it's look like problems of lock order:

ast_write
locks chan->lock (from channel.c) then pvt->lock

connection made call pvt->lock (from find_call_locked()) then pvt->owner->lock == chan->lock

classical deadlock.

we need something like grab_owner() from chan_alsa.c. IMHO

see ASTERISK-4440 too


By: Michael Jerris (mikej) 2005-07-12 17:07:22

It appears that everyone has lost interest in this.  Is anyone following up onthe remaining issues in this bug?
By: Pash (opa__) 2005-07-12 21:40:28

i use -D 06-02-05 HEAD with dead3a+ASTERISK-4389 because it is stable enough.
but current head is more unstable.

and i want for dead3b


By: Michael Jerris (mikej) 2005-07-20 22:42:40

pcadach-  what is the status of this?  is this patch dead or what is necessary to move it forward?
By: twisted (twisted) 2005-07-26 21:27:45

Unknown if disclaimer is on file --twisted
By: Michael Jerris (mikej) 2005-07-28 17:32:14

Ok, all other open h.323 bugs have been committed, we need response and testing results on cvs-head for this to move anywhere.
By: Michael Jerris (mikej) 2005-07-31 21:01:17

suspending this bug due to lack of response.  If somone can get this updated to current head and respond with status, feel free to  reopen.  Also, we need verification of the disclaimer on this before it can be looked at.