<p>Patch set 6:<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/10899">View Change</a></p><p>21 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10899/6//COMMIT_MSG">Commit Message:</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/10899/6//COMMIT_MSG@13">Patch Set #6, Line 13:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The JIRA issue needs to be in here, or else it will not be picked up appropriately when released.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10899/6/channels/chan_pjsip.c">File channels/chan_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/10899/6/channels/chan_pjsip.c@1537">Patch Set #6, Line 1537:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, session->pending_media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should not be done here.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10899/6/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/10899/6/include/asterisk/res_pjsip_session.h@218">Patch Set #6, Line 218:</a> <code style="font-family:monospace,monospace"> /* Media stats result saving */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This could use a better description, for example:</p><p style="white-space: pre-wrap; word-wrap: break-word;">Media statistics for negotiated RTP streams</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c">File res/res_pjsip_session.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/10899/6/res/res_pjsip_session.c@227">Patch Set #6, Line 227:</a> <code style="font-family:monospace,monospace"> rtp_tmp = ao2_bump(media->rtp);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is not necessary. You don't need to bump the ref, you can directly use media->rtp.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@242">Patch Set #6, Line 242:</a> <code style="font-family:monospace,monospace"> AST_VECTOR_APPEND(&sip_session->media_stats, stats_tmp);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think before appending the stats any existing stats for the local SSRC should be removed from the vector. This gives a guarantee that the vector won't contain duplicates for the same stream.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1416">Patch Set #6, Line 1416:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should not be done.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1538">Patch Set #6, Line 1538:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should not be done.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1547">Patch Set #6, Line 1547:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should not be done.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1576">Patch Set #6, Line 1576:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should not be done.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1647">Patch Set #6, Line 1647:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should not be done.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1699">Patch Set #6, Line 1699:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should not be done.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1707">Patch Set #6, Line 1707:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should not be done.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1715">Patch Set #6, Line 1715:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should not be done.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1721">Patch Set #6, Line 1721:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, session->pending_media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should not be done.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1729">Patch Set #6, Line 1729:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, session->pending_media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should not be done.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1735">Patch Set #6, Line 1735:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, session->pending_media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should not be done.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1746">Patch Set #6, Line 1746:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, session->pending_media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should not be done.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1754">Patch Set #6, Line 1754:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, session->pending_media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should not be done.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1762">Patch Set #6, Line 1762:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, session->pending_media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should not be done.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@2702">Patch Set #6, Line 2702:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, session->pending_media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this should be moved above the SWAP and session->active_media_state used instead - because at first glance someone may assume that this is incorrect when it's not - it's saving the active media state stats before destroying the underlying media sessions.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_t38.c">File res/res_pjsip_t38.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/10899/6/res/res_pjsip_t38.c@342">Patch Set #6, Line 342:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session, state->media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">These are not actually required, as T.38 doesn't use RTP or have statistics so they will do nothing.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/10899">change 10899</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/10899"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ib25c2d3fc4da084aecfde2a82c1b1d733bd64fa5 </div>
<div style="display:none"> Gerrit-Change-Number: 10899 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: sungtae kim <pchero21@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation (1000185) </div>
<div style="display:none"> Gerrit-Reviewer: Joshua C. Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: sungtae kim <pchero21@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 13 Feb 2019 12:24:28 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>