<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13842">View Change</a></p><p>12 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/configs/samples/pjsip.conf.sample">File configs/samples/pjsip.conf.sample:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/configs/samples/pjsip.conf.sample@801">Patch Set #1, Line 801:</a> <code style="font-family:monospace,monospace">;incoming_call_offer_pref= ; Sets the preferred codecs, and order to use between</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Tabs have gone a bit bonkers here.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/doc/CHANGES-staging/res_pjsip_incoming_call_offer_pref.txt">File doc/CHANGES-staging/res_pjsip_incoming_call_offer_pref.txt:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/doc/CHANGES-staging/res_pjsip_incoming_call_offer_pref.txt@7">Patch Set #1, Line 7:</a> <code style="font-family:monospace,monospace">offer, and those set in the configuration.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Extend this to include a better explanation with examples.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip.h">File include/asterisk/res_pjsip.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip.h@516">Patch Set #1, Line 516:</a> <code style="font-family:monospace,monospace"> AST_SIP_CALL_CODEC_PREF_LOCAL,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Document the meaning of these.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip_session.h">File include/asterisk/res_pjsip_session.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip_session.h@34">Patch Set #1, Line 34:</a> <code style="font-family:monospace,monospace">#include "asterisk/res_pjsip_session_caps.h"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Any reason to not just use a forward declaration instead?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip_session_caps.h">File include/asterisk/res_pjsip_session_caps.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip_session_caps.h@27">Patch Set #1, Line 27:</a> <code style="font-family:monospace,monospace"> * \brief Allocate a sip session capabilities object.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">SIP is traditionally capitalized :D</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip_session_caps.h@40">Patch Set #1, Line 40:</a> <code style="font-family:monospace,monospace"> */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Document that this will replace any already set or built capabilities.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/include/asterisk/res_pjsip_session_caps.h@61">Patch Set #1, Line 61:</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;"> * Creates a set of joint capabilities between the given remote capabilities,<br> * and pre-configured ones.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This API feels weird, because generally these things return a unique capabilities structure that isn't persisted. I think it's the use of "build" that makes me think that. Perhaps "update" would be better? An explanation that it is stored away would also help.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip.c">File res/res_pjsip.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip.c@949">Patch Set #1, Line 949:</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;"> <configOption name="rtp_keepalive"><br> <synopsis>Number of seconds between RTP comfort noise keepalive packets.</synopsis><br> <description><para><br> At the specified interval, Asterisk will send an RTP comfort noise frame. This may<br> be useful for situations where Asterisk is behind a NAT or firewall and must keep<br> a hole open in order to allow for media to arrive at Asterisk.<br> </para></description><br> </configOption><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">You've doubled this documentation.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip/pjsip_configuration.c">File res/res_pjsip/pjsip_configuration.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip/pjsip_configuration.c@1137">Patch Set #1, Line 1137:</a> <code style="font-family:monospace,monospace"> if (i == AST_SIP_CALL_CODEC_PREF_LOCAL_LIMIT ||</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Explain why this is here</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip_sdp_rtp.c">File res/res_pjsip_sdp_rtp.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip_sdp_rtp.c@420">Patch Set #1, Line 420:</a> <code style="font-family:monospace,monospace"> }</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should return now since this has failed.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip_session/pjsip_session_caps.c">File res/res_pjsip_session/pjsip_session_caps.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip_session/pjsip_session_caps.c@36">Patch Set #1, Line 36:</a> <code style="font-family:monospace,monospace">static void log_caps(int level, const char *file, int line, const char *function,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is there any way we can get additional information in here to tie it back to a channel or endpoint?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13842/1/res/res_pjsip_session/pjsip_session_caps.c@138">Patch Set #1, Line 138:</a> <code style="font-family:monospace,monospace"> /* Save the most preferred one */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Oh! I'd add a comment here that since a stream only carries a single media type there's no need to worry about the type or dealing with multiple, so just pulling the first one really will do the job.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/13842">change 13842</a>. To unsubscribe, or for help writing mail filters, 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/c/asterisk/+/13842"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I35e7a2a0c236cfb6bd9cdf89539f57a1ffefc76f </div>
<div style="display:none"> Gerrit-Change-Number: 13842 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 27 Feb 2020 11:35:27 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>