<p>Kevin Harwell <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/6936">View Change</a></p><p>Patch set 3:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(2 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/6936/3/main/stream.c">File main/stream.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6936/3/main/stream.c@70">Patch Set #3, Line 70:</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;">    * \brief The group that the stream is part of<br>         */<br>   int group;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think things could be simplified by moving the msid/mslabel stuff onto the stream itself. Although you probably would want two separate fields derived from the msid. One for the unique stream id and the other for the stream group. Then you wouldn't need a third field representing the group.</p><p style="white-space: pre-wrap; word-wrap: break-word;">You could then also have a is_in_group method on the stream that tests if streams are part of the same group.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6936/3/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/6936/3/res/res_pjsip_session.c@611">Patch Set #3, Line 611:</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;">            if (ast_strlen_zero(group_session_media->remote_mslabel) ||<br>                        strcmp(group_session_media->remote_mslabel, session_media->remote_mslabel)) {<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">If the remote_mslabel is derived from the msid this may not match sometimes since the msid would different for each stream. You'd need to break apart the msid into msid value and msid appdata (which is typically the group id).</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/6936">change 6936</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/6936"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Id6299aa031efe46254edbdc7973c534d54d641ad </div>
<div style="display:none"> Gerrit-Change-Number: 6936 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 31 Oct 2017 16:30:50 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>