<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/3471/">https://reviewboard.asterisk.org/r/3471/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">While I appreciate the contribution to Asterisk and the intended purpose of this patch, at this time, I don't think this patch is appropriate for inclusion.
(1) Having custom file formats via the proposed playlists.txt is not something to encourage. Asterisk has an understood approach to defining its configuration; adding a custom schema creates a burden on system administrators as "yet another thing to understand". Even the new configuration framework/sorcery API in Asterisk 12+ still builds on the schemas defined for .conf files.
(2) By creating your own file format, you discard a substantial amount of work that has gone into the existing file reading APIs. Those APIs allow for you to check a .conf file to determine if it has changed and needs to be re-read - by rolling your own format, you are having to read the format each time Asterisk needs to determine if anything has changed in the MOH definition. While you could implement a similar mechanism, re-inventing the wheel should be a sign that something is not correct with this approach.
(3) Even assuming there was a playlists.conf, I don't understand how there is a substantial benefit of having a playlists file over files in a directory. The files in the directory can already be re-scanned for new files. The files don't even have to exist in that directory: symbolic links allow for a user to have the actual files located in some other location on the server. This feels like a lot of extra work for very little benefit.
Now, looking at the actual use case quoted on the issue:
"One of the things I needed was the ability to set the music on hold class for a call based on information gathered at the time of the call."
That's fine: but how does the CHANNEL function's musicclass attribute (https://wiki.asterisk.org/wiki/display/AST/Asterisk+11+Function_CHANNEL) not already allow for this?
"This patch allows us to build a MOH class and playlists "on the fly" that are then active when the caller puts calls on hold or if the AGI/AMI needs to play back MOH. Without this patch all combinations of music files and all MOH classes would have to be defined in the configuration file or database before Asterisk is started. In theory you could modify configuration "on the fly" however if I recall a reload of MOH kills other calls on hold or other nasty things happened."
(1) The approach taken here gains a small amount of dynamic ability - that can mostly be captured by existing mechanisms - at a performance and maintenance cost. That's not acceptable.
(2) The goal of having musiconhold 'discover' music classes at run-time is laudable, but is also possible with frameworks in Asterisk 12/trunk today. This doesn't require playlists files, a defined directory structure, or other non-standard approaches. If musiconhold was made to use sorcery, two things would be possible:
(a) It would - when querying a realtime backend - grab the requested class if it existed or fail if the class did not. This would not require a reload of the module when adding a new class.
(b) If using a static backend (such as a conf file), a reload would still be necessary. However, since sorcery guarantees thread safety and that existing operations in flight continue on without being affected, this would not result in any of the situations you may have run into in the past.
The point is, there are mechanisms to achieve the functionality you're trying to achieve, and the approach taken here chooses not to take them.
If you'd like to re-work the patch to use the proposed frameworks, we'd love to help point you in the right direction and assist with the effort. You can continue the discussion of those approaches on the asterisk-dev mailing list or in #asterisk-dev.
</pre>
<br />
<p>- Matt Jordan</p>
<br />
<p>On April 23rd, 2014, 9:02 a.m. CDT, Vitezslav Novy wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Vitezslav Novy.</div>
<p style="color: grey;"><i>Updated April 23, 2014, 9:02 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-23636">ASTERISK-23636</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This patch introduces another approach to dynamically controlled MoH.
Unlike realtime this way is file based.
As a switch between normal and alternative behavior, boolean variable
'dynamic' is used in MoH config file.
By setting
dynamic=yes
new behavior is switched on.
How dynamic behavior works
All static MoH classes in musiconhold.conf and realtime are ignored, except [default]
class. On the other hand dynamic class named 'default' is ignored too.
New variable 'dynamic_dir' defines directory, where dynamic classes are
defined. Each first level subdirectory of dynamic_dir defines one MoH class
with same name as directory name.
If class directory contains playlist file 'playlist.txt' content of
the file defines audiofiles in class and their order. Otherwise directory
is scanned same way as for standard MoH class with mode=files.
Playlist expects one file on line, without path and without extension.
Files must be placed in class directory.
If first line of playlist contains exactly one character '%', files will be
ordered randomly.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Original 'static' behavior with dynamic=no
Dynamic class without playlist
Dynamic class with playlist
Random ordering with % on first line of playlist
% as audio file name
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/res/res_musiconhold.c <span style="color: grey">(412900)</span></li>
<li>/trunk/configs/musiconhold.conf.sample <span style="color: grey">(412900)</span></li>
<li>/trunk/CHANGES <span style="color: grey">(412900)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3471/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>