<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/2328/">https://reviewboard.asterisk.org/r/2328/</a>
</td>
</tr>
</table>
<br />
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/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, Mark Michelson, rmudgett, and Matt Jordan.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated Feb. 14, 2013, 2:57 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Changes</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;">Bat it back.</pre>
</td>
</tr>
</table>
<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;">Richard had a shopping list of improvements to be made. Here is the copypasta.
1) Support for ${LIMIT_CONNECT_FILE} is missing. This file is played at
the beginning of the bridge. This can be implemented as a one time
interval hook with an immediate timeout that fires as soon as the channel
joins the bridge.
* Addressed as described. It's worth noting that contrary to documentation,
normal Asterisk doesn't actually say the remaining time at the beginning of the
conference if no LIMIT_CONNECT_FILE is specified. This patch changes things to
match with the documentation.
2) The interval_hook_time_cmp() needs to compare a sequence number of the
order of hooks added in addition to the interval_trip_time. This will
ensure that when timeouts happen at the same time, the oldest interval
expiring at that time will execute first.
* Implemented as suggested.
3) Need to add the test events associated with interval hooks. Notably
the end bridge timeout event. There may be other timeout events that I am
not aware of.
* Added to the limits duration callback. The other relevant test event was
for the S option and for L with only the duration argument, and they are
currently executed through a different mechanism that is unaffected by this
work. That might be changed soon.
4) ast_bridge_features_set_limits() must not save a pointer to the limits
struct in the hook. In this case the peer channel will likely have a
stale pointer when ast_bridge_call() is changed to not expect to get the
peer back. Also transfers could move the peer channel to another bridge.
ast_bridge_features_set_limits() should copy the limits information into
its own structure so it is independent.
* ast_bridge_features_set_limits now copies the contents of the limits
struct into a freshly allocated struct and applies it to the bridge channel's
features struct. Only one can be applied to a bridge channel at a time right
now and I'm unsure if that should be changed or if a mechanism for overwriting
it should be provided.
5) I think it is a good feature of the interval hooks to know about the
"timeleft" macro sound filename. This allows the LIMIT_CONNECT_FILE and
LIMIT_WARNING_FILE to be set to "timeleft" and get the time left output.
* timeleft macro sound name is now read by the hook rather than reinterpreted
at the consumer level.
6) I don't particularly like the fact that struct
ast_bridge_features_limits has fixed sized sound filenames.
* Changed to use string fields. It might still be limited from a practical
standpoint by the dial application and/or features.c.
7) I don't think that ast_bridge_features_interval_hook() needs to have a
strict flag. It should always do that if it is a repeating interval.
* Strict flag removed and all intervals are now treated strictly.</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;">Every single scenario I've been testing all along, which covers everything in the bridge test as well as some extra stuff involving setting of specific sound file variables.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-20872">ASTERISK-20872</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> (updated)</h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/team/group/bridge_construction/bridges/bridge_builtin_interval_features.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/team/group/bridge_construction/include/asterisk/bridging.h <span style="color: grey">(381469)</span></li>
<li>/team/group/bridge_construction/include/asterisk/bridging_features.h <span style="color: grey">(381469)</span></li>
<li>/team/group/bridge_construction/main/bridging.c <span style="color: grey">(381469)</span></li>
<li>/team/group/bridge_construction/main/features.c <span style="color: grey">(381469)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2328/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>