Summary:	ASTERISK-02594: [patch] Native MOH without mpg123
Reporter:	Anthony Minessale (anthm)	Labels:
Date Opened:	2004-10-13 14:30:42	Date Closed:	2008-01-15 15:18:10.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Resources/res_musiconhold
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) native_moh_take100.diff ( 1) native_moh_take100.rev10.diff ( 2) native_moh_take100.rev11.diff ( 3) native_moh_take100.rev2.diff ( 4) native_moh_take100.rev3.diff ( 5) native_moh_take100.rev4.diff ( 6) native_moh_take100.rev5.diff ( 7) native_moh_take100.rev6.diff ( 8) native_moh_take100.rev7.diff ( 9) native_moh_take100.rev8.diff (10) native_moh_take100.rev9.diff (11) native_moh.diff.txt (12) rawplayer.c
Description:	This is the moh_files portion of the chanspy/moh bug 2379 isolated to just patch the music on hold. To use: edit musiconhold.conf and add a new section [moh_files] you now add classes like the [classes] section except in this format: classname => <directory_path>[,r] e.g. default => /music,r (,r indicates optional randomization) will use all the files in /music and randomize them the directory can contain any file that asterisk can play with app_playback and can contain combinations of formats (although .raw or .pcm probably are most efficient) to use mp3's check out asterisk-addons from cvs and install format_mp3 cvs co asterisk-addons cd asterisk-addons/format_mp3 if your asterisk is running: make autoload if not: make install   ****** ADDITIONAL INFORMATION ****** paypal anthmct@yahoo.com Disclaimer on file
Comments:	By: brc007 (brc007) 2004-10-15 23:48:48 Works great for me! I've been using it for about 3 weeks. By: paradise (paradise) 2004-10-16 00:22:58 Mark: pleeeeeeeeease consider to add this great patch to the CVS. thanks By: Brian West (bkw918) 2004-10-18 11:37:02 It slices, it dices, it plays sound files... brought to by the guy that brought you chanspy, valetparking and much more... he brings you the yet another excellent patch i'm sure you'll love.  Lets hear it for anthm!!!!  :P *the crowd roars*   bkw By: paradise (paradise) 2004-10-19 16:03:44 so anthm: please consider to add this great patch to CVS HEAD and make it possible to all * users to get rid of mpg123 By: rwjblue (rwjblue) 2004-10-20 20:22:16 Awesome!!!!  Works much better than mpg123, and also supports native formats.  Great job.  Let's get this into CVS! By: Matt Gibson (flewid) 2004-10-26 13:27:38 Just a note here guys, This patch works friggin great, and I love it, but I have found a slight issue. When using this patch and CVS-HEAD (haven't tested with 1.0 or 1.2), and trying to get queues to jump out of the queue and go to voicemail if no queuemember answers  it will segfault when it gets to stepping out of the queue. I have tried many things including not using safe_asterisk, trying to make it 'step out' to hangup, or to SayUnixTime(), all provide the same results, segfault. Removing this patch however fixes the issues happening. I'm no c coder so I have no idea where to start fixing this, but I thought I should let someone know. Thanks again for the great patch, hopefully we(you guys) can fix it for the queing problem mentioned above. By: twisted (twisted) 2004-11-14 21:09:58.000-0600 status request... what's goin on with this? --Housekeeping By: Kevin P. Fleming (kpfleming) 2004-11-16 19:02:26.000-0600 I have just uploaded a reworked version of Brian's patch, with the following changes: - moh_files_state structure attached to a channel is now freed when the channel is freed - unusued variables in moh_files_state structure removed - computation and use of "elapsed time" skipped when class is in randomize mode, because it makes no sense (and causes problems) - when native moh was called more than once on a channel before, if the second call was for a different class this was ignored, and moh from the original class was played - file scanning now ignores non-regular files (including directories) - file scanning will now skip files whose total pathname length will not fit in the array (previously it would store only the portion that fit in the array, leading to broken paths) - when files are present in multiple formats (say, "foo.gsm", "foo.ul" and "foo.wav"), they would get put into the list multiple times; this no longer happens - properly use "params" argument provided by ast_activate_generator, instead of working around it - properly initialize entire moh_files_state structure to zeroes, instead of only first four bytes - code cleaned up, reformatted to match coding guidelines, and simplified wherever possible Note that I did not really touch the non-native-format parts of this patch, other than re-indentation. This version has been tested on my non-production server and appears to work without problems, but I would appreciate anyone using the previous version of this patch (especially if you _also_ use old-style mpg123 moh as well) to test this version and report back here. By: Matt Gibson (flewid) 2004-11-17 00:54:36.000-0600 Hi, Great work on the improvements, but alas it still crashes on dropping out of a queue with CVS-HEAD 11/13/04 update: uploaded 'backtrace_nov1604.txt' which is a 'bt full' trace of the corefile that asterisk spit out ---- debug from asterisk cli -- Executing Queue("Zap/2-1", "techqueue1|nt|||20") in new stack 2004-11-17 01:58:34 DEBUG[17722]: app_queue.c:1752 queue_exec: queue: techqueue1, options: nt, url:, announce: , timeout: 20, priority: 0    -- Started music on hold, class 'default', on Zap/2-1 2004-11-17 01:58:34 DEBUG[17722]: channel.c:1137 ast_settimeout: Scheduling timer at 160 sample intervals 2004-11-17 01:58:34 DEBUG[17722]: app_queue.c:708 ring_one: (Parallel) Trying 'Agent/1111' with metric 0 2004-11-17 01:58:34 NOTICE[17722]: app_queue.c:819 wait_for_answer: No one is answering queue 'techqueue1'    -- Executing Wait("Zap/2-1", "2") in new stack 2004-11-17 01:58:34 DEBUG[17722]: channel.c:1388 ast_read: Generator got voice, switching to phase locked mode 2004-11-17 01:58:34 DEBUG[17722]: channel.c:1137 ast_settimeout: Scheduling timer at 0 sample intervals 2004-11-17 01:58:34 NOTICE[17722]: res_musiconhold.c:173 ast_moh_files_next: Zap/2-1 Opened file 1 '/var/lib/asterisk/mohmp3-notvariable/phil_hii-adelita'    -- Executing Playback("Zap/2-1", "queues/qh-tech-messages-busy") in new stack Killed ..root@asterisk:(/etc/asterisk)% edited on: 11-17-04 00:59 edited on: 11-17-04 01:11 By: Kevin P. Fleming (kpfleming) 2004-11-17 07:31:47.000-0600 OK, I've tested this with queues, but not with chan_agent, so that appears to be where the issue is. I notice from your message log that moh got started for the caller, but never stopped before your Playback() was run... can you post the relevant bit of your extensions.conf so I can see exactly what is going on (I know it's simple, but I'd like to see it anyway <G>). It _appears_ from the backtrace that the moh stream is being closed while its still in use, because Playback() is trying to open a new one. What we need to find out is why the moh stream is not being stopped when the caller leaves the queue. By: Matt Gibson (flewid) 2004-11-17 07:53:59.000-0600 Hi, Here's the relevant part of my extensions.conf :) Happens whether it drops out to 'hangup' or to 'sayunixtime' or anything else i've tried.. ----- [techqueue1] exten => s,1,Answer exten => s,2,SetMusicOnHold(default) exten => s,3,DigitTimeout,5 exten => s,4,ResponseTimeout,10 exten => s,5,SetVar(ALERT_INFO=1) exten => s,6,Background(silence/1) ; set the callerid to per queue exten => s,7,SetCIDName(qh - techq1) exten => s,8,Background(queues/qh-tech-queue)     " "Welcome to Quickhost technical support, you may "hangup using #, or press * to leave a voicemail at anyt$ ; this next line sets it to call techqueue1 ; with no retries on timeout (n) ; without allowing caller to hangup with * (H) ; with allowing called user to transfer (t) ; Queue(queuename|options|optionalurl|announceoverride|timeout) ; example: Queue(dave|t|||45) exten => s,9,Queue(techqueue1|nt|||20) ; this is where we drop do if the queue times out exten => s,10,Wait(2) exten => s,11,Playback(queues/qh-tech-messages-busy) ; all of our agents are currently occupied, please leave us a message and we exten => s,12,Voicemail(s7000)                       ; will return your call within 24 hours. thank you for choosing quickhost exten => s,13,Hangup ; this is what happens if user presses * exten => *,1,Answer exten => *,2,Playback(queues/qh-tech-message) ; please leave us a message and we will return your call within 24 hours. thankyou for choosing qh exten => *,3,Voicemail(s7000) exten => *,4,Hangup By: Kevin P. Fleming (kpfleming) 2004-11-17 08:03:00.000-0600 I have just reproduced the problem here (without using chan_agent)... stay tuned. By: Kevin P. Fleming (kpfleming) 2004-11-17 08:11:09.000-0600 Apply the attached "app_queue_moh.patch" to your *, and let me know the results. If it solves the problem for you (it did here, and seems to be an obvious fix), I'll open another bug for it, since the problem is actually in app_queue. By: Matt Gibson (flewid) 2004-11-17 09:12:21.000-0600 Yup, this seems to work. Thanks for the great work! I'll post here if there's anything else I encounter. By: Kevin P. Fleming (kpfleming) 2004-11-17 09:16:46.000-0600 Already posted as bug 2891, since it seemed to be an obvious fix. By: Kevin P. Fleming (kpfleming) 2004-11-17 09:17:43.000-0600 Oh, and just to keep my karma in line, I do have a disclaimer on file :-) By: paradise (paradise) 2004-11-24 15:54:29.000-0600 just repeating the old question: is there any reason to not adding this patch/feature to the CVS HEAD (even STABLE branch). I suppose it has been tested enough since last 45 days. it works great with native formats and mp3s! playing mp3s(by spawning mpg123) for moh just eats CPU, causes possible latencies and makes * unstable. By: Brian West (bkw918) 2004-11-24 16:25:00.000-0600 Its ready.. By: zoa (zoa) 2004-11-28 07:28:35.000-0600 from a performance point of view, i think it would be cool to have start multiple native streams at the same time, but with different codecs. E.g start one stream with the .ul versions, one with the .gsm version, one with the .g729 version etc. The patch is now super when your clients all use the same codec. By: Kevin P. Fleming (kpfleming) 2004-11-28 09:51:42.000-0600 This patch does not use codecs, it uses native files. There is no performance difference between playing files of any native format, they are all handled the same way. If you are playing a single set of files (i.e. only one storage format) but need them converted into multiple codecs, you should consider doing that directly (with sox) and then storing them in multiple formats. This will eliminate the need for run-time transcoding. Also, there is a good reason why each channel gets its own stream: this makes it possible to remember where the caller 'was' in the stream and put them back there when they moh is restarted. For applications where moh is started and stopped repeatedly (like app_queue), this is a major improvement. By: Brian West (bkw918) 2004-11-28 11:48:54.000-0600 kpfleming when you have a file in gsm format and you play it to a client thats calling using ulaw.. you do use codecs.. you have to setup a translation path from one to the other. bkw By: Kevin P. Fleming (kpfleming) 2004-11-28 12:21:24.000-0600 Quoting from my response: "If you are playing a single set of files (i.e. only one storage format) but need them converted into multiple codecs, you should consider doing that directly (with sox) and then storing them in multiple formats. This will eliminate the need for run-time transcoding." If you have ulaw callers, then use sox to copy and convert all your GSM files into .pcm files, and let Asterisk play them directly. Granted, there may not be tools to make native format files for every supported codec (although certainly GSM, ulaw, alaw, and G.729 are available), but it's quite easy to convert the GSM files into many of the formats needed to send to callers without runtime transcoding. By: Brian West (bkw918) 2004-11-28 12:59:54.000-0600 Actually we thought about doing this on the fly :P bkw By: zoa (zoa) 2004-11-28 13:51:09.000-0600 hmm, i dont think you get what i mean. What i mean is: suppose you have some clients using gsm and some using g729, now you have to choose or .gsm or .729 to avoid some translation paths. for a g729 caller we would have to decode and encode if we play .gsm and for a .gsm caller we would need to do the same if we would opt for .g729 files Meaning that we would have to choose for or g729 or gsm files. Or maybe .SLIN files (in this case we would need no decoding only encoding to gsm or g729) So if we would have multiple streams, one for gsm and one for g729 we would never have to do encoding or decoding, regardless if our client connects with g729 or gsm. Of course we would have a little overhead due to playing more streams at the same time, but since this is quite low on cpu usage, we would save cpu cycles already on '1 simultaneous' calls we do.) By: zoa (zoa) 2004-11-28 13:52:53.000-0600 My point being, with the current patch you have to choose what codecs your clients will use when choosing the native file format for moh. No way to do good for all of them at the same time. By: Kevin P. Fleming (kpfleming) 2004-11-28 14:14:10.000-0600 zoa, No, you misunderstood my point. Let's say you have this in musiconhold.conf: [moh_files] default = moh/default/file1 Then, in /usr/lib/asterisk/moh/default, you have: -r--r--r-- root root ???  file1.gsm -r--r--r-- root root ???  file1.pcm -r--r--r-- root root ???  file1.g729 With this configuration, any caller using GSM, G.729 or ulaw as their codec will get to hear this file without any runtime transcoding necessary. That, in a nutshell, is one of the major benefits of this patch (in addition to being able to play MP3 files without using mpg123 or external processes). By: zoa (zoa) 2004-11-28 16:54:08.000-0600 actually, as far as i know, this will not happen, it will just put all the files in one list, and remove the duplicates ( with or without different file formats) and then play that list. The behaviour you describe is what i'd like to see :) zoa. By: Kevin P. Fleming (kpfleming) 2004-11-28 17:39:02.000-0600 No, this _exactly_ how this patch works. Each filename (with its extension stripped off) is passed to ast_openstream, which will find the _best_ version of the available files to open (based on the channel's current codec, the available versions and the translation paths). During testing of my version of the patch I repeatedly put a G.729 caller on hold and executed "show g729" from the CLI, verifying that zero G.729 licenses were in use. The only way this can be true is because I have G.729 versions of my moh files present, so Asterisk can play them directly to the caller. In addition, when PSTN callers are directed to the identical moh playback, they hear a much higher quality version, because then Asterisk can play the ulaw version instead. It _does_ already work this way. If you provide each of your moh sound files in all codec formats that your clients use, Asterisk will never have to transcode the sound files to use them for moh. By: Anthony Minessale (anthm) 2004-12-02 17:53:48.000-0600 Newest version loses the malloc and may solve any small hard-to-find issues By: Kevin P. Fleming (kpfleming) 2004-12-02 18:35:29.000-0600 Your "new" version is not relative to the version I posted, so it contains very few (if any) of the improvements I added. For example, it still puts files into the list multiple times if they are present in multiple formats, it still tries to fastforward to the elapsed time even when it has changed files due to randomization, it still chops off long file paths, it still includes the names of directories in the file list, etc. It would have been much more useful, in my opinion, to base your "no malloc" changes on the most recent version of the patch, rather than the original one. By: Anthony Minessale (anthm) 2004-12-02 19:23:53.000-0600 I didn't notice your patch so it would have been more useful, in my opinion, for you to contact me directly instead of assuming I scrutinize every posting on this bug if you plan to alter my code... perhaps then I would have a copy of it on my box. By: Kevin P. Fleming (kpfleming) 2004-12-02 19:29:04.000-0600 OK, sure, I could have notified you directly... maybe I should have. I thought the whole point of posting patches here was for people to comment on them and follow the discussions related to them. I guess not. I'll go away now. By: Anthony Minessale (anthm) 2004-12-02 19:53:05.000-0600 kpfleming , sorry but the wording in your last post set the not-so-friendly tone of my reply.  Anyway I went back to your patch and redid my changes again since they were fresh in my head anyway it fixes at least 1 segfault situation so it was important I do so. I won't start a flame war over the mantis and I do want ppl to work on the code so I'll take the high road and apologise for my snappy reply now. Thanks for the contribution. By: Kevin P. Fleming (kpfleming) 2004-12-03 08:59:35.000-0600 I too will apologize for the tone of my first reply; it had been a long day and I was not in a good mood :-) However, I do find it quite surprising that you were not aware that other versions of the patch had been uploaded into this bug; I put up my version, then Brian posted a revised version of that. I know when I create a bug Mantis emails me when all bugnotes are added and I try to keep on top of discussion related to my bug, but maybe not everyone does that... I'm a bit concerned about the "no malloc" version of the patch; I know Mark has resisted adding elements to the channel structure before, and now we've gone from adding a pointer to adding a whole strucure. Is it likely that this patch is going to get accepted in this form? I have no problem continuing to maintain my own tree with it incorporated, but I'd also like to work towards getting it included in the main Digium tree. By: Kevin P. Fleming (kpfleming) 2004-12-07 10:25:40.000-0600 Is there any way to refactor ast_openstream and ast_filehelper to do what you need for opening a "standalone" stream, rather then duplicating most of their code? By: Anthony Minessale (anthm) 2004-12-07 12:02:11.000-0600 no because changing prototypes of such a popular function would be horrible anyway forget it, i'm recalling this patch , stay tuned... By: Anthony Minessale (anthm) 2004-12-07 15:16:49.000-0600 Newest rendition: goes back to malloc approach but only puts a void * in channel.h in a generic member called music_state because it didn't seem right to add the files_state obj to channel.h when only 1 loadable mod uses it now various alternate implementations of music can borrow the pointer and assume it to be freed for them. Also the method of keeping track of where we are in the file has been optimized by using a sample count rather than time keeping. I'm still a little concerned how ast_openstream calls ast_generator_stop when the ast_openstream call itself is comming from a generator's alloc but it seems to work. By: Kevin P. Fleming (kpfleming) 2004-12-07 15:23:25.000-0600 I like those changes a lot :-) Especially remove all the gettimeofday() calls, those can be slow. One quibble: the channel.h diff in the patch still references "struct moh_files_state", instead of being a void pointer. Did this compile OK for you? By: Anthony Minessale (anthm) 2004-12-07 18:33:15.000-0600 oops there!, fixed. By: Anthony Minessale (anthm) 2004-12-09 18:21:06.000-0600 keeping up with cvs By: Kevin P. Fleming (kpfleming) 2004-12-11 23:15:31.000-0600 I upgraded our server tonight, to yesterday's CVS, along with a bunch of other changes (none of them moh-related). I also included this new native_moh patch, instead of my previous one. We had various moh-related problems, from totally dead channels (cannot hangup, Asterisk won't shutdown), to loss of moh during transfers, loss of moh while callers were in app_queue, etc. Reverting to the previous native_moh patch cured the problems. I have not yet had time to analyze the differences between the patches, but I'll look into it over the next couple of days. By: Anthony Minessale (anthm) 2004-12-12 10:44:46.000-0600 kpfleming, did you make clean since the struct ast_channel has recently been shortened and that sometimes causes that kind of issue. By: Kevin P. Fleming (kpfleming) 2004-12-12 11:21:00.000-0600 Yes, I always build from a fresh checkout of my internal trees (kept in an SCM). By: Anthony Minessale (anthm) 2004-12-14 10:22:13.000-0600 native_moh_take100.rev3.diff Adjustments to handle strange codecs that request bizzare frame lengths and small bugfix in the pause/resume stuff. By: Anthony Minessale (anthm) 2004-12-14 15:35:40.000-0600 alternate method By: Kevin P. Fleming (kpfleming) 2004-12-14 17:31:55.000-0600 I like this version, I'll give it a try soon :-) I only see one possible flaw; sample_queue is not reset to zero in moh_files_alloc when the channel is changing from one moh class to another, but it appears that it should be. Maybe moh_files_alloc should just memset() the entire state structure to zeroes when the class changes? By: Anthony Minessale (anthm) 2004-12-14 19:10:53.000-0600 good idea! done.. By: Kevin P. Fleming (kpfleming) 2004-12-16 20:20:07.000-0600 I like the rev7 changes, the STREAMASIS flag should help quite a bit. One question though: why the pos/save_pos thing? pos can only get modified in one place, so I don't follow what the value of copying it into save_pos is... When it worked like this before, I had trouble with the saved value being used when it shouldn't, although it appears that isn't happening here (but I have not actually tried running this version yet). By: Anthony Minessale (anthm) 2004-12-17 08:52:09.000-0600 The saved value is for the case where moh_stop is called, then moh_start is called again it naturally tries to ++ the pos so you don't resume in the same exact spot.  This didn't happen before because I didnt close the stream during a pause because of the segfault caused by the streamfile catch-22.  Now that the asis flag fixes the chicken/egg generator stopstream thing I can safely close the stream when moh_stop is called creating the necessity to be able to save the position. I could have alternatly just had the int serve as a flag to use the existing pos value either way i need 2 vars, if i ever need any more TF values i may switch it to a bitmask but i don't forsee it. By: Kevin P. Fleming (kpfleming) 2004-12-17 09:12:42.000-0600 Makes perfect sense, thanks for the explanation! Now I see the real value of this new version of the patch... I'll get it on my test server this weekend and start hammering on it. By: Anthony Minessale (anthm) 2004-12-17 09:36:06.000-0600 Reminder sent to markster How's this look? -Tony By: Anthony Minessale (anthm) 2004-12-17 18:48:46.000-0600 These enhancements add the following: 1) Custom mode will allow files with the extension .raw and .slin as well as .mp3 2) A C raw player so you can use feature #1 better. 3) full reload support.   The older version already supported rescanning files_based dirs and picking   up new classes in the traditional moh but now in addition the cli command,   "moh reload" will recreate all the classes from scratch and even restart   the new music on channels listening in. 4) Die With Dignity.   If you set die_with_dignity => yes in [general] the moh mod will trap a   segfault and kill it's child pids and exit gracefully. I know I made the native patch to eliminate the mpg123 but if you plan on having a big volume of calls listening to moh you may be better off converting them to raw and playing them with rawplayer. make track01.mp3 into track01.raw with sox if it has mp3 support or RTFM on how to do it other ways: sox -c 1 track01.mp3 -t raw -r 8000 -c 1 -s -w track01.raw add this to musiconhold.conf: [classes] default => custom:/var/lib/asterisk/holdmusic_raw,/bin/rawplayer do this in unix... #gcc rawplayer.c -o /bin/rawplayer #chmod a+x /bin/rawplayer By: Mark Spencer (markster) 2004-12-23 20:42:53.000-0600 Merged in CVS, thanks! By: Russell Bryant (russell) 2004-12-24 14:15:43.000-0600 not in 1.0 By: Digium Subversion (svnbot) 2008-01-15 15:18:10.000-0600 Repository: asterisk Revision: 4552 U   trunk/channel.c U   trunk/file.c U   trunk/include/asterisk/channel.h U   trunk/include/asterisk/file.h U   trunk/include/asterisk/musiconhold.h U   trunk/res/res_musiconhold.c ------------------------------------------------------------------------ r4552 | markster | 2008-01-15 15:18:10 -0600 (Tue, 15 Jan 2008) | 2 lines Merge anthm's native MOH patch (bug ASTERISK-2594) he promises me he'll rid it of ast_flag_moh... ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=4552