<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2465/">https://reviewboard.asterisk.org/r/2465/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 25th, 2013, 3:37 p.m. UTC, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">A couple of high-level notes:
1) I'd suggest adding XXX or BUGBUG comments to any places you've touched where ast_bridged_channel() is called. The reason is that currently, this always returns NULL, no matter the current bridging state of a channel. I don't know if there is a long-term goal to make ast_bridged_channel() return a channel if there is only one bridge peer, or if that function will die eventually. But having comments will direct attention to the fact that things might be a bit messed up at the moment. It looked like mixmonitor and chanspy could still work okay, but it appears that directrtpsetup in chan_sip will not work at the moment, even with your addition of checking ast_channel_internal_bridge()
2) I'm surprised that there isn't a ton of code removed from res_rtp_asterisk.c and rtp_engine.c. Is all of the native bridging code from those files still necessary, even with the changes you've made here?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Removed old code, and removed calls to ast_bridged_channel.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 25th, 2013, 3:37 p.m. UTC, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2465/diff/1/?file=36351#file36351line265" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/bridge_construction/bridges/bridge_native_rtp.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">265</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">int</span> <span class="nf">native_rtp_bridge_join</span><span class="p">(</span><span class="k">struct</span> <span class="n">ast_bridge</span> <span class="o">*</span><span class="n">bridge</span><span class="p">,</span> <span class="k">struct</span> <span class="n">ast_bridge_channel</span> <span class="o">*</span><span class="n">bridge_channel</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This function appears to be called for each individual bridge_channel that joins the native bridge. Why do you operate on both channels in the bridge every time this function is called? It seems like everything will happen twice as a result of this behavior.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">When it comes to native bridging it's an all-or-nothing event. Everything has to be native bridged together, or nothing at all. If you don't do this you leave part of the media leg native bridged and the end device may receive media from two different sources. For example: Two channels join this bridge, and one leaves. Without tearing down all native bridging the one that left would still be receiving media from the channel still within the bridge.</pre>
<br />
<p>- Joshua</p>
<br />
<p>On April 23rd, 2013, 4:06 p.m. UTC, Joshua Colp wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Joshua Colp.</div>
<p style="color: grey;"><i>Updated April 23, 2013, 4:06 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This change implements the following:
1. Adds a native RTP bridge technology which does local or remote bridging depending on conditions
2. Makes the bridging core aware of native bridges
3. Calls into the compatible callback of bridging technologies when present to check compatibility
4. Tweaks some logic to cause the bridge to reconfigure when external conditions influence it</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">1. Tested that non-RTP channels do not cause the native RTP bridge to be used
2. Tested that if conditions allow it that remote bridging (ala reinvite) occurs
3. Tested that if remote bridging is not possible that local bridging occurs
4. Tested that if conditions are not correct none of the above happens
5. Tested that external applications cause the bridge to be reconfigured, and alternate technology used if needed</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/team/group/bridge_construction/main/rtp_engine.c <span style="color: grey">(386345)</span></li>
<li>/team/group/bridge_construction/main/bridging.c <span style="color: grey">(386345)</span></li>
<li>/team/group/bridge_construction/channels/chan_sip.c <span style="color: grey">(386345)</span></li>
<li>/team/group/bridge_construction/include/asterisk/rtp_engine.h <span style="color: grey">(386345)</span></li>
<li>/team/group/bridge_construction/bridges/bridge_native_rtp.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/team/group/bridge_construction/apps/app_chanspy.c <span style="color: grey">(386345)</span></li>
<li>/team/group/bridge_construction/apps/app_mixmonitor.c <span style="color: grey">(386345)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2465/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>