[Asterisk-code-review] chan pjsip: Multistream: Use underlying multistream structures. (asterisk[master])

[Richard Mudgett]

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

Change subject: chan_pjsip: Multistream: Use underlying multistream structures.
......................................................................


Patch Set 4: Code-Review-1

(8 comments)

https://gerrit.asterisk.org/#/c/5760/4/channels/chan_pjsip.c
File channels/chan_pjsip.c:

https://gerrit.asterisk.org/#/c/5760/4/channels/chan_pjsip.c@451
PS4, Line 451: 	struct ast_format_cap *cap1;
             : 	struct ast_format_cap *cap2;
cap1 and cap2 are ref leaked.


https://gerrit.asterisk.org/#/c/5760/4/channels/chan_pjsip.c@515
PS4, Line 515: 	ast_channel_nativeformats_set(chan, caps);
ast_channel_stage_snapshot(chan) should be moved to just before this call so we don't hangup chan with the snapshot staging flag set.


https://gerrit.asterisk.org/#/c/5760/4/channels/chan_pjsip.c@2252
PS4, Line 2252: 		*cause = req_data.cause;
              : 		return NULL;
req_data.topology is leaked here.


https://gerrit.asterisk.org/#/c/5760/4/channels/pjsip/dialplan_functions.c
File channels/pjsip/dialplan_functions.c:

https://gerrit.asterisk.org/#/c/5760/4/channels/pjsip/dialplan_functions.c@936
PS4, Line 936: 		size = strlen(ast_format_get_name(fmt)) + 1;
             : 		if (*len < size) {
Ooh.  Pre-existing bug.  We aren't considering the null terminator when appending even though we eventually build a string that removes the trailing comma.  We can write a null terminator one over the remaining buffer size.

The bug still isn't fixed by this patch either.


https://gerrit.asterisk.org/#/c/5760/4/channels/pjsip/dialplan_functions.c@955
PS4, Line 955: 	int i;
             : 
             : 	for (i = 0; i < ast_stream_topology_get_count(session->req_topology); ++i) {
Need to set
buf[0] = '\0';
to ensure that buf is empty if there happen to be no streams in the topology or all streams happen to be declined.


https://gerrit.asterisk.org/#/c/5760/4/channels/pjsip/dialplan_functions.c@963
PS4, Line 963: 		if (ast_stream_get_type(stream) != media_type) {
Need to also skip declined streams.  ast_stream_get_state(stream) == AST_STREAM_STATE_REMOVED

Declined streams may not even have a format cap.


https://gerrit.asterisk.org/#/c/5760/4/include/asterisk/res_pjsip.h
File include/asterisk/res_pjsip.h:

https://gerrit.asterisk.org/#/c/5760/4/include/asterisk/res_pjsip.h@668
PS4, Line 668: 	struct ast_stream_topology *codecs;
I think this struct member should be left the way it was.  This patch spends a lot of effort converting this topology to and from a caps struct.  Also this should be the configured codecs for allowed new audio/video streams.  Unless we are going to add a lot of configuration options to configure individually allowed streams such as audio1=alaw,ulaw and audio2=ilbc,g729 this topology change is overkill.


https://gerrit.asterisk.org/#/c/5760/4/res/res_pjsip_session.c
File res/res_pjsip_session.c:

https://gerrit.asterisk.org/#/c/5760/4/res/res_pjsip_session.c@1818
PS4, Line 1818: 				if (ast_stream_topology_append_stream(session->req_topology, clone_stream) < 0) {
              : 					continue;
clone_stream is leaked here.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0358ea3108e2c06ae43308abddf216a23deccb90
Gerrit-Change-Number: 5760
Gerrit-PatchSet: 4
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Wed, 14 Jun 2017 20:44:53 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170614/e7ba0a2c/attachment.html>