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

[Joshua C. Colp]

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 3: Code-Review-1

(8 comments)

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.

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.

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

https://gerrit.asterisk.org/#/c/10899/3/channels/chan_pjsip.c@1497
PS3, Line 1497: 	ast_sip_session_media_stats_save(refresh_data->media_state);
This is not necessary. This media state conveys a desired state, it doesn't contain RTP instances.


https://gerrit.asterisk.org/#/c/10899/3/channels/chan_pjsip.c@1538
PS3, Line 1538: 		ast_sip_session_media_stats_save(session->pending_media_state);
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.


https://gerrit.asterisk.org/#/c/10899/3/channels/pjsip/dialplan_functions.c
File channels/pjsip/dialplan_functions.c:

https://gerrit.asterisk.org/#/c/10899/3/channels/pjsip/dialplan_functions.c@1054
PS3, Line 1054: 	ast_sip_session_media_stats_save(state->media_state);
Same, not necessary.


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

https://gerrit.asterisk.org/#/c/10899/3/include/asterisk/res_pjsip_session.h@144
PS3, Line 144: 	/*! \brief Related sip session */
             : 	struct ast_sip_session *sip_session;
I don't think storing the session is necessary.


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

https://gerrit.asterisk.org/#/c/10899/3/res/res_pjsip_session.c@212
PS3, Line 212: void ast_sip_session_media_stats_save(struct ast_sip_session_media_state *media_state)
Can't the session be passed in here instead?


https://gerrit.asterisk.org/#/c/10899/3/res/res_pjsip_session.c@229
PS3, Line 229: 		rtp_tmp = ao2_bump(media->rtp);
You are guaranteed that the RTP instance won't go away


https://gerrit.asterisk.org/#/c/10899/3/res/res_pjsip_session.c@231
PS3, Line 231: 		stats_tmp = ast_calloc(1, sizeof(struct ast_rtp_instance_stats));
This allocation could fail


https://gerrit.asterisk.org/#/c/10899/3/res/res_pjsip_session.c@2172
PS3, Line 2172: 	for (idx = 0; idx < AST_VECTOR_SIZE(&session->media_stats); idx++) {
              : 		struct ast_rtp_instance_stats* tmp_stats = AST_VECTOR_GET(&session->media_stats, idx);
              : 		ast_free(tmp_stats);
              : 		AST_VECTOR_REMOVE(&session->media_stats, idx, 1);
              : 	}
I believe you can use AST_VECTOR_RESET here instead



-- 
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: 3
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: Mon, 04 Feb 2019 12:09:08 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190204/e97cc866/attachment-0001.html>