----------------------------------------------------------- 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
