[Asterisk-code-review] chan pjsip: Add support for multiple streams of the same type. (asterisk[master])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/5876 )

Change subject: chan_pjsip: Add support for multiple streams of the same type.
......................................................................


Patch Set 11:

(3 comments)

https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c
File res/res_pjsip_session.c:

https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@379
PS11, Line 379: 	/* If we already have a session media in place in pending then a prior callback has
              : 	 * created it.
              : 	 */
              : 	if (position < AST_VECTOR_SIZE(&media_state->sessions)) {
              : 		return AST_VECTOR_GET(&media_state->sessions, position);
              : 	}
The comment is a little ambiguous? Can media_state always be the session's pending (it is called from t38 though)? If so can this just be session->pending and drop the extra param? Or is pending here describing something more generic and not specific to the session's pending states.


https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@494
PS11, Line 494: 		/* Enforce the user configured maximum stream count for this type by marking it declined and moving on*/
              : 		if (is_stream_limitation_reached(type, session->endpoint, type_streams)) {
              : 			ast_stream_set_state(stream, AST_STREAM_STATE_REMOVED);
              : 			continue;
              : 		}
Should we check this before everything else? Like right after we retrieve the type.


https://gerrit.asterisk.org/#/c/5876/11/res/res_pjsip_session.c@3408
PS11, Line 3408: 		if (is_stream_limitation_reached(ast_stream_get_type(stream), session->endpoint, type_streams)) {
               : 			ast_stream_set_state(stream, AST_STREAM_STATE_REMOVED);
               : 		}
> On an answer it makes sense to decline as we have to. In that case should w
My only thought to that is could there be a stream that is acceptable to the remote that we don't offer because we pruned it out due to limits, but it declined the ones we sent that were below the limit?



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8afd8dd2eb538806a39b887af0abd046266e14c7
Gerrit-Change-Number: 5876
Gerrit-PatchSet: 11
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 22 Jun 2017 21:50:55 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170622/350f390d/attachment-0001.html>