[Asterisk-code-review] bridge softmix: Forward TEXT frames (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/6614 )

Change subject: bridge_softmix:  Forward TEXT frames
......................................................................


Patch Set 5: Code-Review-1

(10 comments)

app_dial/app_queue don't pass AST_FRAME_TEXT_DATA before bridge while they will pass AST_FRAME_TEXT.

https://gerrit.asterisk.org/#/c/6614/5/channels/chan_pjsip.c
File channels/chan_pjsip.c:

https://gerrit.asterisk.org/#/c/6614/5/channels/chan_pjsip.c@2319
PS5, Line 2319: 
Stray/extra blank line.


https://gerrit.asterisk.org/#/c/6614/5/include/asterisk/message.h
File include/asterisk/message.h:

https://gerrit.asterisk.org/#/c/6614/5/include/asterisk/message.h@419
PS5, Line 419: 	char from[AST_MSG_DATA_MAX_FROMTO];
             : 	char to[AST_MSG_DATA_MAX_FROMTO];
Maybe should define the from/to members as offsets into the body[] where the string contents for the from/to start.  That would eliminate the arbitrary 256 max length in favor of a dynamic length that could be larger.  It would also reduce the amount of unnecessary data copied when a frame is duped because of empty or short from/to strings.

struct ast_msg_data {
   unsigned int from; /* Offset of from string from body[] */
   unsigned int to; /* Offset of to string from body[] */
   char body[0];  /* from/to strings stored after body string */
}


https://gerrit.asterisk.org/#/c/6614/5/main/bridge_channel.c
File main/bridge_channel.c:

https://gerrit.asterisk.org/#/c/6614/5/main/bridge_channel.c@1002
PS5, Line 1002: 	if (fr->frametype == AST_FRAME_TEXT) {
              : 		ast_debug(1, "Queuing TEXT frame to '%s': %*.s\n", ast_channel_name(bridge_channel->chan),
              : 			fr->datalen, (char *)fr->data.ptr);
              : 	} else if (fr->frametype == AST_FRAME_TEXT_DATA) {
              : 		struct ast_msg_data *msg = fr->data.ptr;
              : 		ast_debug(1, "Sending TEXT_DATA frame from '%s' to '%s:%s': %s\n",
              : 			msg->from, msg->to, ast_channel_name(bridge_channel->chan), msg->body);
              : 	}
This needs to be removed.  This is development debug code only.  No other frame types are treated like this.


https://gerrit.asterisk.org/#/c/6614/5/main/bridge_channel.c@2358
PS5, Line 2358: 		ast_debug(1, "Sending TEXT frame to '%s': %*.s\n",
Why treat TEXT and TEXT_DATA frames different than any other frames with debug messages?


https://gerrit.asterisk.org/#/c/6614/5/main/bridge_channel.c@2365
PS5, Line 2365: 			ast_debug(1, "Sending TEXT_DATA frame from '%s' to '%s:%s': %s\n",
same


https://gerrit.asterisk.org/#/c/6614/5/main/channel.c
File main/channel.c:

https://gerrit.asterisk.org/#/c/6614/5/main/channel.c@4892
PS5, Line 4892: 	if (ast_channel_tech(chan)->write_text && (ast_format_cap_has_type(ast_channel_nativeformats(chan), AST_MEDIA_TYPE_TEXT))) {
              : 		struct ast_frame f;
              : 
              : 		memset(&f, 0, sizeof(f));
              : 		f.src = "DIALPLAN";
              : 		f.subclass.format = ast_format_t140;
              : 
              : 		if (ast_channel_tech(chan)->properties & AST_CHAN_TP_SEND_TEXT_DATA) {
              : 			f.frametype = AST_FRAME_TEXT_DATA;
              : 			f.datalen = sizeof(*msg) + strlen(msg->body) + 1;
              : 			f.data.ptr = msg;
              : 		} else {
              : 			f.frametype = AST_FRAME_TEXT;
              : 			f.datalen = strlen(msg->body);
              : 			f.data.ptr = ast_strdup(msg->body);
              : 			f.mallocd = AST_MALLOCD_DATA;
              : 		}
              : 
              : 		if (f.data.ptr) {
              : 			res = ast_channel_tech(chan)->write_text(chan, &f);
              : 			ast_frfree(&f);
              : 		}
This is only for sending T.140 text data.  So we should always convert an AST_FRAME_TEXT_DATA to an AST_FRAME_TEXT to send over RTP.


https://gerrit.asterisk.org/#/c/6614/5/main/channel.c@4916
PS5, Line 4916: 		ast_debug(1, "Sending TEXT_DATA from '%s' to %s:%s %s\n",
Development debugging msg


https://gerrit.asterisk.org/#/c/6614/5/main/channel.c@4920
PS5, Line 4920: 		ast_debug(1, "Sending TEXT to %s: %s\n", ast_channel_name(chan), msg->body);
Development debugging msg


https://gerrit.asterisk.org/#/c/6614/5/main/channel.c@4923
PS5, Line 4923: 		ast_log(LOG_WARNING, "Unable to send text message on channel '%s'\n",
I don't think this new warning is a good idea.  Seems like this will spam the log when a channel doesn't support text is in a confbridge exchanging text.


https://gerrit.asterisk.org/#/c/6614/5/res/res_pjsip_messaging.c
File res/res_pjsip_messaging.c:

https://gerrit.asterisk.org/#/c/6614/5/res/res_pjsip_messaging.c@793
PS5, Line 793: 		if (caller->id.name.valid && !ast_strlen_zero(caller->id.name.str)) {
             : 			ast_copy_string(msg->from, caller->id.name.str, AST_MSG_DATA_MAX_FROMTO);
             : 		}
How about using S_COR()

ast_copy_string(msg->from, S_COR(caller->id.name.valid, caller->id.name.str, ""), AST_MSG_DATA_MAX_FROMTO);



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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: Idacf5900bfd5f22ab8cd235aa56dfad090d18489
Gerrit-Change-Number: 6614
Gerrit-PatchSet: 5
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Comment-Date: Mon, 09 Oct 2017 21:49:37 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171009/d95988b7/attachment-0001.html>