[Asterisk-code-review] bridge softmix: Forward TEXT frames (asterisk[13])
George Joseph
asteriskteam at digium.com
Mon Oct 9 20:17:11 CDT 2017
George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/6614 )
Change subject: bridge_softmix: Forward TEXT frames
......................................................................
Patch Set 5:
(10 comments)
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@2234
PS5, Line 2234: data->msg = ast_msg_data_dup(msg);
: if (!data->msg) {
: return NULL;
: }
> data gets leaked in the off nominal path.
fixed
https://gerrit.asterisk.org/#/c/6614/5/channels/chan_pjsip.c@2319
PS5, Line 2319:
> Stray/extra blank line.
fixed
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 */
> }
Done. Kinda.
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.
New functionality that may need to be debugged later. I'll surround it with a DEBUG_ATLEAST but I don't want to remove it.
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?
See above.
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.
I disagree. TEXT_DATA doesn't necessarily imply that it was the result of a SIP MESSAGE or that it should be delivered by SIP MESSAGE. It's just an alternate structure. The channel driver should decide what to do with it.
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
Not removing
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
Not removing
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.
Ok, I'll make this a debug message. :)
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);
why? the "if" is clearer and results in no more code or effort. Hey, at least I didn't do the assignment in the if. :)
--
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: Tue, 10 Oct 2017 01:17:11 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171009/5beb3553/attachment.html>
More information about the asterisk-code-review
mailing list