[Asterisk-code-review] res rtp asterisk / res pjsip: Add support for BUNDLE. (asterisk[master])

[Kevin Harwell]

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

Change subject: res_rtp_asterisk / res_pjsip: Add support for BUNDLE.
......................................................................


Patch Set 2: Code-Review-1

(4 comments)

https://gerrit.asterisk.org/#/c/5981/2/res/res_pjsip_sdp_rtp.c
File res/res_pjsip_sdp_rtp.c:

https://gerrit.asterisk.org/#/c/5981/2/res/res_pjsip_sdp_rtp.c@1396
PS2, Line 1396: 		media->conn = bundle_group_stream->conn;
              : 		media->desc.port = bundle_group_stream->desc.port;
If this is an initial offer by us then a unique address (same conn different port in this case) must be assigned for each m= line.


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

https://gerrit.asterisk.org/#/c/5981/2/res/res_pjsip_session.c@449
PS2, Line 449: 				session_media->mid = ast_strdup(ast_codec_media_type2str(type));
I believe the mid has to be unique for each stream in a bundle. Wouldn't this be possible for a collision to occur?


https://gerrit.asterisk.org/#/c/5981/2/res/res_pjsip_session.c@511
PS2, Line 511: 				/* The ordering of attributes determines our internal identification of the bundle group based on number,
             : 				 * with -1 being not in a bundle group. Since this is only exposed internally for response purposes it's
             : 				 * actually even fine if things move around.
             : 				 */
I think according to the rfc the order does matter as you may need to accept/reject a stream for the connection based on criterion and then move to the next one in the list if rejected.


https://gerrit.asterisk.org/#/c/5981/2/res/res_pjsip_session.c@506
PS2, Line 506: 		/* Skip the BUNDLE at the front */
             : 		mids += 7;
             : 
             : 		while ((attr_mid = strsep(&mids, " "))) {
             : 			if (!strcmp(attr_mid, mid)) {
             : 				/* The ordering of attributes determines our internal identification of the bundle group based on number,
             : 				 * with -1 being not in a bundle group. Since this is only exposed internally for response purposes it's
             : 				 * actually even fine if things move around.
             : 				 */
             : 				return bundle_group;
             : 			}
             : 		}
Could you just use strstr here?



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I96c0920b9f9aca7382256484765a239017973c11
Gerrit-Change-Number: 5981
Gerrit-PatchSet: 2
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Tue, 11 Jul 2017 20:53:22 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170711/3a0bd62e/attachment.html>