[Home]

Summary:ASTERISK-17413: [patch] MONITOR_FILENAME should be MIXMONITOR_FILENAME in documentation of MONITOR_EXEC
Reporter:David Woolley (davidw)Labels:
Date Opened:2011-02-15 11:23:23.000-0600Date Closed:2011-11-30 13:37:15.000-0600
Priority:TrivialRegression?No
Status:Closed/CompleteComponents:Documentation
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) issue18817_mixmonitor_queues_doc.diff
Description:In queues.conf.sample, both the variable used to force the filename for MixMonitor and the variable that gets set to the actual filename are shown as MONITOR_FILENAME.  However, the latter is actually MIXMONITOR_FILENAME, as set by this line in app_mixmonitor.c:

apps/app_mixmonitor.c:  pbx_builtin_setvar_helper(chan, "MIXMONITOR_FILENAME", args.filename);

and as indicated in the API meta-data:

                               <variable name="MIXMONITOR_FILENAME">
                                       <para>Will contain the filename used to record.</para>
                               </variable>


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

Originally noted in 1.6.1.0, but in both cases by code reading.


$ grep MIXMONITOR_FILENAME configs/*
$ grep MIXMONITOR_FILENAME */*.c
apps/app_mixmonitor.c:            <variable name="MIXMONITOR_FILENAME">
apps/app_mixmonitor.c:  pbx_builtin_setvar_helper(chan, "MIXMONITOR_FILENAME", args.filename);
Comments:By: Michael L. Young (elguero) 2011-02-15 19:02:12.000-0600

You are referring to this line?:

; You can specify a post recording command to be executed after the end of
; recording by calling (from the dialplan)
;   Set(MONITOR_EXEC=mv /var/spool/asterisk/monitor/^{MONITOR_FILENAME} /tmp/^{MONITOR_FILENAME})

The problem is that MIXMONITOR_FILENAME is not set until MixMonitor is run.  Therefore, that variable is not set at the point of time that you need to set MONITOR_EXEC.  Therefore, you need to set MONITOR_FILENAME and use that when you are setting MONITOR_EXEC.
By: David Woolley (davidw) 2011-02-16 05:31:04.000-0600

OK.  In that case the sample is still bad, as it implies that MONITOR_FILENAME is read/write. (It also means that MIXMONITOR_FILENAME is of rather limited use!)

CANCEL THIS.  My original bug report was correct, ${MIXMONITOR_FILENAME} is valid when variables are substituted, see comment 132146.


By: Michael L. Young (elguero) 2011-02-16 08:07:12.000-0600

Maybe I am missing something?

MONITOR_FILENAME is a variable.  All it will contain is the name that you want the recording to be saved as.

If you look above the line I posted in the last comment, the documentation mentions setting MONITOR_FILENAME.  The example on how to use the MONITOR_EXEC variable, shows how you can set a command to be run (which moves the recording to the tmp directory) after the recording has ended.

MIXMONITOR_FILENAME is of great use.  MixMonitor is not used just for queues.

This is turning into an asterisk-users conversation.  Basically, I do not see any bugs at all or mis-information.
By: David Woolley (davidw) 2011-02-18 10:25:34.000-0600

MIXMONITOR_FILENAME *is* set before the variable substitution is performed on the post processing command.  I re-instate my full original bug report.

.....
       pbx_builtin_setvar_helper(chan, "MIXMONITOR_FILENAME", args.filename);
       launch_monitor_thread(chan, args.filename, flags.flags, readvol, writevol, args.post_process);
.....

static void launch_monitor_thread(struct ast_channel *chan, const char *filename
, unsigned int flags,

.....

       postprocess2[0] = 0;
       /* If a post process system command is given attach it to the structure */
       if (!ast_strlen_zero(post_process)) {
               char *p1, *p2;

               p1 = ast_strdupa(post_process);
               for (p2 = p1; *p2 ; p2++) {
                       if (*p2 == '^' && *(p2+1) == '{') {
                               *p2 = '$';
                       }
               }
               pbx_substitute_variables_helper(chan, p1, postprocess2, sizeof(postprocess2) - 1);
......
       ast_pthread_create_detached_background(&thread, NULL, mixmonitor_thread, mixmonitor);
}
By: Michael L. Young (elguero) 2011-02-21 09:01:00.000-0600

Not to be dense here.  Technically, it would appear that you are right.  But, remember the user can choose to use Monitor or MixMonitor.  The documentation actually is less confusing to the user because the variable MONITOR_FILENAME can be used for both cases, unless I am missing something.  Therefore, it actually provides for more flexibility since the user can switch between Monitor or MixMonitor without having to change their dial plan.

IMHO, the documentation is fine the way it is.  I hope someone else will comment on this.
By: Leif Madsen (lmadsen) 2011-04-19 09:32:12

It seems like a documentation note should be added to reflect the comments here. While the documentation may be "correct" the way it is, I think additional information to clear up any additional use cases would be ideal.

If someone can supply a patch I would be more than happy to review and commit.
By: Leif Madsen (lmadsen) 2011-05-10 14:49:41

Ping?
By: David Woolley (davidw) 2011-05-11 06:06:53

Will try and get round to this, but I probably need to do it in my own time.
By: Michael L. Young (elguero) 2011-05-18 13:56:10

Davidw

How does that wording sound?  Does that cover it?

PS This was against svn trunk


By: Leif Madsen (lmadsen) 2011-11-02 14:37:43.857-0500

If that works, then I'd be happy to commit the changes. Just ping me on IRC when resolved :)

BTW: it sounds fine to me.
By: Leif Madsen (lmadsen) 2011-11-30 13:20:52.220-0600

I can only assume everyone is ok with the wording, so I'm going to commit it.