[Asterisk-code-review] res rtp asterisk.c: Fix rtp source address learning for brok... (asterisk[13])

[Pirmin Walthert]

Pirmin Walthert has uploaded this change for review. ( https://gerrit.asterisk.org/7237


Change subject: res_rtp_asterisk.c: Fix rtp source address learning for broken clients
......................................................................

res_rtp_asterisk.c: Fix rtp source address learning for broken clients

Some clients do not send rtp packets every ptime ms. This can lead to
situations in which the rtp source learning algorithm will never learn
the address of the client. This has been discovered on a Mac mini with
a pjsip based softphone after updating to Sierra: as soon as USB
headsets are involved, the softphone will send the second packet 30ms
after the first, the third 30ms after the second and the fourth 1ms
after the third. So in the old implmentation the rtp source learning
algorithm was repeatedly reset on the fourth packet.

The patch changes the algorithm in a way that doesn't take the arrival
time between two consecutive packets into account but the time between
the first and the last packet of a learning sequence.

The patch also fixes a second problem: when a user was using a wrong
value for the probation setting there was a LOG_WARNING output stating
that the value had been set to the default value instead. However
the code for setting the value back to defaults was missing.

ASTERISK-27421 #close

Change-Id: If778fe07678a6fd2041eaca7cd78267d0ef4fc6c
---
M res/res_rtp_asterisk.c
1 file changed, 21 insertions(+), 14 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/37/7237/1

diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index a9bdf68..ab5a696 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -114,6 +114,7 @@
 #define ZFONE_PROFILE_ID 0x505a
 
 #define DEFAULT_LEARNING_MIN_SEQUENTIAL 4
+#define DEFAULT_LEARNING_MIN_DURATION 30
 
 #define SRTP_MASTER_KEY_LEN 16
 #define SRTP_MASTER_SALT_LEN 14
@@ -150,6 +151,7 @@
 #endif
 static int strictrtp = DEFAULT_STRICT_RTP; /*!< Only accept RTP frames from a defined source. If we receive an indication of a changing source, enter learning mode. */
 static int learning_min_sequential = DEFAULT_LEARNING_MIN_SEQUENTIAL; /*!< Number of sequential RTP frames needed from a single source during learning mode to accept new source. */
+static int learning_min_duration = DEFAULT_LEARNING_MIN_DURATION; /*!< Lowest acceptable timeout between the first and the last sequential RTP frame. */
 #ifdef HAVE_PJPROJECT
 static int icesupport = DEFAULT_ICESUPPORT;
 static struct sockaddr_in stunaddr;
@@ -230,7 +232,7 @@
 struct rtp_learning_info {
 	struct ast_sockaddr proposed_address;	/*!< Proposed remote address for strict RTP */
 	struct timeval start;	/*!< The time learning mode was started */
-	struct timeval received; /*!< The time of the last received packet */
+	struct timeval received; /*!< The time of the first received packet */
 	int max_seq;	/*!< The highest sequence number received */
 	int packets;	/*!< The number of remaining packets before the source is accepted */
 };
@@ -2781,26 +2783,28 @@
  * \retval non-zero if probation mode should continue
  */
 static int rtp_learning_rtp_seq_update(struct rtp_learning_info *info, uint16_t seq)
-{
-	/*
-	 * During the learning mode the minimum amount of media we'll accept is
-	 * 10ms so give a reasonable 5ms buffer just in case we get it sporadically.
-	 */
-	if (!ast_tvzero(info->received) && ast_tvdiff_ms(ast_tvnow(), info->received) < 5) {
-		/*
-		 * Reject a flood of packets as acceptable for learning.
-		 * Reset the needed packets.
-		 */
-		info->packets = learning_min_sequential - 1;
-	} else if (seq == (uint16_t) (info->max_seq + 1)) {
+{	
+	if (seq == (uint16_t) (info->max_seq + 1)) {
 		/* packet is in sequence */
 		info->packets--;
 	} else {
 		/* Sequence discontinuity; reset */
 		info->packets = learning_min_sequential - 1;
+		info->received = ast_tvnow();
+	}
+	
+	/*
+	 * During the learning mode the minimum amount of media we'll accept is
+	 * 10ms. So check whether we have received the four packets in less than
+	 * a reasonable amount of time. */
+	if (ast_tvzero(info->received)) {
+		info->received = ast_tvnow();
+	} else if (!info->packets && (ast_tvdiff_ms(ast_tvnow(), info->received) < learning_min_duration )) {
+		/* Packet flood; reset */
+		info->packets = learning_min_sequential - 1;
+		info->received = ast_tvnow();
 	}
 	info->max_seq = seq;
-	info->received = ast_tvnow();
 
 	return info->packets;
 }
@@ -6483,6 +6487,7 @@
 	dtmftimeout = DEFAULT_DTMF_TIMEOUT;
 	strictrtp = DEFAULT_STRICT_RTP;
 	learning_min_sequential = DEFAULT_LEARNING_MIN_SEQUENTIAL;
+	learning_min_duration = DEFAULT_LEARNING_MIN_DURATION;
 
 	/** This resource is not "reloaded" so much as unloaded and loaded again.
 	 * In the case of the TURN related variables, the memory referenced by a
@@ -6550,7 +6555,9 @@
 		if ((sscanf(s, "%d", &learning_min_sequential) <= 0) || learning_min_sequential <= 0) {
 			ast_log(LOG_WARNING, "Value for 'probation' could not be read, using default of '%d' instead\n",
 				DEFAULT_LEARNING_MIN_SEQUENTIAL);
+			learning_min_sequential = DEFAULT_LEARNING_MIN_SEQUENTIAL;
 		}
+		learning_min_duration = (learning_min_sequential - 1) * 10;
 	}
 #ifdef HAVE_PJPROJECT
 	if ((s = ast_variable_retrieve(cfg, "general", "icesupport"))) {

-- 
To view, visit https://gerrit.asterisk.org/7237
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: newchange
Gerrit-Change-Id: If778fe07678a6fd2041eaca7cd78267d0ef4fc6c
Gerrit-Change-Number: 7237
Gerrit-PatchSet: 1
Gerrit-Owner: Pirmin Walthert <infos at nappsoft.ch>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171116/c1958843/attachment.html>