<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/5854">View Change</a></p><p>Patch set 3:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ugh. This patch has gotten much larger than I expected.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The bridge->tech_pvt vector approach doesn't seem to be working like I had<br>thought it would. Sorry. It doesn't handle suspend/unsuspend channels<br>without causing problems restarting the native bridge. The unsuspend also<br>seems to be why the join logic had to execute native_rtp_bridge_start()<br>completely even though it is redundant when smartbridge officially joins<br>the two channels to the technology.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I agree that we need to cashe not only the rtp instances but also the<br>other glue stuff you are now caching. However, I think we need to save<br>that information in the bridge_channel->tech_pvt data and make that<br>tech_pvt pointer's lifetime controlled directly by the join/leave<br>callbacks and not the frame hook attach/detach functions. Changing who<br>creates the data struct is a small change since the frame hook<br>attach/detach functions are only called by the join/leave callbacks<br>anyway. Because of how frame hooks are detached, the tech_pvt still has<br>to be an ao2 object.</p><p style="white-space: pre-wrap; word-wrap: break-word;">native_rtp_bridge_start() needs to check if both bridge_channel->tech_pvt<br>pointers are not NULL before proceeding. If either is NULL then a channel<br>hasn't been joined to the bridge technology yet.</p><p>(12 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/5854/3//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5854/3//COMMIT_MSG@11">Patch Set #3, Line 11:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">native bridge and been destroyed. This patch effectively causes the<br>frame hook to keep a reference to both the audio and video rtp<br>instances until after the hook has been detached.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The frame hook reference is no longer valid.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c">File bridges/bridge_native_rtp.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@47">Patch Set #3, Line 47:</a> <code style="font-family:monospace,monospace">#include "asterisk/linkedlists.h"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why was this include added?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@161">Patch Set #3, Line 161:</a> <code style="font-family:monospace,monospace"> AST_VECTOR_INIT(&data->instances, 4);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">AST_VECTOR_INIT() can fail to allocate the initial vector array.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@207">Patch Set #3, Line 207:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> struct channel_instance_data *data = ao2_alloc(sizeof(*data),<br> channel_instance_data_destructor);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Use ao2_alloc_options() since the object doesn't need any lock.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@273">Patch Set #3, Line 273:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> RAII_VAR(struct ast_format_cap *, cap0,<br> ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT), ao2_cleanup);<br> RAII_VAR(struct ast_format_cap *, cap1,<br> ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT), ao2_cleanup);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@301">Patch Set #3, Line 301:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ast_debug(2, "Locally RTP bridged '%s' and '%s' in stack\n",<br> ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));<br> ast_verb(4, "Locally RTP bridged '%s' and '%s' in stack\n",<br> ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@322">Patch Set #3, Line 322:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ast_debug(2, "Remotely bridged '%s' and '%s' - media will flow directly between them\n",<br> ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));<br> ast_verb(4, "Remotely bridged '%s' and '%s' - media will flow directly between them\n",<br> ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@554">Patch Set #3, Line 554:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> data0 = get_channel_instance_data(bridge, bc0->chan);<br> if (!data0) {<br> data0 = create_channel_instance_data(bridge, bc0->chan);<br> if (!data0) {<br> return 0;<br> }<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@561">Patch Set #3, Line 561:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> data1 = get_channel_instance_data(bridge, bc1->chan);<br> if (!data1) {<br> data1 = create_channel_instance_data(bridge, bc1->chan);<br> if (!data1) {<br> return 0;<br> }<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">same</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@571">Patch Set #3, Line 571:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (native_type == AST_RTP_GLUE_RESULT_FORBID) {<br> ast_assert(native_type != AST_RTP_GLUE_RESULT_FORBID);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This assert is useless as it will always fail on a valid condition.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@740">Patch Set #3, Line 740:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ao2_cleanup(data);<br> bridge_channel->tech_pvt = NULL;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd swap these two lines for safety so the tech_pvt doesn't briefly point to stale data.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5854/3/bridges/bridge_native_rtp.c@781">Patch Set #3, Line 781:</a> <code style="font-family:monospace,monospace">static void native_rtp_bridge_unsuspend(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/5854">change 5854</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/5854"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I9e1ac49fa4af68d64826ccccd152593cf8cdb21a </div>
<div style="display:none"> Gerrit-Change-Number: 5854 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 20 Jun 2017 18:42:43 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>