<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">(8 comments)</p><p style="white-space: pre-wrap; word-wrap: break-word;">This code saves stats in a lot of places - off-nominal, cases where<br>no stats would exist, and some nominals. The result of this is that<br>the vector of stats can end up containing stats for RTP that was<br>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<br>vector for the different scenarios - in particular re-negotiation.<br>Should it only be when a session media goes away that its stats go<br>into the vector, for example.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;"><br>Hi Joshua, thank you for your kind reviewing.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The purpose of this fixes are, providing a detail call quality info for session. The Asterisk provides RTCPReceived/RTCPSent AMI event already, but it requires more statistics work to the listener and may need more other SIP analyzers - which is not an easy to doing it.</p><p style="white-space: pre-wrap; word-wrap: break-word;">So, to provide more detail info, I thought save the all the media stats even it was re-negotiation. Because it could be also having some media transferring stats.</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_state_free(refresh_data->media_state);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This is not necessary. This media state conveys a desired state, it doesn't contain RTP instances.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Okay, I removed it.</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_state_reset(session->pending_media_state);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This pending media state may have both old and new RTP instances, in the case of old it is possible […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I couldn't catch up what case would be. Do you have idea how this should be fixed to?</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_state_free(state->media_state);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Same, not necessary.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Fixed it.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't think storing the session is necessary.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Because of ast_sip_session_media_stats_save() gets only the media_state as a parameter. So I needed related session's info somehow.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Can't the session be passed in here instead?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I've designed this function as a sibling of ast_sip_session_media_state_*() function series. And it actually working as a pair with those functions. So, I think it would be better having a same parameters like others.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You are guaranteed that the RTP instance won't go away</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes. :)</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This allocation could fail</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Right, added Null check.</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;"> ast_sip_session_remove_supplements(session);<br> AST_LIST_HEAD_DESTROY(&session->supplements);<br><br> /* remove all saved media stats */<br> A<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I believe you can use AST_VECTOR_RESET here instead</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes, I've fixed it.</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: 4 </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, 06 Feb 2019 21:58:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>