<p style="white-space: pre-wrap; word-wrap: break-word;">Hi Joshua,</p><p style="white-space: pre-wrap; word-wrap: break-word;">Thank you for your kind reviewing. I've fixed what you mentioned. :)</p><p><a href="https://gerrit.asterisk.org/10899">View Change</a></p><p>19 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/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 statistics for negotiated RTP streams */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This could use a better description, for example: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Agree. :) I've fixed it.</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"> }</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This is not necessary. You don't need to bump the ref, you can directly use media->rtp.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I've got it. 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/6/res/res_pjsip_session.c@242">Patch Set #6, Line 242:</a> <code style="font-family:monospace,monospace"> if (ret) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think before appending the stats any existing stats for the local SSRC should be removed from the […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I've got it. Added check and remove function.</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"> struct ast_sip_session_media_state *media_state)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should not be done.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/6/res/res_pjsip_session.c@1538">Patch Set #6, Line 1538:</a> <code style="font-family:monospace,monospace"> pjsip_inv_session *inv_session = session->inv_session;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should not be done.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/6/res/res_pjsip_session.c@1547">Patch Set #6, Line 1547:</a> <code style="font-family:monospace,monospace"> if (inv_session->state == PJSIP_INV_STATE_DISCONNECTED) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should not be done.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/6/res/res_pjsip_session.c@1576">Patch Set #6, Line 1576:</a> <code style="font-family:monospace,monospace"> */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should not be done.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/6/res/res_pjsip_session.c@1647">Patch Set #6, Line 1647:</a> <code style="font-family:monospace,monospace"> joint_cap = ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should not be done.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/6/res/res_pjsip_session.c@1699">Patch Set #6, Line 1699:</a> <code style="font-family:monospace,monospace"> if (!cloned) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should not be done.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/6/res/res_pjsip_session.c@1707">Patch Set #6, Line 1707:</a> <code style="font-family:monospace,monospace"> ast_sip_session_media_state_free(media_state);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should not be done.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/6/res/res_pjsip_session.c@1715">Patch Set #6, Line 1715:</a> <code style="font-family:monospace,monospace"> return 0;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should not be done.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/6/res/res_pjsip_session.c@1721">Patch Set #6, Line 1721:</a> <code style="font-family:monospace,monospace"> }</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should not be done.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/6/res/res_pjsip_session.c@1729">Patch Set #6, Line 1729:</a> <code style="font-family:monospace,monospace"> if (on_sdp_creation) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should not be done.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/6/res/res_pjsip_session.c@1735">Patch Set #6, Line 1735:</a> <code style="font-family:monospace,monospace"> }</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should not be done.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/6/res/res_pjsip_session.c@1746">Patch Set #6, Line 1746:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_WARNING, "Failed to create UPDATE properly.\n");</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should not be done.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/6/res/res_pjsip_session.c@1754">Patch Set #6, Line 1754:</a> <code style="font-family:monospace,monospace"> if (generate_new_sdp) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should not be done.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/6/res/res_pjsip_session.c@1762">Patch Set #6, Line 1762:</a> <code style="font-family:monospace,monospace"> ast_sorcery_object_get_id(session->endpoint));</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should not be done.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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/6/res/res_pjsip_session.c@2702">Patch Set #6, Line 2702:</a> <code style="font-family:monospace,monospace"> * final session reference to be released but if both STATE and invite_tsx are NULL,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think this should be moved above the SWAP and session->active_media_state used instead - because a […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I've got it. I moved it! :)</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_state_free(state->media_state);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">These are not actually required, as T.38 doesn't use RTP or have statistics so they will do nothing.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Okay, removed 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: 7 </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 22:02:21 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>