<p>George Joseph <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/6443">View Change</a></p><p>Patch set 4:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(7 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/6443/4/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/6443/4/res/res_rtp_asterisk.c@4644">Patch Set #4, Line 4644:</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;">#define RTCP_VALID_MASK (RTCP_VERSION_MASK | (0xfe << 16))<br>#define RTCP_VALID_VALUE (RTCP_VERSION | (RTCP_PT_SR << 16))<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I take it that your intent here by using 0xfe is to include both SR(200) and RR(201) as valid but it took me 15 minutes to figure that out. Comments would be good.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6443/4/res/res_rtp_asterisk.c@4688">Patch Set #4, Line 4688:</a> <code style="font-family:monospace,monospace">first_word & RTCP_VERSION_MASK) == RTCP_VERSION</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is just checking 2 bits enough to determine that it's a valid rtcp packet? I'd at least add the packet type. You could use rtcp_payload_type2str as a check.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6443/4/res/res_rtp_asterisk.c@4707">Patch Set #4, Line 4707:</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;"> /*<br> * Require SSRC to match for strictrtp before we use the<br> * sender's address for symmetrical RTP to send our<br> * RTCP reports.<br> */<br> ssrc_seen = rtp->strict_rtp_state == STRICT_RTP_OPEN;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The comment and the statement don't really match.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6443/4/res/res_rtp_asterisk.c@4720">Patch Set #4, Line 4720:</a> <code style="font-family:monospace,monospace">ssrc_valid</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This implies it's a "valid" ssrc but an ssrc isn't really valid or invalid. You might want to rename this (and themssrc_valid) to something like "use_ssrc", etc.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6443/4/res/res_rtp_asterisk.c@4729">Patch Set #4, Line 4729:</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;"> pt = (first_word & 0xff0000) >> 16;<br> rc = (first_word & 0x1f000000) >> 24;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Since you made masks for version and "valid" how about masks for these as well.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6443/4/res/res_rtp_asterisk.c@4755">Patch Set #4, Line 4755:</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;"> /* We don't know what min_length should be so disable the check */<br> min_length = length;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Isn't this also going to catch an unknown packet type? In that case, shouldn't we skip it?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6443/4/res/res_rtp_asterisk.c@4766">Patch Set #4, Line 4766:</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;"> /* Get the RTCP record SSRC if defined for the record */<br> ssrc_valid = 1;<br> switch (pt) {<br> case RTCP_PT_SR:<br> case RTCP_PT_RR:<br> rtcp_report = ast_rtp_rtcp_report_alloc(rc);<br> if (!rtcp_report) {<br> return &ast_null_frame;<br> }<br> rtcp_report->reception_report_count = rc;<br><br> ssrc = ntohl(rtcpheader[i + 1]);<br> rtcp_report->ssrc = ssrc;<br> break;<br> case RTCP_PT_FUR:<br> case RTCP_PT_PSFB:<br> ssrc = ntohl(rtcpheader[i + 1]);<br> break;<br> case RTCP_PT_SDES:<br> case RTCP_PT_BYE:<br> default:<br> ssrc = 0;<br> ssrc_valid = 0;<br> break;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why not combine this switch with the earlier one?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/6443">change 6443</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/6443"/><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: I67d89e3c27db83efa0e6b52734f73c88ac2939e2 </div>
<div style="display:none"> Gerrit-Change-Number: 6443 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@digium.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: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 13 Sep 2017 12:41:52 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>