[Asterisk-code-review] res pjsip: Handle deferred SDP hold/unhold properly. (asterisk[master])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged.

Change subject: res_pjsip: Handle deferred SDP hold/unhold properly.
......................................................................


res_pjsip: Handle deferred SDP hold/unhold properly.

Some SIP devices indicate hold/unhold using deferred SDP reinvites. In
other words, they provide no SDP in the reinvite.

A typical transaction that starts hold might look something like this:

* Device sends reinvite with no SDP
* Asterisk sends 200 OK with SDP indicating sendrecv on streams.
* Device sends ACK with SDP indicating sendonly on streams.

At this point, PJMedia's SDP negotiator saves Asterisk's local state as
being recvonly.

Now, when the device attempts to unhold, it again uses a deferred SDP
reinvite, so we end up doing the following:

* Device sends reinvite with no SDP
* Asterisk sends 200 OK with SDP indicating recvonly on streams
* Device sends ACK with SDP indicating sendonly on streams

The problem here is that Asterisk offered recvonly, and by RFC 3264's
rules, if an offer is recvonly, the answer has to be sendonly. The
result is that the device is not taken off hold.

What is supposed to happen is that Asterisk should indicate sendrecv in
the 200 OK that it sends. This way, the device has the freedom to
indicate sendrecv if it wants the stream taken off hold, or it can
continue to respond with sendonly if the purpose of the reinvite was
something else (like a session timer refresher).

The fix here is to alter the SDP negotiator's state when we receive a
reinvite with no SDP. If the negotiator's state is currently in the
recvonly or inactive state, then we alter our local state to be
sendrecv. This way, we allow the device to indicate the stream state as
desired.

ASTERISK-25854 #close
Reported by Robert McGilvray

Change-Id: I7615737276165eef3a593038413d936247dcc6ed
---
M res/res_pjsip_session.c
1 file changed, 47 insertions(+), 0 deletions(-)

Approvals:
  Richard Mudgett: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved; Verified



diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index 80fe8f3..59d759f 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -1125,7 +1125,54 @@
 	}
 
 	if (!sdp_info->sdp) {
+		const pjmedia_sdp_session *local;
+		int i;
+
 		ast_queue_unhold(session->channel);
+
+		pjmedia_sdp_neg_get_active_local(session->inv_session->neg, &local);
+		if (!local) {
+			return PJ_FALSE;
+		}
+
+		/*
+		 * Some devices indicate hold with deferred SDP reinvites (i.e. no SDP in the reinvite).
+		 * When hold is initially indicated, we
+		 * - Receive an INVITE with no SDP
+		 * - Send a 200 OK with SDP, indicating sendrecv in the media streams
+		 * - Receive an ACK with SDP, indicating sendonly in the media streams
+		 *
+		 * At this point, the pjmedia negotiator saves the state of the media direction so that
+		 * if we are to send any offers, we'll offer recvonly in the media streams. This is
+		 * problematic if the device is attempting to unhold, though. If the device unholds
+		 * by sending a reinvite with no SDP, then we will respond with a 200 OK with recvonly.
+		 * According to RFC 3264, if an offerer offers recvonly, then the answerer MUST respond
+		 * with sendonly or inactive. The result of this is that the stream is not off hold.
+		 *
+		 * Therefore, in this case, when we receive a reinvite while the stream is on hold, we
+		 * need to be sure to offer sendrecv. This way, the answerer can respond with sendrecv
+		 * in order to get the stream off hold. If this is actually a different purpose reinvite
+		 * (like a session timer refresh), then the answerer can respond to our sendrecv with
+		 * sendonly, keeping the stream on hold.
+		 */
+		for (i = 0; i < local->media_count; ++i) {
+			pjmedia_sdp_media *m = local->media[i];
+			pjmedia_sdp_attr *recvonly;
+			pjmedia_sdp_attr *inactive;
+
+			recvonly = pjmedia_sdp_attr_find2(m->attr_count, m->attr, "recvonly", NULL);
+			inactive = pjmedia_sdp_attr_find2(m->attr_count, m->attr, "inactive", NULL);
+			if (recvonly || inactive) {
+				pjmedia_sdp_attr *to_remove = recvonly ?: inactive;
+				pjmedia_sdp_attr *sendrecv;
+
+				pjmedia_sdp_attr_remove(&m->attr_count, m->attr, to_remove);
+
+				sendrecv = pjmedia_sdp_attr_create(session->inv_session->pool, "sendrecv", NULL);
+				pjmedia_sdp_media_add_attr(m, sendrecv);
+			}
+		}
+
 		return PJ_FALSE;
 	}
 

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I7615737276165eef3a593038413d936247dcc6ed
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>