Re: [asterisk-dev] [Code Review] 4510: app_confbridge (13): file playback blocks dtmf
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4510/ --- (Updated March 26, 2015, 12:04 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 433446 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 433344 branches/13/include/asterisk/bridge_channel.h 433344 branches/13/apps/app_confbridge.c 433344 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
Re: [asterisk-dev] [Code Review] 4510: app_confbridge (13): file playback blocks dtmf
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4510/#review14801 --- branches/13/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4510/#comment25397 No parameters list. - Jonathan Rose On March 19, 2015, 4:59 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4510/ --- (Updated March 19, 2015, 4:59 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 433195 branches/13/include/asterisk/bridge_channel.h 433195 branches/13/apps/app_confbridge.c 433195 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
Re: [asterisk-dev] [Code Review] 4510: app_confbridge (13): file playback blocks dtmf
On March 24, 2015, 12:01 p.m., rmudgett wrote: branches/13/main/bridge_channel.c, line 1575 https://reviewboard.asterisk.org/r/4510/diff/2/?file=72687#file72687line1575 Just if (digit) is sufficient. If anything, you should assert: ast_assert(0 = digit) because passing in a negative value is invalid. Oh yeah whoops. I had it at that, but then changed it to 0 thinking I was going to need to pass in a -1 for submenu functionality, but we decided to drop that for now. I'll change it back. - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4510/#review14798 --- On March 19, 2015, 4:59 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4510/ --- (Updated March 19, 2015, 4:59 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 433195 branches/13/include/asterisk/bridge_channel.h 433195 branches/13/apps/app_confbridge.c 433195 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
Re: [asterisk-dev] [Code Review] 4510: app_confbridge (13): file playback blocks dtmf
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4510/#review14798 --- branches/13/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4510/#comment25391 Missing param doxygen. I think this is also needed in the v11 patch. branches/13/main/bridge_channel.c https://reviewboard.asterisk.org/r/4510/#comment25392 Remove this comment as it was for the assert that was removed and it isn't true anymore. branches/13/main/bridge_channel.c https://reviewboard.asterisk.org/r/4510/#comment25393 Just if (digit) is sufficient. If anything, you should assert: ast_assert(0 = digit) because passing in a negative value is invalid. - rmudgett On March 19, 2015, 4:59 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4510/ --- (Updated March 19, 2015, 4:59 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 433195 branches/13/include/asterisk/bridge_channel.h 433195 branches/13/apps/app_confbridge.c 433195 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
Re: [asterisk-dev] [Code Review] 4510: app_confbridge (13): file playback blocks dtmf
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4510/ --- (Updated March 24, 2015, 3:30 p.m.) Review request for Asterisk Developers. Changes --- Addressed review feedback. 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 (updated) - branches/13/main/bridge_channel.c 433344 branches/13/include/asterisk/bridge_channel.h 433344 branches/13/apps/app_confbridge.c 433344 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
Re: [asterisk-dev] [Code Review] 4510: app_confbridge (13): file playback blocks dtmf
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4510/#review14816 --- Ship it! Ship It! - rmudgett On March 24, 2015, 3:30 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4510/ --- (Updated March 24, 2015, 3:30 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 433344 branches/13/include/asterisk/bridge_channel.h 433344 branches/13/apps/app_confbridge.c 433344 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
Re: [asterisk-dev] [Code Review] 4510: app_confbridge (13): file playback blocks dtmf
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4510/ --- (Updated March 19, 2015, 4:59 p.m.) Review request for Asterisk Developers. Changes --- Addressed review findings. Also decided to leave the playback_and_continue code alone for now as it currently handles what it needs to do on its own. 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 (updated) - branches/13/main/bridge_channel.c 433195 branches/13/include/asterisk/bridge_channel.h 433195 branches/13/apps/app_confbridge.c 433195 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
Re: [asterisk-dev] [Code Review] 4510: app_confbridge (13): file playback blocks dtmf
---
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 --
Re: [asterisk-dev] [Code Review] 4510: app_confbridge (13): file playback blocks dtmf
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4510/#review14729 --- branches/13/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4510/#comment25296 This could use a bit more documentation, specifically, 1) When should I use this instead of the similarly-named play_sound_file() 2) Why are a bridge_channel and a channel passed in? - Mark Michelson On March 17, 2015, 6 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, 6 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
[asterisk-dev] [Code Review] 4510: app_confbridge (13): file playback blocks dtmf
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4510/ --- 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
