[asterisk-dev] [Code Review] Don't delay DTMF in core bridge while listening for DTMF features

Wed Feb 9 03:33:38 CST 2011

> On 2011-02-07 11:38:55, David Vossel wrote:
> > /team/oej/pinesounds-dtmf-feature-delay-1.4/res/res_features.c, lines 1528-1533
> > <https://reviewboard.asterisk.org/r/1092/diff/1/?file=15457#file15457line1528>
> >
> >     If operation is going to be overloaded to mean multiple things, it needs to be updated in the function's documentation.  I'd also recommend creating an enum to represent the different values operation can be.  This helps self document the code.

Agreed

> On 2011-02-07 11:38:55, David Vossel wrote:
> > /team/oej/pinesounds-dtmf-feature-delay-1.4/res/res_features.c, lines 1549-1552
> > <https://reviewboard.asterisk.org/r/1092/diff/1/?file=15457#file15457line1549>
> >
> >     why was this check added?

Because when using the "check" mode, there's no feature to copy to and we crashed here.

> On 2011-02-07 11:38:55, David Vossel wrote:
> > /team/oej/pinesounds-dtmf-feature-delay-1.4/res/res_features.c, lines 1580-1584
> > <https://reviewboard.asterisk.org/r/1092/diff/1/?file=15457#file15457line1580>
> >
> >     It would also be useful to create a new return value indicating that the feature was just being looked for and was found.

Agreed. Now, this is a bug fix and a new value would change the API in releases. I think we need different code for trunk than for releases in order to avoid breaking things.

> On 2011-02-07 11:38:55, David Vossel wrote:
> > /team/oej/pinesounds-dtmf-feature-delay-1.4/res/res_features.c, lines 1586-1589
> > <https://reviewboard.asterisk.org/r/1092/diff/1/?file=15457#file15457line1586>
> >
> >     I don't understand this check.

See above.

> On 2011-02-07 11:38:55, David Vossel wrote:
> > /team/oej/pinesounds-dtmf-feature-delay-1.4/res/res_features.c, lines 2357-2360
> > <https://reviewboard.asterisk.org/r/1092/diff/1/?file=15457#file15457line2357>
> >
> >     Since we already have an if else case for both DTMF end and DTMF begin, why do we need to combine them both now?  It just increases indentation and complexity.

I will review this. In the first version of the code I had much more code in common for BEGIN and END frames. It's not much now so I guess I can break out and duplicate code.

> On 2011-02-07 11:38:55, David Vossel wrote:
> > /team/oej/pinesounds-dtmf-feature-delay-1.4/res/res_features.c, lines 2394-2400
> > <https://reviewboard.asterisk.org/r/1092/diff/1/?file=15457#file15457line2394>
> >
> >     Could there ever be a scenario where multiple DTMF begins are received before the DTMF ends come through?  If so, the assumption the use of this variable makes may not hold true.

The RTP code does not allow that. If we get a BEGIN during incoming DTMF, it will send an END frame first to the bridge. I don't know how other channels handle it, but I assumed that was the "norm".

- Olle E

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

On 2011-02-03 02:55:01, Olle E Johansson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1092/
> -----------------------------------------------------------
> 
> (Updated 2011-02-03 02:55:01)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> When a channel listens for DTMF in the core bridge, the outbound DTMF is not sent until we have received DTMF_END. For a long DTMF, this is a disaster. We send 4 seconds of DTMF to Asterisk, asterisk starts sending after and the call is interrupted for a total of 8 secs.
> 
> With this change, the DTMF_BEGIN frame is inspected and checked. If it matches a feature code, we wait for DTMF_END and activate the feature as before. If it doesn't match a feature, the frame is forwarded along with the DTMF_END without delay. By doing it this way, DTMF is not delayed.
> 
> I've also changed two DTMF relay events to actually use the same duration as the incoming DTMF.
> 
> I have no experience of messing around with features.c and bridges, so please review this carefully. It is proven to work both in a 1.6.0 version and 1.4.
> 
> (I will remove the red code... Hard to miss)
> 
> 
> This addresses bug 18738.
>     https://issues.asterisk.org/view.php?id=18738
> 
> 
> Diffs
> -----
> 
>   /team/oej/pinesounds-dtmf-feature-delay-1.4/res/res_features.c 305981 
> 
> Diff: https://reviewboard.asterisk.org/r/1092/diff
> 
> 
> Testing
> -------
> 
> Testing done with multiple phones in 1.6.0 and 1.4 versions of this patch.
> 
> 
> Thanks,
> 
> Olle E
> 
>

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