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

[David Vossel]

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


I understand what you are doing, and this makes sense to me.  Most of my comments are coding style related, although I did have one functional concern about the use of the sendingdtmf variable.  While the coding style comments may seem like I'm just nitpicking, I believe they are necessary in reducing complexity for everyone maintaining the project.


/team/oej/pinesounds-dtmf-feature-delay-1.4/res/res_features.c
<https://reviewboard.asterisk.org/r/1092/#comment6553>

    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.



/team/oej/pinesounds-dtmf-feature-delay-1.4/res/res_features.c
<https://reviewboard.asterisk.org/r/1092/#comment6554>

    why was this check added?



/team/oej/pinesounds-dtmf-feature-delay-1.4/res/res_features.c
<https://reviewboard.asterisk.org/r/1092/#comment6555>

    It would also be useful to create a new return value indicating that the feature was just being looked for and was found.



/team/oej/pinesounds-dtmf-feature-delay-1.4/res/res_features.c
<https://reviewboard.asterisk.org/r/1092/#comment6558>

    I don't understand this check.



/team/oej/pinesounds-dtmf-feature-delay-1.4/res/res_features.c
<https://reviewboard.asterisk.org/r/1092/#comment6556>

    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.



/team/oej/pinesounds-dtmf-feature-delay-1.4/res/res_features.c
<https://reviewboard.asterisk.org/r/1092/#comment6557>

    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.


- David


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/20110207/a49119fd/attachment-0001.htm>