Handle extremely out of order RFC 2833 DTMF

The current implementation of RFC 2833 DTMF handling in res_rtp_asterisk will,
if a packet arrives out of order, drop the packet.  This is to prevent
duplicate ton 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 the most part, this doesn't matter.  For a particular digit, so long as a
BEGIN packet arrives before the first END packet, the digit will be produced.
If subsequent BEGIN packets arrive interleaved with the ENDs, they will be
dropped; likewise, if the BEGIN or END packets themselves are out of order,
those packets are dropped but sufficient information is conveyed to the
Asterisk core to produce the appropriate digit.

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
is a real problem.  When an END arrives before any BEGINs, the END packet is
dropped - but at the same time, it causes subsequent BEGIN packets for that
digit to be ignored.  When the next in order END packet arrives, it too is
dropped - Asterisk believes that there was no initial BEGIN.

The solution this patch provides is to trust the END packet to convey the
information needed for the Asterisk core to produce the DTMF digit.  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 retransmission)
Then we send the END frame up to the Asterisk core.  It contains enough
DTMF information for Asterisk to produce the digit.

On the other hand, if we receive a BEGIN or continuation packet that occurs
with a timestamp equal to or less then the last END timestamp, then we've
received something out of order - but we already have received enough
information to produce the digit.  These packets are dropped.

Much thanks goes to Olle Johansson (oej) for providing the idea for this
solution.

Review: https://reviewboard.asterisk.org/r/2033/

(closes issue ASTERISK-18404)
Reported by: Stephane Chazelas
Tested by: Matt Jordan
........

Merged revisions 370252 from http://svn.asterisk.org/svn/asterisk/branches/1.8
........

Merged revisions 370271 from http://svn.asterisk.org/svn/asterisk/branches/10


git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@370272 65c4cc65-6c06-0410-ace0-fbb531ad65f3

1 files changed, 36 insertions, 23 deletions

diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index 6c6aa1578..d18729ddf 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -191,12 +191,13 @@ struct ast_rtp {
 	int rtpkeepalive;		/*!< Send RTP comfort noice packets for keepalive */
 
 	/* DTMF Reception Variables */
-	char resp;
-	unsigned int lastevent;
-	unsigned int dtmf_duration;     /*!< Total duration in samples since the digit start event */
-	unsigned int dtmf_timeout;      /*!< When this timestamp is reached we consider END frame lost and forcibly abort digit */
+	char resp;                        /*!< The current digit being processed */
+	unsigned int last_seqno;          /*!< The last known sequence number for any DTMF packet */
+	unsigned int last_end_timestamp;  /*!< The last known timestamp received from an END packet */
+	unsigned int dtmf_duration;       /*!< Total duration in samples since the digit start event */
+	unsigned int dtmf_timeout;        /*!< When this timestamp is reached we consider END frame lost and forcibly abort digit */
 	unsigned int dtmfsamples;
-	enum ast_rtp_dtmf_mode dtmfmode;/*!< The current DTMF mode of the RTP stream */
+	enum ast_rtp_dtmf_mode dtmfmode;  /*!< The current DTMF mode of the RTP stream */
 	/* DTMF Transmission Variables */
 	unsigned int lastdigitts;
 	char sending_digit;	/*!< boolean - are we sending digits */
@@ -2155,8 +2156,10 @@ static struct ast_frame *create_dtmf_frame(struct ast_rtp_instance *instance, en
 		rtp->dtmfsamples = 0;
 		return &ast_null_frame;
 	}
-	ast_debug(1, "Sending dtmf: %d (%c), at %s\n", rtp->resp, rtp->resp,
-		  ast_sockaddr_stringify(&remote_address));
+	ast_debug(1, "Creating %s DTMF Frame: %d (%c), at %s\n",
+		type == AST_FRAME_DTMF_END ? "END" : "BEGIN",
+		rtp->resp, rtp->resp,
+		ast_sockaddr_stringify(&remote_address));
 	if (rtp->resp == 'X') {
 		rtp->f.frametype = AST_FRAME_CONTROL;
 		rtp->f.subclass.integer = AST_CONTROL_FLASH;
@@ -2220,12 +2223,12 @@ static void process_dtmf_rfc2833(struct ast_rtp_instance *instance, unsigned cha
 	}
 
 	if (ast_rtp_instance_get_prop(instance, AST_RTP_PROPERTY_DTMF_COMPENSATE)) {
-		if ((rtp->lastevent != timestamp) || (rtp->resp && rtp->resp != resp)) {
+		if ((rtp->last_end_timestamp != timestamp) || (rtp->resp && rtp->resp != resp)) {
 			rtp->resp = resp;
 			rtp->dtmf_timeout = 0;
 			f = ast_frdup(create_dtmf_frame(instance, AST_FRAME_DTMF_END, ast_rtp_instance_get_prop(instance, AST_RTP_PROPERTY_DTMF_COMPENSATE)));
 			f->len = 0;
-			rtp->lastevent = timestamp;
+			rtp->last_end_timestamp = timestamp;
 			AST_LIST_INSERT_TAIL(frames, f, frame_list);
 		}
 	} else {
@@ -2242,31 +2245,41 @@ static void process_dtmf_rfc2833(struct ast_rtp_instance *instance, unsigned cha
 		}
 		new_duration = (new_duration & ~0xFFFF) | samples;
 
-		/* The second portion of this check is to not mistakenly
-		 * stop accepting DTMF if the seqno rolls over beyond
-		 * 65535.
-		 */
-		if (rtp->lastevent > seqno && rtp->lastevent - seqno < 50) {
-			/* Out of order frame. Processing this can cause us to
-			 * improperly duplicate incoming DTMF, so just drop
-			 * this.
-			 */
-			return;
-		}
-
 		if (event_end & 0x80) {
 			/* End event */
-			if ((rtp->lastevent != seqno) && rtp->resp) {
+			if ((rtp->last_seqno != seqno) && (timestamp > rtp->last_end_timestamp)) {
+				rtp->last_end_timestamp = timestamp;
 				rtp->dtmf_duration = new_duration;
+				rtp->resp = resp;
 				f = ast_frdup(create_dtmf_frame(instance, AST_FRAME_DTMF_END, 0));
 				f->len = ast_tvdiff_ms(ast_samp2tv(rtp->dtmf_duration, rtp_get_rate(&f->subclass.format)), ast_tv(0, 0));
 				rtp->resp = 0;
 				rtp->dtmf_duration = rtp->dtmf_timeout = 0;
 				AST_LIST_INSERT_TAIL(frames, f, frame_list);
+			} else if (rtpdebug) {
+				ast_debug(1, "Dropping duplicate or out of order DTMF END frame (seqno: %d, ts %d, digit %c)\n",
+					seqno, timestamp, resp);
 			}
 		} else {
 			/* Begin/continuation */
 
+			/* The second portion of the seqno check is to not mistakenly
+			 * stop accepting DTMF if the seqno rolls over beyond
+			 * 65535.
+			 */
+			if ((rtp->last_seqno > seqno && rtp->last_seqno - seqno < 50)
+				|| timestamp <= rtp->last_end_timestamp) {
+				/* Out of order frame. Processing this can cause us to
+				 * improperly duplicate incoming DTMF, so just drop
+				 * this.
+				 */
+				if (rtpdebug) {
+					ast_debug(1, "Dropping out of order DTMF frame (seqno %d, ts %d, digit %c)\n",
+						seqno, timestamp, resp);
+				}
+				return;
+			}
+
 			if (rtp->resp && rtp->resp != resp) {
 				/* Another digit already began. End it */
 				f = ast_frdup(create_dtmf_frame(instance, AST_FRAME_DTMF_END, 0));
@@ -2290,7 +2303,7 @@ static void process_dtmf_rfc2833(struct ast_rtp_instance *instance, unsigned cha
 			rtp->dtmf_timeout = timestamp + rtp->dtmf_duration + dtmftimeout;
 		}
 
-		rtp->lastevent = seqno;
+		rtp->last_seqno = seqno;
 	}
 
 	rtp->dtmfsamples = samples;