[Asterisk-code-review] chan sip.c: Fix T.38 issues caused by leaving a bridge. (asterisk[13])

[Anonymous Coward]

Anonymous Coward #1000019 has submitted this change and it was merged.

Change subject: chan_sip.c: Fix T.38 issues caused by leaving a bridge.
......................................................................


chan_sip.c: Fix T.38 issues caused by leaving a bridge.

chan_sip could not handle AST_T38_TERMINATED frames being sent to it when
the channel left the bridge.  The action resulted in overlapping outgoing
reINVITEs.  The testsuite tests/fax/sip/directmedia_reinvite_t38 was not
happy.

* Force T.38 to be remembered as locally bridged.  Now when the channel
leaves the native RTP bridge after T.38, the channel remembers that it has
already reINVITEed the media back to Asterisk.  It just needs to terminate
T.38 when the AST_T38_TERMINATED arrives.

* Prevent redundant AST_T38_TERMINATED from causing problems.  Redundant
AST_T38_TERMINATED frames could cause overlapping outgoing reINVITEs if
they happen before the T.38 state changes to disabled.  Now the T.38 state
is set to disabled before the reINVITE is sent.

ASTERISK-25582 #close

Change-Id: I53f5c6ce7d90b3f322a942af1a9bcab6d967b7ce
---
M channels/chan_sip.c
1 file changed, 30 insertions(+), 9 deletions(-)

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



diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index 3b6c005..4e0d811 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -7517,8 +7517,10 @@
 			AST_SCHED_DEL_UNREF(sched, p->t38id, dialog_unref(p, "when you delete the t38id sched, you should dec the refcount for the stored dialog ptr"));
 			change_t38_state(p, T38_REJECTED);
 			transmit_response_reliable(p, "488 Not acceptable here", &p->initreq);
-		} else if (p->t38.state == T38_ENABLED)
+		} else if (p->t38.state == T38_ENABLED) {
+			change_t38_state(p, T38_DISABLED);
 			transmit_reinvite_with_sdp(p, FALSE, FALSE);
+		}
 		break;
 	case AST_T38_REQUEST_PARMS: {		/* Application wants remote's parameters re-sent */
 		struct ast_control_t38_parameters parameters = p->t38.their_parms;
@@ -10676,6 +10678,10 @@
 		} else if (udptlportno > 0) {
 			if (debug)
 				ast_verbose("Got T.38 Re-invite without audio. Keeping RTP active during T.38 session.\n");
+
+			/* Force media to go through us for T.38. */
+			memset(&p->redirip, 0, sizeof(p->redirip));
+
 			/* Prevent audio RTCP reads */
 			if (p->owner) {
 				ast_channel_set_fd(p->owner, 1, -1);
@@ -13786,6 +13792,29 @@
 static int transmit_reinvite_with_sdp(struct sip_pvt *p, int t38version, int oldsdp)
 {
 	struct sip_request req;
+
+	if (t38version) {
+		/* Force media to go through us for T.38. */
+		memset(&p->redirip, 0, sizeof(p->redirip));
+	}
+	if (p->rtp) {
+		if (t38version) {
+			/* Silence RTCP while audio RTP is inactive */
+			ast_rtp_instance_set_prop(p->rtp, AST_RTP_PROPERTY_RTCP, 0);
+			if (p->owner) {
+				/* Prevent audio RTCP reads */
+				ast_channel_set_fd(p->owner, 1, -1);
+			}
+		} else if (ast_sockaddr_isnull(&p->redirip)) {
+			/* Enable RTCP since it will be inactive if we're coming back
+			 * with this reinvite */
+			ast_rtp_instance_set_prop(p->rtp, AST_RTP_PROPERTY_RTCP, 1);
+			if (p->owner) {
+				/* Enable audio RTCP reads */
+				ast_channel_set_fd(p->owner, 1, ast_rtp_instance_fd(p->rtp, 1));
+			}
+		}
+	}
 
 	reqprep(&req, p, ast_test_flag(&p->flags[0], SIP_REINVITE_UPDATE) ?  SIP_UPDATE : SIP_INVITE, 0, 1);
 
@@ -32683,14 +32712,6 @@
 	} else if (!ast_sockaddr_isnull(&p->redirip)) {
 		memset(&p->redirip, 0, sizeof(p->redirip));
 		changed = 1;
-
-		if (p->rtp) {
-			/* Enable RTCP since it will be inactive if we're coming back
-			 * from a reinvite */
-			ast_rtp_instance_set_prop(p->rtp, AST_RTP_PROPERTY_RTCP, 1);
-			/* Enable audio RTCP reads */
-			ast_channel_set_fd(chan, 1, ast_rtp_instance_fd(p->rtp, 1));
-		}
 	}
 
 	if (vinstance) {

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

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