[asterisk-dev] [Code Review] 2620: bridge features: Make one touch Monitor and MixMonitor into bridge features

[jrose]

> On June 18, 2013, 2:23 p.m., Matt Jordan wrote:
> > /trunk/CHANGES, lines 170-173
> > <https://reviewboard.asterisk.org/r/2620/diff/3/?file=39807#file39807line170>
> >
> >     I don't think we should have this be a generic failure sound for all features.
> >     
> >     There is already xferfailsound and pickupfailsound. If those aren't specified, is featurefailsound used instead? Do those options override this particular option? If not, how do I configure a generic feature failure sound but not play a failure sound when pickup fails?
> >     
> >     Having different failure sounds for different features provides more functionality that having one master failure sound. Since we already have two failure sounds for different features, it would be more disruptive to introduce a generic failure sound than simply to provide failure sounds for one touch recording.

changed to recordingfailsound. Documentation updated to follow suit.


> On June 18, 2013, 2:23 p.m., Matt Jordan wrote:
> > /trunk/bridges/bridge_builtin_features.c, line 484
> > <https://reviewboard.asterisk.org/r/2620/diff/3/?file=39809#file39809line484>
> >
> >     This error message doesn't account for what just happened.
> >     
> >     The problem isn't just that the bridge isn't automon capable - it is technically capable of it, as the channel has the feature hook. The problem is there isn't anyone in the bridge to start recording on.
> >     
> >     "Cannot start AutoMonitor for %s - no peer in bridge" is more reflective of the actual condition.

This also happens for 3+ party bridges, so per our discussion:

"Cannot start AutoMonitor for %s - can not determine peer in bridge"


> On June 18, 2013, 2:23 p.m., Matt Jordan wrote:
> > /trunk/bridges/bridge_builtin_features.c, lines 532-548
> > <https://reviewboard.asterisk.org/r/2620/diff/3/?file=39809#file39809line532>
> >
> >     This feels wrong. If we don't get one of these parameters off our channel, we immediately go to the peer, blow all of our values away, and use the peers?
> >     
> >     What if I don't want to specify a prefix? Now it goes to the peers and uses its format instead?
> >     
> >     This applies to MixMonitor as well.

Nah, that wasn't the intention. The intention was just to use individual values from the peer channel when they are unavailable.

In retrospect, we should probably only use the peer's values if all of these are unavailable.  I'll adjust this to account for that. But just keep in mind that the way it's done here is how it was being done in features.c before. Here's the snippet:

	touch_format = pbx_builtin_getvar_helper(caller_chan, "TOUCH_MONITOR_FORMAT");
	touch_monitor = pbx_builtin_getvar_helper(caller_chan, "TOUCH_MONITOR");
	touch_monitor_prefix = pbx_builtin_getvar_helper(caller_chan, "TOUCH_MONITOR_PREFIX");

	if (!touch_format)
		touch_format = pbx_builtin_getvar_helper(callee_chan, "TOUCH_MONITOR_FORMAT");

	if (!touch_monitor)
		touch_monitor = pbx_builtin_getvar_helper(callee_chan, "TOUCH_MONITOR");

	if (!touch_monitor_prefix)
		touch_monitor_prefix = pbx_builtin_getvar_helper(callee_chan, "TOUCH_MONITOR_PREFIX");

The only real difference is that it didn't bother to lock any of the channels or duplicate any of the string values.


> On June 18, 2013, 2:23 p.m., Matt Jordan wrote:
> > /trunk/bridges/bridge_builtin_features.c, line 570
> > <https://reviewboard.asterisk.org/r/2620/diff/3/?file=39809#file39809line570>
> >
> >     Capitalize filename if it starts a new sentence

This is another thing that was just replicating a verbose message from the old features.c implementation.


- jrose


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2620/#review8908
-----------------------------------------------------------


On June 17, 2013, 7:08 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2620/
> -----------------------------------------------------------
> 
> (Updated June 17, 2013, 7:08 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and rmudgett.
> 
> 
> Bugs: asterisk-21553
>     https://issues.asterisk.org/jira/browse/asterisk-21553
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Adds Monitor and MixMonitor as bridge features.  In order to avoid using PBX execution and autoservice, I added some additional API for MixMonitor provided through loadable function callbacks provided by app_mixmonitor.
> 
> In order to make One Touch MixMonitor behave more like its Monitor equivalent, I went ahead and added a couple channel variables for playing start and stop recording sounds (TOUCH_MIXMONITOR_MESSAGE_START and TOUCH_MIXMONITOR_MESSAGE_STOP)
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 391828 
>   /trunk/apps/app_mixmonitor.c 391828 
>   /trunk/bridges/bridge_builtin_features.c 391828 
>   /trunk/configs/features.conf.sample 391828 
>   /trunk/include/asterisk/features_config.h 391828 
>   /trunk/include/asterisk/mixmonitor.h PRE-CREATION 
>   /trunk/main/features_config.c 391828 
>   /trunk/main/mixmonitor.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2620/diff/
> 
> 
> Testing
> -------
> 
> Tried both Monitor and MixMonitor with various channel variables set for filename/format and start/stop sounds. They appear to be working as one would expect.
> Also checked the behavior of MixMonitor with app_mixmonitor loaded and unloaded. As expected, it is rejected smoothly when MixMonitor is not loaded.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130618/31eed7d1/attachment-0001.htm>