<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/3485/">https://reviewboard.asterisk.org/r/3485/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 19th, 2014, 9:11 a.m. CDT, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/3485/diff/5/?file=58486#file58486line951" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/include/asterisk/bridge.h</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">typedef void (*transfer_channel_cb)(struct ast_channel *chan, void *user_data,</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">typedef void (*transfer_channel_cb)(struct ast_channel *chan, struct transfer_channel_data *user_data,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">951</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * \param terminal Set to true if the channel will hang up immediately after</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">952</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * this function returns successfully so that hooks can know when media</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">953</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * shouldn't be played to the transferrer.</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Terminal does not feel like a good name for this parameter.
What's more, in most blind transfers, media is never played to the transferer. The context of this is definitely not clear from the description of the function.
The fact that we have a single parameter - which ripples throughout the _entire_ codebase - merely to work around chan_sip feels like a mistake. Ideally, the blind transfer routine would be smart enough to bump the reference count of the channel to prevent it getting nuked before attempting the playback and - if the channel is marked to be hung up - not attempt the playback in the first place.</pre>
</blockquote>
<p>On May 19th, 2014, 11:33 a.m. CDT, <b>Jonathan Rose</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;">There is no way to prevent the playback based on the channel's current hangup state. The hangup state generally hasn't changed by the time we enter the announcement function. The blind transfer itself isn't what initiates the hangup. It's the response we get from the phone to the SIP notify after returning from everything in the bridge blind transfer code. At best we'd end up with a race condition between the announcement subscription and hanging up on the BYE we get from the phones.
This stuff worked with chan_sip in Asterisk 11 because Asterisk 11's chan_sip had logic specifically in place to handle deferring the final notify (the one that triggers the BYE from phones) when parking completed.
Perhaps it would be more palpable if instead of adding this to flag to the API call, I instead added it to the transfer channel data wrapper. As for a name change, how about just 'disable_media'?
Alternatively, I can start investigating what it would take to be able to defer the final notify of chan_sip blind transfers. I know you said we don't want to go down that road right now, but it's basically the only real way of getting this to behave like it behaved in Asterisk 11.</pre>
</blockquote>
<p>On May 19th, 2014, 11:58 a.m. CDT, <b>Jonathan Rose</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;">On second thought, I can't really move the variable to the wrapper either since the transfer channel callback data isn't wrapped until it is already in the bridging function. Meh.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ah, but on the other hand... I could set the variable in the transfer_channel_cb since that gets executed before the parked channel is put into the bridge anyway. That solves that problem.</pre>
<br />
<p>- Jonathan</p>
<br />
<p>On May 16th, 2014, 5:02 p.m. CDT, Jonathan Rose wrote:</p>
<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, Matt Jordan and Mark Michelson.</div>
<div>By Jonathan Rose.</div>
<p style="color: grey;"><i>Updated May 16, 2014, 5:02 p.m.</i></p>
<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;">If a PJSIP endpoint attempts to blind transfer to a parking extension, there is an override to the normal transfer logic that can make things act a little weird. We noticed that this would leave various phones hanging on transfer screens without progressing. When the transfer was considered successful, PJSIP deferred the actual action of sending the 200 notify and the actual trigger for that happening never occurs when the transfer is to a parking extension.
In order to handle this, the bridge function that handles blind transfers now returns a different value if a call was parked and if the channel driver needs to react differently in this case, it can. In the case of PJSIP, we respond to transfers to park by immediately sending the notify with 200 OK sip frag instead of deferring the action.</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;">Before patch:
* Blind transfer on Polycom SPIP: Phone is on the blind transfer screen until it either manually hangs up or 60 seconds pass and Asterisk terminates the session.
After the patch:
* Blind transfer on Polycom SPIP: Phone immediately leaves the blind transfer screen and goes back to idle mode.</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/res/res_pjsip_refer.c <span style="color: grey">(413303)</span></li>
<li>/branches/12/res/parking/parking_bridge_features.c <span style="color: grey">(413303)</span></li>
<li>/branches/12/res/parking/parking_applications.c <span style="color: grey">(413303)</span></li>
<li>/branches/12/main/parking.c <span style="color: grey">(413303)</span></li>
<li>/branches/12/main/manager.c <span style="color: grey">(413303)</span></li>
<li>/branches/12/main/bridge_basic.c <span style="color: grey">(413303)</span></li>
<li>/branches/12/main/bridge.c <span style="color: grey">(413303)</span></li>
<li>/branches/12/include/asterisk/parking.h <span style="color: grey">(413303)</span></li>
<li>/branches/12/include/asterisk/bridge.h <span style="color: grey">(413303)</span></li>
<li>/branches/12/channels/sig_analog.c <span style="color: grey">(413303)</span></li>
<li>/branches/12/channels/chan_skinny.c <span style="color: grey">(413303)</span></li>
<li>/branches/12/channels/chan_sip.c <span style="color: grey">(413303)</span></li>
<li>/branches/12/channels/chan_oss.c <span style="color: grey">(413303)</span></li>
<li>/branches/12/channels/chan_mgcp.c <span style="color: grey">(413303)</span></li>
<li>/branches/12/channels/chan_iax2.c <span style="color: grey">(413303)</span></li>
<li>/branches/12/channels/chan_dahdi.c <span style="color: grey">(413303)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3485/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>