[Asterisk-code-review] res_pjsip_session: Don't restrict non-audio default streams to sendrecv. (asterisk[17])

[Joshua Colp]

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

Change subject: res_pjsip_session: Don't restrict non-audio default streams to sendrecv.
......................................................................

res_pjsip_session: Don't restrict non-audio default streams to sendrecv.

The state of the default audio stream is used for hold/unhold so we
restrict it to sendrecv as the core does not handle when it changes as
a result of hold/unhold.

This restriction does not apply to other media types though so we now
only restrict it to audio. This allows the other default streams to
store their state at all values, and not just sendrecv and removed.

ASTERISK-28783

Change-Id: I139740f38cea7f7d92a876ec2631ef50681f6625
---
M res/res_pjsip_session.c
1 file changed, 12 insertions(+), 11 deletions(-)

Approvals:
  Joshua Colp: Looks good to me, approved; Approved for Submit
  Benjamin Keith Ford: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, but someone else must approve



diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index c1e89c5..a20c2ea 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -760,8 +760,8 @@
 				ast_stream_free(stream);
 				return -1;
 			}
-			/* For backwards compatibility with the core default streams are always sendrecv */
-			if (!ast_sip_session_is_pending_stream_default(session, stream)) {
+			/* For backwards compatibility with the core the default audio stream is always sendrecv */
+			if (!ast_sip_session_is_pending_stream_default(session, stream) || strcmp(media, "audio")) {
 				if (pjmedia_sdp_media_find_attr2(remote_stream, "sendonly", NULL)) {
 					/* Stream state reflects our state of a stream, so in the case of
 					 * sendonly and recvonly we store the opposite since that is what ours
@@ -871,12 +871,16 @@
 	RAII_VAR(struct sdp_handler_list *, handler_list, NULL, ao2_cleanup);
 	int res;
 
+	/* We need a null-terminated version of the media string */
+	ast_copy_pj_str(media, &local->media[index]->desc.media, sizeof(media));
+
 	/* For backwards compatibility we only reflect the stream state correctly on
-	 * the non-default streams. This is because the stream state is also used for
-	 * signaling that someone has placed us on hold. This situation is not handled
-	 * currently and can result in the remote side being sort of placed on hold too.
+	 * the non-default streams and any non-audio streams. This is because the stream
+	 * state of the default audio stream is also used for signaling that someone has
+	 * placed us on hold. This situation is not handled currently and can result in
+	 * the remote side being sorted of placed on hold too.
 	 */
-	if (!ast_sip_session_is_pending_stream_default(session, asterisk_stream)) {
+	if (!ast_sip_session_is_pending_stream_default(session, asterisk_stream) || strcmp(media, "audio")) {
 		/* Determine the state of the stream based on our local SDP */
 		if (pjmedia_sdp_media_find_attr2(local_stream, "sendonly", NULL)) {
 			ast_stream_set_state(asterisk_stream, AST_STREAM_STATE_SENDONLY);
@@ -891,9 +895,6 @@
 		ast_stream_set_state(asterisk_stream, AST_STREAM_STATE_SENDRECV);
 	}
 
-	/* We need a null-terminated version of the media string */
-	ast_copy_pj_str(media, &local->media[index]->desc.media, sizeof(media));
-
 	set_mid_and_bundle_group(session, session_media, remote, remote->media[index]);
 	set_remote_mslabel_and_stream_group(session, session_media, remote, remote->media[index], asterisk_stream);
 
@@ -1965,8 +1966,8 @@
 			return -1;
 		}
 
-		/* For backwards compatibility with the core default streams are always sendrecv */
-		if (!ast_sip_session_is_pending_stream_default(session, stream)) {
+		/* For backwards compatibility with the core the default audio stream is always sendrecv */
+		if (!ast_sip_session_is_pending_stream_default(session, stream) || strcmp(media, "audio")) {
 			if (pjmedia_sdp_media_find_attr2(remote_stream, "sendonly", NULL)) {
 				/* Stream state reflects our state of a stream, so in the case of
 				 * sendonly and recvonly we store the opposite since that is what ours

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

Gerrit-Project: asterisk
Gerrit-Branch: 17
Gerrit-Change-Id: I139740f38cea7f7d92a876ec2631ef50681f6625
Gerrit-Change-Number: 13983
Gerrit-PatchSet: 2
Gerrit-Owner: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200325/7a5972d2/attachment-0001.html>