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

[Terry Wilson]

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


Since I was working on an issue in the same area that would benefit from this patch (the Sonus DTMF issue), I went ahead and cleaned up the issues as I found them as well. I also addressed David's comments and added the fix for the Sonus DTMF issue which was related.

I posted the modified changes at https://reviewboard.asterisk.org/r/1125/

Olle, if the fixes there meet with your approval, feel free to commit them (or, I can if you would rather).


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

    Did you mean to change this from tmpfeature to &builtin_features[x]? It looks like this might be a copy/paste issue from the similar call above.



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

    I would take out the 'feature' argument and just pass NULL to the feature_interpret_helper here since we're just checking whether a feature code exists or not. feature_interpret_helper behaves differently if 'feature' is passed (not that the single call to ast_feature_check actually passes anything there). It just seems like it might be a little cleaner that way.
    
    Also, being a static function, it probably shouldn't be named ast_*



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

    The last argument of ast_dtmf_stream is the number of milliseconds to wait between sending each digit. I don't think it affects the duration of the digit, so it doesn't seem right to pass f->len there.
    
    If it is correct, then f can be NULL during both of these calls, so accessing f->len would segfault in that case.



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

    If we have already matched the first digit in a multi-digit feature code, this will end up sending only the last digit of the code, because ast_feature_check will return FEATURE_RETURN_STOREDIGITS on the first char and FEATURE_RETURN_PASSDIGITS on the second.
    
    In the case where there is more than one digit, we will need to fall through to stream the digits if they happen to match with feature_interpret.


- Terry


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/20110223/77fbbda7/attachment-0001.htm>