[prev in list] [next in list] [prev in thread] [next in thread]
List: asterisk-dev
Subject: [asterisk-dev] [Code Review] Incorrect Voicemail duration reported
From: "mjordan" <reviewboard () asterisk ! org>
Date: 2011-08-30 18:28:43
Message-ID: 20110830182843.24020.97470 () hotblack ! digium ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1403/
-----------------------------------------------------------
Review request for Asterisk Developers and russelb.
Summary
-------
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.
Unfortunately, this has had the following side effects:
1. Voicemails that do contain some silence but are still valid are being dropped, \
particularly if the minsecs parameter is sufficiently large 2. Voicemail duration \
(reported through the email user option) is substantially off.
Both of these are significant issues - one, that we could be disposing of potentially \
important voicemails when the system administrator and its users would not expect us \
to (minsecs does not imply that it is the minimum amount of audio needed, but the \
minimum length of the entire message); two that we obviously report a duration that \
does not match the length of the attached sound file to the user.
Rolling back this patch resolves these issues - voicemail duration is calculated \
correctly (within +/- 1 second) and voicemail duration is calculated only minus the \
trailing silence.
However, since this removes a feature implemented in a prior patch, I expect there to \
be significant discussion with this proposed change.
This addresses bugs ASTERISK-16981 and ASTERISK-2234.
https://issues.asterisk.org/jira/browse/ASTERISK-16981
https://issues.asterisk.org/jira/browse/ASTERISK-2234
Diffs
-----
/branches/1.8/main/app.c 333944
Diff: https://reviewboard.asterisk.org/r/1403/diff
Testing
-------
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)
Thanks,
mjordan
[Attachment #5 (text/html)]
<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/1403/">https://reviewboard.asterisk.org/r/1403/</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 and russelb.</div>
<div>By mjordan.</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;">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.
Unfortunately, this has had the following side effects:
1. Voicemails that do contain some silence but are still valid are being dropped, \
particularly if the minsecs parameter is sufficiently large 2. Voicemail duration \
(reported through the email user option) is substantially off.
Both of these are significant issues - one, that we could be disposing of potentially \
important voicemails when the system administrator and its users would not expect us \
to (minsecs does not imply that it is the minimum amount of audio needed, but the \
minimum length of the entire message); two that we obviously report a duration that \
does not match the length of the attached sound file to the user.
Rolling back this patch resolves these issues - voicemail duration is calculated \
correctly (within +/- 1 second) and voicemail duration is calculated only minus the \
trailing silence.
However, since this removes a feature implemented in a prior patch, I expect there to \
be significant discussion with this proposed change.</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)</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/main/app.c <span style="color: grey">(333944)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1403/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic