[asterisk-dev] [Code Review] Handle (extremely) out of order RFC2833 DTMF in res_rtp_asterisk

[Mark Michelson]

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

Ship it!


I went through the various possibilities of out-of-order DTMF frames, and this appears to handle them all properly. The RFC 2833 DTMF test still passing is a good indication that all is well too.

- Mark


On July 12, 2012, 12:04 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2033/
> -----------------------------------------------------------
> 
> (Updated July 12, 2012, 12:04 p.m.)
> 
> 
> Review request for Asterisk Developers and Olle E Johansson.
> 
> 
> Summary
> -------
> 
> As ASTERISK-18404 notes, the current implementation of RFC2833 DTMF handling in res_rtp_asterisk will, if a packet arrives out of order, drop the packet.  This is to prevent duplicate tone generation in the Asterisk core.  Since the RTP layer does not buffer data itself, this is the only option the RTP layer currently has for handling packets that arrive out of order.
> 
> For certain sequences of DTMF packets - most notably when, for a particular digit, an END packet arrives before any BEGIN packet for that digit - this causes the entire digit to be lost.  The END packet is erroneously dropped, even though it occurs before any BEGIN packets for that digit, because the DTMF handling in res_rtp_asterisk assumes that we must have at least one BEGIN packet before an END.  If it receives an END and no digit is currently being 'handled', it will drop the END packet.
> 
> Originally, we thought of using a DTMF aware jitter buffer to handle this situation, and allowing all frames to pass up through the RTP layer to the Asterisk core where they would be re-ordered by the jitter buffer (see https://reviewboard.asterisk.org/r/2010/).  While this worked, it did introduce a delay in the DTMF processing, required timestamps on the DTMF frames (along with some mild manipulation of said timestamps), and was relatively complex, in that it required substantial changes to func_jitterbuffer.
> 
> Olle hit upon a simpler solution - simply trust the END packet to convey the information required.  If we receive an END packet, and it:
> * Has a timestamp greater then the last timestamp received from an END packet
> * Does not have the same sequence number as the last received sequence number (and is thus not an END packet retransmitted)
> Then send the END frame up to the Asterisk core.  It contains the appropriate DTMF duration and sufficient information for Asterisk to produce the DTMF digit.
> 
> On the other hand, if we receive a BEGIN or continuation packet that occurs with a timestamp equal to the last END timestamp, then we've received something out of order - probably the END packet.  Since the END packet contained all of the information we needed, drop the BEGIN/continuation packets that were for that DTMF digit.
> 
> This patch uses Olle's approach (and cleans up some handling in the process_dtmf_rfc2833 code as well - the rtp->lastevent field was used as both a timestamp and a sequence number, which is just wrong).  It handles the situation described in ASTERISK-18404.
> 
> This review resolves the intent of https://reviewboard.asterisk.org/r/2010/, thereby replacing it.  As such, that review - and the tests that went along with it - will be closed out.  The unit tests/functional tests for that patch will be refactored to apply only to voice jitter buffers, as they are still useful.
> 
> 
> This addresses bug ASTERISK-18404.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18404
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/res/res_rtp_asterisk.c 369992 
> 
> Diff: https://reviewboard.asterisk.org/r/2033/diff
> 
> 
> Testing
> -------
> 
> Passed the Test Suite's rfc2833_dtmf_detect test.
> 
> Passed a modified DTMF pcap which consisted of:
> 
> Digit   Seqno    Timestamp   Mark
>  1      8318     3659342064  End
>  1      8315     3659342064  
>  1      8316     3659342064  
>  1      8314     3659342064  
>  1      8317     3659342064  
>  1      8319     3659342064  End
>  1      8320     3659342064  End
> 
> Without this patch, this digit will not be detected by the current DTMF handling.  Because the END packet arrived before a BEGIN, it gets dropped, but not before it sets the last known seqno to 8318.  All BEGIN packets after that occur before that seqno, and are dropped as out of order.  The last two END packets are then dropped as well, as no BEGIN packet was 'seen'.
> 
> 
> Thanks,
> 
> Matt
> 
>

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