[asterisk-dev] [Code Review] Fix incorrect jitterbuffer overflow when timing source changes are indicated by control frames

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1814/#review5802
-----------------------------------------------------------


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.


/branches/1.8/main/jitterbuf.c
<https://reviewboard.asterisk.org/r/1814/#comment10644>

    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.



/branches/1.8/main/jitterbuf.c
<https://reviewboard.asterisk.org/r/1814/#comment10641>

    Unlike with check_resync(), passing a pointer to delay isn't necessary here. Just pass by value instead.



/branches/1.8/main/jitterbuf.c
<https://reviewboard.asterisk.org/r/1814/#comment10642>

    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.



/branches/1.8/main/jitterbuf.c
<https://reviewboard.asterisk.org/r/1814/#comment10643>

    Spaces here, too.


- Mark


On March 13, 2012, 12:54 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1814/
> -----------------------------------------------------------
> 
> (Updated March 13, 2012, 12:54 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> 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.
> 
> 
> This addresses bug ASTERISK-18964.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18964
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/jitterbuf.c 358724 
> 
> Diff: https://reviewboard.asterisk.org/r/1814/diff
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> Matt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120313/77cbf77c/attachment.htm>