<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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 13th, 2012, 5:03 p.m., <b>Mark Michelson</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;">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>
</blockquote>
<p>On March 13th, 2012, 5:38 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;">This change is limited to chan_iax2's usage of the jitter buffer, as only chan_iax2 uses the JB_TYPE_CONTROL frames. If you look at the implementations of jb_put_adaptive, it makes the assumption that all data being put into the jitter buffer is of type JB_TYPE_VOICE - so other channel drivers would never run into this problem in the first place. With respect to fixedjitterbuf... it is its own thing, so none of this applies.
That aside, would other channel drivers ever need to pass data other then "voice" (let's call it "media") to a jitter buffer? I don't know for sure, but I suspect not. For example, why would SIP ever need to interleave some piece of signalling data with an associated RTP stream? The only thing I could think of was DTMF over SIP INFO, but DTMF - over SIP INFO or embedded in the RTP stream - is already handled when the Asterisk frames are created and passed into the native bridge.
In general, I think abstracting away the Jitter Buffer frame types from the things that use the Jitter Buffer was the correct decision. If the abstract Jitter Buffer API ever has to be smart enough to make decisions on a wide variety of frame types, I would expect that to be a new feature and outside the scope of something that should go into 1.8.
</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Okay, this explanation satisfies me.</pre>
<br />
<p>- Mark</p>
<br />
<p>On March 13th, 2012, 5:45 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, 5:45 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>