[Asterisk-code-review] res_pjsip_sdp_rtp: Improve detecting of lack of RTP activity (asterisk[18])

[Joshua Colp]

Joshua Colp has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/18230 )

Change subject: res_pjsip_sdp_rtp: Improve detecting of lack of RTP activity
......................................................................

res_pjsip_sdp_rtp: Improve detecting of lack of RTP activity

Change RTP timer behavior for detecting RTP only after two-way
SDP channel establishment. Ignore detecting after receiving 183
with SDP or while direct media is used.
Make rtp_timeout and rtp_timeout_hold options consistent to rtptimeout
and rtpholdtimeout options in chan_sip.

ASTERISK-26689 #close
ASTERISK-29929 #close

Change-Id: I07326d5b9c40f25db717fd6075f6f3a8d77279eb
---
M channels/chan_pjsip.c
M res/res_pjsip_sdp_rtp.c
2 files changed, 23 insertions(+), 35 deletions(-)

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



diff --git a/channels/chan_pjsip.c b/channels/chan_pjsip.c
index e8fbb3d..61c4cfb 100644
--- a/channels/chan_pjsip.c
+++ b/channels/chan_pjsip.c
@@ -337,14 +337,6 @@
 		ast_sockaddr_setnull(&media->direct_media_addr);
 		changed = 1;
 		if (media->rtp) {
-			/* Direct media has ended - reset time of last received RTP packet
-			 * to avoid premature RTP timeout. Synchronisation between the
-			 * modification of direct_mdedia_addr+last_rx here and reading the
-			 * values in res_pjsip_sdp_rtp.c:rtp_check_timeout() is provided
-			 * by the channel's lock (which is held while this function is
-			 * executed).
-			 */
-			ast_rtp_instance_set_last_rx(media->rtp, time(NULL));
 			ast_rtp_instance_set_prop(media->rtp, AST_RTP_PROPERTY_RTCP, 1);
 			if (position != -1) {
 				ast_channel_set_fd(chan, position + AST_EXTENDED_FDS, ast_rtp_instance_fd(media->rtp, 1));
diff --git a/res/res_pjsip_sdp_rtp.c b/res/res_pjsip_sdp_rtp.c
index e1d1701..3e10cfd 100644
--- a/res/res_pjsip_sdp_rtp.c
+++ b/res/res_pjsip_sdp_rtp.c
@@ -106,9 +106,10 @@
 {
 	struct ast_sip_session_media *session_media = (struct ast_sip_session_media *)data;
 	struct ast_rtp_instance *rtp = session_media->rtp;
-	int elapsed;
-	int timeout;
 	struct ast_channel *chan;
+	int elapsed;
+	int now;
+	int timeout;
 
 	if (!rtp) {
 		return 0;
@@ -119,41 +120,37 @@
 		return 0;
 	}
 
-	/* Get channel lock to make sure that we access a consistent set of values
-	 * (last_rx and direct_media_addr) - the lock is held when values are modified
-	 * (see send_direct_media_request()/check_for_rtp_changes() in chan_pjsip.c). We
-	 * are trying to avoid a situation where direct_media_addr has been reset but the
-	 * last-rx time was not set yet.
-	 */
-	ast_channel_lock(chan);
-
-	elapsed = time(NULL) - ast_rtp_instance_get_last_rx(rtp);
+	/* Store these values locally to avoid multiple function calls */
+	now = time(NULL);
 	timeout = ast_rtp_instance_get_timeout(rtp);
-	if (elapsed < timeout) {
-		ast_channel_unlock(chan);
+
+	/* If the channel is not in UP state or call is redirected
+	 * outside Asterisk return for later check.
+	 */
+	if (ast_channel_state(chan) != AST_STATE_UP || !ast_sockaddr_isnull(&session_media->direct_media_addr)) {
+		/* Avoiding immediately disconnect after channel up or direct media has been stopped */
+		ast_rtp_instance_set_last_rx(rtp, now);
 		ast_channel_unref(chan);
-		return (timeout - elapsed) * 1000;
+		/* Recheck after half timeout for avoiding possible races
+		* and faster reacting to cases while there is no an RTP at all.
+		*/
+		return timeout * 500;
 	}
 
-	/* Last RTP packet was received too long ago
-	 * - disconnect channel unless direct media is in use.
-	 */
-	if (!ast_sockaddr_isnull(&session_media->direct_media_addr)) {
-		ast_debug_rtp(3, "(%p) RTP not disconnecting channel '%s' for lack of %s RTP activity in %d seconds "
-			"since direct media is in use\n", rtp, ast_channel_name(chan),
-			ast_codec_media_type2str(session_media->type), elapsed);
-		ast_channel_unlock(chan);
+	elapsed = now - ast_rtp_instance_get_last_rx(rtp);
+	if (elapsed < timeout) {
 		ast_channel_unref(chan);
-		return timeout * 1000; /* recheck later, direct media may have ended then */
+		return (timeout - elapsed) * 1000;
 	}
 
 	ast_log(LOG_NOTICE, "Disconnecting channel '%s' for lack of %s RTP activity in %d seconds\n",
 		ast_channel_name(chan), ast_codec_media_type2str(session_media->type), elapsed);
 
+	ast_channel_lock(chan);
 	ast_channel_hangupcause_set(chan, AST_CAUSE_REQUESTED_CHAN_UNAVAIL);
-	ast_softhangup(chan, AST_SOFTHANGUP_DEV);
-
 	ast_channel_unlock(chan);
+
+	ast_softhangup(chan, AST_SOFTHANGUP_DEV);
 	ast_channel_unref(chan);
 
 	return 0;
@@ -2234,8 +2231,7 @@
 	}
 
 	if (ast_rtp_instance_get_timeout(session_media->rtp)) {
-		session_media->timeout_sched_id = ast_sched_add_variable(sched,
-			ast_rtp_instance_get_timeout(session_media->rtp) * 1000, rtp_check_timeout,
+		session_media->timeout_sched_id = ast_sched_add_variable(sched,	500, rtp_check_timeout,
 			session_media, 1);
 	}
 

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

Gerrit-Project: asterisk
Gerrit-Branch: 18
Gerrit-Change-Id: I07326d5b9c40f25db717fd6075f6f3a8d77279eb
Gerrit-Change-Number: 18230
Gerrit-PatchSet: 2
Gerrit-Owner: Boris P. Korzun <drtr0jan at yandex.ru>
Gerrit-Reviewer: Friendly Automation
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/20220406/a60f981d/attachment-0001.html>