<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/9200">View Change</a></p><p>Patch set 9:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(13 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/9200/9/configure.ac">File configure.ac:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9200/9/configure.ac@2346">Patch Set #9, Line 2346:</a> <code style="font-family:monospace,monospace"> AST_C_COMPILE_CHECK([PJSIP_INV_ACCEPT_MULTIPLE_SDP_ANSWERS], [pjsip_cfg()->endpt.accept_multiple_sdp_answers = 0;], [pjsip.h])</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Wouldn't it be simpler to move this line up to just after the PJSIP_TLS_TRANSPORT_PROTO line so you don't have to save and restore the flags again?</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/9200/9/include/asterisk/res_pjsip.h">File include/asterisk/res_pjsip.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9200/9/include/asterisk/res_pjsip.h@640">Patch Set #9, Line 640:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /*! Follow forked media with a different To tag */<br> unsigned int follow_early_media_fork;<br> /*! Accept updated SDPs on non-100rel 18X and 2XX responses with the same To tag */<br> unsigned int accept_multiple_sdp_answers;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">These will have to go at the end of the ast_sip_endpoint struct to not break ABI. They can go where they belong here in master though.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c">File res/res_pjsip.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1030">Patch Set #9, Line 1030:</a> <code style="font-family:monospace,monospace"> <configOption name="follow_early_media_fork" default="yes"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">FYI: Putting the default value here is useless. The only default value that means anything is in the ast_sorcery_object_field_register() lines.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1035">Patch Set #9, Line 1035:</a> <code style="font-family:monospace,monospace"> To tag on the subsequent response is different that the previous one,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/different that/different than that of/</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1039">Patch Set #9, Line 1039:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> <para><br> This option must also be enabled in the <literal>system</literal><br> section for it to take effect here.<br> </para><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Make this a note:</p><p style="white-space: pre-wrap; word-wrap: break-word;"><note><para>...</para></note></p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1050">Patch Set #9, Line 1050:</a> <code style="font-family:monospace,monospace"> To tag on the subsequent response is the same as that the previous one,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/as that/as that of/</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1054">Patch Set #9, Line 1054:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> <para><br> This option must also be enabled in the <literal>system</literal><br> section for it to take effect here.<br> </para><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Make this a note:</p><p style="white-space: pre-wrap; word-wrap: break-word;"><note><para>...</para></note></p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1632">Patch Set #9, Line 1632:</a> <code style="font-family:monospace,monospace"> To tag on the subsequent response is different that the previous one,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/different that/different than that of/</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1635">Patch Set #9, Line 1635:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> <para><br> This option must also be enabled on endpoints that require<br> this functionality.<br> </para><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Make this a note:</p><p style="white-space: pre-wrap; word-wrap: break-word;"><note><para>...</para></note></p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1646">Patch Set #9, Line 1646:</a> <code style="font-family:monospace,monospace"> To tag on the subsequent response is the same as that the previous one,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/as that/as that of/</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1649">Patch Set #9, Line 1649:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> <para><br> This option must also be enabled on endpoints that require<br> this functionality.<br> </para><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Make this a note:</p><p style="white-space: pre-wrap; word-wrap: break-word;"><note><para>...</para></note></p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip/config_system.c">File res/res_pjsip/config_system.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip/config_system.c@107">Patch Set #9, Line 107:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">#ifdef HAVE_PJSIP_INV_ACCEPT_MULTIPLE_SDP_ANSWERS<br> pjsip_cfg()->endpt.accept_multiple_sdp_answers = system->accept_multiple_sdp_answers;<br>#else<br> ast_log(LOG_WARNING, "follow_early_media_fork_same_tag is not supported in this version of pjproject. Ignoring\n");<br>#endif<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This warning is unconditional if the pjproject version does not support the feature. You need to give the warning when the user tries to enable the option.</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (system>accept_multiple_sdp_answers) {<br> ast_log(...);<br>}</pre></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip_session.c">File res/res_pjsip_session.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip_session.c@3093">Patch Set #9, Line 3093:</a> <code style="font-family:monospace,monospace"> bail = 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Setting bail here is not needed as it is initialized to this.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/9200">change 9200</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/9200"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I64d071942b79adb2f0a4e13137389b19404fe3d6 </div>
<div style="display:none"> Gerrit-Change-Number: 9200 </div>
<div style="display:none"> Gerrit-PatchSet: 9 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Alexei Gradinari <alex2grad@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 22 Jun 2018 21:14:20 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>