<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/1443/">https://reviewboard.asterisk.org/r/1443/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 20th, 2011, 4:19 p.m., <b>rmudgett</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/1443/diff/1/?file=20700#file20700line13290" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/apps/app_voicemail.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int advanced_options(struct ast_channel *chan, struct ast_vm_user *vmu, struct vm_state *vms, int msg, int option, signed char record_gain)</pre></td>
</tr>
</tbody>
<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">13290</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/* If no sound_duration pointer was passed in, use the duration pointer */</span></pre></td>
</tr>
<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">13291</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">sound_duration</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
<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">13292</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">sound_duration</span> <span class="o">=</span> <span class="n">duration</span><span class="p">;</span></pre></td>
</tr>
<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">13293</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span></pre></td>
</tr>
<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">13294</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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;">I don't think pointing sound_duration to duration is a good thing here. The actual value set could be unexpectedly changed if the order of setting duration and sound_duration is changed in the ast_play_and_record_full() function.
Since this is an internal function, the callers can be changed to always pass in the sound_duration value.</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'd prefer not to require people to pass in pointers that they don't intend to use. The other option is I could check for sound_duration before using it - there aren't too many places that read / write it.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 20th, 2011, 4:19 p.m., <b>rmudgett</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/1443/diff/1/?file=20701#file20701line329" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/include/asterisk/app.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int ast_play_and_wait(struct ast_channel *chan, const char *fn);</pre></td>
</tr>
</tbody>
<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">329</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * \param sound_duration pointer to integer for storing length of the recording minus all silence</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;">Spacing irregularity at the start of line.</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;">Will fix</pre>
<br />
<p>- mjordan</p>
<br />
<p>On September 16th, 2011, 3:48 p.m., mjordan wrote:</p>
<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 and rmudgett.</div>
<div>By mjordan.</div>
<p style="color: grey;"><i>Updated Sept. 16, 2011, 3:48 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;">This is a different take on a solution that was previously proposed here: https://reviewboard.asterisk.org/r/1403/ - which I've now closed out.
The problem is still the same: The patch for ASTERISK-2234 is viewed as a regression in ASTERISK-16981. ASTERISK-2234 changed the way voicemail duration is calculated in the presence of silence. Instead of the duration being the length of the voicemail minus whatever silence accumulates at the end of a call (when that call ends in silence), the duration calculation was changed to include only the time in the audio stream that was not considered to be silence. Consider a voicemail system set up to ensure a message has a minimum duration of 4 seconds, and a silence cutoff of 6 seconds. This patch solved the following situation:
1. A person begins recording with 4 seconds of silence
2. Person says "hello?", taking 1 second
3. Person waits for 6 seconds and is hung up on
Prior to this patch, the voicemail's duration would be calculated at 5 seconds and would be allowed through; post-patch the duration would be calculated at 1 second and would be discarded.
The previous patch simply rolled out ASTERISK-2234. After some discussion, this patch takes a different approach.
This patch keeps the previous approach of calculating the silence throughout the duration of the voicemail, but also records the duration of the actual file. The functions in app.c responsible for this mechanism report both durations back up to the caller. A caller of the method must supply a pointer for the duration; it does not have to supply a pointer for the sound-only duration. Those applications (app_voicemail, app_minivm) that make calculations as to whether or not they will keep a message can use either duration (with this patch, they will use the sound-only duration), but will report externally to users the duration of the entire message.
This fixes the bug with the e-mail notifications, while keeping the behavior intended by ASTERISK-2234.</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;">Tested using 1.8 and the following:
minsecs = 4
silencethreshold = 128
maxsilence = 10
* Leaving voicemails with silence in the beginning, punctuated by audio, followed by 10 seconds of silence (message kept with correct duration)
* Leaving voicmeails that consisted of all silence (message dropped)
* Leaving voicemails of silence ended by '#' (message dropped)
* Leaving voicemail with silence, audio, silence, audio, silence, followed by '#' (message kept with correct duration)
Also tested with the Asterisk TestSuite's leave voicemail tests.</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-16981">ASTERISK-16981</a>,
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-2234">ASTERISK-2234</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/apps/app_dial.c <span style="color: grey">(336232)</span></li>
<li>/branches/1.8/apps/app_followme.c <span style="color: grey">(336232)</span></li>
<li>/branches/1.8/apps/app_meetme.c <span style="color: grey">(336232)</span></li>
<li>/branches/1.8/apps/app_minivm.c <span style="color: grey">(336232)</span></li>
<li>/branches/1.8/apps/app_voicemail.c <span style="color: grey">(336232)</span></li>
<li>/branches/1.8/include/asterisk/app.h <span style="color: grey">(336232)</span></li>
<li>/branches/1.8/main/app.c <span style="color: grey">(336232)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1443/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>