[Asterisk-code-review] bridge native rtp: Keep references to rtp instances on hook... (asterisk[13])
Richard Mudgett
asteriskteam at digium.com
Tue Jun 20 13:42:43 CDT 2017
Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/5854 )
Change subject: bridge_native_rtp: Keep references to rtp instances on hook data
......................................................................
Patch Set 3: Code-Review-1
(12 comments)
Ugh. This patch has gotten much larger than I expected.
The bridge->tech_pvt vector approach doesn't seem to be working like I had
thought it would. Sorry. It doesn't handle suspend/unsuspend channels
without causing problems restarting the native bridge. The unsuspend also
seems to be why the join logic had to execute native_rtp_bridge_start()
completely even though it is redundant when smartbridge officially joins
the two channels to the technology.
I agree that we need to cashe not only the rtp instances but also the
other glue stuff you are now caching. However, I think we need to save
that information in the bridge_channel->tech_pvt data and make that
tech_pvt pointer's lifetime controlled directly by the join/leave
callbacks and not the frame hook attach/detach functions. Changing who
creates the data struct is a small change since the frame hook
attach/detach functions are only called by the join/leave callbacks
anyway. Because of how frame hooks are detached, the tech_pvt still has
to be an ao2 object.
native_rtp_bridge_start() needs to check if both bridge_channel->tech_pvt
pointers are not NULL before proceeding. If either is NULL then a channel
hasn't been joined to the bridge technology yet.
https://gerrit.asterisk.org/#/c/5854/3//COMMIT_MSG
Commit Message:
https://gerrit.asterisk.org/#/c/5854/3//COMMIT_MSG@11
PS3, Line 11: native bridge and been destroyed. This patch effectively causes the
: frame hook to keep a reference to both the audio and video rtp
: instances until after the hook has been detached.
The frame hook reference is no longer valid.
https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c
File bridges/bridge_native_rtp.c:
https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@47
PS3, Line 47: #include "asterisk/linkedlists.h"
Why was this include added?
https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@161
PS3, Line 161: AST_VECTOR_INIT(&data->instances, 4);
AST_VECTOR_INIT() can fail to allocate the initial vector array.
https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@207
PS3, Line 207: struct channel_instance_data *data = ao2_alloc(sizeof(*data),
: channel_instance_data_destructor);
Use ao2_alloc_options() since the object doesn't need any lock.
https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@273
PS3, Line 273: RAII_VAR(struct ast_format_cap *, cap0,
: ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT), ao2_cleanup);
: RAII_VAR(struct ast_format_cap *, cap1,
: ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT), ao2_cleanup);
Pre-existing bug: cap0 and cap1 are not checked for failure. Also allocating them unconditionally is a waste if the AST_RTP_GLUE_RESULT_REMOTE is not the selected case.
https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@301
PS3, Line 301: ast_debug(2, "Locally RTP bridged '%s' and '%s' in stack\n",
: ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));
: ast_verb(4, "Locally RTP bridged '%s' and '%s' in stack\n",
: ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));
The added debug log message is redundant with the existing verbose message. This is patch debugging and should not be left in the final patch.
https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@322
PS3, Line 322: ast_debug(2, "Remotely bridged '%s' and '%s' - media will flow directly between them\n",
: ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));
: ast_verb(4, "Remotely bridged '%s' and '%s' - media will flow directly between them\n",
: ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));
The added debug log message is redundant with the existing verbose message. This is patch debugging and should not be left in the final patch.
https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@554
PS3, Line 554: data0 = get_channel_instance_data(bridge, bc0->chan);
: if (!data0) {
: data0 = create_channel_instance_data(bridge, bc0->chan);
: if (!data0) {
: return 0;
: }
: }
The bridge may not be using this technology right now. We are testing to see if the technology is compatible or still compatible with the current bridge. Either way we cannot get the cached instance data. We must create the instance data every time here.
https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@561
PS3, Line 561: data1 = get_channel_instance_data(bridge, bc1->chan);
: if (!data1) {
: data1 = create_channel_instance_data(bridge, bc1->chan);
: if (!data1) {
: return 0;
: }
: }
same
https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@571
PS3, Line 571: if (native_type == AST_RTP_GLUE_RESULT_FORBID) {
: ast_assert(native_type != AST_RTP_GLUE_RESULT_FORBID);
This assert is useless as it will always fail on a valid condition.
https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@740
PS3, Line 740: ao2_cleanup(data);
: bridge_channel->tech_pvt = NULL;
I'd swap these two lines for safety so the tech_pvt doesn't briefly point to stale data.
https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@781
PS3, Line 781: static void native_rtp_bridge_unsuspend(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel)
A channel can get suspended from a bridge so it can play a sound file or it can get suspended to process a COLP update interception dialplan routine. Either way it doesn't technically leave the bridge although the technology may treat it that way.
This patch doesn't handle unsuspending a channel and restarting the bridge right. It appears that the channel instance data gets appended to the vector each time the channel is unsuspended.
--
To view, visit https://gerrit.asterisk.org/5854
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e1ac49fa4af68d64826ccccd152593cf8cdb21a
Gerrit-Change-Number: 5854
Gerrit-PatchSet: 3
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Tue, 20 Jun 2017 18:42:43 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170620/768bde95/attachment-0001.html>
More information about the asterisk-code-review
mailing list