[Asterisk-code-review] ConfBridge: Make some announcements asynchronous. (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: ConfBridge: Make some announcements asynchronous.
......................................................................


Patch Set 6: Code-Review-1

(11 comments)

https://gerrit.asterisk.org/#/c/3504/6/apps/app_confbridge.c
File apps/app_confbridge.c:

PS6, Line 1709: 	if (!ast_strlen_zero(filename) && !sound_file_exists(filename)) {
              : 		return 0;
              : 	}
This can be improved to skip saying bad numbers and empty filenames or non-existent files.

if (ast_strlen_zero()) {
   if (say_number < 0) {
      return 0;
   }
} else if (!sound_file_exists(fname)) {
   return 0;
}


PS6, Line 1874: 	aptd->initiator = initiator ? ast_channel_ref(initiator) : NULL;
              : 	if (aptd->initiator) {
This could be:

aptd->initiator = initiator;
if (initiator) {
   ast_channel_ref(initiator);
   ...
}


Line 1907: 	async_datastore = ast_channel_datastore_find(initiator, &async_datastore_info, NULL);
Lock/unlock initiator around ast_channel_datastore_find().


PS6, Line 1952: 	if (!ast_strlen_zero(filename) && !sound_file_exists(filename)) {
              : 		return 0;
              : 	}
This can be improved to skip saying bad numbers and empty filenames or non-existent files.

if (ast_strlen_zero()) {
   if (say_number < 0) {
      return 0;
   }
} else if (!sound_file_exists(fname)) {
   return 0;
}


Line 1982: int async_play_sound_ready(struct ast_channel *chan)
This should be a void return function.  The purpose of this function is to let the announcer channel start playing.  What could you do if it "fails" to awaken the announcer?


Line 1987: 	async_datastore = ast_channel_datastore_find(chan, &async_datastore_info, NULL);
Lock/unlock chan around ast_channel_datastore_find().


Line 2133: static int join_callback(struct ast_bridge_channel *bridge_channel, void *ignore)
The return value of a join/leave hook is useless to the bridging system since they can only be called once if ever.


Line 2278: 	ast_bridge_join_hook(&user.features, join_callback, NULL, NULL, AST_BRIDGE_HOOK_REMOVE_ON_PULL);
Add this hook just before joining the bridge.  This way if the hook fails to get added you can call async_play_sound_ready() to unblock the announcer channel.

The remove on pull flag is meaningless for join hooks.


Line 2469: 		play_sound_helper(conference, sound_to_play, 0);
Pre-existing: This should be play_sound_file()


Line 2473: 		async_play_sound_helper(conference, sound_to_play, 0, user->chan);
This be async_play_sound_file()


https://gerrit.asterisk.org/#/c/3504/6/apps/confbridge/include/confbridge.h
File apps/confbridge/include/confbridge.h:

Line 406: int async_play_sound_ready(struct ast_channel *chan);
Doxygen?

Wake up the announcer channel to play to the bridge because the specific channel is ready to also hear it from the bridge.


-- 
To view, visit https://gerrit.asterisk.org/3504
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie486bb3de1646d50894489030326a423e594ab0a
Gerrit-PatchSet: 6
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes