[Asterisk-code-review] bridge softmix: Forward TEXT frames (asterisk[13])
Richard Mudgett
asteriskteam at digium.com
Wed Oct 11 13:22:43 CDT 2017
Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/6614 )
Change subject: bridge_softmix: Forward TEXT frames
......................................................................
Patch Set 7: Code-Review-1
(20 comments)
* app_sendtext.c needs to be updated to support the new tech callback. Otherwise we are breaking it for chan_pjsip.
* Local channels/core_local/core_unreal need to support the new frame type or local channels in confbridges going to pjsip channels will have unsatisfactory results.
* CLI "core show channeltype" in channel.c needs to be updated to deal with the new tech callback.
* ast_write() should be updated to handle the new frame type.
Probably the best way to handle these cases is to have chan_pjsip still implement a send_text callback and the places where it matters prefer using the new callback.
https://gerrit.asterisk.org/#/c/6614/7//COMMIT_MSG
Commit Message:
https://gerrit.asterisk.org/#/c/6614/7//COMMIT_MSG@19
PS7, Line 19: will send an MESSAGE when it gets a TEXT frame. On a normal
s/an/a/
https://gerrit.asterisk.org/#/c/6614/7//COMMIT_MSG@21
PS7, Line 21: correctly. bridge_softmix was not though so messages weren't
s/though //
https://gerrit.asterisk.org/#/c/6614/7//COMMIT_MSG@33
PS7, Line 33: setting the new AST_CHAN_TP_SEND_TEXT_DATA flag in its tech
The master version of this patch should remove the need for the AST_CHAN_TP_SEND_TEXT_DATA flag. The presence or absence of the tech callback is sufficient going forward.
This could be done in a follow on patch for master.
https://gerrit.asterisk.org/#/c/6614/7//COMMIT_MSG@50
PS7, Line 50: * A new function "ast_sendtext_data" was added to channel which
s/channel/channel.c/
https://gerrit.asterisk.org/#/c/6614/7//COMMIT_MSG@55
PS7, Line 55: * bridge_channel now calls ast_sendtext_data for TEXT_DATA frame
s/bridge_channel/bridge_channel.c/
https://gerrit.asterisk.org/#/c/6614/7/CHANGES
File CHANGES:
https://gerrit.asterisk.org/#/c/6614/7/CHANGES@25
PS7, Line 25: other participants in the call.
de blob
https://gerrit.asterisk.org/#/c/6614/7/bridges/bridge_softmix.c
File bridges/bridge_softmix.c:
https://gerrit.asterisk.org/#/c/6614/7/bridges/bridge_softmix.c@685
PS7, Line 685: ast_debug(1, "Received %s frame from '%s:%s': %s\n", frame_type,
Use ast_log(LOG_DEBUG,... instead. ast_debug() includes the DEBUG_ATLEAST() test.
https://gerrit.asterisk.org/#/c/6614/7/bridges/bridge_softmix.c@686
PS7, Line 686: ast_msg_data_get_from(msg), ast_channel_name(bridge_channel->chan),
: ast_msg_data_get_body(msg));
The debug message parameters assumes that the frame type is always TEXT_DATA when it could also be simply TEXT.
https://gerrit.asterisk.org/#/c/6614/7/channels/chan_pjsip.c
File channels/chan_pjsip.c:
https://gerrit.asterisk.org/#/c/6614/7/channels/chan_pjsip.c@114
PS7, Line 114: .send_text_data = chan_pjsip_sendtext_data,
Should keep the .send_text callback for other parts of the system that check to see if the channel driver supports text. (e.g., app_sendtext)
https://gerrit.asterisk.org/#/c/6614/7/include/asterisk/message.h
File include/asterisk/message.h:
https://gerrit.asterisk.org/#/c/6614/7/include/asterisk/message.h@441
PS7, Line 441: * \param from The display name of the sender
: * \param to The display name of destination
: * \param body The message body
From, to, and body can be NULL.
https://gerrit.asterisk.org/#/c/6614/7/main/bridge_channel.c
File main/bridge_channel.c:
https://gerrit.asterisk.org/#/c/6614/7/main/bridge_channel.c@1004
PS7, Line 1004: ast_debug(1, "Queuing TEXT frame to '%s': %*.s\n", ast_channel_name(bridge_channel->chan),
Use ast_log(LOG_DEBUG,... instead. ast_debug() includes the DEBUG_ATLEAST() test.
https://gerrit.asterisk.org/#/c/6614/7/main/bridge_channel.c@1008
PS7, Line 1008: ast_debug(1, "Sending TEXT_DATA frame from '%s' to '%s:%s': %s\n",
same
https://gerrit.asterisk.org/#/c/6614/7/main/bridge_channel.c@2366
PS7, Line 2366: if (DEBUG_ATLEAST(1)) {
: ast_debug(1, "Sending TEXT_DATA frame from '%s' to '%s:%s': %s\n",
: ast_msg_data_get_from((struct ast_msg_data *)fr->data.ptr),
: ast_msg_data_get_to((struct ast_msg_data *)fr->data.ptr),
: ast_channel_name(bridge_channel->chan),
: ast_msg_data_get_body((struct ast_msg_data *)fr->data.ptr));
: }
DEBUG_ATLEAST isn't needed as ast_debug(1... already does it.
https://gerrit.asterisk.org/#/c/6614/7/main/bridge_channel.c@2360
PS7, Line 2360: case AST_FRAME_TEXT:
: ast_debug(1, "Sending TEXT frame to '%s': %*.s\n",
: ast_channel_name(bridge_channel->chan), fr->datalen, (char *)fr->data.ptr);
: ast_sendtext(bridge_channel->chan, fr->data.ptr);
: break;
: case AST_FRAME_TEXT_DATA:
: if (DEBUG_ATLEAST(1)) {
: ast_debug(1, "Sending TEXT_DATA frame from '%s' to '%s:%s': %s\n",
: ast_msg_data_get_from((struct ast_msg_data *)fr->data.ptr),
: ast_msg_data_get_to((struct ast_msg_data *)fr->data.ptr),
: ast_channel_name(bridge_channel->chan),
: ast_msg_data_get_body((struct ast_msg_data *)fr->data.ptr));
: }
: ast_sendtext_data(bridge_channel->chan, (struct ast_msg_data *)fr->data.ptr);
: break;
Why not just push this code down into ast_write()?
https://gerrit.asterisk.org/#/c/6614/7/main/channel.c
File main/channel.c:
https://gerrit.asterisk.org/#/c/6614/7/main/channel.c@4901
PS7, Line 4901: } else {
: f.frametype = AST_FRAME_TEXT;
: f.datalen = body_len;
Move body_len definition and initialization to here since this is the only place where it is used.
https://gerrit.asterisk.org/#/c/6614/7/main/channel.c@4934
PS7, Line 4934: msg = ast_msg_data_create(AST_MSG_DATA_SOURCE_TYPE_UNKNOWN, "", "", text);
You can pass NULL instead of "" for the from and to parameters.
https://gerrit.asterisk.org/#/c/6614/7/main/channel.c@4938
PS7, Line 4938: return ast_sendtext_data(chan, msg);
msg is leaked
https://gerrit.asterisk.org/#/c/6614/7/main/message.c
File main/message.c:
https://gerrit.asterisk.org/#/c/6614/7/main/message.c@1361
PS7, Line 1361: enum ast_msg_data_source_type source;
To avoid possible padding, move source to between body_offset and buf. An enum base type could be char, short, or int depending on compiler options and the number of enum values defined or enum values assigned.
https://gerrit.asterisk.org/#/c/6614/7/main/message.c@1407
PS7, Line 1407: if (!body) {
Add a comment that empty bodies are specifically allowed:
/*
* Zero length messages are allowed for SIP by
* RFC3428 (See the end of section 9).
*/
https://gerrit.asterisk.org/#/c/6614/7/main/message.c@1418
PS7, Line 1418: dest = ast_calloc(1, len);
calloc isn't necessary since you manually set all members anyway.
--
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: 7
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: Wed, 11 Oct 2017 18:22:43 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171011/7a5a521b/attachment.html>
More information about the asterisk-code-review
mailing list