[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 4:

(8 comments)

> (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.


Hi Joshua, thank you for your kind reviewing.

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.

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.

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_state_free(refresh_data->media_state);
> This is not necessary. This media state conveys a desired state, it doesn't contain RTP instances.
Okay, I removed it.


https://gerrit.asterisk.org/#/c/10899/3/channels/chan_pjsip.c@1538
PS3, Line 1538: 		ast_sip_session_media_state_reset(session->pending_media_state);
> This pending media state may have both old and new RTP instances, in the case of old it is possible  […]
I couldn't catch up what case would be. Do you have idea how this should be fixed to?


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_state_free(state->media_state);
> Same, not necessary.
Fixed it.


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.
Because of ast_sip_session_media_stats_save() gets only the media_state as a parameter. So I needed related session's info somehow.


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?
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.


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
Yes. :)


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
Right, added Null check.


https://gerrit.asterisk.org/#/c/10899/3/res/res_pjsip_session.c@2172
PS3, Line 2172: 	ast_sip_session_remove_supplements(session);
              : 	AST_LIST_HEAD_DESTROY(&session->supplements);
              : 
              : 	/* remove all saved media stats */
              : 	A
> I believe you can use AST_VECTOR_RESET here instead
Yes, I've fixed 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: 4
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, 06 Feb 2019 21:58:19 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190206/8cb88f88/attachment.html>