[Asterisk-code-review] res_rtp_asterisk: More robust timestamp checking (asterisk[18])

[Friendly Automation]

Friendly Automation has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/15842 )

Change subject: res_rtp_asterisk: More robust timestamp checking
......................................................................

res_rtp_asterisk: More robust timestamp checking

We assume that a timestamp value of 0 represents an 'uninitialized'
timestamp, but 0 is a valid value. Add a simple wrapper to be able to
differentiate between whether the value is set or not.

This also removes the fix for ASTERISK~28812 which should not be
needed if we are checking the last timestamp appropriately.

ASTERISK-29030 #close

Change-Id: Ie70d657d580d9a1f2877e25a6ef161c5ad761cf7
---
M res/res_rtp_asterisk.c
1 file changed, 19 insertions(+), 9 deletions(-)

Approvals:
  Joshua Colp: Looks good to me, but someone else must approve
  Kevin Harwell: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved
  Friendly Automation: Approved for Submit



diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index 607fd91..20504cb 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -356,6 +356,11 @@
 	int schedid;
 };
 
+typedef struct {
+	unsigned int ts;
+	unsigned char is_set;
+} optional_ts;
+
 /*! \brief RTP session description */
 struct ast_rtp {
 	int s;
@@ -392,7 +397,7 @@
 	/* DTMF Reception Variables */
 	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 */
+	optional_ts 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;
@@ -5536,12 +5541,13 @@
 	}
 
 	if (ast_rtp_instance_get_prop(instance, AST_RTP_PROPERTY_DTMF_COMPENSATE)) {
-		if ((rtp->last_end_timestamp != timestamp) || (rtp->resp && rtp->resp != resp)) {
+		if (!rtp->last_end_timestamp.is_set || rtp->last_end_timestamp.ts != 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->last_end_timestamp = timestamp;
+			rtp->last_end_timestamp.ts = timestamp;
+			rtp->last_end_timestamp.is_set = 1;
 			AST_LIST_INSERT_TAIL(frames, f, frame_list);
 		}
 	} else {
@@ -5560,8 +5566,9 @@
 
 		if (event_end & 0x80) {
 			/* End event */
-			if ((rtp->last_seqno != seqno) && ((timestamp > rtp->last_end_timestamp) || ((timestamp == 0) && (rtp->last_end_timestamp == 0)))) {
-				rtp->last_end_timestamp = timestamp;
+			if (rtp->last_seqno != seqno && (!rtp->last_end_timestamp.is_set || timestamp > rtp->last_end_timestamp.ts)) {
+				rtp->last_end_timestamp.ts = timestamp;
+				rtp->last_end_timestamp.is_set = 1;
 				rtp->dtmf_duration = new_duration;
 				rtp->resp = resp;
 				f = ast_frdup(create_dtmf_frame(instance, AST_FRAME_DTMF_END, 0));
@@ -5581,7 +5588,8 @@
 			 * 65535.
 			 */
 			if ((rtp->last_seqno > seqno && rtp->last_seqno - seqno < 50)
-				|| timestamp <= rtp->last_end_timestamp) {
+			   || (rtp->last_end_timestamp.is_set
+				  && timestamp <= rtp->last_end_timestamp.ts)) {
 				/* Out of order frame. Processing this can cause us to
 				 * improperly duplicate incoming DTMF, so just drop
 				 * this.
@@ -6671,7 +6679,7 @@
 	 * re-transmissions of the last dtmf end still.  Feed those to the
 	 * core so they can be filtered accordingly.
 	 */
-	if (rtp->last_end_timestamp == timestamp) {
+	if (rtp->last_end_timestamp.is_set && rtp->last_end_timestamp.ts == timestamp) {
 		ast_debug_rtp(1, "(%p, %p) RTP feeding packet with duplicate timestamp to core\n", instance, instance1);
 		return -1;
 	}
@@ -7279,7 +7287,8 @@
 			rtp->cycles = 0;
 			prev_seqno = 0;
 			rtp->last_seqno = 0;
-			rtp->last_end_timestamp = 0;
+			rtp->last_end_timestamp.ts = 0;
+			rtp->last_end_timestamp.is_set = 0;
 			if (rtp->rtcp) {
 				rtp->rtcp->expected_prior = 0;
 				rtp->rtcp->received_prior = 0;
@@ -8518,7 +8527,8 @@
 
 	/* Need to reset the DTMF last sequence number and the timestamp of the last END packet */
 	rtp->last_seqno = 0;
-	rtp->last_end_timestamp = 0;
+	rtp->last_end_timestamp.ts = 0;
+	rtp->last_end_timestamp.is_set = 0;
 
 	if (strictrtp && rtp->strict_rtp_state != STRICT_RTP_OPEN
 		&& !ast_sockaddr_isnull(addr) && ast_sockaddr_cmp(addr, &rtp->strict_rtp_address)) {

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/15842
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 18
Gerrit-Change-Id: Ie70d657d580d9a1f2877e25a6ef161c5ad761cf7
Gerrit-Change-Number: 15842
Gerrit-PatchSet: 2
Gerrit-Owner: Sean Bright <sean at seanbright.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210430/f4bb3e45/attachment-0001.html>