<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. 13, 2013, 3:34 p.m.</i></p>




<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&#39;s worth noting that contrary to documentation,
normal Asterisk doesn&#39;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&#39;s
features struct. Only one can be applied to a bridge channel at a time right
now and I&#39;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
&quot;timeleft&quot; macro sound filename.  This allows the LIMIT_CONNECT_FILE and
LIMIT_WARNING_FILE to be set to &quot;timeleft&quot; 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&#39;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&#39;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&#39;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/apps/app_dial.c <span style="color: grey">(381343)</span></li>

 <li>/team/group/bridge_construction/include/asterisk/bridging_features.h <span style="color: grey">(381343)</span></li>

 <li>/team/group/bridge_construction/main/bridging.c <span style="color: grey">(381343)</span></li>

 <li>/team/group/bridge_construction/main/features.c <span style="color: grey">(381343)</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>