[Home]

Summary:ASTERISK-10718: RealTime MusicOnHold
Reporter:Sergey Tamkovich (sergee)Labels:
Date Opened:2007-11-08 12:59:29.000-0600Date Closed:2007-11-27 18:44:49.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Resources/res_musiconhold
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) moh-realtime-final-r89168.diff
( 1) moh-realtime-r89114.diff
( 2) moh-realtime-v2-r89133.diff
( 3) moh-rt-final-v2-r89168.diff
( 4) moh-rt-final-v3.2-r89168.diff
( 5) moh-rt-final-v4-r89168.diff
Description:Some parts of asterisk support RealTime, but others don't. This patch is an implementation of RT for res_musiconhold. Now you can store MOH classes in text config and RT engine. Classes from RT engine override classes from text config.

To use it, add a line to your extconfig.conf:
musiconhold => mysql,asterisk

Every time MOH starts, search is perfomed first in RT engine, then in memory (linked list of classes created from musiconhold.conf).

If MOH class is found in RT engine - separate structure is created in memory. This structure is not added to linked list (mohclasses) and will be destroyed as soon as MOH will stop.

****** ADDITIONAL INFORMATION ******

MySQL table for MOH-RT:

CREATE TABLE `musiconhold` (
 `name` varchar(80) NOT NULL,
 `directory` varchar(255) NOT NULL default '',
 `application` varchar(255) NOT NULL default '',
 `mode` varchar(80) NOT NULL default '',
 `digit` char(1) NOT NULL default '',
 `sort` varchar(16) NOT NULL default '',
 `format` varchar(16) NOT NULL default '',
 PRIMARY KEY  (`name`)
);
Comments:By: Sergey Tamkovich (sergee) 2007-11-08 13:12:19.000-0600

tested with mode=files, works well. shouldn't have any problems with other modes too.

By: Mark Michelson (mmichelson) 2007-11-08 18:09:54.000-0600

Tested with mode=files and also found no obvious faults.

I also tested with mode=custom (set to play mp3's using mpg123) and mode=quietmp3. These modes had a problem when using realtime which did not show itself when using static classes defined in musiconhold.conf.

When using static moh classes, subsequent calls would all attach to the running mpg123 process; however, when using realtime, a new mpg123 process was spawned each time a call was made. This meant that after only a few calls (maybe 10), my computer was running noticeably slower than usual. After investigating the source, this is because ast_pthread_create_background is called every time moh is started if using realtime. I think this could be avoided by checking whether mohclass->thread is NULL (Edit: Or whatever a pthread_t is when unititialized), and only call ast_pthread_create_background if this is the case.

I'll be testing more tomorrow in the attempt to find any other strange edge cases that fail when using realtime moh. Thanks for the submission though! I know there are many people who will be very happy once this is merged.



By: Leif Madsen (lmadsen) 2007-11-08 18:11:50.000-0600

If I get a chance I have a topology that this would go into perfectly. I'll report back if I get a chance.

By: Sergey Tamkovich (sergee) 2007-11-09 01:42:34.000-0600

putnopvut, i thougt about it before submiting this patch.

there is a dillema:

if more then one user use the same class, then we create a structure for it, when the first user request it. For other users we can either create a separate structure for each next user (1 user = 1 structure = 1 process if needed) as i did it in this patch,
or we can attach them all to structure/process of the first user, but then it won't be truly realtime, because other users won't cactch changes made after 1st user started his MOH. It would be something like cached RT moh class.

I think i can add a special switch to musiconhold.conf:
rtcacheclasses=yes|no

which would allow users to choose between tru-RT-heavy-cpu & cached-RT-low-cpu methods. What do you think?

By: Leif Madsen (lmadsen) 2007-11-09 07:58:30.000-0600

I think anytime you give someone a choice, it's a good thing(tm)

By: Mark Michelson (mmichelson) 2007-11-09 10:22:39.000-0600

sergee:

I think you're on the right track, but even if you want to enable true realtime for each user, I think it could be done better.

For instance, the way it's implemented now, if I were to call into a queue and wait through several different cycles of trying to reach queue members (with periodic announcements played in between), this would mean spawning mpg123 processes each time music on hold restarts (plus this also means that I always hear the same snippet of the first file in my playlist every time). Furthermore, once I hang up, none of the spawned mpg123 processes are killed, so multiple calls can add up quickly.

My suggestion is to do the following. When a channel requests for Asterisk to start music on hold, do the realtime lookup and spawn the new mpg123 (or whatever application is used) process and associate that process with the channel that started it. If that same channel requests to start music on hold again, then reattach to the process that that channel initially spawned. Then, once that channel is hung up, kill the process. By doing this, you'll have true realtime on per-call basis without having a bunch of extra processes that aren't used.

Even if you choose not to do what I suggest above, I still think you should kill the process on hangup just so they don't build up and cause huge slowdown.

By: Atis Lezdins (atis) 2007-11-09 10:34:45.000-0600

Isn't the mpg123 way obsolete? Some external script can be useful only for some audio streaming.. For multi-user system - it's much better to convert files to native formats, whenever file is uploaded.

By: Mark Michelson (mmichelson) 2007-11-09 10:43:28.000-0600

atis:

I know I would never deploy Asterisk using the mpg123 method, but it's still a valid option and invariably, people will use it, so it should work as well as we can make it.

By: Sergey Tamkovich (sergee) 2007-11-09 12:33:17.000-0600

ok so 2 changes on the way:

1. add caching of RT classes.
2. don't destroy MOH class on stop. destroy it only with channel.

By: Sergey Tamkovich (sergee) 2007-11-10 14:57:19.000-0600

Patch is ready!

1. It implements caching of RT classes.
2. It doesn't destroy moh class on stop, so music shouldn't restart each time.



By: Sergey Tamkovich (sergee) 2007-11-11 14:04:04.000-0600

Last version is available. Code cleaned up and a few minor issues are improved. Please use this version for tests. Thank you!

By: Mark Michelson (mmichelson) 2007-11-13 18:01:09.000-0600

I planned on testing this some more today, but my realtime setup got screwed up somehow. Once I get things up and running again, I'll test some more.

By: Mark Michelson (mmichelson) 2007-11-15 17:33:21.000-0600

I gave the latest patch a whirl and still have the same issue as before when using mp3 as the mode.

From looking at the code, it appears that you check to see if moh is already running on the channel by checking state and state->class. The problem is that these values are only allocated populated when using mode=files.

By: Sergey Tamkovich (sergee) 2007-11-19 04:44:42.000-0600

putnopvut, Thank you very much for your patience! Please look at the latest patch. I tested it with mode=mp3 intensively, seems to work correctly now.

By: Mark Michelson (mmichelson) 2007-11-19 14:33:19.000-0600

Great job! I tested and things seem to be working great. I'm going to run valgrind while running this and see if there are any memory issues which might not be obvious just from inspecting the code. I'll report back with anything I find. Thanks for your hard work in creating this patch!

By: Mark Michelson (mmichelson) 2007-11-19 16:47:25.000-0600

Okay, valgrind did find a couple of problems, and after inspecting the code, it should have been much more obvious that the problems existed. I love valgrind :)

