[Asterisk-code-review] res pjsip session Added rtcp stats result vector into the se... (asterisk[master])

[sungtae kim]

sungtae kim has posted comments on this change. ( https://gerrit.asterisk.org/10899 )

Change subject: res_pjsip_session Added rtcp stats result vector into the session
......................................................................


Patch Set 7:

(19 comments)

Hi Joshua,

Thank you for your kind reviewing. I've fixed what you mentioned. :)

https://gerrit.asterisk.org/#/c/10899/6/include/asterisk/res_pjsip_session.h
File include/asterisk/res_pjsip_session.h:

https://gerrit.asterisk.org/#/c/10899/6/include/asterisk/res_pjsip_session.h@218
PS6, Line 218: 	/* Media statistics for negotiated RTP streams */
> This could use a better description, for example: […]
Agree. :) I've fixed it.


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c
File res/res_pjsip_session.c:

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@227
PS6, Line 227: 	}
> This is not necessary. You don't need to bump the ref, you can directly use media->rtp.
I've got it. Removed it. :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@242
PS6, Line 242: 		if (ret) {
> I think before appending the stats any existing stats for the local SSRC should be removed from the  […]
I've got it. Added check and remove function.


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1416
PS6, Line 1416: 	struct ast_sip_session_media_state *media_state)
> Should not be done.
Removed it. :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1538
PS6, Line 1538: 	pjsip_inv_session *inv_session = session->inv_session;
> Should not be done.
Removed it. :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1547
PS6, Line 1547: 	if (inv_session->state == PJSIP_INV_STATE_DISCONNECTED) {
> Should not be done.
Removed it. :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1576
PS6, Line 1576: 			 */
> Should not be done.
Removed it. :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1647
PS6, Line 1647: 					joint_cap = ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT);
> Should not be done.
Removed it. :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1699
PS6, Line 1699: 					if (!cloned) {
> Should not be done.
Removed it. :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1707
PS6, Line 1707: 						ast_sip_session_media_state_free(media_state);
> Should not be done.
Removed it. :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1715
PS6, Line 1715: 					return 0;
> Should not be done.
Removed it. :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1721
PS6, Line 1721: 		}
> Should not be done.
Removed it. :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1729
PS6, Line 1729: 		if (on_sdp_creation) {
> Should not be done.
Removed it. :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1735
PS6, Line 1735: 	}
> Should not be done.
Removed it. :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1746
PS6, Line 1746: 		ast_log(LOG_WARNING, "Failed to create UPDATE properly.\n");
> Should not be done.
Removed it. :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1754
PS6, Line 1754: 			if (generate_new_sdp) {
> Should not be done.
Removed it. :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1762
PS6, Line 1762: 		ast_sorcery_object_get_id(session->endpoint));
> Should not be done.
Removed it. :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@2702
PS6, Line 2702: 			 * final session reference to be released but if both STATE and invite_tsx are NULL,
> I think this should be moved above the SWAP and session->active_media_state used instead - because a […]
I've got it. I moved it! :)


https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_t38.c
File res/res_pjsip_t38.c:

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_t38.c@342
PS6, Line 342: 		ast_sip_session_media_state_free(state->media_state);
> These are not actually required, as T.38 doesn't use RTP or have statistics so they will do nothing.
Okay, removed it. :)



-- 
To view, visit https://gerrit.asterisk.org/10899
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib25c2d3fc4da084aecfde2a82c1b1d733bd64fa5
Gerrit-Change-Number: 10899
Gerrit-PatchSet: 7
Gerrit-Owner: sungtae kim <pchero21 at gmail.com>
Gerrit-Reviewer: Friendly Automation (1000185)
Gerrit-Reviewer: Joshua C. Colp <jcolp at digium.com>
Gerrit-Reviewer: sungtae kim <pchero21 at gmail.com>
Gerrit-Comment-Date: Wed, 13 Feb 2019 22:02:21 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190213/8b91db62/attachment-0001.html>