<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/4473/">https://reviewboard.asterisk.org/r/4473/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 12th, 2015, 8:41 p.m. UTC, <b>Matt Jordan</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;">1. For this to go into Asterisk 13, tests will need to be provided that cover the new parameter. (Really, those tests should be written regardless)
2. The CHANGES file will need to get updated with the new option.</pre>
</blockquote>
<p>On March 12th, 2015, 10:03 p.m. UTC, <b>rmudgett</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;">Actually I'd prefer that the rpid_immediate option not exist at all and the code it controls to just be removed. Sending connected line updates back to the caller _before_ getting connected doesn't really make sense. This is what the REDIRECTING information is supposed to be doing.</pre>
</blockquote>
<p>On March 12th, 2015, 11:48 p.m. UTC, <b>gareth</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;">I disagree, providing connected line updates pre-answer makes perfect sense. Why would I want to know the connected-line name only after the call has been answered?
That assumes the call even is answered, sending the connected-line information immediately allows the phone to include the name in it's call history.
If no call-forwarding and/or re-addressing has taken place why would REDIRECTING be used?</pre>
</blockquote>
<p>On March 13th, 2015, 5:25 p.m. UTC, <b>rmudgett</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;">OK, that makes sense but the code that rpid_immediate controls doesn't. As described in the review description, that code immediately sends redundant "180 Ringing" or "183 Progress" messages at best and at worst lies that the other end is ringing with inaccurate connected line information.</pre>
</blockquote>
<p>On March 13th, 2015, 8:33 p.m. UTC, <b>Matt Jordan</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;">I'm not sure what this setting is attempting to do then. The description of the new setting makes it sound like it controls sending connected line information prior to Answer, which fits gareth's interpretation. Whether or not it sends multiple 180/183 provisional responses does not seem to be implied in the description of the setting.
If setting 'rpid_immediate' to 'yes' still results in extraneous 180/183 provisional responses, then I'm not sure this patch fixes the underlying issue.</pre>
</blockquote>
<p>On March 13th, 2015, 9:30 p.m. UTC, <b>rmudgett</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;">The option does exactly the same thing as chan_sip and will be disabled by default just like chan_sip.
When disabled it does not send the extraneous 180/183 messages. The connected line information simply waits until there is a real reason to send the 180/183 messages. e.g., Such as when the outgoing channel receives a 180/183 message. When the outgoing channel receives a 180/183 message, that message may also contain updated/correct connected line information from the peer.
When enabled it sends the connected line update information *immediately* on a 180/183 message regardless of if the far end has received the call yet or the connected line information came from the far end. The core may do some filtering to eliminate an update event if the connected line information does not change. However, when the peer receives a 180/183 message the caller will also get the 180/183 message generated because of the ast_indicate(AST_CONTROL_RINGING/AST_CONTROL_PROGRESS).
The key word is immediate. I suppose I should sprinkle immediate a few more times in the option description. I doubt anyone even enables the option in chan_sip because it is undocumented, disabled by default, and will cause this odd behaviour when enabled.
These are the reasons why I think the code the option controls should just be removed and the option not even exist. The code doesn't really help and the extraneous 180/183 messages cause problems when enabled.</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;">I have rpid_immediate enabled. I know about the option because I searched through chan_sip.c to find out why RPID updates were not working as expected.
Until the SIP protocol gains a "here is the new RPID info" response there is always going to be the problem of piggy-backing that information on a response which already had a meaning.
Patch looks fine.</pre>
<br />
<p>- gareth</p>
<br />
<p>On March 13th, 2015, 5:13 p.m. UTC, rmudgett 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.</div>
<div>By rmudgett.</div>
<p style="color: grey;"><i>Updated March 13, 2015, 5:13 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-24781">ASTERISK-24781</a>
</div>
<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;">Incoming PJSIP call legs that have not been answered yet send unnecessary
"180 Ringing" or "183 Progress" messages every time a connected line
update happens. If the outgoing channel is also PJSIP then the incoming
channel will always send a "180 Ringing" or "183 Progress" message when
the outgoing channel sends the INVITE.
Consequences of these unnecessary messages:
* The caller can start hearing ringback before the far end even gets the
call.
* Many phones tend to grab the first connected line information and refuse
to update the display if it changes. The first information is not likely
to be correct if the call goes to an endpoint not under the control of the
first Asterisk box.
When connected line first went into Asterisk in v1.8, chan_sip received an
undocumented option "rpid_immediate" that defaults to disabled. When
enabled, the option immediately passes connected line update information
to the caller in "180 Ringing" or "183 Progress" messages as described
above.
* Added "rpid_immediate" option to prevent unnecessary "180 Ringing" or
"183 Progress" messages. The default is "no" to disable sending the
unnecessary messages.
</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;">* Ran the tests/channels/pjsip testsuite tests. They still pass.
* Made a call chain as follows: 100 -> * -> * -> * -> 200. With the patch
there are no unnecessary messages. Without the patch there were several
"180 Ringing" messages sent back to the caller.
</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/13/res/res_pjsip/pjsip_configuration.c <span style="color: grey">(432895)</span></li>
<li>/branches/13/res/res_pjsip.c <span style="color: grey">(432895)</span></li>
<li>/branches/13/include/asterisk/res_pjsip.h <span style="color: grey">(432895)</span></li>
<li>/branches/13/configs/samples/pjsip.conf.sample <span style="color: grey">(432895)</span></li>
<li>/branches/13/channels/chan_pjsip.c <span style="color: grey">(432895)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4473/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>