[asterisk-dev] [Code Review] 4477: app_confbridge: file playback blocks dtmf

[rmudgett]

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



branches/11/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/4477/#comment25208>

    It seems like this function could be written in terms of somthing like action_playback_and_continue() where the cur_dtmf passed in is "".



branches/11/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/4477/#comment25199>

    Space after commas.



branches/11/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/4477/#comment25209>

    This function has the potential to allow a user to blow the stack because of the recursion.
    
    Say *2 toggles locking the conference.  If you repeatedly lock/unlock the conference and not allow the prompt playback to complete you recurse another level.  Eventually you can blow the stack.



branches/11/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/4477/#comment25205>

    When this function is called, the user may or may not be in the softmix_bridge.
    
    I've looked at most menu actions and this seems to be OK to do.



branches/11/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/4477/#comment25204>

    Interrupting this sequence with DTMF is going to be hilarious.
    
    "There are" 45 "others in the conference"
    
    Interrupt "There are" with a DTMF menu request and the first part is interrupted and the remaining in the series will resume playing after the menu action.



branches/11/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/4477/#comment25203>

    This function appears to be called by post join actions and join_conference_bridge().  These calls happen before the softmix_bridge is actually joined.  Allowing execution of menu options here is likely to cause problems.
    
    I've looked at most menu actions and this seems to be OK to do.



branches/11/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/4477/#comment25200>

    Long line wrap.  Really should continue lines indented one tab stop.
    
    Horizontal space is precious while vertical space is cheap.



branches/11/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/4477/#comment25202>

    We haven't even joined the softmix_bridge yet.  Allowing execution of menu options here is likely to cause problems.
    
    I've looked at most menu actions and this seems to be OK to do.



branches/11/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/4477/#comment25207>

    Why isn't this using conf_playback()?



branches/11/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/4477/#comment25206>

    This changes the playback_and_continue menu action behaviour.
    
    Before
    * - starts playback and continue to allow DTMF collection
        The user can then press 1 to get *1 action to trigger.
    *1 - submenu action
    
    After
    * - starts playback and continue
    The user now must restart DTMF entry to match *1.
    


- rmudgett


On March 11, 2015, 12:05 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4477/
> -----------------------------------------------------------
> 
> (Updated March 11, 2015, 12:05 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24864
>     https://issues.asterisk.org/jira/browse/ASTERISK-24864
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   branches/11/apps/confbridge/include/confbridge.h 432763 
>   branches/11/apps/confbridge/conf_config_parser.c 432763 
>   branches/11/apps/app_confbridge.c 432763 
> 
> Diff: https://reviewboard.asterisk.org/r/4477/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/20150311/481da354/attachment-0001.html>