Anyway, here are the issues, starting with the easy one:

1. In local_ast_moh_cleanup, if you are not using cached realtime classes, then ast_moh_destroy_one is called. In here, the moh class is freed. Upon return, you immediately check the value of state->class->realtime. Because the class was just freed, you are reading invalid memory. A simple rearrangement of if's and the use of an else will easily clear up this issue.

2. This one is a bit trickier to explain. In local_ast_moh_start, if we determine that we are using a realtime moh class, we call ast_pthread_create_background with that class as the argument. Later, if we determine that the channel already had an moh class associated with it, we free the moh class which we sent as the argument to monmp3thread, and "replace" it with the channel's moh class. The problem here is that the  monmp3thread still refers to the class which was freed, meaning there are invalid reads and writes depending on timing. Under heavy loads, I could see this potentially crashing Asterisk. As a test, I moved the check to see if the channel already had an moh class associated with it to before the call to ast_pthread_create_background, and I didn't see any more valgrind problems.


While I'm pointing out problems, I thought I'd also point out a couple of minor things. One is that the code has a compile warning in local_ast_moh_stop due to mixed declarations and code. The problem is that you declare the moh_files_state after calling ast_clear_flag and ast_deactivate_generator. Another small issue is that if no statically defined moh class is found, there is a warning printed on the console saying that the moh class could not be found. This warning should be changed to state that the moh class wasn't found in memory, so we're going to check realtime instead. This will help to prevent some possible confusion.

Keep up the good work! I know I'm probably being a pain, but it's good to have as many potential bugs cleared up now before people start using it in production.

By: Sergey Tamkovich (sergee) 2007-11-20 06:35:17.000-0600

putnopvut, Thanks again!

1. fixed
2. not sure (i'm valgrind noob)
3. fixed
4. warning changed to debug message.

By: Mark Michelson (mmichelson) 2007-11-21 13:02:50.000-0600

I just gave this a test, and it looks like you're one "else" away from having the mp3 part perfect. In local_ast_moh_start, just add an else before

if (ast_pthread_create_background(&mohclass->thread, NULL, monmp3thread, mohclass)) {

I'm going to be out of town and unable to test this for the next four days, but I will be back in the office on Monday and will do some regression tests.

By: Sergey Tamkovich (sergee) 2007-11-21 14:27:21.000-0600

Here is the last "else" :)

By: Mark Michelson (mmichelson) 2007-11-26 17:15:32.000-0600

I ended up getting busy with some other work and didn't get a chance to do regression tests on this today. I'll do it tomorrow though and report my findings.

By: Digium Subversion (svnbot) 2007-11-27 18:44:49.000-0600

Repository: asterisk
Revision: 89946

U   trunk/CHANGES
U   trunk/configs/extconfig.conf.sample
U   trunk/configs/musiconhold.conf.sample
U   trunk/res/res_musiconhold.c

------------------------------------------------------------------------
r89946 | mmichelson | 2007-11-27 18:44:48 -0600 (Tue, 27 Nov 2007) | 24 lines

Adding support for realtime music on hold. The following are the main points:

1. When moh is started, we search first in memory to find the class. If we do not
  find it in memory, we search realtime instead.

2. When moh is restarted (as in, it had been started on this particular channel, stopped,
  and now we're starting it again), if using the "files" mode, then realtime will always
  be rechecked. If you are using other modes, however, we will simply reattach to the external
  running process which was playing moh earlier in the call. This is a necessary compromise so that
  we don't end up with too many background processes.

3. musiconhold.conf has a general section now. It has one option: cachertclasses. If set to yes,
  then moh classes found in realtime will be added to the in-memory list. This has the advantage
  of not requiring database lookups each time moh is started, but it has the disadvantage of not
  truly being realtime.

I have tested this for functionality, and it passes. I also tested this under valgrind and there
are no memory problems reported under typical use.

Special thanks to Sergee for implementing this feature and enduring my complaints on the bugtracker!

(closes issue ASTERISK-10718, reported and patched by sergee)


------------------------------------------------------------------------