<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/3362/">https://reviewboard.asterisk.org/r/3362/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 29th, 2014, 8:12 p.m. EDT, <b>Corey Farrell</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/3362/diff/4/?file=56930#file56930line206" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/funcs/func_periodic_hook.c</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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">206</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">ast_tvdiff_ms</span><span class="p">(</span><span class="n">now</span><span class="p">,</span> <span class="n">state</span><span class="o">-></span><span class="n">last_hook</span><span class="p">)</span> <span class="o">></span> <span class="n">state</span><span class="o">-></span><span class="n">interval</span> <span class="o">*</span> <span class="mi">1000</span><span class="p">)</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;">state->last_hook is never pre-initialized to ast_tvnow(), so hooks would always trigger on first check after being created or re-enabled. Unless this behaviour is documented I would expect the first run after create or resume to be delayed state->interval seconds.</pre>
</blockquote>
<p>On March 31st, 2014, 8:06 a.m. EDT, <b>Russell Bryant</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;">Fair. I had originally intended for it to run immediately, but I think it makes more sense to delay.</pre>
</blockquote>
<p>On March 31st, 2014, 11:42 a.m. EDT, <b>jamuel</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;">So is the implication then if I were to set the interval to 30 seconds that the hook wouldn't run for 30 seconds and then run every 30 seconds? That seems more counter intuitive. I'd think that it should run and then repeat every interval seconds.</pre>
</blockquote>
<p>On March 31st, 2014, 12:10 p.m. EDT, <b>Russell Bryant</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;">Yes, that's right. I keep going back and forth but I think I agree with you that running immediately makes the most sense.</pre>
</blockquote>
<p>On March 31st, 2014, 1:06 p.m. EDT, <b>jamuel</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;">It seems to make more sense especially when the interval is "really long". When the interval is just a few seconds it doesn't really matter but when it's say 30s or 60s (or even longer) then it seems weird to have to wait that long for it to run the first time--especially in your original use case of wanting to beep to indicate call recording . . .</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;">I'm not going to hold up this review for this finding, but I disagree. Please point me to one implementation of an interval where the first run is immediately. I can't think of any.
Also with first run delayed by interval, if you want a beep immediately you can just play one. With no delay on first interval it's impossible to prevent the immediate beep.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 29th, 2014, 8:12 p.m. EDT, <b>Corey Farrell</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/3362/diff/4/?file=56930#file56930line273" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/funcs/func_periodic_hook.c</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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">273</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">sscanf</span><span class="p">(</span><span class="n">args</span><span class="p">.</span><span class="n">interval</span><span class="p">,</span> <span class="s">"%u"</span><span class="p">,</span> <span class="o">&</span><span class="n">interval</span><span class="p">)</span> <span class="o">!=</span> <span class="mi">1</span><span class="p">)</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;">Need checks for !ast_strlen_zero on all args. Maybe also a verify the exten@context exists?</pre>
</blockquote>
<p>On March 31st, 2014, 8:06 a.m. EDT, <b>Russell Bryant</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;">I think this is all handled. For interval, sscanf() will just fail on an empty string. For exten and context, defaults of "s" and "default" are applied.
Also, I don't think it should check for the exten to exist. If it doesn't, errors should show up when it tries to run. Extensions can be added/removed dynamically, so I think we should just assume the best here.</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;">same => n,Set(BEEP_ID=${PERIODIC_HOOK()})
I believe the above dialplan would crash. With no parameters all fields of args remain NULL.
same => n,Set(BEEP_ID=${PERIODIC_HOOK(5)})
This will result in a value for args.interval, but all other parameters NULL.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 29th, 2014, 8:12 p.m. EDT, <b>Corey Farrell</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/3362/diff/4/?file=56930#file56930line299" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/funcs/func_periodic_hook.c</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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">299</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">state</span><span class="o">-></span><span class="n">disabled</span> <span class="o">=</span> <span class="mi">1</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;">Can we detach the audio hook here? I know we escape the hook callback very early but it still feels wasteful to keep it enabled.
Possibly remove state->disabled variable?</pre>
</blockquote>
<p>On March 31st, 2014, 8:06 a.m. EDT, <b>Russell Bryant</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;">As you said, the callback does just about nothing, so I don't think it really hurts to keep it.
But honestly, I would have done it this way if the audiohooks API was built for dynamic adding/removing. It really isn't. No other code does that. It all relies on channel destruction to tear it down.</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;">I took a closer look at audiohook.c, I agree the removal isn't very pretty.</pre>
<br />
<p>- Corey</p>
<br />
<p>On March 31st, 2014, 12:11 p.m. EDT, Russell Bryant 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.</div>
<div>By Russell Bryant.</div>
<p style="color: grey;"><i>Updated March 31, 2014, 12:11 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 commit introduces a new dialplan function, PERIODIC_HOOK().
It allows you run to a dialplan hook on a channel periodically. The
original use case that inspired this was the ability to play a beep
periodically into a call being recorded. The implementation is much
more generic though and could be used for many other things.
The implementation makes heavy use of existing Asterisk components.
It uses a combination of Local channels and ChanSpy() to run some
custom dialplan and inject any audio it generates into an active call.
The other important bit of the implementation is how it figures out
when to trigger the beep playback. This implementation uses the
audiohook API, even though it's not actually touching the audio in any
way. It's a convenient way to get a callback and check if it's time
to kick off another beep. It would be nice if this was timer event
based instead of polling based, but unfortunately I don't see a way to
do it that won't interfere with other things.
</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;">Called the following extension (100@test), both letting it run all the way through, as well as hanging up at various points in the middle.
[macro-beep]
exten => s,1,Answer()
same => n,Verbose(1,Channel name: ${ARG1})
same => n,Verbose(1,Hook ID: ${ARG2})
same => n,Playback(beep)
[test]
exten => 100,1,Answer()
same => n,Set(BEEP_ID=${PERIODIC_HOOK(beep,5)})
same => n,Wait(20)
same => n,Set(PERIODIC_HOOK(${BEEP_ID})=off)
same => n,Wait(20)
same => n,Set(PERIODIC_HOOK(${BEEP_ID})=on)
same => n,Wait(20)
same => n,Hangup()
</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>/trunk/funcs/func_periodic_hook.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/CHANGES <span style="color: grey">(411572)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3362/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>