<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/1814/">https://reviewboard.asterisk.org/r/1814/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The problem I have with this change is that it is only useful for chan_iax2. Other channel types using a jitterbuffer will not get the benefit since they do not call the functions in include/jitterbuf.c directly. Most typical jitterbuffer usage goes through the interface in include/asterisk/abstract_jb.h. In main/abstract_jb.c, you can see that ast_jb_put() specifically filters out anything that is not a voice or DTMF frame. Once you change that, your changes to main/jitterbuf.c will be useful to all channel types. You'll also need to alter fixed_jb_put() in main/fixedjitterbuf.c so that it understands that it may receive frame types other than voice.
In Asterisk 10, you'll also find that you'll need to update func_jitterbuffer.c so that it will attempt to place non-voice frames into the jitterbuffer.</pre>
<br />
<div>
<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/1814/diff/1/?file=26097#file26097line152" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/jitterbuf.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; ">void jb_destroy(jitterbuf *jb)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">139</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">jb</span><span class="o">-></span><span class="n">info</span><span class="p">.</span><span class="n">cnt_delay_discont</span> <span class="o">></span> <span class="mi">3</span><span class="p">)</span> <span class="p">{</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">138</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p"><span class="hl">(</span>(</span><span class="n">jb</span><span class="o">-></span><span class="n">info</span><span class="p">.</span><span class="n">cnt_delay_discont</span> <span class="o">></span> <span class="mi">3</span><span class="p">)</span> <span class="o"><span class="hl">||</span></span><span class="hl"> </span><span class="p"><span class="hl">(</span></span><span class="n"><span class="hl">type</span></span><span class="hl"> </span><span class="o"><span class="hl">!=</span></span><span class="hl"> </span><span class="n"><span class="hl">JB_TYPE_VOICE</span></span><span class="p"><span class="hl">))</span></span><span class="hl"> </span><span class="p">{</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">140</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/* resync the jitterbuffer<span class="hl"> </span>*/</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">139</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/* resync the jitterbuffer<span class="hl">, immediately if not a voice frame</span>*/</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Is it correct to have this as type != JB_TYPE_VOICE, or do you want to specifically target type == JB_TYPE_CONTROL? Since the description on the review specifically mentioned control frames, I want to be sure this is the intended change.</pre>
</div>
<br />
<div>
<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/1814/diff/1/?file=26097#file26097line173" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/jitterbuf.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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int history_put(jitterbuf *jb, long ts, long now, long ms, long *delay)</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">158</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">int</span> <span class="nf">history_put</span><span class="p">(</span><span class="n">jitterbuf</span> <span class="o">*</span><span class="n">jb</span><span class="p">,</span> <span class="kt">long</span> <span class="n">ts</span><span class="p">,</span> <span class="kt">long</span> <span class="n">now</span><span class="p">,</span> <span class="kt">long</span> <span class="n">ms</span><span class="p">,</span> <span class="kt">long</span> <span class="o">*</span><span class="n">delay</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Unlike with check_resync(), passing a pointer to delay isn't necessary here. Just pass by value instead.</pre>
</div>
<br />
<div>
<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/1814/diff/1/?file=26097#file26097line537" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/jitterbuf.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 jb_dbgqueue(jitterbuf *jb)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">513</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">numts</span> <span class="o">=</span> <span class="mi">0</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">521</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">check_resync</span><span class="p">(</span><span class="n">jb</span><span class="p">,</span><span class="n">ts</span><span class="p">,</span><span class="n">now</span><span class="p">,</span><span class="n">ms</span><span class="p">,</span><span class="n">type</span><span class="p">,</span><span class="o">&</span><span class="n">delay</span><span class="p">))</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I know it's kind of a lost cause in this file, but since you're changing this line anyway, might as well add some spaces between the parameters.</pre>
</div>
<br />
<div>
<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/1814/diff/1/?file=26097#file26097line556" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/jitterbuf.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 jb_dbgqueue(jitterbuf *jb)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">532</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="hl"> </span><span class="k"><span class="hl">if</span></span><span class="hl"> </span><span class="p"><span class="hl">(</span></span><span class="n">history_put</span><span class="p">(</span><span class="n">jb</span><span class="p">,</span><span class="n">ts</span><span class="p">,</span><span class="n">now</span><span class="p">,</span><span class="n">ms</span><span class="p"><span class="hl">))</span></span><span class="hl"> </span><span class="p"><span class="hl">{</span></span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">528</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">history_put</span><span class="p">(</span><span class="n">jb</span><span class="p">,</span><span class="n">ts</span><span class="p">,</span><span class="n">now</span><span class="p">,</span><span class="n">ms</span><span class="p"><span class="hl">,</span></span><span class="o"><span class="hl">&</span></span><span class="n"><span class="hl">delay</span></span><span class="p"><span class="hl">);</span></span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Spaces here, too.</pre>
</div>
<br />
<p>- Mark</p>
<br />
<p>On March 13th, 2012, 12:54 p.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 March 13, 2012, 12:54 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;">When a change in time occurs, such that the timestamps associated with frames being placed into an adaptive Jitter Buffer are significantly different then the previously inserted frames, the adaptive Jitter Buffer checks to see if it needs to be resynched to the new time frame (based on an initially configurable re-synchronization threshold). If three consecutive packets break the threshold, the jitter buffer re-synchs itself to the new timestamps. This currently only occurs when the history is calculated, and hence only on VOICE frames.
CONTROL frames, on the other hand, are never passed to the history calculations. Because of this, if the jump in time is greater then the maximum allowed length of the Jitter Buffer, the CONTROL frames are dropped and no re-synchronization occurs. Because the re-synch does not happen, subsequent VOICE frames are counted as overflow of the Jitter Buffer, and are also dropped. In some scenarios, this can lead to 'stuttery' audio as some number of VOICE frames are dropped.
This patch allows CONTROL frames to re-synch the Jitter Buffer. As CONTROL frames are very unlikely to occur in multiples, it performs the resynchornization on any CONTROL frame that breaks the re-synch threshold.</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;">Testing was done by the initial issue reporter. I've also verified that the patch does not adversely effect nominal operation of IAX2 channels with a forced jitter buffer.
Unit tests were written to cover this functionality and to verify that other behavior did not change with this patch (see review https://reviewboard.asterisk.org/r/1815/). Since the unit tests are a new module, I put them under their own review against trunk. Without this patch, the unit test jitterbuffer_resynch_control will fail, as the jitter buffer will not be resynced and will instead overflow. The rest of the unit tests have the same result with and without this patch.</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-18964">ASTERISK-18964</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/jitterbuf.c <span style="color: grey">(358724)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1814/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>