<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/4157/">https://reviewboard.asterisk.org/r/4157/</a>
</td>
</tr>
</table>
<br />
<table bgcolor="#e0e0e0" width="100%" cellpadding="8" style="border: 1px gray solid;">
<tr>
<td>
<h1 style="margin-right: 0.2em; padding: 0; font-size: 10pt;">This change has been marked as submitted.</h1>
</td>
</tr>
</table>
<br />
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers and Joshua Colp.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated Nov. 8, 2014, 6 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Changes</h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Committed in revision 427582</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">When Josh merged his "please stop lying to me bridging layer" patch, the fax/sip/directmedia_reinvite_t38 test started failing. After some investigation, we figured out why.
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, 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 (since direct media + T.38 is a path of craziness that I don't want to contemplate. This is what you get when you embed an antiquated PSTN-based protocol into an unreliable UDP stream. Blech). As such, I didn't change that callback.
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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">SIP and PJSIP tests pass (at least, the ones that should be passing).
The fax/sip/directmedia_reinvite_t38 test now passes, as audio is now re-INVITED back to Asterisk for the surviving channel.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/12/bridges/bridge_native_rtp.c <span style="color: grey">(427539)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4157/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>