<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/1716/">https://reviewboard.asterisk.org/r/1716/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 8th, 2012, 12:06 p.m., <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This approach is not going to work for a few reasons:
1. This change assumes that both channels are SIP channels. Besides the fact that the assumption is made in a dangerous way, H.323 and Skinny also support UDPTL and would not see the benefits of this change.
2. This change places bridging logic into a read callback. Having the reading channel peek across the bridge to get the other channel's private data should be avoided if at all possible.
3. This change does not do any sort of compatibility checks between the bridged channels.
I did some digging in the Asterisk trunk code and found that there is an ast_udptl_bridge() function already defined in udptl.c. It appears to do what the goal of this patch is, plus it adds necessary checks for compatibility and other possible reasons why the bridge might fail. The problem is that there is no code that actually uses ast_udptl_bridge() in Asterisk trunk. I think the proper approach to this change is to add appropriate calls to ast_udptl_bridge() where they should go. I think the way to go is to change the .bridge callbacks in UDPTL-capable channel drivers to point to a bridge function which can then call into ast_rtp_instance_bridge() or into ast_udptl_bridge() depending on the type of media being bridged.</pre>
</blockquote>
<p>On February 9th, 2012, 10:34 a.m., <b>Joshua Colp</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Yes, things have to be *exactly* right to be able to do this. For example if the negotiated maximum IFP size differs (and in a lot of cases it does) you can't send the packet directly to the other side, you have to go through the UDPTL stack so it can resize things. After going through doing the T.38 support and seeing the real world implementations I personally just never explored direct bridging them due to this. There just aren't as many cases where they would be compatible as you would hope. Additionally the number of packets involved in comparison to RTP is small, so while you would get a performance gain with a large number at smaller numbers you just wouldn't get that much.
I don't want to deter you from doing this of course so the approach Mark has outlined above is the correct way. You will need to add compatibility checks for error correction, max ifp, and max datagram.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">>1. This change assumes that both channels are SIP channels. Besides the fact that the assumption is made in a dangerous way, H.323 and Skinny also support UDPTL and would not see the benefits of this change.
Yes, it is a SIP only feature. If one channels is not SIP, then the UDPTL pakets get processed in the current fashion.
>2. This change places bridging logic into a read callback. Having the reading channel peek across the bridge to get the other channel's private data should be avoided if at all possible.
Counterproposal? The idea was to forward the udptl packages, as soon as the arrive to have the smallest possible delay which is feasible.
>3. This change does not do any sort of compatibility checks between the bridged channels.
The idea is not to do _any_ compatibility checks. The idea is to do "real" pass through. So i will rewrite it so, that all t.38 parameter from the sdp from both channels will be passed. e.g. The T.38 re-INVITE doing channel send us T38FaxMaxDatagram:1400 and T38FaxUdpEC:t38UDPRedundancy, we will fordward to the second channel T38FaxMaxDatagram:1400 and T38FaxUdpEC:t38UDPRedundancy. Same for the 200 OK coming back then.</pre>
<br />
<p>- vrban</p>
<br />
<p>On February 8th, 2012, 9:43 a.m., vrban wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By vrban.</div>
<p style="color: grey;"><i>Updated Feb. 8, 2012, 9:43 a.m.</i></p>
<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;">New feature to send udptl packets directly between both call legs without extrac them. Like p2p for RTP.
</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;">Send a T.38 fax across two SIP channels. And the incoming udptl packets from first call leg where forwarded to the T.38 target at the second call leg and fax was received.
SIP Carrier -> T.38 -> asterisk with this new feature -> T.38 -> asterisk 1.8 with res_fax</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>trunk/channels/chan_sip.c <span style="color: grey">(354394)</span></li>
<li>trunk/channels/sip/include/sip.h <span style="color: grey">(354394)</span></li>
<li>trunk/include/asterisk/udptl.h <span style="color: grey">(354394)</span></li>
<li>trunk/main/udptl.c <span style="color: grey">(354394)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1716/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>