[Asterisk-code-review] ConfBridge: Rework announcer channel methodology (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: ConfBridge: Rework announcer channel methodology
......................................................................


Patch Set 1: Code-Review-1

(11 comments)

https://gerrit.asterisk.org/#/c/3503/1/apps/app_confbridge.c
File apps/app_confbridge.c:

Line 963: struct hangup_data
Doxygen the members


Line 1360: 	/* To make sure playback_chan has the same language of that profile */
s/of that/of the bridge/


Line 1368: 	conference->playback_queue = ast_taskprocessor_get(conference->name, TPS_REF_DEFAULT);
Taskprocessor names should be unique and conference->name may not be that unique in the system even though it is unique to confbridges.

Look at usage of ast_taskprocessor_build_name().

I suggest creating a name of the form:
ConfBridge/<conference->name>
This way the names are grouped in "core show taskprocessors" output.


Line 1397: 	ast_autoservice_start(conference->playback_chan);
Is starting and stopping autoservice on the announcer channel not trading one join/leave process for another?

Maybe the created taskprocesor should have a custom listener so when there are no tasks the thread services the announcer channel read queue.


Line 1627: struct playback_task_data {
Doxygen


PS1, Line 1630: 	int say_number;
              : 	ast_mutex_t lock;
              : 	ast_cond_t cond;
              : 	int playback_finished;
Reorder these members as there is likely padding added when you change from ints to ast_mutex_t.


PS1, Line 1653: 	/* Don't try to play if the playback channel has been hung up */
              : 	if (!ptd->conference->playback_chan) {
              : 		return 0;
              : 	}
This early exit has likely left the channel waiting forever for the file to play because the ptd->playback_finished never gets set and the waiter never woken up.


Line 1707: 			ast_log(LOG_WARNING, "Unable to play file '%s' to conference\n", filename);
Add conference name to msg.


Line 1709: 			ast_log(LOG_WARNING, "Unable to say number '%d' to conference\n", say_number);
Add conference name to msg.


https://gerrit.asterisk.org/#/c/3503/1/apps/confbridge/conf_chan_announce.c
File apps/confbridge/conf_chan_announce.c:

Line 168: 		ast_channel_unref(chan);
Delete.  Left over unref that wasn't removed.


https://gerrit.asterisk.org/#/c/3503/1/apps/confbridge/include/confbridge.h
File apps/confbridge/include/confbridge.h:

Line 230: 	struct ast_taskprocessor *playback_queue;
doxygen.  Also why put this pointer at the end instead of simply replacing the playback_lock?


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

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