[Asterisk-code-review] chan pjsip: Multistream: Modify use of ast sip session media (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/5851 )

Change subject: chan_pjsip: Multistream: Modify use of ast_sip_session_media
......................................................................


Patch Set 2: Code-Review-1

(3 comments)

This is pulling the comments from the previous review that still need to be addressed.

https://gerrit.asterisk.org/#/c/5851/2/include/asterisk/res_pjsip_session.h
File include/asterisk/res_pjsip_session.h:

https://gerrit.asterisk.org/#/c/5851/2/include/asterisk/res_pjsip_session.h@126
PS2, Line 126: 	/*! Lock for vector of session media */
             : 	ast_mutex_t media_streams_lock;
             : 	/*! Media streams */
             : 	AST_VECTOR(, struct ast_sip_session_media *) media;
Comment pulled from original review:
==========
This is going to need a lock to protect access.  The serializer and channel threads have contention for the vector.

* The T.38 frame hooks run under the channel thread.
* The chan pvt for chan_pjsip frame read/write run under the channel thread. Though if the vector is copied to the chan pvt this case may not be a problem.
* I think most other accesses are run under the serializer thread.

The lock is needed even more now because the vector can change during a call if streams are added/removed.
==========

The media_streams_lock isn't used yet.


https://gerrit.asterisk.org/#/c/5851/2/res/res_pjsip_session.c
File res/res_pjsip_session.c:

https://gerrit.asterisk.org/#/c/5851/2/res/res_pjsip_session.c@257
PS2, Line 257: 		old = AST_VECTOR_GET(&session->media, position);
Need to check if the old position exists before we can get it.

Can this vector have empty positions for declined streams?


https://gerrit.asterisk.org/#/c/5851/2/res/res_pjsip_session.c@423
PS2, Line 423: static int handle_negotiated_sdp(struct ast_sip_session *session, const pjmedia_sdp_session *local, const pjmedia_sdp_session *remote)
             : {
             : 	int i;
             : 
             : 	for (i = 0; i < local->media_count; ++i) {
             : 		struct ast_sip_session_media *session_media;
             : 		if (!remote->media[i]) {
             : 			continue;
             : 		}
             : 
             : 		/* If we're handling negotiated streams, then we should already have set
             : 		 * up session media instances that correspond to the local SDP, and there should
             : 		 * be the same number of session medias as there are local streams
             : 		 */
             : 		ast_assert(i < AST_VECTOR_SIZE(&session->media));
             : 
             : 		session_media = AST_VECTOR_GET(&session->media, i);
             : 		if (handle_negotiated_sdp_session_media(session_media, session, local, remote, i)) {
             : 			return -1;
             : 		}
             : 	}
             : 
             : 	return 0;
             : }
The queuing of the null frame is missing.  I think there was a reason the null frame was queued when all streams were successfully negotiated.  I don't know if it is still required though.



-- 
To view, visit https://gerrit.asterisk.org/5851
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I144b04f43633387b8e42a43ef3b25d7c5682b451
Gerrit-Change-Number: 5851
Gerrit-PatchSet: 2
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 15 Jun 2017 21:02:29 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170615/878a13e8/attachment-0001.html>