<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/1815/">https://reviewboard.asterisk.org/r/1815/</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:22 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 test documentation should be altered to make sure to explicitly state that only the adaptive jitterbuffer is tested.
With the exception of the resynch test, the voice and control versions of each test are exactly the same, just with a different frame type specified for jb_put(). The common code can be factored out into functions that take the test name and type of frame to place in the jitterbuffer. This will greatly reduce the size of this file.</pre>
</blockquote>
<p>On March 14th, 2012, 8:23 a.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;">I do think the test documentation should be clearer about what is being tested - main/jitterbuf.c (so I'll probably rename this to test_jitterbuf, etc.)
This is *not* a unit test for the adaptive jitter buffer or that abstract jitter buffer API.
The fact that the adaptive jitter buffer happens to currently use jitterbuf.c as its implementation is a detail that is abstracted away. That implementation could be changed, and as long as the abstract API doesn't change, it shouldn't effect a unit test written to exercise that code (or at least, that's the theory). In this particular case, these unit tests are concerned with the handling of JB_TYPE_CONTROL frames by jitterbuf.c, as a code change to that handling is what prompted the writing of these unit tests. The abstract API and the adaptive jitter buffer do not use those frames - so if this was a unit test for the abstract API / adaptive jitter buffer, there'd be no way to exercise that functionality. On top of that, the unit tests don't even attempt to access the adaptive jitter buffer through the abstract API - so there's no way you could say that this actually tests the adaptive jitter buffer.
I'll refactor some of the portions of the tests - some of the reading of the frames as well can probably be refactored out. Given that each test 'pair' requires its own particular insertion function, the SLOC may not be reduced by as much as it might appear, although it would help if we ever decided to test any of jitterbuf.c's frame types.
</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;">Sure, when I said "adaptive jitterbuffer" I meant the jitterbuffer implementation in main/jitterbuf.c. I tend to think of that jitterbuffer as the adaptive one, and the one in main/fixedjitterbuf.c as the fixed one. Specifying that this tests main/jitterbuf.c will satisfy what I was driving at in my comment while also being more technically accurate.</pre>
<br />
<p>- Mark</p>
<br />
<p>On March 14th, 2012, 9:22 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 March 14, 2012, 9:22 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;">This patch adds unit tests to cover major functions in jitterbuf.c. This include jb_put, jb_get, jb_next, as well as general information and statistic gathering. The scenarios include:
1) Nominal - adding voice/control frames and making sure that all added frames are removed and in the correct time slots
2) Out of Order - some voice/control frames arrive out of order with respect to the previous frame. The tests check that when reading from the jitter buffer, the frames are reordered
3) Lost - some voice/control frames never arrive. In the case of voice, the jitter buffer should interpolate the frame; in the case of control, the jitter buffer should report that no frame exists at that time slot
4) Late - the arrival time of some voice/control frames does not occur at the same rate as surrounding frames
5) Resync - test that voice/control frames that break the resync threshold trigger a resync of the jitter buffer
6) Overflow - test that adding voice/control frames that exceed the maximum time length of the buffer are dropped
This change was prompted by https://reviewboard.asterisk.org/r/1814 - not all tests will pass without that 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>/trunk/tests/test_jitterbuf.c <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1815/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>