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

[Mark Michelson]

Mark Michelson has posted comments on this change.

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


Patch Set 1:

(4 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
It's really silly to do this. Each doxygen line will be along the lines of

/*! The conference */
/*! The lock that protects the condition */
/*! The condition that is waited on */
/*! Whether the channel is hung up */

The purpose of each member is self evident, and doxygen serves no purpose other than to essentially rewrite the declaration as a sentence.


Line 1397: 	ast_autoservice_start(conference->playback_chan);
> Is starting and stopping autoservice on the announcer channel not trading o
Starting and stopping autoservice is much lighter weight than joining or leaving a bridge (especially as a departable channel).

Starting autoservice involves allocating an autoservice structure and adding it to a list. Stopping autoservice is the reverse of that.

Joining a bridge involves spinning up a departable thread, allocating a bridge channel, adding the bridge channel to the bridge's list of participants, setting the BRIDGEPEER channel variable on every channel in the bridge (which involves locking the bridge and every channel in it), checking the bridge for possible reconfiguration, alerting the mixing technology that there is a new member, and sending a Stasis event for the channel joining the bridge. And that's just from my memory. There may be other things that happen too. And the depart involves all of the same things happening again.

It may be slightly lighter weight to try to have a taskprocessor listener service the channel when there are no tasks, but I find that it's a lot easier and less error-prone to use existing APIs than try to mimic them here.


Line 1627: struct playback_task_data {
> Doxygen
See my previous statement on this.


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 
I initially only added the playback_queue member. It was later that I realized the playback_lock wasn't being used any longer, so I removed it. I never thought to move the playback_queue up to where the playback_lock previously was.


-- 
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: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes