[asterisk-dev] [Code Review] Modify bridging to properly evaluate DTMF after first warning is played

[Jeff Peeler]

----- "Kaloyan Kovachev" <kkovachev at varna.net> wrote:

> Hello,
>  probably a bit OT here, but still in place, because this patch is
> just a
> workaround (see below)
> 
> On Mon, 16 Feb 2009 23:19:29 -0000, Jeff Peeler wrote
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > http://reviewboard.digium.com/r/163/
> > -----------------------------------------------------------
> > 
> > Review request for Asterisk Developers.
> > 
> > Summary
> > -------
> > 
> > The main problem is currently if the Dial flag L is used with a
> warning
> sound, DTMF is not evaluated after the first warning sound.
> > 
> > These changes are actually pretty simple in retrospect.
> ast_generic_bridge
> has been modified to:
> > - return AST_BRIDGE_COMPLETE when DTMF is detected so that possible
> feature
> matches are evaluated in res_features
> > - set a flag for playing the warning which ensures that if a
> scheduled
> warning is missed multiple warnings are not played back to back (due
> to a
> feature evaluation or waiting for digits)
> > 
> > ast_channel_bridge was modified to store the nexteventts in the
> ast_bridge_config structure as that information was lost every time
> ast_channel_bridge was reentered, causing a hangup due to incorrect
> time
> calculations.
> 
> the later was part of the patch for
> http://bugs.digium.com/view.php?id=6335,
> but i don't think it is the best fix. The problem is that nexteventts
> is used
> for both playing the warning and timing out the feature request ...
> thats why
> later it was necessary to change:
> if (to <= 0) {
>         res = AST_BRIDGE_COMPLETE;
>         break;
> ...
> to
> if (to <= 0) {
>          if (!config->timelimit) {
>          res = AST_BRIDGE_COMPLETE;
>          break;
> 
> to prevent dropping the call before the limit is reached, but when a
> feature
> is attempted and L() is used.
> 
>  So the best fix would be not to use nexeteventts for both and i think
> the
> place of the warnings is not inside the bridge ... is it possible by
> using the
> new audiohooks API to play the warnings to the channels from a
> different thread?
>  Combined with the new bridging API (or using the method from bug
> 6335) with a
> scheduled execution for the warnings will free the bridge from that
> code and
> will leave nexeteventts for the features only ... this what I am
> planning to
> do when the bridging API i included (as another attemt for RTCC
> patch), but
> still can't figure out how to play a file to the channel via
> adiohooks.
> 
> > 
> > This addresses bug 14315.
> >     http://bugs.digium.com/view.php?id=14315
> > 
> > Diffs
> > -----
> > 
> >   /branches/1.4/include/asterisk/channel.h 176209 
> >   /branches/1.4/main/channel.c 176209 
> >   /branches/1.4/res/res_features.c 176209
> > 
> > Diff: http://reviewboard.digium.com/r/163/diff
> > 
> > Testing
> > -------
> > 
> > Tested exact scenario pretty extensively. Also made sure normal
> bridging
> works as expected as well as a time limited bridge with no warning.
> > 
> > Thanks,
> > 
> > Jeff

I wouldn't say this is exactly off topic, but here I was trying to minimize changes to the core.  More confusing (to me, but related) than the dual role of nexteventts is that the start_time for the bridge is set to the start of the feature and then later reset to the true bridge start_time from the backup_config. I also thought about playing the warnings not in the bridge. Changes such as these are more appropriate for trunk though. 

I assume you've been keeping up with the new review here:
http://reviewboard.digium.com/r/93/

-- 
Jeff Peeler
Digium, Inc. | Software Developer
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at www.digium.com & www.asterisk.org