<p style="white-space: pre-wrap; word-wrap: break-word;">This code saves stats in a lot of places - off-nominal, cases where no stats would exist, and some nominals. The result of this is that the vector of stats can end up containing stats for RTP that was never used, or duplicate stats for the same RTP.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I think the specific expected behavior needs to be defined for the vector for the different scenarios - in particular re-negotiation. Should it only be when a session media goes away that its stats go into the vector, for example.</p><p>Patch set 3:<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>8 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10899/3/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/3/channels/chan_pjsip.c@1497">Patch Set #3, Line 1497:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(refresh_data->media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is not necessary. This media state conveys a desired state, it doesn't contain RTP instances.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/3/channels/chan_pjsip.c@1538">Patch Set #3, Line 1538:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(session->pending_media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This pending media state may have both old and new RTP instances, in the case of old it is possible for the statistics to end up in your vector multiple times.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10899/3/channels/pjsip/dialplan_functions.c">File channels/pjsip/dialplan_functions.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/3/channels/pjsip/dialplan_functions.c@1054">Patch Set #3, Line 1054:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_stats_save(state->media_state);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same, not necessary.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10899/3/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/3/include/asterisk/res_pjsip_session.h@144">Patch Set #3, Line 144:</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;"> /*! \brief Related sip session */<br> struct ast_sip_session *sip_session;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think storing the session is necessary.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10899/3/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/3/res/res_pjsip_session.c@212">Patch Set #3, Line 212:</a> <code style="font-family:monospace,monospace">void ast_sip_session_media_stats_save(struct ast_sip_session_media_state *media_state)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can't the session be passed in here instead?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/3/res/res_pjsip_session.c@229">Patch Set #3, Line 229:</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;">You are guaranteed that the RTP instance won't go away</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/3/res/res_pjsip_session.c@231">Patch Set #3, Line 231:</a> <code style="font-family:monospace,monospace"> stats_tmp = ast_calloc(1, sizeof(struct ast_rtp_instance_stats));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This allocation could fail</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10899/3/res/res_pjsip_session.c@2172">Patch Set #3, Line 2172:</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;"> for (idx = 0; idx < AST_VECTOR_SIZE(&session->media_stats); idx++) {<br> struct ast_rtp_instance_stats* tmp_stats = AST_VECTOR_GET(&session->media_stats, idx);<br> ast_free(tmp_stats);<br> AST_VECTOR_REMOVE(&session->media_stats, idx, 1);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I believe you can use AST_VECTOR_RESET here instead</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: 3 </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: Mon, 04 Feb 2019 12:09:08 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>