[Asterisk-code-review] Implement deaf mode for confbridge (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: Implement deaf mode for confbridge
......................................................................


Patch Set 4: Code-Review-1

(7 comments)

https://gerrit.asterisk.org/#/c/2933/4//COMMIT_MSG
Commit Message:

Line 7: Implement deaf mode for confbridge
app_confbridge: Implement deaf mode.

Deaf mode makes a channel able to speak to its bridged
peers but not hear its peers.  ConfBridge deaf mode support has new configuration options, CLI commands, AMI actions,
and DTMF menu actions.

* Added deaf mode support to the bridge framework core.


https://gerrit.asterisk.org/#/c/2933/4/CHANGES
File CHANGES:

PS4, Line 77:  * Added support for deaf participants with CLI commands, manager actions
            :    and ConfBridge DTMF actions to toggle the deaf state.
Please define what deaf means.  When I first saw this patch I thought it had something to do with the hearing impaired which didn't make sense.


https://gerrit.asterisk.org/#/c/2933/4/apps/app_confbridge.c
File apps/app_confbridge.c:

PS4, Line 481: 	case CONF_SOUND_DEAFENED:
             : 		return S_OR(custom_sounds->deafened, "conf-deafened");
             : 	case CONF_SOUND_UNDEAFENED:
             : 		return S_OR(custom_sounds->undeafened, "conf-undeafened");
> Where are these sound files? Currently they won't exist in a user's asteris
@Kevin: Looking at the code, non-existent files should just cause a warning message when they are played.  I don't think there are any other effects.


https://gerrit.asterisk.org/#/c/2933/4/bridges/bridge_softmix.c
File bridges/bridge_softmix.c:

PS4, Line 1037: 			if (bridge_channel->features && bridge_channel->features->deaf) {
              : 				/* For deaf channels post a null frame */
              : 				sc->write_frame.frametype = AST_FRAME_NULL;
              : 			} else {
              : 				/* Make SLINEAR write frame from local buffer */
              : 				sc->write_frame.frametype = AST_FRAME_VOICE;
              : 				ao2_t_replace(sc->write_frame.subclass.format, cur_slin,
              : 					"Replace softmix channel slin format");
              : 				sc->write_frame.datalen = softmix_datalen;
              : 				sc->write_frame.samples = softmix_samples;
              : 				memcpy(sc->final_buf, buf, softmix_datalen);
              : 
              : 				/* process the softmix channel's new write audio */
              : 				softmix_process_write_audio(&trans_helper, ast_channel_rawwriteformat(bridge_channel->chan), sc);
              : 			}
As the code is currently, you generate null frames that get discarded immediately in ast_bridge_channel_queue_frame() and no silence frame even gets to the channel.

Revert all changes in this file.  Deaf mode applies to all bridge technologies and won't mess up softmix in particular when you just replace frames with silence in ast_bridge_channel_queue_frame().


https://gerrit.asterisk.org/#/c/2933/4/include/asterisk/bridge_features.h
File include/asterisk/bridge_features.h:

Line 279: 	unsigned int deaf:1;
This flag needs to be copied in bridge.c:ast_bridge_features_merge() just like mute.


https://gerrit.asterisk.org/#/c/2933/4/main/bridge_channel.c
File main/bridge_channel.c:

PS4, Line 975: 	if (fr->frametype == AST_FRAME_VOICE &&
             : 	    (bridge_channel->features && bridge_channel->features->deaf)) {
Unnecessary parentheses

if (fr->frametype == AST_FRAME_VOICE
    && bridge_channel->features
    && bridge_channel->features->deaf) {
}


PS4, Line 975: 	if (fr->frametype == AST_FRAME_VOICE &&
             : 	    (bridge_channel->features && bridge_channel->features->deaf)) {
             : 		short buf[fr->samples];
             : 		struct ast_frame sframe = {
             : 			.frametype = AST_FRAME_VOICE,
             : 			.subclass.format = ast_format_slin,
             : 			.data.ptr = buf,
             : 			.samples = fr->samples,
             : 			.datalen = sizeof(buf),
             : 		};
             : 		memset(buf, 0, sizeof(buf));
             : 		dup = ast_frdup(&sframe);
             : 	} else {
The number of samples for the slin frame is not right when the incoming frame's sample rate is not 8kHz.

samples = (fr->samples * 8000) / ast_format_get_sample_rate(fr->subclass.format)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2628bdda080fd2b1e914212de50dd26de9d3e96
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Timo Teräs <timo.teras at iki.fi>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Timo Teräs <timo.teras at iki.fi>
Gerrit-HasComments: Yes