Re: [asterisk-dev] [Code Review] 3471: Filesystem based dynamic MoH classes
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3471/ --- (Updated April 28, 2014, 3:26 p.m.) Status -- This change has been discarded. Review request for Asterisk Developers. Bugs: ASTERISK-23636 https://issues.asterisk.org/jira/browse/ASTERISK-23636 Repository: Asterisk Description --- 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. Diffs - /trunk/res/res_musiconhold.c 412900 /trunk/configs/musiconhold.conf.sample 412900 /trunk/CHANGES 412900 Diff: https://reviewboard.asterisk.org/r/3471/diff/ Testing --- 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 Thanks, Vitezslav Novy -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3471: Filesystem based dynamic MoH classes
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3471/#review11750 --- 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. - Matt Jordan On April 23, 2014, 9:02 a.m., Vitezslav Novy wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3471/ --- (Updated April 23, 2014, 9:02 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-23636 https://issues.asterisk.org/jira/browse/ASTERISK-23636 Repository: Asterisk Description --- 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
Re: [asterisk-dev] [Code Review] 3471: Filesystem based dynamic MoH classes
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3471/ --- (Updated April 23, 2014, 2:02 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-23636 https://issues.asterisk.org/jira/browse/ASTERISK-23636 Repository: Asterisk Description --- 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. Diffs (updated) - /trunk/res/res_musiconhold.c 412900 /trunk/configs/musiconhold.conf.sample 412900 /trunk/CHANGES 412900 Diff: https://reviewboard.asterisk.org/r/3471/diff/ Testing --- 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 Thanks, Vitezslav Novy -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 3471: Filesystem based dynamic MoH classes
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3471/ --- Review request for Asterisk Developers. Bugs: ASTERISK-23636 https://issues.asterisk.org/jira/browse/ASTERISK-23636 Repository: Asterisk Description --- 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. Diffs - /trunk/res/res_musiconhold.c 412895 /trunk/configs/musiconhold.conf.sample 412895 /trunk/CHANGES 412895 Diff: https://reviewboard.asterisk.org/r/3471/diff/ Testing --- 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 Thanks, Vitezslav Novy -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3471: Filesystem based dynamic MoH classes
Please explain what the benefit of this over the existing methods are. I would prefer using musiconhold.conf and have a mode that you list files in by just adding them to a [section]. That way, you could just include the playlists in musiconhold.conf without changing or ignoring. The different classes could be combined instead of ignored. /O On 22 Apr 2014, at 15:57, Vitezslav Novy reviewbo...@asterisk.org wrote: This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3471/ Review request for Asterisk Developers. By Vitezslav Novy. Bugs: ASTERISK-23636 Repository: Asterisk Description 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. Testing 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 Diffs /trunk/res/res_musiconhold.c (412895) /trunk/configs/musiconhold.conf.sample (412895) /trunk/CHANGES (412895) View Diff -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3471: Filesystem based dynamic MoH classes
On 22 Apr 2014, at 16:41, Vitezslav Novy vn...@vnovy.net wrote: On 04/22/14 16:07, Olle E. Johansson wrote: Please explain what the benefit of this over the existing methods are. 1/ It allows create/delete/modify MoH classes on fly without module reload and without realtime feature. Why not create a realtime text driver and you are there? 2/ It introduces concept of playlist. What is the benefit of a playlist? I would prefer using musiconhold.conf and have a mode that you list files in by just adding them to a [section]. That way, you could just include the playlists in musiconhold.conf without changing or ignoring. The different classes could be combined instead of ignored. If you speak about audio files in [section] of musiconhold.conf I think it is step in opposite direction than this patch tries to go, i.e. more flexibility without module reload. How do you change the playlists then? Curious. What can update/modify playlists but not issue a reload of a class. But I agree with you that combining dynamic classes and classes defined in config file is better idea then ignoring classes in config file when dynamic classes are on. Ignoring will cause a lot of support. That is a bad idea. /O -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 3471: Filesystem based dynamic MoH classes
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3471/#review11720 --- /trunk/CHANGES https://reviewboard.asterisk.org/r/3471/#comment21496 Since this patch involves new features and doesn't really have anything to do with ARI or the new PJSIP channel driver, it would almost certainly belong in trunk and not in 12.X The diff is showing up as being against trunk, so I suspect this is just a misplaced CHANGES note. Just up above is changes from Asterisk 12 to Asterisk 13, so this would go there. /trunk/res/res_musiconhold.c https://reviewboard.asterisk.org/r/3471/#comment21495 You've got a number of coding style violations that need to be addressed. https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines - Jonathan Rose On April 22, 2014, 8:57 a.m., Vitezslav Novy wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3471/ --- (Updated April 22, 2014, 8:57 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-23636 https://issues.asterisk.org/jira/browse/ASTERISK-23636 Repository: Asterisk Description --- 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. Diffs - /trunk/res/res_musiconhold.c 412895 /trunk/configs/musiconhold.conf.sample 412895 /trunk/CHANGES 412895 Diff: https://reviewboard.asterisk.org/r/3471/diff/ Testing --- 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 Thanks, Vitezslav Novy -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
