<p>Joshua Colp has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/6171">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">bridge: Fix stream topology/participant locking and video misrouting.<br><br>This change fixes a few locking issues and some video misrouting.<br><br>1. When accessing the stream topology of a channel the channel lock<br>must be held to guarantee the topology remains valid.<br><br>2. When a channel was joined to a bridge the bridge specific<br>implementation for stream mapping was not invoked, causing video<br>to be misrouted for a brief period of time.<br><br>ASTERISK-27182<br><br>Change-Id: I5d2f779248b84d41c5bb3896bf22ba324b336b03<br>---<br>M bridges/bridge_softmix.c<br>M main/bridge.c<br>M main/bridge_channel.c<br>3 files changed, 51 insertions(+), 22 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/71/6171/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/bridges/bridge_softmix.c b/bridges/bridge_softmix.c<br>index c5428a8..b359948 100644<br>--- a/bridges/bridge_softmix.c<br>+++ b/bridges/bridge_softmix.c<br>@@ -577,13 +577,18 @@<br>       struct ast_stream_topology *joiner_video = NULL;<br>      struct ast_stream_topology *existing_video = NULL;<br>    struct ast_bridge_channel *participant;<br>+      int res;<br> <br>   joiner_video = ast_stream_topology_alloc();<br>   if (!joiner_video) {<br>          return;<br>       }<br> <br>- if (append_source_streams(joiner_video, ast_channel_name(joiner->chan), ast_channel_get_stream_topology(joiner->chan))) {<br>+      ast_channel_lock(joiner->chan);<br>+   res = append_source_streams(joiner_video, ast_channel_name(joiner->chan), ast_channel_get_stream_topology(joiner->chan));<br>+      ast_channel_unlock(joiner->chan);<br>+<br>+      if (res) {<br>            goto cleanup;<br>         }<br> <br>@@ -596,13 +601,18 @@<br>           if (participant == joiner) {<br>                  continue;<br>             }<br>-            if (append_source_streams(existing_video, ast_channel_name(participant->chan),<br>-                            ast_channel_get_stream_topology(participant->chan))) {<br>+            ast_channel_lock(participant->chan);<br>+              res = append_source_streams(existing_video, ast_channel_name(participant->chan),<br>+                          ast_channel_get_stream_topology(participant->chan));<br>+              ast_channel_unlock(participant->chan);<br>+            if (res) {<br>                    goto cleanup;<br>                 }<br>     }<br> <br>+ ast_channel_lock(joiner->chan);<br>    joiner_topology = ast_stream_topology_clone(ast_channel_get_stream_topology(joiner->chan));<br>+       ast_channel_unlock(joiner->chan);<br>  if (!joiner_topology) {<br>               goto cleanup;<br>         }<br>@@ -617,7 +627,9 @@<br>                if (participant == joiner) {<br>                  continue;<br>             }<br>+            ast_channel_lock(participant->chan);<br>               participant_topology = ast_stream_topology_clone(ast_channel_get_stream_topology(participant->chan));<br>+             ast_channel_unlock(participant->chan);<br>             if (!participant_topology) {<br>                  goto cleanup;<br>                 }<br>@@ -753,12 +765,16 @@<br>                      continue;<br>             }<br> <br>+         ast_channel_lock(participant->chan);<br>               remove_destination_streams(participant_topology, ast_channel_name(leaver->chan), ast_channel_get_stream_topology(participant->chan));<br>+          ast_channel_unlock(participant->chan);<br>             ast_channel_request_stream_topology_change(participant->chan, participant_topology, NULL);<br>                 ast_stream_topology_free(participant_topology);<br>       }<br> <br>+ ast_channel_lock(leaver->chan);<br>    remove_destination_streams(leaver_topology, "", ast_channel_get_stream_topology(leaver->chan));<br>+ ast_channel_unlock(leaver->chan);<br>  ast_channel_request_stream_topology_change(leaver->chan, leaver_topology, NULL);<br>   ast_stream_topology_free(leaver_topology);<br> <br>@@ -1657,6 +1673,7 @@<br>          }<br> <br>          ast_bridge_channel_lock(participant);<br>+                ast_channel_lock(participant->chan);<br>               topology = ast_channel_get_stream_topology(participant->chan);<br> <br>          for (i = 0; i < ast_stream_topology_get_count(topology); ++i) {<br>@@ -1668,6 +1685,7 @@<br>                             break;<br>                        }<br>             }<br>+            ast_channel_unlock(participant->chan);<br>             ast_bridge_channel_unlock(participant);<br>       }<br> }<br>@@ -1702,16 +1720,9 @@<br> <br>      /* First traversal: re-initialize all of the participants' stream maps */<br>         AST_LIST_TRAVERSE(&bridge->channels, participant, entry) {<br>-            int size;<br>-<br>          ast_bridge_channel_lock(participant);<br>-                size = ast_stream_topology_get_count(ast_channel_get_stream_topology(participant->chan));<br>-<br>-              AST_VECTOR_FREE(&participant->stream_map.to_channel);<br>-         AST_VECTOR_FREE(&participant->stream_map.to_bridge);<br>-<br>-               AST_VECTOR_INIT(&participant->stream_map.to_channel, size);<br>-           AST_VECTOR_INIT(&participant->stream_map.to_bridge, size);<br>+            AST_VECTOR_RESET(&participant->stream_map.to_channel, AST_VECTOR_ELEM_CLEANUP_NOOP);<br>+          AST_VECTOR_RESET(&participant->stream_map.to_bridge, AST_VECTOR_ELEM_CLEANUP_NOOP);<br>            ast_bridge_channel_unlock(participant);<br>       }<br> <br>@@ -1726,6 +1737,8 @@<br>   AST_LIST_TRAVERSE(&bridge->channels, participant, entry) {<br>             int i;<br>                struct ast_stream_topology *topology;<br>+<br>+             ast_channel_lock(participant->chan);<br> <br>            topology = ast_channel_get_stream_topology(participant->chan);<br> <br>@@ -1766,6 +1779,8 @@<br>                   }<br>                     ast_bridge_channel_unlock(participant);<br>               }<br>+<br>+         ast_channel_unlock(participant->chan);<br>     }<br> }<br> <br>diff --git a/main/bridge.c b/main/bridge.c<br>index a1a1a6f..b732d5f 100644<br>--- a/main/bridge.c<br>+++ b/main/bridge.c<br>@@ -446,7 +446,12 @@<br>    * media types vector. This way all streams map to the same media type index for<br>       * a given channel.<br>    */<br>-  ast_bridge_channel_stream_map(bridge_channel);<br>+       if (bridge_channel->bridge->technology->stream_topology_changed) {<br>+          bridge_channel->bridge->technology->stream_topology_changed(<br>+                        bridge_channel->bridge, bridge_channel);<br>+  } else {<br>+             ast_bridge_channel_stream_map(bridge_channel);<br>+       }<br> }<br> <br> /*!<br>diff --git a/main/bridge_channel.c b/main/bridge_channel.c<br>index 2e94300..1427fd0 100644<br>--- a/main/bridge_channel.c<br>+++ b/main/bridge_channel.c<br>@@ -2344,16 +2344,23 @@<br>  case AST_FRAME_NULL:<br>          break;<br>        default:<br>-             if (fr->stream_num > 0 &&<br>-                              (fr->stream_num >= (int)AST_VECTOR_SIZE(&bridge_channel->stream_map.to_channel) ||<br>-                              AST_VECTOR_GET(&bridge_channel->stream_map.to_channel, fr->stream_num) == -1)) {<br>-                   /* Nowhere to write to, so drop it */<br>-                        break;<br>-               }<br>+            /* Assume that there is no mapped stream for this */<br>+         num = -1;<br> <br>-         /* Find what stream number to write to for the channel */<br>-            num = fr->stream_num < 0 ? -1 :<br>-                        AST_VECTOR_GET(&bridge_channel->stream_map.to_channel, fr->stream_num);<br>+            if (fr->stream_num > -1) {<br>+                     ast_bridge_channel_lock(bridge_channel);<br>+                     if (fr->stream_num < (int)AST_VECTOR_SIZE(&bridge_channel->stream_map.to_channel)) {<br>+                            num = AST_VECTOR_GET(&bridge_channel->stream_map.to_channel, fr->stream_num);<br>+                      }<br>+                    ast_bridge_channel_unlock(bridge_channel);<br>+<br>+                        /* If there is no mapped stream after checking the mapping then there is nowhere<br>+                      * to write this frame to, so drop it.<br>+                        */<br>+                  if (num == -1) {<br>+                             break;<br>+                       }<br>+            }<br> <br>          /* Write the frame to the channel. */<br>                 bridge_channel->activity = BRIDGE_CHANNEL_THREAD_SIMPLE;<br>@@ -2983,7 +2990,9 @@<br> <br> void ast_bridge_channel_stream_map(struct ast_bridge_channel *bridge_channel)<br> {<br>+    ast_channel_lock(bridge_channel->chan);<br>    ast_stream_topology_map(ast_channel_get_stream_topology(bridge_channel->chan),<br>             &bridge_channel->bridge->media_types, &bridge_channel->stream_map.to_bridge,<br>                 &bridge_channel->stream_map.to_channel);<br>+      ast_channel_unlock(bridge_channel->chan);<br> }<br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/6171">change 6171</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/6171"/><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: newchange </div>
<div style="display:none"> Gerrit-Change-Id: I5d2f779248b84d41c5bb3896bf22ba324b336b03 </div>
<div style="display:none"> Gerrit-Change-Number: 6171 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Joshua Colp <jcolp@digium.com> </div>