Summary:	ASTERISK-02138: [patch] MOH pthread rather than fork
Reporter:	jacs (jacs)	Labels:
Date Opened:	2004-07-29 08:48:28	Date Closed:	2011-06-07 14:05:09
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) moh.tar.gz ( 1) mohulaw.diff.gz ( 2) mohulaw.diff.txt
Description:	Fork and threads don't go well together. I have integrated the mpg123 library, with some bug fixes, into a piped thread for MOH. ****** ADDITIONAL INFORMATION ****** need to apply "thread_stack_diff.txt" from  bug ASTERISK-2032067 for diffs to work. However, under Linux the stack code is not needed.
Comments:	By: Mark Spencer (markster) 2004-07-29 14:25:35 I'm *very* nervous about having mpg123 or similar as a thread since that means that potentially bad mp3 data could cause a segfault of Asterisk. What if we allowed a music on hold class which simply ready from a pipe or socket, then we could use something like icecast or what not. Mark By: Brian West (bkw918) 2004-07-30 00:45:36 how about we just convert all the muisc to ulaw files using sox.. and live happy with that ? By: cloos (cloos) 2004-07-30 02:22:45 Using the formats as provided in asterisk/formats/ is IM(!sH)O by far the best choice for MoH. Esp. if one could store copies in each format one expects to serve and ref the files w/o affix.  Just like for Playback(). In any case, getting rid of the fork(2) is The Right Thing To Do. (Again, IM(!sH)O.) But supporting casts, whether via named pipe, unix socket, (uni) inet socket, multicast socket (did I miss anything? :)  would also be useful for some, whether they have multiple * boxen (perhaps one per so many PRIs) or just want to tap into a broadcast already in place on their lan. By: jacs (jacs) 2004-07-30 07:42:09 The way I have implemented this is that there is the MOH thread and a decoder thread that pipes to the moh thread. The decoder thread could easily be changed to read from say icecast or just read raw files (as I did for testing). I've taken out all the exit statements in mpglib :-) and if there are errors it should just stop playing the mp3 file and loop round the file list again giving suitable warning messages. I don't think the current status where if mpg123 crashes cos of a bad mp file then mpg123 just gets forked again for the same thing to happen is very good. By: Mark Spencer (markster) 2004-08-01 15:21:36 Well but the mpg123 process is isolated from Asterisk such that nothing it does risks the stability of Asterisk.  That's what I"m taling about. By: Brian West (bkw918) 2004-08-11 23:22:07 Good idea but many of us don't think its ready.  Refine the idea and resubmit at a later date.  We are pushing for RC2. Thanks, Brian By: () 2004-08-12 04:22:57 I would prefer not to close this until a way forward is determined. Are we saying that any form of built in decoder is a bad idea? If so then fine. I'll leave it for history and for use in FreeBSD as the fork code does not work in FreeBSD. If a decoder is ok then what? Do we just accept raw, pcm, mp3, vorbis? Is mpg123lib a bad idea then would madlib be better? Accepting icecast streams leaves the same issues as it just forwards the encoded stream so we still need some form of decoder. Is the ulaw idea of brian of any interest its simple enough to do? Chris By: Brian West (bkw918) 2004-08-12 11:53:02 well if you convert everything to ulaw... then no decoder is needed... its just raw ulaw/pcm and you can pipe that down any channel without much CPU or trouble. By: jacs (jacs) 2004-08-12 12:09:52 Yes I know its straight forward. I'll generate a set of diffs when I get back from holiday. By: twisted (twisted) 2004-08-24 13:42:37 Hope you had a great holiday, any updates? By: twisted (twisted) 2004-08-25 18:08:03 Reminder sent to jacs Need an update or diffs as promised within the next 24 hours or so, or we'll have to close this back out again. By: () 2004-08-26 13:29:45 I've only just got back .. give me a chance:-) I have to work as well! By: () 2004-09-06 09:50:54 Here is an update that allows ulaw files to be played as a MOH forked process. It has been tested on FreeBSD and SUSE 9.0. By: twisted (twisted) 2004-10-27 15:45:34 Status request/update request for current cvs.   -Housekeeping By: jacs (jacs) 2004-10-29 03:33:21 If chanspy gets added then this is of no use anymore ... I believe. By: Brian West (bkw918) 2004-10-29 22:25:27 chanspy isn't going in it will need to be rewritten once the new translation stuff is done.  Also the MOH patch is up as a standalone patch. bkw By: () 2004-11-01 09:21:22.000-0600 I just realised that I have two lives on this bug system (jacs & chrisstenton)!  Could someone remove chrisstenton. Mohulaw.diff.gz still works on the current cvs head(1/11/04). I have uploaded it as a text file as well for easy reading. I have a signed disclaimer on record. Chris By: twisted (twisted) 2004-11-14 21:42:19.000-0600 Okay... I removed chrisstenton, as per your request.  Now, we need to go forth with this bug. By: twisted (twisted) 2004-11-14 21:43:08.000-0600 For futre reference, user 1035 is jacs (chrisstenton) By: Brian West (bkw918) 2004-11-14 22:15:10.000-0600 Can we move all this to bug 2639?  Its all related really. By: zoa (zoa) 2004-11-15 01:33:16.000-0600 i'd love to see a MOH option for multiple encodings of the files. eg .ul for ulaw, .gsm for gsm, .g729 etc Ok, we'd have to spawn multiple threads of the player, one for each codec we want to support, and we will have slightly more disk reads, but we would save still save bigtime on CPU when doing this. By: jacs (jacs) 2004-11-15 06:04:43.000-0600 Zoa, Look at bug 2639 for all file types. The only good/bad issue with the chanspy version is that it starts a new instance for each moh session. By: Anthony Minessale (anthm) 2004-11-15 10:45:49.000-0600 FYI "New Instance" is simply a small cluster of pointers 1 to the list of files and another to which file you are currently listening to the thread that plays the music is the same thread that the channel aready lives in if you choose. The files based moh is in essance just a playback stream it uses the existing stream api.