[Asterisk-code-review] res pjsip session: properly handle SDP from a forked call wi... (asterisk[master])

[George Joseph]

George Joseph has uploaded this change for review. ( https://gerrit.asterisk.org/8379


Change subject: res_pjsip_session: properly handle SDP from a forked call with early media
......................................................................

res_pjsip_session: properly handle SDP from a forked call with early media

In handle_negotiated_sdp, use session->active_media_state when
session->pending_media_state is empty. The 200's SDP should be
fed into handle_negotiated_sdp_session_media together with
the already negotiated state, which is now in session->active_media_state
instead. Only if both the session's pending & active media are empty,
should handle_negotiated_sdp break.

ASTERISK-27441

Change-Id: If0d5150ffe6f38d8a854831fef37942258d4629c
---
M res/res_pjsip_session.c
1 file changed, 34 insertions(+), 19 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/79/8379/1

diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index fcd190b..ebbe395 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -874,16 +874,29 @@
 {
 	int i;
 	struct ast_stream_topology *topology;
+	struct ast_sip_session_media_state* media_state;
 	unsigned int changed = 0;
 
-	/* This situation can legitimately happen when an SDP is received in a
-	 * 183 Session Progress message.  In that case, everything's been done
-	 * by the time this function is called and there are no more pending
-	 * streams.
-	 */
-	if (!session->pending_media_state->topology) {
+	if (session->pending_media_state->topology) {
+		/* Normal case: use pending media state to negotiate;
+		 * the pending state becomes the active state after negotiation
+		 */
+		media_state = session->pending_media_state;
+
+	} else if (session->active_media_state) {
+		/* This happens when we're negotiated media after receiving a 183,
+		 * and we're now receiving a 200 with new SDP.
+		 * In this case, there is active_media_state, but the pending_media_state
+		 * has been reset.
+		 */
 		ast_debug(1, "Pending topology was NULL for channel '%s'\n",
-			session->channel ? ast_channel_name(session->channel) : "unknown");
+				  session->channel ? ast_channel_name(session->channel) : "unknown");
+		// Use active_media_state to negotiate
+		media_state = session->active_media_state;
+
+	} else {
+		ast_log(LOG_WARNING, "No pending or active media state for channel '%s'\n",
+				  session->channel ? ast_channel_name(session->channel) : "unknown");
 		return 0;
 	}
 
@@ -892,11 +905,11 @@
 	 * the local SDP, and there should be the same number of session medias
 	 * and streams as there are local SDP streams
 	 */
-	if (ast_stream_topology_get_count(session->pending_media_state->topology) != local->media_count
-		|| AST_VECTOR_SIZE(&session->pending_media_state->sessions) != local->media_count) {
+	if (ast_stream_topology_get_count(media_state->topology) != local->media_count
+		|| AST_VECTOR_SIZE(&media_state->sessions) != local->media_count) {
 		ast_log(LOG_WARNING, "Local SDP for channel '%s' contains %d media streams while we expected it to contain %u\n",
 			session->channel ? ast_channel_name(session->channel) : "unknown",
-			ast_stream_topology_get_count(session->pending_media_state->topology), local->media_count);
+			ast_stream_topology_get_count(media_state->topology), local->media_count);
 		return -1;
 	}
 
@@ -908,8 +921,8 @@
 			continue;
 		}
 
-		session_media = AST_VECTOR_GET(&session->pending_media_state->sessions, i);
-		stream = ast_stream_topology_get_stream(session->pending_media_state->topology, i);
+		session_media = AST_VECTOR_GET(&media_state->sessions, i);
+		stream = ast_stream_topology_get_stream(media_state->topology, i);
 
 		/* The stream state will have already been set to removed when either we
 		 * negotiate the incoming SDP stream or when we produce our own local SDP.
@@ -945,8 +958,8 @@
 			continue;
 		}
 
-		session_media = AST_VECTOR_GET(&session->pending_media_state->sessions, i);
-		stream = ast_stream_topology_get_stream(session->pending_media_state->topology, i);
+		session_media = AST_VECTOR_GET(&media_state->sessions, i);
+		stream = ast_stream_topology_get_stream(media_state->topology, i);
 
 		if (ast_stream_get_state(stream) == AST_STREAM_STATE_REMOVED
 			&& session_media->handler) {
@@ -960,7 +973,7 @@
 	}
 
 	/* Update the topology on the channel to match the accepted one */
-	topology = ast_stream_topology_clone(session->pending_media_state->topology);
+	topology = ast_stream_topology_clone(media_state->topology);
 	if (topology) {
 		ast_channel_set_stream_topology(session->channel, topology);
 	}
@@ -971,16 +984,18 @@
 	}
 
 	/* Add all the file descriptors from the pending media state */
-	for (i = 0; i < AST_VECTOR_SIZE(&session->pending_media_state->read_callbacks); ++i) {
+	for (i = 0; i < AST_VECTOR_SIZE(&media_state->read_callbacks); ++i) {
 		struct ast_sip_session_media_read_callback_state *callback_state;
 
-		callback_state = AST_VECTOR_GET_ADDR(&session->pending_media_state->read_callbacks, i);
+		callback_state = AST_VECTOR_GET_ADDR(&media_state->read_callbacks, i);
 		ast_channel_internal_fd_set(session->channel, i + AST_EXTENDED_FDS, callback_state->fd);
 	}
 
 	/* Active and pending flip flop as needed */
-	SWAP(session->active_media_state, session->pending_media_state);
-	ast_sip_session_media_state_reset(session->pending_media_state);
+	if (media_state == session->pending_media_state) {
+		SWAP(session->active_media_state, session->pending_media_state);
+		ast_sip_session_media_state_reset(session->pending_media_state);
+	}
 
 	ast_channel_unlock(session->channel);
 

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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If0d5150ffe6f38d8a854831fef37942258d4629c
Gerrit-Change-Number: 8379
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: lvl <digium at lvlconsultancy.nl>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180223/cbaa4c8d/attachment.html>