<p>Joshua Colp <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/5981">View Change</a></p><p>Patch set 2:</p><p>(4 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/5981/2/res/res_pjsip_sdp_rtp.c">File res/res_pjsip_sdp_rtp.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5981/2/res/res_pjsip_sdp_rtp.c@1396">Patch Set #2, Line 1396:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> media->conn = bundle_group_stream->conn;<br> media->desc.port = bundle_group_stream->desc.port;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">If this is an initial offer by us then a unique address (same conn differen</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The session media is marked as unbundled on an offer, so it'll use the above logic like normal.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5981/2/res/res_pjsip_session.c">File res/res_pjsip_session.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5981/2/res/res_pjsip_session.c@449">Patch Set #2, Line 449:</a> <code style="font-family:monospace,monospace"> session_media->mid = ast_strdup(ast_codec_media_type2str(type));</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I believe the mid has to be unique for each stream in a bundle. Wouldn't th</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5981/2/res/res_pjsip_session.c@511">Patch Set #2, Line 511:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* The ordering of attributes determines our internal identification of the bundle group based on number,<br> * with -1 being not in a bundle group. Since this is only exposed internally for response purposes it's<br> * actually even fine if things move around.<br> */<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think according to the rfc the order does matter as you may need to accep</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can you elaborate on this?</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5981/2/res/res_pjsip_session.c@506">Patch Set #2, Line 506:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* Skip the BUNDLE at the front */<br> mids += 7;<br><br> while ((attr_mid = strsep(&mids, " "))) {<br> if (!strcmp(attr_mid, mid)) {<br> /* The ordering of attributes determines our internal identification of the bundle group based on number,<br> * with -1 being not in a bundle group. Since this is only exposed internally for response purposes it's<br> * actually even fine if things move around.<br> */<br> return bundle_group;<br> }<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Could you just use strstr here?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I thought about it but didn't want to in case there was "video", "video-430594", etc.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/5981">change 5981</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/5981"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I96c0920b9f9aca7382256484765a239017973c11 </div>
<div style="display:none"> Gerrit-Change-Number: 5981 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 11 Jul 2017 23:28:24 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>