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

[Joshua Colp]

Joshua Colp 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:

(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 differen
The session media is marked as unbundled on an offer, so it'll use the above logic like normal.


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 th
It is, however I've found in order to have Firefox actually add the additional media streams the mid has to match the existing one present. Using new ones doesn't seem to work.


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 accep
Can you elaborate on this?

The message above is referring to the order of the bundle group attribute within the SDP. That is the first group of mids is 0, second group is 1, etc. That ordering doesn't have meaning except internally since I use the position as the identifier.


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?
I thought about it but didn't want to in case there was "video", "video-430594", etc.



-- 
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: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Tue, 11 Jul 2017 23:28:24 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170711/a6bfb0a2/attachment.html>