<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/2010/">https://reviewboard.asterisk.org/r/2010/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 6th, 2012, 1:32 p.m., <b>Mark Michelson</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/2010/diff/1/?file=29629#file29629line2286" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/res/res_rtp_asterisk.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 void process_dtmf_rfc2833(struct ast_rtp_instance *instance, unsigned char *data, int len, unsigned int seqno, unsigned int timestamp, struct ast_sockaddr *addr, int payloadtype, int mark, struct frame_list *frames)</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">2286</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">f</span><span class="o">-></span><span class="n">ts</span> <span class="o">=</span> <span class="n">timestamp</span> <span class="o">/</span> <span class="p">(</span><span class="n">rtp_get_rate</span><span class="p">(</span><span class="o">&</span><span class="n">f</span><span class="o">-></span><span class="n">subclass</span><span class="p">.</span><span class="n">format</span><span class="p">)</span> <span class="o">/</span> <span class="mi">1000</span><span class="p">)</span><span class="cm">/* + new_duration*/</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;">Does this comment indicate there's more to be done here?</pre>
</blockquote>
<p>On July 6th, 2012, 7:46 p.m., <b>Matt Jordan</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;">Nope, that leaked through from testing. Adding timestamps to the DTMF frames is new with this patch, and - initially - it seemed like it would be worth trying to add the specified DTMF duration to the timestamp on each frame.
As it turns out, this is a bad idea. When I treated the specified duration of a DTMF digit as an actual time delta between BEGIN and END frames, multiple DTMF digits would inevitably overlap - UAs tend to send the packets specifying the DTMF much faster then what the durations imply. This meant the jitter buffer - which was trusting the timestamps it was given - would end up passing out multiple BEGIN frames for different digits before properly ENDing the digits. Hilarity ensued in the bridging layer.
So... yeah. Removed.</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;">So. About that.
After I wrote this I went back to work on finishing up my functional tests. Unfortunately, one of the functional tests I wrote pretty much shot my theory full of holes.
For certain sequences of DTMF digits, you *have* to ensure that all of the END frames have a timestamp that is greater then the BEGIN frames. END and BEGIN frames can collide with frames of the same type - but if an END frame and a BEGIN frame collides, you will end up with duplicated digits if you have the following sequence:
BEGIN
END
BEGIN
END
END
This is because as it turns out, for a given DTMF digit, all BEGIN/END packets tend to have the same timestamp. When they're put into a jitter buffer, depending on the type, the jitter buffer either "drops" the frame since its a collision, or appends the frame to the last received frame. Either way, it creates a confusing situation and does not properly re-order the frames. Since we need - at a minimum - for all of the BEGINs to have the same timestamp; the ENDs to have the same timestamp; and all END frames to have a timestamp greater then the BEGINs, there's three candidate fields that could be used to nudge the END frame's timestamps:
* sequence number - not a good candidate, however, since its a monotonically increasing number and doesn't convey time information.
* Raw DTMF duration - not a good candidate either - for the reason I outlined in my original comment. It almost guarantees that subsequent DTMF digits will overlap in time, leading to all sorts of confusion. Emulating the actual duration is better handled by the Asterisk core.
* Length of frame in ms - about the best candidate we have. Its highly unlikely that another DTMF frame would ever overlap, as the values are typically around 90ms.
So, this has now been modified to add the f->len value to f->ts, if the frame type is END. The tests that exposed this will be posted shortly.</pre>
<br />
<p>- Matt</p>
<br />
<p>On July 5th, 2012, 11:43 a.m., Matt Jordan 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.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated July 5, 2012, 11:43 a.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;">The current RTP stack in Asterisk does not have any inbound buffering capabilities. For RFC 2833 DTMF packets that arrive out of order, the RTP layer does the best it can, and - if it detects packets that have arrived out of order, or, in general, don't make "sense" (packets marked as an 'end' when no 'begin' occurred, etc.) - it attempts to prevent duplicate DTMF or other weird scenarios. This may include dropping the packets. Unfortunately, this means that - in some situations - some DTMF digits may have very odd durations or may not occur at all, if a sufficient number of packets arrive out of order.
Providing buffering in the RTP layer isn't really the correct answer here - putting a buffer on the receiving side of the RTP stack would have some fairly large ramifications on channel read operations. Since we already have the capability to put a jitter buffer on the read side of a channel in Asterisk 10/11, this patch expands the capabilities of these jitter buffers to 'understand' DTMF.
This works in the following way:
1. If no Jitter Buffer exists on the channel that owns the RTP instance, the behavior of the RTP instance is unchanged (it will drop out of order RFC 2833 DTMF)
2. When a Jitter Buffer is placed on a channel using func_jitterbuffer, a new control frame is sent to the channel. The channel, if it supports RTP, lets its RTP instance know that a jitter buffer it cares about may be in existance.
3. The RTP instance queries the frame hooks to see if a jitter buffer capable of handling DTMF exists. If it does, it sets a flag to allow out of order DTMF packets to flow up into the channel technology (and from there, into the bridging core)
4. The abstract jitter buffer code now handles the DTMF that flows in and out of it. This includes handling multiple END frames, handling BEGIN frames without END frames - the usual stuff that the RTP layer would have taken care of for us.</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;">Unit testing: This patch includes a new abstract_jb test. This checks the following:
1. Nominal creation of fixed/adaptive voice/dtmf jitter buffers
2. Nominal putting/retrieval of frames from fixed/adaptive voice/dtmf jitter buffers
3. Overflow in said jitter buffers
4. Out of order handling in said jitter buffers
5. Multiple DTMF END packets in DTMF jitter buffers
Functional testing in the Asterisk Test Suite: A new func_jitterbuffer test was written. RFC2833_dtmf_detect passed with no modifications, showing that the RTP layer - without a DTMF jitter buffer - worked the same. A new test, RFC2833_dtmf_jitterbuffer, was written that also successfully re-ordered the out of order DTMF. These tests will be put up on a separate review.
System testing: lots of smashing on IVRs with a DTMF jitter buffer. Successfully handled lots of short DTMF durations, long DTMF durations, etc.
</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-18404">ASTERISK-18404</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>/trunk/addons/chan_ooh323.c <span style="color: grey">(369624)</span></li>
<li>/trunk/channels/chan_alsa.c <span style="color: grey">(369624)</span></li>
<li>/trunk/channels/chan_console.c <span style="color: grey">(369624)</span></li>
<li>/trunk/channels/chan_dahdi.c <span style="color: grey">(369624)</span></li>
<li>/trunk/channels/chan_gtalk.c <span style="color: grey">(369624)</span></li>
<li>/trunk/channels/chan_h323.c <span style="color: grey">(369624)</span></li>
<li>/trunk/channels/chan_jingle.c <span style="color: grey">(369624)</span></li>
<li>/trunk/channels/chan_local.c <span style="color: grey">(369624)</span></li>
<li>/trunk/channels/chan_mgcp.c <span style="color: grey">(369624)</span></li>
<li>/trunk/channels/chan_oss.c <span style="color: grey">(369624)</span></li>
<li>/trunk/channels/chan_sip.c <span style="color: grey">(369624)</span></li>
<li>/trunk/channels/chan_skinny.c <span style="color: grey">(369624)</span></li>
<li>/trunk/channels/chan_unistim.c <span style="color: grey">(369624)</span></li>
<li>/trunk/channels/misdn_config.c <span style="color: grey">(369624)</span></li>
<li>/trunk/funcs/func_frame_trace.c <span style="color: grey">(369624)</span></li>
<li>/trunk/funcs/func_jitterbuffer.c <span style="color: grey">(369624)</span></li>
<li>/trunk/include/asterisk/abstract_jb.h <span style="color: grey">(369624)</span></li>
<li>/trunk/include/asterisk/frame.h <span style="color: grey">(369624)</span></li>
<li>/trunk/include/asterisk/framehook.h <span style="color: grey">(369624)</span></li>
<li>/trunk/include/asterisk/rtp_engine.h <span style="color: grey">(369624)</span></li>
<li>/trunk/tests/test_abstract_jb.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/res/res_rtp_asterisk.c <span style="color: grey">(369624)</span></li>
<li>/trunk/main/rtp_engine.c <span style="color: grey">(369624)</span></li>
<li>/trunk/main/jitterbuf.c <span style="color: grey">(369624)</span></li>
<li>/trunk/main/framehook.c <span style="color: grey">(369624)</span></li>
<li>/trunk/main/abstract_jb.c <span style="color: grey">(369624)</span></li>
<li>/trunk/main/channel.c <span style="color: grey">(369624)</span></li>
<li>/trunk/main/file.c <span style="color: grey">(369624)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2010/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>