<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/3433/">https://reviewboard.asterisk.org/r/3433/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Well, this is really cool. I love reviews that mostly remove code in order to simplify something. Some concerns:
1) Documentation wise, there is a mixture of nomenclature used for the subchannels of a local channel. You have settled on the terms "master" and "outbound" for the two. In other places, they are referred to as ";1" and ";2" since that's how they end up getting named, and in other places they are referred to as "owner" and "chan" since that's what they're called in the unreal pvt structure. I think that something needs to be settled on so we can be consistent in what we call things.
2) Implementation wise, the biggest flaw I can spot is that the state of a bridge may change for the worse between when the optimization task is queued and when the optimization task actually executes. For instance, a third party may enter a bridge during that gap, resulting in a potentially bizarre channel arrangement afterwards.
3) Also implementation wise, I notice that the "swap" optimizations are the only kind you attempt to do. Any particular reason you do not handle "merge" optimizations?</pre>
<br />
<p>- Mark Michelson</p>
<br />
<p>On April 9th, 2014, 7:49 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.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, Matt Jordan, Mark Michelson, and rmudgett.</div>
<div>By Joshua Colp.</div>
<p style="color: grey;"><i>Updated April 9, 2014, 7:49 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 removes Unreal/Local channel optimization from the core and isolates it in a bridging technology implementation. The implementation attempts to optimize when channels join a bridge using it, instead of attempting to optimize constantly as frames pass. This review is not just for the code itself but the approach in general. Is this something we'd like? Is it viable?
Now the con...
Since this is implemented as a bridging technology and not as part of the bridging core it can not optimize completely when multi-party bridges are involved. If you have a chain in the middle that'll go anyway but no merging of bridges will occur.</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;">Since this has been an idea and side project I've only done some tests myself with large numbers of Local channels in chains (100, 200, 300) and confirmed that stuff works as expected.</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>/branches/12/main/core_unreal.c <span style="color: grey">(411867)</span></li>
<li>/branches/12/main/core_local.c <span style="color: grey">(411867)</span></li>
<li>/branches/12/main/bridge.c <span style="color: grey">(411867)</span></li>
<li>/branches/12/include/asterisk/core_unreal.h <span style="color: grey">(411867)</span></li>
<li>/branches/12/bridges/bridge_unreal.c <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3433/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>