[Asterisk-code-review] res pjsip sdp rtp: Fix multiple keepalive scheduled items. (asterisk[certified/13.1])

[Matt Jordan]

Matt Jordan has submitted this change and it was merged.

Change subject: res_pjsip_sdp_rtp: Fix multiple keepalive scheduled items.
......................................................................


res_pjsip_sdp_rtp: Fix multiple keepalive scheduled items.

The keepalive support in res_pjsip_sdp_rtp currently assumes
that a stream will only be negotiated once. This is false.
If the stream is replaced and later added back it can be
negotiated again causing multiple keepalive scheduled items
to exist. This change explicitly deletes the existing
keepalive scheduled item before adding the new one.

The res_pjsip_sdp_rtp module also does not stop RTP
keepalives or timeout timer if the stream has been
replaced. This change adds a callback to the session media
interface to allow a media stream to be stopped without
the resources being destroyed. This allows the scheduled
items and RTP to be stopped when the stream no longer
exists.

ASTERISK-25356 #close

Change-Id: Ibe6a7cc0927c87326fd5f1c0d4ad889dbfbea1de
---
M include/asterisk/res_pjsip_session.h
M res/res_pjsip_sdp_rtp.c
M res/res_pjsip_session.c
3 files changed, 29 insertions(+), 3 deletions(-)

Approvals:
  Mark Michelson: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Matt Jordan: Looks good to me, approved



diff --git a/include/asterisk/res_pjsip_session.h b/include/asterisk/res_pjsip_session.h
index 5af19ff..7fbeddc 100644
--- a/include/asterisk/res_pjsip_session.h
+++ b/include/asterisk/res_pjsip_session.h
@@ -351,6 +351,12 @@
 	int (*apply_negotiated_sdp_stream)(struct ast_sip_session *session, struct ast_sip_session_media *session_media, const struct pjmedia_sdp_session *local, const struct pjmedia_sdp_media *local_stream,
 		const struct pjmedia_sdp_session *remote, const struct pjmedia_sdp_media *remote_stream);
 	/*!
+	 * \brief Stop a session_media created by this handler but do not destroy resources
+	 * \param session The session for which media is being stopped
+	 * \param session_media The media to destroy
+	 */
+	void (*stream_stop)(struct ast_sip_session_media *session_media);
+	/*!
 	 * \brief Destroy a session_media created by this handler
 	 * \param session The session for which media is being destroyed
 	 * \param session_media The media to destroy
diff --git a/res/res_pjsip_sdp_rtp.c b/res/res_pjsip_sdp_rtp.c
index 8ac1bb8..0b43125 100644
--- a/res/res_pjsip_sdp_rtp.c
+++ b/res/res_pjsip_sdp_rtp.c
@@ -1290,6 +1290,7 @@
 		 * a NAT. This way there won't be an awkward delay before media starts flowing in some
 		 * scenarios.
 		 */
+		AST_SCHED_DEL(sched, session_media->keepalive_sched_id);
 		session_media->keepalive_sched_id = ast_sched_add_variable(sched, 500, send_keepalive,
 			session_media, 1);
 	}
@@ -1341,13 +1342,23 @@
 	pj_strdup2(tdata->pool, &stream->conn->addr, transport->external_media_address);
 }
 
+/*! \brief Function which stops the RTP instance */
+static void stream_stop(struct ast_sip_session_media *session_media)
+{
+	if (!session_media->rtp) {
+		return;
+	}
+
+	AST_SCHED_DEL(sched, session_media->keepalive_sched_id);
+	AST_SCHED_DEL(sched, session_media->timeout_sched_id);
+	ast_rtp_instance_stop(session_media->rtp);
+}
+
 /*! \brief Function which destroys the RTP instance when session ends */
 static void stream_destroy(struct ast_sip_session_media *session_media)
 {
 	if (session_media->rtp) {
-		AST_SCHED_DEL(sched, session_media->keepalive_sched_id);
-		AST_SCHED_DEL(sched, session_media->timeout_sched_id);
-		ast_rtp_instance_stop(session_media->rtp);
+		stream_stop(session_media);
 		ast_rtp_instance_destroy(session_media->rtp);
 	}
 	session_media->rtp = NULL;
@@ -1360,6 +1371,7 @@
 	.create_outgoing_sdp_stream = create_outgoing_sdp_stream,
 	.apply_negotiated_sdp_stream = apply_negotiated_sdp_stream,
 	.change_outgoing_sdp_stream_media_address = change_outgoing_sdp_stream_media_address,
+	.stream_stop = stream_stop,
 	.stream_destroy = stream_destroy,
 };
 
@@ -1370,6 +1382,7 @@
 	.create_outgoing_sdp_stream = create_outgoing_sdp_stream,
 	.apply_negotiated_sdp_stream = apply_negotiated_sdp_stream,
 	.change_outgoing_sdp_stream_media_address = change_outgoing_sdp_stream_media_address,
+	.stream_stop = stream_stop,
 	.stream_destroy = stream_destroy,
 };
 
diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index 79aab76..8f603c7 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -362,6 +362,13 @@
 			}
 		}
 	}
+
+	if (session_media->handler && session_media->handler->stream_stop) {
+		ast_debug(1, "Stopping SDP media stream '%s' as it is not currently negotiated\n",
+			session_media->stream_type);
+		session_media->handler->stream_stop(session_media);
+	}
+
 	return CMP_MATCH;
 }
 

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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ibe6a7cc0927c87326fd5f1c0d4ad889dbfbea1de
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: certified/13.1
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>