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

Wed Feb 13 06:24:28 CST 2019

Joshua C. Colp 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 6: Code-Review-1

(21 comments)

https://gerrit.asterisk.org/#/c/10899/6//COMMIT_MSG
Commit Message:

https://gerrit.asterisk.org/#/c/10899/6//COMMIT_MSG@13
PS6, Line 13: 
The JIRA issue needs to be in here, or else it will not be picked up appropriately when released.

https://gerrit.asterisk.org/#/c/10899/6/channels/chan_pjsip.c
File channels/chan_pjsip.c:

https://gerrit.asterisk.org/#/c/10899/6/channels/chan_pjsip.c@1537
PS6, Line 1537: 		ast_sip_session_media_stats_save(session, session->pending_media_state);
Should not be done here.

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 stats result saving */
This could use a better description, for example:

Media statistics for negotiated RTP streams

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: 		rtp_tmp = ao2_bump(media->rtp);
This is not necessary. You don't need to bump the ref, you can directly use media->rtp.

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@242
PS6, Line 242: 		AST_VECTOR_APPEND(&sip_session->media_stats, stats_tmp);
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.

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1416
PS6, Line 1416: 		ast_sip_session_media_stats_save(session, media_state);
Should not be done.

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1538
PS6, Line 1538: 		ast_sip_session_media_stats_save(session, media_state);
Should not be done.

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1547
PS6, Line 1547: 		ast_sip_session_media_stats_save(session, media_state);
Should not be done.

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1576
PS6, Line 1576: 			ast_sip_session_media_stats_save(session, media_state);
Should not be done.

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1647
PS6, Line 1647: 						ast_sip_session_media_stats_save(session, media_state);
Should not be done.

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1699
PS6, Line 1699: 						ast_sip_session_media_stats_save(session, media_state);
Should not be done.

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

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1715
PS6, Line 1715: 					ast_sip_session_media_stats_save(session, media_state);
Should not be done.

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1721
PS6, Line 1721: 			ast_sip_session_media_stats_save(session, session->pending_media_state);
Should not be done.

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1729
PS6, Line 1729: 			ast_sip_session_media_stats_save(session, session->pending_media_state);
Should not be done.

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1735
PS6, Line 1735: 				ast_sip_session_media_stats_save(session, session->pending_media_state);
Should not be done.

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1746
PS6, Line 1746: 				ast_sip_session_media_stats_save(session, session->pending_media_state);
Should not be done.

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@1754
PS6, Line 1754: 			ast_sip_session_media_stats_save(session, session->pending_media_state);
Should not be done.

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

https://gerrit.asterisk.org/#/c/10899/6/res/res_pjsip_session.c@2702
PS6, Line 2702: 	ast_sip_session_media_stats_save(session, session->pending_media_state);
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.

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_stats_save(session, state->media_state);
These are not actually required, as T.38 doesn't use RTP or have statistics so they will do nothing.

-- 
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: 6
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 12:24:28 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190213/94f53b74/attachment.html>