<p>Sean Bright has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/5995">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">res_musiconhold: Remove 'special' MP3 modes<br><br>The modes mp3, mp3nb, quietmp3, and quietmp3nb are specializations of<br>the 'custom' mode that could just as easily be configured through<br>musiconhold.conf.<br><br>Remove the hard-coded mpg123 checks and add examples to the sample<br>configuration file showing how to replicate the removed modes.<br><br>Change-Id: I865fbee781c36f9cf9f6b5076f95f60b34fa7d64<br>---<br>M UPGRADE.txt<br>M configs/samples/musiconhold.conf.sample<br>M res/res_musiconhold.c<br>3 files changed, 135 insertions(+), 150 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/95/5995/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/UPGRADE.txt b/UPGRADE.txt<br>index eb05b03..f9793cf 100644<br>--- a/UPGRADE.txt<br>+++ b/UPGRADE.txt<br>@@ -32,6 +32,13 @@<br> ARI. As a result, the 'DataGet' AMI action as well as the 'data get'<br> CLI command have been removed.<br> <br>+Music On Hold:<br>+ - The built-in modes - mp3, mp3nb, quietmp3, and quietmp3nb - have been<br>+ removed in favor of the 'custom' mode. These four modes were internally<br>+ launching mpg123 with varying command line arguments, all of which is<br>+ achievable using the 'custom' mode directly. See musiconhold.conf.sample<br>+ for examples of how to mimic the behavior of the removed modes.<br>+<br> From 14.4.0 to 14.5.0:<br> <br> Core:<br>diff --git a/configs/samples/musiconhold.conf.sample b/configs/samples/musiconhold.conf.sample<br>index 741bde6..c263ca5 100644<br>--- a/configs/samples/musiconhold.conf.sample<br>+++ b/configs/samples/musiconhold.conf.sample<br>@@ -13,10 +13,6 @@<br> ; valid mode options:<br> ; files -- read files from a directory in any Asterisk supported<br> ; media format<br>-; quietmp3 -- default<br>-; mp3 -- loud<br>-; mp3nb -- unbuffered<br>-; quietmp3nb -- quiet unbuffered<br> ; custom -- run a custom application (See examples below)<br> <br> ; =========<br>@@ -120,3 +116,30 @@<br> ;kill_escalation_delay=500<br> ; Send signals to just the child process instead of all descendants<br> ;kill_method=process<br>+<br>+;<br>+; In Asterisk 15, some of the built-in modes (mp3, mp3nb, quietmp3, and<br>+; quietmp3nb) were removed in favor of the 'custom' mode. The following example<br>+; classes demonstrate how to configure a music class to mimic the same behavior<br>+; as the removed modes:<br>+;<br>+; [mp3] ; Loud, buffered<br>+; mode = custom<br>+; directory = /var/lib/asterisk/mohmp3<br>+; application = /usr/bin/mpg123 -q -s --mono -r 8000 -f 8192<br>+;<br>+; [quietmp3] ; Quiet, buffered<br>+; mode = custom<br>+; directory = /var/lib/asterisk/mohmp3<br>+; application = /usr/bin/mpg123 -q -s --mono -r 8000 -f 4096<br>+;<br>+; [mp3nb] ; Loud, unbuffered<br>+; mode = custom<br>+; directory = /var/lib/asterisk/mohmp3<br>+; application = /usr/bin/mpg123 -q -s --mono -r 8000 -b 2048 -f 8192<br>+;<br>+; [quietmp3nb] ; Quiet, unbuffered<br>+; mode = custom<br>+; directory = /var/lib/asterisk/mohmp3<br>+; application = /usr/bin/mpg123 -q -s --mono -r 8000 -b 2048 -f 4096<br>+;<br>diff --git a/res/res_musiconhold.c b/res/res_musiconhold.c<br>index e4bb7a2..fcf3563 100644<br>--- a/res/res_musiconhold.c<br>+++ b/res/res_musiconhold.c<br>@@ -141,17 +141,15 @@<br> char save_pos_filename[PATH_MAX];<br> };<br> <br>-#define MOH_QUIET (1 << 0)<br>-#define MOH_SINGLE (1 << 1)<br>-#define MOH_CUSTOM (1 << 2)<br>-#define MOH_RANDOMIZE (1 << 3)<br>-#define MOH_SORTALPHA (1 << 4)<br>+#define MOH_CUSTOM (1 << 0)<br>+#define MOH_RANDOMIZE (1 << 1)<br>+#define MOH_SORTALPHA (1 << 2)<br> #define MOH_RANDSTART (MOH_RANDOMIZE | MOH_SORTALPHA) /*!< Sorted but start at random position */<br>-#define MOH_SORTMODE (3 << 3)<br>+#define MOH_SORTMODE (3 << 1)<br> <br>-#define MOH_CACHERTCLASSES (1 << 5) /*!< Should we use a separate instance of MOH for each user or not */<br>-#define MOH_ANNOUNCEMENT (1 << 6) /*!< Do we play announcement files between songs on this channel? */<br>-#define MOH_PREFERCHANNELCLASS (1 << 7) /*!< Should queue moh override channel moh */<br>+#define MOH_CACHERTCLASSES (1 << 3) /*!< Should we use a separate instance of MOH for each user or not */<br>+#define MOH_ANNOUNCEMENT (1 << 4) /*!< Do we play announcement files between songs on this channel? */<br>+#define MOH_PREFERCHANNELCLASS (1 << 5) /*!< Should queue moh override channel moh */<br> <br> /* Custom astobj2 flag */<br> #define MOH_NOTDELETED (1 << 30) /*!< Find only records that aren't deleted? */<br>@@ -209,9 +207,7 @@<br> <br> static struct ao2_container *mohclasses;<br> <br>-#define LOCAL_MPG_123 "/usr/local/bin/mpg123"<br>-#define MPG_123 "/usr/bin/mpg123"<br>-#define MAX_MP3S 256<br>+#define MAX_FILE_COUNT 256<br> <br> static void moh_parse_options(struct ast_variable *var, struct mohclass *mohclass);<br> static int reload(void);<br>@@ -548,136 +544,106 @@<br> .write_format_change = moh_files_write_format_change,<br> };<br> <br>-static int spawn_mp3(struct mohclass *class)<br>+#define IS_URL(x) (!(strncasecmp(x, "http://", sizeof("http://") - 1) \<br>+ && strncasecmp(x, "https://", sizeof("https://") - 1)))<br>+<br>+static int external_audio_proc_spawn(struct mohclass *class)<br> {<br> int fds[2];<br>- int files = 0;<br>- char fns[MAX_MP3S][80];<br>- char *argv[MAX_MP3S + 50];<br>+ char fns[MAX_FILE_COUNT][80];<br>+ char *argv[MAX_FILE_COUNT + 50];<br> char xargs[256];<br> char *argptr;<br> int argc = 0;<br>- DIR *dir = NULL;<br>- struct dirent *de;<br>+ int is_real_directory = 0;<br> <br>- <br>+ /* Format arguments for argv vector */<br>+ ast_copy_string(xargs, class->args, sizeof(xargs));<br>+ argptr = xargs;<br>+ while (!ast_strlen_zero(argptr)) {<br>+ argv[argc++] = argptr;<br>+ strsep(&argptr, " ");<br>+ }<br>+<br> if (!strcasecmp(class->dir, "nodir")) {<br>- files = 1;<br>+ /* Nothing to do */<br>+ } else if (IS_URL(class->dir)) {<br>+ ast_copy_string(fns[0], class->dir, sizeof(fns[0]));<br>+ argv[argc++] = fns[0];<br> } else {<br>- dir = opendir(class->dir);<br>- if (!dir && strncasecmp(class->dir, "http://", 7)) {<br>- ast_log(LOG_WARNING, "%s is not a valid directory\n", class->dir);<br>+ int files = 0;<br>+ struct dirent *de;<br>+ DIR *dir = opendir(class->dir);<br>+<br>+ if (!dir) {<br>+ ast_log(LOG_WARNING, "Cannot access '%s': %s\n", class->dir, strerror(errno));<br> return -1;<br> }<br>- }<br> <br>- if (!ast_test_flag(class, MOH_CUSTOM)) {<br>- argv[argc++] = "mpg123";<br>- argv[argc++] = "-q";<br>- argv[argc++] = "-s";<br>- argv[argc++] = "--mono";<br>- argv[argc++] = "-r";<br>- argv[argc++] = "8000";<br>- <br>- if (!ast_test_flag(class, MOH_SINGLE)) {<br>- argv[argc++] = "-b";<br>- argv[argc++] = "2048";<br>- }<br>- <br>- argv[argc++] = "-f";<br>- <br>- if (ast_test_flag(class, MOH_QUIET))<br>- argv[argc++] = "4096";<br>- else<br>- argv[argc++] = "8192";<br>- <br>- /* Look for extra arguments and add them to the list */<br>- ast_copy_string(xargs, class->args, sizeof(xargs));<br>- argptr = xargs;<br>- while (!ast_strlen_zero(argptr)) {<br>- argv[argc++] = argptr;<br>- strsep(&argptr, ",");<br>- }<br>- } else {<br>- /* Format arguments for argv vector */<br>- ast_copy_string(xargs, class->args, sizeof(xargs));<br>- argptr = xargs;<br>- while (!ast_strlen_zero(argptr)) {<br>- argv[argc++] = argptr;<br>- strsep(&argptr, " ");<br>- }<br>- }<br>-<br>- if (!strncasecmp(class->dir, "http://", 7)) {<br>- ast_copy_string(fns[files], class->dir, sizeof(fns[files]));<br>- argv[argc++] = fns[files];<br>- files++;<br>- } else if (dir) {<br>- while ((de = readdir(dir)) && (files < MAX_MP3S)) {<br>- if ((strlen(de->d_name) > 3) && <br>- ((ast_test_flag(class, MOH_CUSTOM) && <br>- (!strcasecmp(de->d_name + strlen(de->d_name) - 4, ".raw") || <br>- !strcasecmp(de->d_name + strlen(de->d_name) - 4, ".sln"))) ||<br>- !strcasecmp(de->d_name + strlen(de->d_name) - 4, ".mp3"))) {<br>+ while ((de = readdir(dir)) && (files < MAX_FILE_COUNT)) {<br>+ if (strlen(de->d_name) > 3 &&<br>+ (ast_ends_with(de->d_name, ".raw") ||<br>+ ast_ends_with(de->d_name, ".sln") ||<br>+ ast_ends_with(de->d_name, ".mp3"))) {<br> ast_copy_string(fns[files], de->d_name, sizeof(fns[files]));<br> argv[argc++] = fns[files];<br> files++;<br> }<br> }<br>- }<br>- argv[argc] = NULL;<br>- if (dir) {<br>+<br> closedir(dir);<br>+<br>+ if (!files) {<br>+ ast_log(LOG_WARNING, "Found no files in '%s'\n", class->dir);<br>+ return -1;<br>+ }<br>+<br>+ is_real_directory = 1;<br> }<br>- if (pipe(fds)) { <br>- ast_log(LOG_WARNING, "Pipe failed\n");<br>+<br>+ argv[argc] = NULL;<br>+<br>+ if (pipe(fds)) {<br>+ ast_log(LOG_WARNING, "pipe() failed: %s\n", strerror(errno));<br> return -1;<br> }<br>- if (!files) {<br>- ast_log(LOG_WARNING, "Found no files in '%s'\n", class->dir);<br>- close(fds[0]);<br>- close(fds[1]);<br>- return -1;<br>- }<br>- if (!strncasecmp(class->dir, "http://", 7) && time(NULL) - class->start < respawn_time) {<br>+<br>+ if (IS_URL(class->dir) && time(NULL) - class->start < respawn_time) {<br> sleep(respawn_time - (time(NULL) - class->start));<br> }<br> <br> time(&class->start);<br>+<br> class->pid = ast_safe_fork(0);<br> if (class->pid < 0) {<br>+ ast_log(LOG_WARNING, "fork() failed: %s\n", strerror(errno));<br> close(fds[0]);<br> close(fds[1]);<br>- ast_log(LOG_WARNING, "Fork failed: %s\n", strerror(errno));<br> return -1;<br> }<br>- if (!class->pid) {<br>- if (ast_opt_high_priority)<br>+<br>+ if (class->pid == 0) {<br>+ /* Child */<br>+ if (ast_opt_high_priority) {<br> ast_set_priority(0);<br>+ }<br> <br> close(fds[0]);<br>- /* Stdout goes to pipe */<br>+<br>+ /* stdout goes to pipe */<br> dup2(fds[1], STDOUT_FILENO);<br> <br> /* Close everything else */<br> ast_close_fds_above_n(STDERR_FILENO);<br> <br>- /* Child */<br>- if (strncasecmp(class->dir, "http://", 7) && strcasecmp(class->dir, "nodir") && chdir(class->dir) < 0) {<br>+ if (is_real_directory && chdir(class->dir) < 0) {<br> ast_log(LOG_WARNING, "chdir() failed: %s\n", strerror(errno));<br> _exit(1);<br> }<br> setpgid(0, getpid());<br>- if (ast_test_flag(class, MOH_CUSTOM)) {<br>- execv(argv[0], argv);<br>- } else {<br>- /* Default install is /usr/local/bin */<br>- execv(LOCAL_MPG_123, argv);<br>- /* Many places have it in /usr/bin */<br>- execv(MPG_123, argv);<br>- /* Check PATH as a last-ditch effort */<br>- execvp("mpg123", argv);<br>- }<br>+<br>+ execv(argv[0], argv);<br>+<br> /* Can't use logger, since log FDs are closed */<br> fprintf(stderr, "MOH: exec failed: %s\n", strerror(errno));<br> close(fds[1]);<br>@@ -686,6 +652,7 @@<br> /* Parent */<br> close(fds[1]);<br> }<br>+<br> return fds[0];<br> }<br> <br>@@ -734,7 +701,7 @@<br> }<br> }<br> <br>-static void *monmp3thread(void *data)<br>+static void *external_audio_proc_monitor(void *data)<br> {<br> #define MOH_MS_INTERVAL 100<br> <br>@@ -749,10 +716,10 @@<br> deadline.tv_usec = 0;<br> for(;/* ever */;) {<br> pthread_testcancel();<br>- /* Spawn mp3 player if it's not there */<br>+ /* Spawn audio player if it's not there */<br> if (class->srcfd < 0) {<br>- if ((class->srcfd = spawn_mp3(class)) < 0) {<br>- ast_log(LOG_WARNING, "Unable to spawn mp3player\n");<br>+ if ((class->srcfd = external_audio_proc_spawn(class)) < 0) {<br>+ ast_log(LOG_WARNING, "Unable to spawn external audio process\n");<br> /* Try again later */<br> sleep(500);<br> continue;<br>@@ -767,7 +734,7 @@<br> /* Pause some amount of time */<br> if (ast_poll(&pfd, 1, -1) > 0) {<br> if (ast_timer_ack(class->timer, 1) < 0) {<br>- ast_log(LOG_ERROR, "Failed to acknowledge timer for mp3player\n");<br>+ ast_log(LOG_ERROR, "Failed to acknowledge timer for external audio process\n");<br> return NULL;<br> }<br> /* 25 samples per second => 40ms framerate => 320 samples */<br>@@ -800,7 +767,7 @@<br> <br> if ((strncasecmp(class->dir, "http://", 7) && strcasecmp(class->dir, "nodir")) && AST_LIST_EMPTY(&class->members))<br> continue;<br>- /* Read mp3 audio */<br>+ /* Read audio */<br> len = ast_format_determine_length(class->format, res);<br> <br> if ((res2 = read(class->srcfd, sbuf, len)) != len) {<br>@@ -1263,31 +1230,36 @@<br> return 0;<br> }<br> <br>+static struct ast_timer *timer_init(void)<br>+{<br>+ struct ast_timer *timer = ast_timer_open();<br>+<br>+ if (!timer) {<br>+ ast_log(LOG_WARNING, "Unable to create timer: %s\n", strerror(errno));<br>+ return NULL;<br>+ }<br>+<br>+ if (ast_timer_set_rate(timer, 25)) {<br>+ ast_log(LOG_WARNING, "Unable to set 40ms frame rate: %s\n", strerror(errno));<br>+ ast_timer_close(timer);<br>+ return NULL;<br>+ }<br>+<br>+ return timer;<br>+}<br>+<br> static int init_app_class(struct mohclass *class)<br> {<br>- if (!strcasecmp(class->mode, "custom")) {<br>- ast_set_flag(class, MOH_CUSTOM);<br>- } else if (!strcasecmp(class->mode, "mp3nb")) {<br>- ast_set_flag(class, MOH_SINGLE);<br>- } else if (!strcasecmp(class->mode, "quietmp3nb")) {<br>- ast_set_flag(class, MOH_SINGLE | MOH_QUIET);<br>- } else if (!strcasecmp(class->mode, "quietmp3")) {<br>- ast_set_flag(class, MOH_QUIET);<br>- }<br>+ ast_set_flag(class, MOH_CUSTOM);<br> <br> class->srcfd = -1;<br>+ class->timer = timer_init();<br> <br>- if (!(class->timer = ast_timer_open())) {<br>- ast_log(LOG_WARNING, "Unable to create timer: %s\n", strerror(errno));<br>+ if (!class->timer) {<br> return -1;<br> }<br>- if (class->timer && ast_timer_set_rate(class->timer, 25)) {<br>- ast_log(LOG_WARNING, "Unable to set 40ms frame rate: %s\n", strerror(errno));<br>- ast_timer_close(class->timer);<br>- class->timer = NULL;<br>- }<br> <br>- if (ast_pthread_create_background(&class->thread, NULL, monmp3thread, class)) {<br>+ if (ast_pthread_create_background(&class->thread, NULL, external_audio_proc_monitor, class)) {<br> ast_log(LOG_WARNING, "Unable to create moh thread...\n");<br> if (class->timer) {<br> ast_timer_close(class->timer);<br>@@ -1331,9 +1303,7 @@<br> }<br> return -1;<br> }<br>- } else if (!strcasecmp(moh->mode, "mp3") || !strcasecmp(moh->mode, "mp3nb") || <br>- !strcasecmp(moh->mode, "quietmp3") || !strcasecmp(moh->mode, "quietmp3nb") || <br>- !strcasecmp(moh->mode, "httpmp3") || !strcasecmp(moh->mode, "custom")) {<br>+ } else if (!strcasecmp(moh->mode, "custom")) {<br> if (init_app_class(moh)) {<br> if (unref) {<br> moh = mohclass_unref(moh, "unreffing potential new moh class (init_app_class_failed)");<br>@@ -1519,26 +1489,11 @@<br> }<br> ast_set_flag(mohclass, MOH_RANDOMIZE);<br> }<br>- } else if (!strcasecmp(mohclass->mode, "mp3") || !strcasecmp(mohclass->mode, "mp3nb") || !strcasecmp(mohclass->mode, "quietmp3") || !strcasecmp(mohclass->mode, "quietmp3nb") || !strcasecmp(mohclass->mode, "httpmp3") || !strcasecmp(mohclass->mode, "custom")) {<br>-<br>- if (!strcasecmp(mohclass->mode, "custom"))<br>- ast_set_flag(mohclass, MOH_CUSTOM);<br>- else if (!strcasecmp(mohclass->mode, "mp3nb"))<br>- ast_set_flag(mohclass, MOH_SINGLE);<br>- else if (!strcasecmp(mohclass->mode, "quietmp3nb"))<br>- ast_set_flag(mohclass, MOH_SINGLE | MOH_QUIET);<br>- else if (!strcasecmp(mohclass->mode, "quietmp3"))<br>- ast_set_flag(mohclass, MOH_QUIET);<br>+ } else if (!strcasecmp(mohclass->mode, "custom")) {<br>+ ast_set_flag(mohclass, MOH_CUSTOM);<br> <br> mohclass->srcfd = -1;<br>- if (!(mohclass->timer = ast_timer_open())) {<br>- ast_log(LOG_WARNING, "Unable to create timer: %s\n", strerror(errno));<br>- }<br>- if (mohclass->timer && ast_timer_set_rate(mohclass->timer, 25)) {<br>- ast_log(LOG_WARNING, "Unable to set 40ms frame rate: %s\n", strerror(errno));<br>- ast_timer_close(mohclass->timer);<br>- mohclass->timer = NULL;<br>- }<br>+ mohclass->timer = timer_init();<br> <br> /* Let's check if this channel already had a moh class before */<br> if (state && state->class) {<br>@@ -1550,7 +1505,7 @@<br> mohclass = mohclass_ref(state->class, "using existing class from state");<br> }<br> } else {<br>- if (ast_pthread_create_background(&mohclass->thread, NULL, monmp3thread, mohclass)) {<br>+ if (ast_pthread_create_background(&mohclass->thread, NULL, external_audio_proc_monitor, mohclass)) {<br> ast_log(LOG_WARNING, "Unable to create moh...\n");<br> if (mohclass->timer) {<br> ast_timer_close(mohclass->timer);<br>@@ -1656,7 +1611,7 @@<br> tbytes = tbytes + bytes;<br> }<br> <br>- ast_debug(1, "mpg123 pid %d and child died after %d bytes read\n",<br>+ ast_debug(1, "audio pid %d and child died after %d bytes read\n",<br> class->pid, tbytes);<br> <br> class->pid = 0;<br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/5995">change 5995</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/5995"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: I865fbee781c36f9cf9f6b5076f95f60b34fa7d64 </div>
<div style="display:none"> Gerrit-Change-Number: 5995 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Sean Bright <sean.bright@gmail.com> </div>