[asterisk-dev] I want to make my first patch but have run into a problem and don't know how to progress.

[Mark Michelson]

On 10/06/2016 12:52 PM, John Kiniston wrote:
> I got all fired up on getting up to date with the current LTS version 
> of Asterisk while at Astricon and I'd like to make my first patch.

Awesome! Hopefully this will be the first of many to come.

> I want to modify bridge_builtin_features.c to honor the"MONITOR_EXEC" 
> and "MONITOR_EXEC_ARGS" variables like Monitor() does so that my one 
> touch recordings using automon get post processed.
>
> I'm no C programmer but I took a class at the local community college 
> uh I guess it's been two decades ago now so forgive me.
>
> I found the code in res/res_monitor.c that handles setting the execute 
> application and running it and I've copied what looked like the part I 
> needed into bridges/bridge_builtin_
> features.c and with a few minor changes it almost works.
>
> The part where it's breaking down is getting the format of the 
> recorded file which I think should be touch_format I'm getting (null) 
> so I reckon set_touch_variables doesn't do what I hoped it did.
>
> Would someone please help me get the file-name extension or point me 
> in a better direction if this doesn't look like a good idea?

I'm going to answer your question in two ways here: first, from the 
perspective of helping to understand the code you've written, and second 
addressing the "if this doesn't look like a good idea" angle.

Let's examine what set_touch_variables does. set_touch_variables first 
determines which channel variables it needs to be looking at. Assuming 
you're using monitor and not mixmonitor, that means that it will look up 
the TOUCH_MONITOR_FORMAT, TOUCH_MONITOR, and TOUCH_MONITOR_PREFIX 
channel variables. It will place the values of each into the 
touch_monitor_format, touch_monitor, and touch_monitor_prefix local 
variables, respectively. In your case, it appears that 
touch_mointor_format ends up NULL, which most likely stems from the fact 
that the TOUCH_MONITOR_FORMAT variable is not set on the channel which 
was passed into set_touch_variables. So later when you start outputting 
your debug, since touch_monitor_format is NULL, the glibc print routines 
turn that into "(null)".

Why don't you see the same thing happen in the res_monitor.c code? In 
that code, the ast_monitor_start() function requires a file format to be 
passed to it, and that format ends up getting set on the ast_monitor 
structure that gets created. That structure then gets set on the 
channel, and it can be later accessed by calling 
ast_channel_monitor(chan). When ast_monitor_stop() is called, it is able 
to get that saved format off the channel and use that for its 
post-processing. You could potentially do the same thing to get the file 
format by getting the value of ast_channel_monitor(chan)->format in your 
code.

So now let's talk about whether your approach is the right one to take. 
One thing you may have noticed when doing your experimental coding is 
that the code that you copy/pasted comes from ast_monitor_stop(). 
ast_monitor_stop() is a C-level API call that is called by several 
places in the code, including in bridge_builtin_features.c when 
one-touch monitoring is stopped. This means, at least the way I see it, 
that the use of MONITOR_EXEC and MONITOR_EXEC_ARGS should already be in 
a position where they could "just work" with builtin automon. We can 
take a closer look at ast_monitor_stop() and see that the guard for 
determining if MONITOR_EXEC and MONITOR_EXEC_ARGS is used is:

     if (ast_channel_monitor(chan)->joinfiles && 
!ast_strlen_zero(ast_channel_monitor(chan)->filename_base))

If MONITOR_EXEC and MONITOR_EXEC_ARGS is currently not working, it is 
likely because one or both of these conditions are not met. Let's 
examine them one by one.

ast_channel_monitor(chan)->joinfiles is a boolean that indicates if 
files should be joined together once recording is done. With the 
Monitor() application, this gets set true by setting the 'm' option (see 
start_monitor_exec() if you want to follow how that is done, 
specifically looking at MON_FLAG_MIX). Otherwise, though, this value is 
false. When ast_monitor_start() is called, it does not automatically set 
joinfiles to true. There is a function, though, called 
ast_monitor_setjoinfiles() you can use to set this value to true.

ast_channel_monitor(chan)->filename_base is the base filename being used 
for monitor recordings. "base filename" means the filename minus the 
extra bits indicating the audio direction. The only reason this would 
not be set is if there were some catastrophic error. ast_strlen_zero() 
is a utility function that returns true if the input string is either 
NULL or zero-length. So since filename_base is almost certainly more 
than zero characters long, it's likely that ast_strlen_zero() of that is 
false, and negating that makes it true.

Summary:
This probably means that you could get the functionality you desire by 
calling ast_monitor_setjoinfiles(chan, 1) after ast_monitor_start() in 
start_automonitor() of bridge_builtin_features.c. This approach would be 
better than copying/pasting because it keeps the code in one spot and 
makes use of already tested code.

Hope this was helpful,
Mark Michelson
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20161010/c7c4fb09/attachment.html>