[asterisk-commits] mjordan: branch 12 r427582 - /branches/12/bridges/bridge_native_rtp.c

[SVN commits to the Asterisk project]

Author: mjordan
Date: Sat Nov  8 18:00:09 2014
New Revision: 427582

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=427582
Log:
bridge_native_rtp: Fix T.38 issues with remote bridges

After r425242 the fax/sip/directmedia_reinvite_t38 test started failing due to
the surviving channel not being re-INVITEd back from T.38 to audio. This patch
fixes that bug - a deeper explanation of what happened follows.

When two RTP channels are in a native bridge, the bridging layer will
investigate each via the get_rtp_info glue callback. This callback returns the
native bridge preference of the channel *at that moment in time* (that part is
key). At different points during the bridging, the native bridging layer will
inform the RTP capable channels of the status of the bridge via the update_peer
glue callback.

In a T.38 scenario with audio direct media, the sequence of events will often
look like the following:
 * SIP/A and SIP/B both have audio and enter a native bridge.
 * Asterisk re-INVITEs audio between SIP/A and SIP/B directly (via an
   update_peer callback).
 * SIP/A sends a re-INVITE to T.38, which causes Asterisk to send a re-INVITE
   to T.38 to SIP/B. Assuming everyone 200 OKs the process, the UDPTL stack
   receives UDPTL packets in Asterisk from both endpoints. From the perspective
   of the channels, we are now in a local bridge for T.38, even though we are
   technically still in a remote bridge in bridge_native_rtp. (YAY!)
 * When one side hangs up, bridge_native_rtp is told to stop bridging. It then
   re-evaluates the channels and asks them how they are bridged - and since
   T.38 is enabled, they reply with a Local bridge (which is correct), but is
   wrong because the audio portion is still technically in a remote bridge.
 * Asterisk releases the surviving channel, whose audio is *not* re-INVITED
   back to Asterisk as bridge_native_rtp incorrectly assumes that it was in a
   local bridge.

Ironically, prior to r425242, this used to work mostly due to a fluke in the
bridging layer.

The purpose of the get_rtp_info callback shouldn't be modified: it should tell
the bridging layer what kind of bridge the channel prefers at that moment in
time. If you have T.38 enabled, that *must* be a local bridge, as the UDPTPL
stack must be in the media path. As such, this patch does not modify that
part of the code.

However, we have to tell the channels to re-evaluate themselves when they come
out of a native bridge, since we can no longer trust the get_rtp_info callbacks
when the native bridge is being stopped. Something else may have changed in the
channels, and they may now be lying to us. As such, this patch makes it so that
we unilaterally tell the channels that they are no longer bridged via the
update_peer callback. This is actually what the channels expect anyway: code in
both chan_sip and chan_pjsip's callbacks look at the T.38 state and - if they
were in T.38 - send a re-INVITE to get the audio back to Asterisk.

Review: https://reviewboard.asterisk.org/r/4157/

Modified:
    branches/12/bridges/bridge_native_rtp.c

Modified: branches/12/bridges/bridge_native_rtp.c
URL: http://svnview.digium.com/svn/asterisk/branches/12/bridges/bridge_native_rtp.c?view=diff&rev=427582&r1=427581&r2=427582
==============================================================================
--- branches/12/bridges/bridge_native_rtp.c (original)
+++ branches/12/bridges/bridge_native_rtp.c Sat Nov  8 18:00:09 2014
@@ -229,10 +229,7 @@
 		}
 		break;
 	case AST_RTP_GLUE_RESULT_REMOTE:
-		if (!target) {
-			glue0->update_peer(bc0->chan, NULL, NULL, NULL, NULL, 0);
-			glue1->update_peer(bc1->chan, NULL, NULL, NULL, NULL, 0);
-		} else {
+		if (target) {
 			/*
 			 * If a target was provided, it is being put on hold and should expect to
 			 * receive media from Asterisk instead of what it was previously connected to.
@@ -246,6 +243,11 @@
 		break;
 	case AST_RTP_GLUE_RESULT_FORBID:
 		break;
+	}
+
+	if (!target && native_type != AST_RTP_GLUE_RESULT_FORBID) {
+		glue0->update_peer(bc0->chan, NULL, NULL, NULL, NULL, 0);
+		glue1->update_peer(bc1->chan, NULL, NULL, NULL, NULL, 0);
 	}
 
 	ast_debug(2, "Discontinued RTP bridging of '%s' and '%s' - media will flow through Asterisk core\n",