[asterisk-dev] [Code Review]: A Jitter Buffer for out of order RFC2833 DTMF handling

Matt Jordan reviewboard at asterisk.org
Mon Jul 9 09:11:00 CDT 2012 


> On July 9, 2012, 1:39 a.m., Olle E Johansson wrote:
> > Matt, please take a look at my rana-dtmf-duration branch as this changes the behavior of DTMF in RTP quite a lot. There's a lot of issues with DTMF, one of the biggest is that we don't update the duration that we receive on the inbound stream to the outbound stream. In that branch I send "continue" DTMF frames across the bridge to update the current state and also queue if we get a new DTMF while we are still playing out the old one. Feel free to contact me if you have any questions about it.
> 
> Matt Jordan wrote:
>     Hi Olle!
>     
>     I'm unclear - did you mean that your rana-dtmf-duration branch changes the behavior of the RTP layer, or that this patch changes the behavior of the RTP layer?  If you meant that this patch changes the behavior of DTMF in the RTP layer - it actually doesn't.  In fact, it goes out of its way to preserve the existing behavior when a DTMF capable jitter buffer does not exist.  The only thing that changes in the existing behavior is the inclusion of time information on the ast_frame objects, which would be completely unused by the bridging layers without a jitter buffer.  Semantically, without a jitter buffer on the read side of the channel, this patch does nothing.
>     
>     I agree it does change the RTP layer behavior with the inclusion of a DTMF capable jitter buffer - but in that case, all it does is stop the RTP layer from dropping packets that arrive out of order.  The actual formation of the DTMF packets, as well as their encoded durations are kept the same.  The previously mentioned timestamps are nudged slightly on the END DTMF packets, but that does not affect the emulated duration of the DTMF in the Asterisk core, or on the outbound DTMF.
>     
>     The purpose of this patch was not to address duration issues - I agree that's a problem, but it's a completely separate problem from what this patch is attempting to address.  It is something that we may want to look into for a future version of Asterisk.
>     
>     Since this patch preserves 'nominal' behavior in the RTP layer, as well as the presentation of BEGIN/END frames to the bridging layer, this patch should play nicely with future changes to both the RTP and bridging layers.  Specifically, with regards to your rana-dtmf-duration branch, the changes should not impact the bridging layer deciding to alter the DTMF state for an outbound channel - so things should merge together well.
> 
> Olle E Johansson wrote:
>     Well, I'm not sure that your patch is needed if we capture the continue packets... Adding a jitter buffer adds delay, so you may cause more harm than your solving, Matt. We already have a lot of issues in deployed servers with DTMF delay, so it's not a good solution to add even more. I'll be happy to discuss this over jabber or any other realtime way. The review board is not much of realtime, is it?
>     
>     /O

I'm in #asterisk-dev now if you'd like to discuss it.  If not, I'll reply here a bit later with answers to your concerns :-)


- Matt


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


On July 7, 2012, 10:01 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2010/
> -----------------------------------------------------------
> 
> (Updated July 7, 2012, 10:01 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> 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.
> 
> 
> This addresses bug ASTERISK-18404.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18404
> 
> 
> Diffs
> -----
> 
>   /trunk/main/channel.c 369768 
>   /trunk/main/file.c 369768 
>   /trunk/main/fixedjitterbuf.c 369768 
>   /trunk/main/framehook.c 369768 
>   /trunk/main/jitterbuf.c 369768 
>   /trunk/main/rtp_engine.c 369768 
>   /trunk/res/res_rtp_asterisk.c 369769 
>   /trunk/tests/test_abstract_jb.c PRE-CREATION 
>   /trunk/include/asterisk/abstract_jb.h 369768 
>   /trunk/include/asterisk/frame.h 369768 
>   /trunk/include/asterisk/framehook.h 369768 
>   /trunk/include/asterisk/rtp_engine.h 369768 
>   /trunk/main/abstract_jb.c 369768 
>   /trunk/addons/chan_ooh323.c 369768 
>   /trunk/channels/chan_h323.c 369769 
>   /trunk/channels/chan_jingle.c 369769 
>   /trunk/channels/chan_local.c 369769 
>   /trunk/channels/chan_mgcp.c 369769 
>   /trunk/channels/chan_motif.c 369769 
>   /trunk/channels/chan_oss.c 369769 
>   /trunk/channels/chan_sip.c 369769 
>   /trunk/channels/chan_skinny.c 369769 
>   /trunk/channels/chan_unistim.c 369769 
>   /trunk/channels/misdn_config.c 369769 
>   /trunk/funcs/func_frame_trace.c 369768 
>   /trunk/funcs/func_jitterbuffer.c 369768 
>   /trunk/channels/chan_alsa.c 369769 
>   /trunk/channels/chan_console.c 369769 
>   /trunk/channels/chan_dahdi.c 369769 
>   /trunk/channels/chan_gtalk.c 369769 
> 
> Diff: https://reviewboard.asterisk.org/r/2010/diff
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> Matt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120709/e9cc36a7/attachment-0001.htm>

More information about the asterisk-dev mailing list