<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/8883">View Change</a></p><p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(15 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/8883/2/include/asterisk/rtp_engine.h">File include/asterisk/rtp_engine.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8883/2/include/asterisk/rtp_engine.h@540">Patch Set #2, Line 540:</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;"> /* Per the RFC 0 should not be used, so we treat it as an unsupported extension placeholder */<br> AST_RTP_EXTENSION_UNSUPPORTED = 0,<br> /* abs-send-time from https://tools.ietf.org/html/draft-alvestrand-rmcat-remb-03 */<br> AST_RTP_EXTENSION_ABS_SEND_TIME,<br> /* The maximum number of known RTP extensions */<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Missing ! to start doxygen comment. /*! */</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8883/2/include/asterisk/rtp_engine.h@741">Patch Set #2, Line 741:</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;"> /* The extension is not negotiated and is not flowing */<br> AST_RTP_EXTENSION_DIRECTION_NONE,<br> /* Send and receive */<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Missing ! to start doxygen comment. /*! */</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8883/2/main/rtp_engine.c">File main/rtp_engine.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8883/2/main/rtp_engine.c@732">Patch Set #2, Line 732:</a> <code style="font-family:monospace,monospace"> if (id == -1) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should we check this way instead of looking only for -1 to be safe?</p><p style="white-space: pre-wrap; word-wrap: break-word;">if (id <= 0) {<br>} else {<br>}</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8883/2/main/rtp_engine.c@838">Patch Set #2, Line 838:</a> <code style="font-family:monospace,monospace"> struct rtp_extmap extmap_none = {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should be declared static const struct rtp_extmap.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8883/2/main/rtp_engine.c@875">Patch Set #2, Line 875:</a> <code style="font-family:monospace,monospace"> size_t count = 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Does not need to be initialized. It is always set by AST_VECTOR_SIZE().</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8883/2/main/rtp_engine.c@894">Patch Set #2, Line 894:</a> <code style="font-family:monospace,monospace"> if (id <= AST_VECTOR_SIZE(&instance->extmap_unique_ids)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">There seems to be a hole in the treatment of id: -1, 1 to max seems to be valid values. What happens when zero is passed in?</p><p style="white-space: pre-wrap; word-wrap: break-word;">if (0 < id && id <= AST_VECTOR_SIZE()) {<br>}</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8883/2/main/rtp_engine.c@910">Patch Set #2, Line 910:</a> <code style="font-family:monospace,monospace"> if (id <= AST_VECTOR_SIZE(&instance->extmap_unique_ids)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">There seems to be a hole in the treatment of id: -1, 1 to max seems to be valid values. What happens when zero is passed in?</p><p style="white-space: pre-wrap; word-wrap: break-word;">if (0 < id && id <= AST_VECTOR_SIZE()) {<br>}</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8883/2/res/res_pjsip_sdp_rtp.c">File res/res_pjsip_sdp_rtp.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8883/2/res/res_pjsip_sdp_rtp.c@161">Patch Set #2, Line 161:</a> <code style="font-family:monospace,monospace"> int idx = -1;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should call this id instead since that is what is put in it?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8883/2/res/res_pjsip_sdp_rtp.c@183">Patch Set #2, Line 183:</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;"> if ((ast_rtp_instance_extmap_count(other_session_media->rtp) + 1) > idx) {<br> idx = ast_rtp_instance_extmap_count(other_session_media->rtp) + 1;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Rather than calling the function twice which gets and releases the instance lock.</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">other_id = ast_rtp_instance_extmap_count(other_session_media->rtp) + 1;<br>if (idx < other_id) {<br> idx = other_id;<br>}</pre></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8883/2/res/res_pjsip_sdp_rtp.c@1153">Patch Set #2, Line 1153:</a> <code style="font-family:monospace,monospace"> char tmp[256];</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">tmp is always a useless name. extmap_value[]</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8883/2/res/res_pjsip_sdp_rtp.c@1214">Patch Set #2, Line 1214:</a> <code style="font-family:monospace,monospace"> char direction_str[10] = "", uri[64], attributes[64] = "";</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It is best if you declare one variable per line.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I seem to recall that URI's can get quite large. 64 seems small.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8883/2/res/res_pjsip_sdp_rtp.c@1245">Patch Set #2, Line 1245:</a> <code style="font-family:monospace,monospace"> if (sscanf(uri_str, "%63s %63s", uri, attributes) < 1) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Since we own the uri_str buffer we can modify it and point uri and attributes to appropriate spots in the buffer rather than having the hard coded limits of the uri[] and attributes[].</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8883/2/res/res_rtp_asterisk.c">File res/res_rtp_asterisk.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8883/2/res/res_rtp_asterisk.c@4417">Patch Set #2, Line 4417:</a> <code style="font-family:monospace,monospace"> int hdrlen = 12, res, ice, ext = 0, abs_send_time_id;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It is best to declare one variable per line.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8883/2/res/res_rtp_asterisk.c@4424">Patch Set #2, Line 4424:</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;"> /* 4 bytes for the shared information, 4 bytes for abs-send-time */<br> hdrlen += 8;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This seems wrong. We are adding two uint32's plus a uint24 for a total of 4 + 4 + 3 = 11.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8883/2/res/res_rtp_asterisk.c@5309">Patch Set #2, Line 5309:</a> <code style="font-family:monospace,monospace"> unsigned int now_msw = 0, now_lsw = 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It is best if one variable declaration per line.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/8883">change 8883</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/8883"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I508deac557867b1e27fc7339be890c8018171588 </div>
<div style="display:none"> Gerrit-Change-Number: 8883 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Joshua Colp <jcolp@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: Thu, 03 May 2018 20:53:44 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>