[asterisk-dev] [Code Review] 4510: app_confbridge (13): file playback blocks dtmf

[rmudgett]

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


A lot of the v11 patch findings apply to this review patch as well.


branches/13/include/asterisk/bridge_channel.h
<https://reviewboard.asterisk.org/r/4510/#comment25320>

    since 13.3.0



branches/13/include/asterisk/bridge_channel.h
<https://reviewboard.asterisk.org/r/4510/#comment25319>

    This can only be called from within DTMF bridge hooks.



branches/13/include/asterisk/bridge_channel.h
<https://reviewboard.asterisk.org/r/4510/#comment25323>

    Need to add:
    void ast_bridge_channel_feature_digit_remove(struct ast_bridge_channel *bridge_channel, int remove);
    
    Where the remove parameter can be 0-n digits to remove from the digit collection buffer or -1 to clear the digit collection buffer.



branches/13/include/asterisk/bridge_channel.h
<https://reviewboard.asterisk.org/r/4510/#comment25321>

    Add the distinction of non-DTMF bridge hooks to the note.
    
    \note This is intended to be called by non-DTMF bridge hooks and the bridge channel thread.  Calling from a DTMF bridge hook can potentially cause unbounded recursion.
    



branches/13/main/bridge_channel.c
<https://reviewboard.asterisk.org/r/4510/#comment25326>

    This assertion is not true anymore and protection from DTMF hooks adding too many digits is needed.



branches/13/main/bridge_channel.c
<https://reviewboard.asterisk.org/r/4510/#comment25328>

    Pull this up out of the loop and change the if test to if (digit).  This way you don't need to set digit = -1 later.



branches/13/main/bridge_channel.c
<https://reviewboard.asterisk.org/r/4510/#comment25325>

    This should be pulled out into its own function as a refactoring improvement.
    
    I was initially thinking that the extracted function would be used by the ConfBridge playback_and_continue DTMF hook to wait for digits if a digit did not interrupt playback but it does not.  The playback_and_continue just exits the menu instead.



branches/13/main/bridge_channel.c
<https://reviewboard.asterisk.org/r/4510/#comment25324>

    You forgot about the playback_and_continue ConfBridge menu action that should not clear the collected digits before calling.  That DTMF hook needs a flag to say not to clear the collected digits buffer before calling.
    
    ast_bridge_dtmf_hook() needs to be able to pass this do not clear digit collection buffer before calling hook flag.



branches/13/main/bridge_channel.c
<https://reviewboard.asterisk.org/r/4510/#comment25327>

    If true we need to return and empty the digit collection buffer.



branches/13/main/bridge_channel.c
<https://reviewboard.asterisk.org/r/4510/#comment25329>

    This can be simplified to:
    dfmf_len = strlen()
    if (!dtmf_len) { return }


- rmudgett


On March 17, 2015, 1 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4510/
> -----------------------------------------------------------
> 
> (Updated March 17, 2015, 1 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24864
>     https://issues.asterisk.org/jira/browse/ASTERISK-24864
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This is the Asterisk 13 version of the following: https://reviewboard.asterisk.org/r/4477/
> 
> Attempting to execute DTMF in a confbridge while file playback (prompt, announcement, etc) is occurring is not allowed. You have to wait until the sound file has completed before entering DTMF. This patch fixes it so that app_confbridge now monitors for dtmf key presses during file playback. If a key is pressed playback stops and it executes the matched menu option. Unlike the Asterisk 11 patch this version does not re-queue the dtmf frame, but instead uses an already available function that monitors for dtmf presses during playback.
> 
> 
> Diffs
> -----
> 
>   branches/13/main/bridge_channel.c 433004 
>   branches/13/include/asterisk/bridge_channel.h 433004 
>   branches/13/apps/confbridge/include/confbridge.h 433004 
>   branches/13/apps/confbridge/conf_state_multi_marked.c 433004 
>   branches/13/apps/app_confbridge.c 433004 
> 
> Diff: https://reviewboard.asterisk.org/r/4510/diff/
> 
> 
> Testing
> -------
> 
> Manual testing done. Setup a basic conference bridge that allowed both regular and admin users to enter. Ran through various menu options to make sure the sound file playback would stop (no longer have to wait) and a new option was executed when appropriate. Also ran the app_confbridge testsuite tests to make sure they still passed.
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150318/049aa456/attachment-0001.html>