Summary:	ASTERISK-06644: [patch] optionally record audio of Page command, to then make a ReplayLastPage sound available
Reporter:	Jeff Saxe (jeffsaxe)	Labels:
Date Opened:	2006-03-28 21:06:27.000-0600	Date Closed:	2006-05-03 17:41:35
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Applications/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) PageRecordAudioForConvenientReplay.patch ( 1) PageRecordAudioForConvenientReplay-2.patch
Description:	Sometimes you're in a noisy environment and you don't clearly hear a paged announcement. Rather than asking other humans around you what was just announced, or trying to find or call the person who announced it and asking him or her to repeat it, you can use this patch. I added an "|r" option flag to the Page command which simply carries through to the "r" option of the MeetMe conference command on which it is constructed. Then, assuming the MEETME_RECORDINGFILE variable is carefully set, the audio content of the page is routinely recorded in a (same-named) sound file every time a paged announcement is made. Then another extension can be made to Playback that file; this second extension can be publicized among the phone system users as the "please repeat the most recent page" extension. The only problem is that using SetGlobalVar appears to be globally persistent even to other calls, so if one wants other audio-recorded conference rooms to use the default, derived-and-timestamped names for their .wav files, then one needs to get rid of the hard-coded filename just before invoking any other MeetMe with the "r" option. A slight annoyance; anyone else is welcome to suggest a way the recording filename could be forced only for the duration of the Page command, without spilling over into other MeetMe's. ****** ADDITIONAL INFORMATION ****** Sample use in extensions.conf: exten => 8222,1,Set(TIMEOUT(absolute)=60) exten => 8222,n,SetGlobalVar(MEETME_RECORDINGFILE=ReplayLastPage) exten => 8222,n,Page(Local/1267@page&Local/1265@page|r) exten => 8333,1,Playback(ReplayLastPage) exten => 8333,n,Hangup ;other ordinary conference rooms get recorded using default timestamped filename exten => 1264,1,SetGlobalVar(MEETME_RECORDINGFILE) exten => 1264,n,MeetMe(101,Mcsqr)
Comments:	By: Russell Bryant (russell) 2006-03-28 21:53:37.000-0600 Before anyone asks, I have already discussed the disclaimer with the poster of this patch.  He will be faxing it tomorrow.  :) By: Jeff Saxe (jeffsaxe) 2006-03-29 08:03:00.000-0600 I faxed in my disclaimer this morning. I don't seem to be able to alter the "No" to "Yes" next to the Disclaimer question, but maybe some administrator can. Thanks. By: BJ Weschke (bweschke) 2006-03-29 22:37:33.000-0600 I think this is a pretty cool idea, but we probably should try to find a better way to arrange for the replay filename to be setup. A collision here could be really dangerous. By: Jeff Saxe (jeffsaxe) 2006-03-30 09:09:50.000-0600 I fully agree. Since paging (in a busy environment) is likely to happen a lot, it's too risky to be setting and unsetting a single global variable for the next meetme that happens, hoping nobody else steps on it. What if a critical yearly board meeting were scheduled in advance to record into a special, legally-archived WAV file, and a half-second later, just before the participants entered it, someone does a page down in the lobby? Then the entire board meeting would be recorded into the ReplayLastPage file, and the next page that happens would wipe it out. I notice in the source to app_meetme.c that the recording filename is essentially always pulled from one named Asterisk global variable, or if that variable doesn't exist, then it constructs it on the fly with the confid's and such. As a *very* quick hack, we (meaning I) could add exactly one alternate global variable name by just changing the incoming flag from "r" to "R" (which is not used at present); then the code could just pull from a non-colliding global variable called MEETME_ALTERNATERECORDINGFILE or something. That variable would be set globally in extensions.conf and left alone, and then normal conference recording (with the "r" flag) would go back to the behavior it has currently. That's not scalable, though: it provides only *one* alternate filename, and it consumes a flag letter, and it's a disgusting hack. Better would be to add one additional argument to the MeetMe() command itself. I'm obviously less familiar with this code, in particular the incoming-argument-parse stuff, but I could probably take a swing at this. So in addition to confno, pin, options, the fourth (optional) argument would be a recording filename. This argument would be unknown to the rest of the Asterisk apps, so all other clients of MeetMe would ignore it, and it would retain the current behavior (either the MEETME_RECORDINGFILE variable or the constructed name). But the Page() command, when it was itself called with the "r" argument that I've just added in this patch, would then call MeetMe() with the normal lowercase "r" flag and with a fourth argument for the filename. This seems quite safe; the filename is only handed from one piece of code to another, never stepping on the global variable that controls all other conference recordings, and at the end of that page, it all vaporizes. This filename, constructed by Page() and passed to MeetMe(), could *itself* be retrieved from a hard-coded global variable like PAGE_RECORDINGFILE (set in extensions.conf), with a default of "ReplayLastPage" as I've named it. Or better yet, the default filename could be automatically constructed as "ReplayLastPage-" concatenated with the extension number that was used to dial the Page() command! That way three different paging extensions, which reached the Sales, Marketing, and Customer Service departments... exten => 8221,1,Set(TIMEOUT(absolute)=60) exten => 8221,n,Page(Local/1267@page&Local/1265@page|r) exten => 8222,1,Set(TIMEOUT(absolute)=60) exten => 8222,n,Page(Local/1380@page&Local/1382@page&Local/1386@page|r) exten => 8223,1,Set(TIMEOUT(absolute)=60) exten => 8223,n,Page(Local/1577@page&Local/1589&Local/1544@page@page|r) ...would automagically create separate replay files, and the corresponding replay-last-page extensions could be trivially created as... exten => 8331,1,Playback(ReplayLastPage-8221) exten => 8331,n,Hangup exten => 8332,1,Playback(ReplayLastPage-8223) exten => 8332,n,Hangup exten => 8333,1,Playback(ReplayLastPage-8223) exten => 8333,n,Hangup Cool and easy to use! Or, we could go completely hog wild and add another incoming argument to Page() itself, to be controlled on an individual extensions.conf line basis, so the paging extensions would be set up as... exten => 8221,1,Set(TIMEOUT(absolute)=60) exten => 8221,n,Page(Local/1267@page&Local/1265@page|r|ReplayLastPage-Sales) exten => 8331,1,Playback(ReplayLastPage-Sales) exten => 8331,n,Hangup That seems like a lot of extra work for such a specialized feature, though. The ability to pass an arbitrary filename into MeetMe(), from any kind of client or application, strikes me as generally useful. The ability to pass an arbitrary filename into Page() strikes me as superfluous and pedantic. By: Jeff Saxe (jeffsaxe) 2006-03-30 09:14:09.000-0600 Note that I'm quite confident of my initial app_page patch that merely parsed and reacted to one more flag, but if I took on adding another argument to MeetMe(), I'd want someone very knowledgeable to look at the code before merging it in! I don't want to leak memory, step on string buffers, or crash Asterisk or anything; I understand that phone switch code needs to be bulletproof. By: Jeff Saxe (jeffsaxe) 2006-04-03 10:53:55 OK, I have a small amount of egg on my face. Being new to Asterisk, I didn't know precisely how substitution variables worked, but when I delved into the pbx.c routine that retrieves a variable's value, I noticed that it tried multiple variable storage areas, apparently first the channel variables and then global variables as a fallback! Excellent! This is precisely what one would hope for. First see if there's a local-scope variable specifying the value, then if there isn't one, try a global variable of the same name, then if there isn't one there, return NULL. So all the rest of the strategizing notes I've written here in this bug can be ignored; there's no need to modify app_meetme.c at all, no need to pass in a specific filename. All I have to do is use Set() instead of SetGlobalVar() in my extensions.conf call to the Page() command: exten => 8221,1,Set(TIMEOUT(absolute)=60) exten => 8221,n,Set(MEETME_RECORDINGFILE=ReplayLastPage-Sales) exten => 8221,n,Page(Local/1267@page&Local/1265@page|r) exten => 8222,1,Set(TIMEOUT(absolute)=60) exten => 8222,n,Set(MEETME_RECORDINGFILE=ReplayLastPage-Marketing) exten => 8222,n,Page(Local/1380@page&Local/1382@page&Local/1386@page|r) exten => 8223,1,Set(TIMEOUT(absolute)=60) exten => 8223,n,Set(MEETME_RECORDINGFILE=ReplayLastPage-CustomerService) exten => 8223,n,Page(Local/1577@page&Local/1589@page&Local/1544@page|r) exten => 8331,1,Playback(ReplayLastPage-Sales) exten => 8331,n,Hangup exten => 8332,1,Playback(ReplayLastPage-Marketing) exten => 8332,n,Hangup exten => 8333,1,Playback(ReplayLastPage-CustomerService) exten => 8333,n,Hangup This works perfectly, and none of it steps on the global variable MEETME_RECORDINGFILE at all. The entire desired functionality was added with only the original, simple patch to app_page.c, so in my humble opinion that's ready to be merged into the trunk. I'll be happy to document this new behavior on voip-info.org's Wiki about the Page() command, including the specific usage with the Set() for the filename. By: Jeff Saxe (jeffsaxe) 2006-05-03 07:59:01 Good morning! I don't know what the protocol is for pestering the SVN maintainers monitoring my little patch, so I'm just posting here. As far as I am personally concerned (biased, of course), this patch is ready to be merged into the main distribution. I supposed it should be tested / flogged, but the changes are so simple, and so similar to the other option-passing right there in the code, that I can't imagine there are any problems with it. I did just now change the module-self-description string comment within the code to advise the user to use Set() instead of SetGlobalVar(), as I figured out in my notes of 2006-04-03, so I re-diff'ed it against the latest SVN (24379 -- no other changes had been made by others in the meantime). A "-2" version of the patch is now attached to this bug. By: BJ Weschke (bweschke) 2006-05-03 13:46:57 JeffSaxe: u got our attention. :) This is a "nitpick" but can you please fix the whitespace on the application description lines you've added to not include tabs like the other lines already in the description? Aside from that, I think we're ready. By: BJ Weschke (bweschke) 2006-05-03 17:41:34 Committed to /trunk. Thanks!