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

(8 comments)

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

https://gerrit.asterisk.org/#/c/5760/5/channels/chan_pjsip.c@434
PS5, Line 434: *!
             :  * \brief Determine if topologies have compatible formats
             :  *
             :  * This will return true if ANY formats in the topology are compatible with one another.
             :  * It does not guarantee that each individual stream is compatible with the other streams.
             :  *
             :  * XXX When supporting true multistream, we will need to be sure to mark which streams from
             :  * top1 are compatible with which streams from top2. Then the ones that are not compatible
             :  * will need to be marked as "removed" so that they are negotiated as expected.
             :  *
             :  * \param top1 Topology 1
             :  * \param top2 Topology 2
             :  * \retval 1 The topologies have at least one compatible format
             :  * \retval 0 The topologies have no compatible formats or an error occurred.
             :  */
             : static int compatible_formats_exist(struct ast_stream_topology *top, struct ast_format_cap *cap)
The doxygen no longer agrees with the prototype.


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

https://gerrit.asterisk.org/#/c/5760/5/channels/pjsip/dialplan_functions.c@952
PS5, Line 952: static int media_offer_read_av(struct ast_sip_session *session, char *buf,
             : 			       size_t len, enum ast_media_type media_type)
Hmm.  Maybe this should only get the first stream of type and return that codec list.  The current code will generate an accumulated codec list of all streams of the type.  Any common codecs of the streams will get listed multiple times.


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

https://gerrit.asterisk.org/#/c/5760/5/include/asterisk/res_pjsip.h@683
PS5, Line 683: 	/*! Capabilities in topology form */
             : 	struct ast_stream_topology *topology;
Since this is for master only, I'd add this next to the codecs member since they represent the same information and it avoids possible struct padding.


https://gerrit.asterisk.org/#/c/5760/5/res/res_pjsip/pjsip_configuration.c
File res/res_pjsip/pjsip_configuration.c:

https://gerrit.asterisk.org/#/c/5760/5/res/res_pjsip/pjsip_configuration.c@2068
PS5, Line 2068: 	ao2_cleanup(endpoint->media.codecs);
Need to free the new topology member here too.


https://gerrit.asterisk.org/#/c/5760/5/res/res_pjsip_sdp_rtp.c
File res/res_pjsip_sdp_rtp.c:

https://gerrit.asterisk.org/#/c/5760/5/res/res_pjsip_sdp_rtp.c@361
PS5, Line 361: 	if (direct_media_enabled) {
             : 		ast_format_cap_get_compatible(session->endpoint->media.codecs, session->direct_media_cap, caps);
             : 	} else {
What was the reason format_cap_only_type() wasn't restored too?

Actually I see that it doesn't really matter that caps has codecs other than the media_type in it so the function is not needed anyway.  The only place it has an effect is when we report in a warning message about no joint codecs.


https://gerrit.asterisk.org/#/c/5760/5/res/res_pjsip_sdp_rtp.c@405
PS5, Line 405: 		ast_format_cap_replace_from_cap(req_caps, joint, AST_MEDIA_TYPE_UNKNOWN);
This should be instead
ast_stream_set_formats(req_stream, joint);

Meaning ast_stream_set_formats() can be pulled out of both if (!req_stream) clauses and placed after the if statement as common code and the req_caps variable is not used anymore.


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

https://gerrit.asterisk.org/#/c/5760/5/res/res_pjsip_session.c@1781
PS5, Line 1781: 			req_stream = ast_stream_topology_get_stream(req_topology, i);
Need check if req_stream is declined and skip it.


https://gerrit.asterisk.org/#/c/5760/5/res/res_pjsip_session.c@1793
PS5, Line 1793: 				ast_format_cap_append_from_cap(req_cap,
              : 						endpoint->media.codecs, AST_MEDIA_TYPE_UNKNOWN);
              : 				/* replace instances of joint caps equivalents in req_caps */
              : 				ast_format_cap_replace_from_cap(req_cap, joint_cap,
              : 						AST_MEDIA_TYPE_UNKNOWN);
This code is modifying req_cap which is the caps from the passed in req_topology.  Probably shouldn't do that.  Also this code is throwing all media type codecs into the mix when it should be media type specific codecs only.

Operate on the clone_stream caps instead.



-- 
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: 5
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: Thu, 15 Jun 2017 20:02:27 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170615/fa7772b6/attachment.html>