<p>Joshua Colp has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/6821">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">bridge_softmix: Reduce topology cloning and improve renegotiation.<br><br>As channels join and leave an SFU the bridge_softmix module<br>needs to renegotiate to add and remove their streams from<br>the other participants. Previously this was done by constructing<br>the ideal stream topology every time but in the case of leave<br>this was incomplete.<br><br>This change makes it so bridge_softmix keeps an ideal stream<br>topology for each channel and uses it when making changes. This<br>ensures that when we request a renegotiation we are always<br>certain that we are aiming for the best stream topology<br>possible. In the case of a channel leaving this ensures that<br>we try to have an existing participant fill their place if<br>a participant has a fixed limit on the maximum number of video<br>streams they allow.<br><br>ASTERISK-27354<br><br>Change-Id: I58070f421ddeadd2844a33b869b052630cf2e514<br>---<br>M bridges/bridge_softmix.c<br>M bridges/bridge_softmix/include/bridge_softmix_internal.h<br>2 files changed, 38 insertions(+), 105 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/21/6821/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 5e0a485..9197df6 100644<br>--- a/bridges/bridge_softmix.c<br>+++ b/bridges/bridge_softmix.c<br>@@ -573,27 +573,24 @@<br> */<br> static void sfu_topologies_on_join(struct ast_bridge_channel *joiner, struct ast_bridge_channels_list *participants)<br> {<br>- struct ast_stream_topology *joiner_topology = NULL;<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>+ struct softmix_channel *sc;<br> <br> joiner_video = ast_stream_topology_alloc();<br> if (!joiner_video) {<br> return;<br> }<br> <br>+ sc = joiner->tech_pvt;<br>+<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>+ sc->topology = ast_stream_topology_clone(ast_channel_get_stream_topology(joiner->chan));<br> ast_channel_unlock(joiner->chan);<br> <br>- if (res) {<br>- goto cleanup;<br>- }<br>-<br>- existing_video = ast_stream_topology_alloc();<br>- if (!existing_video) {<br>+ if (res || !sc->topology) {<br> goto cleanup;<br> }<br> <br>@@ -602,7 +599,7 @@<br> continue;<br> }<br> ast_channel_lock(participant->chan);<br>- res = append_source_streams(existing_video, ast_channel_name(participant->chan),<br>+ res = append_source_streams(sc->topology, ast_channel_name(participant->chan),<br> ast_channel_get_stream_topology(participant->chan));<br> ast_channel_unlock(participant->chan);<br> if (res) {<br>@@ -610,41 +607,22 @@<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>- if (append_all_streams(joiner_topology, existing_video)) {<br>- goto cleanup;<br>- }<br>- ast_channel_request_stream_topology_change(joiner->chan, joiner_topology, NULL);<br>+ ast_channel_request_stream_topology_change(joiner->chan, sc->topology, NULL);<br> <br> AST_LIST_TRAVERSE(participants, participant, entry) {<br>- struct ast_stream_topology *participant_topology;<br>-<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>+<br>+ sc = participant->tech_pvt;<br>+ if (append_all_streams(sc->topology, joiner_video)) {<br> goto cleanup;<br> }<br>- if (append_all_streams(participant_topology, joiner_video)) {<br>- ast_stream_topology_free(participant_topology);<br>- goto cleanup;<br>- }<br>- ast_channel_request_stream_topology_change(participant->chan, participant_topology, NULL);<br>- ast_stream_topology_free(participant_topology);<br>+ ast_channel_request_stream_topology_change(participant->chan, sc->topology, NULL);<br> }<br> <br> cleanup:<br> ast_stream_topology_free(joiner_video);<br>- ast_stream_topology_free(existing_video);<br>- ast_stream_topology_free(joiner_topology);<br> }<br> <br> /*! \brief Function called when a channel is joined into the bridge */<br>@@ -718,65 +696,36 @@<br> return 0;<br> }<br> <br>-static int remove_destination_streams(struct ast_stream_topology *dest,<br>- const char *channel_name,<br>- const struct ast_stream_topology *source)<br>+static void remove_destination_streams(struct ast_stream_topology *topology,<br>+ const char *channel_name)<br> {<br> int i;<br> <br>- for (i = 0; i < ast_stream_topology_get_count(source); ++i) {<br>+ for (i = 0; i < ast_stream_topology_get_count(topology); ++i) {<br> struct ast_stream *stream;<br>- struct ast_stream *stream_clone;<br> <br>- stream = ast_stream_topology_get_stream(source, i);<br>-<br>- stream_clone = ast_stream_clone(stream, NULL);<br>- if (!stream_clone) {<br>- continue;<br>- }<br>+ stream = ast_stream_topology_get_stream(topology, i);<br> <br> if (is_video_dest(stream, channel_name, NULL)) {<br>- ast_stream_set_state(stream_clone, AST_STREAM_STATE_REMOVED);<br>- }<br>-<br>- if (ast_stream_topology_append_stream(dest, stream_clone) < 0) {<br>- ast_stream_free(stream_clone);<br>+ ast_stream_set_state(stream, AST_STREAM_STATE_REMOVED);<br> }<br> }<br>-<br>- return 0;<br> }<br> <br> static int sfu_topologies_on_leave(struct ast_bridge_channel *leaver, struct ast_bridge_channels_list *participants)<br> {<br>- struct ast_stream_topology *leaver_topology;<br> struct ast_bridge_channel *participant;<br>-<br>- leaver_topology = ast_stream_topology_alloc();<br>- if (!leaver_topology) {<br>- return -1;<br>- }<br>+ struct softmix_channel *sc;<br> <br> AST_LIST_TRAVERSE(participants, participant, entry) {<br>- struct ast_stream_topology *participant_topology;<br>-<br>- participant_topology = ast_stream_topology_alloc();<br>- if (!participant_topology) {<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>+ sc = participant->tech_pvt;<br>+ remove_destination_streams(sc->topology, ast_channel_name(leaver->chan));<br>+ ast_channel_request_stream_topology_change(participant->chan, sc->topology, NULL);<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>+ sc = leaver->tech_pvt;<br>+ remove_destination_streams(sc->topology, "");<br>+ ast_channel_request_stream_topology_change(leaver->chan, sc->topology, NULL);<br> <br> return 0;<br> }<br>@@ -805,6 +754,8 @@<br> }<br> <br> bridge_channel->tech_pvt = NULL;<br>+<br>+ ast_stream_topology_free(sc->topology);<br> <br> /* Drop mutex lock */<br> ast_mutex_destroy(&sc->lock);<br>@@ -1055,30 +1006,23 @@<br> <br> AST_LIST_TRAVERSE(participants, participant, entry) {<br> struct ast_stream_topology *original_topology;<br>- struct ast_stream_topology *participant_topology;<br>+ struct softmix_channel *sc;<br> <br> if (participant == source) {<br> continue;<br> }<br> <br>- ast_channel_lock(participant->chan);<br>- original_topology = ast_stream_topology_clone(ast_channel_get_stream_topology(participant->chan));<br>- ast_channel_unlock(participant->chan);<br>- if (!original_topology) {<br>- goto cleanup;<br>- }<br>+ sc = participant->tech_pvt;<br> <br>- participant_topology = ast_stream_topology_clone(original_topology);<br>- if (!participant_topology) {<br>- ast_stream_topology_free(original_topology);<br>+ original_topology = ast_stream_topology_clone(sc->topology);<br>+ if (!original_topology) {<br> goto cleanup;<br> }<br> <br> /* We add all the source streams back in, if any removed streams are already present they will<br> * get used first followed by appending new ones.<br> */<br>- if (append_all_streams(participant_topology, source_video)) {<br>- ast_stream_topology_free(participant_topology);<br>+ if (append_all_streams(sc->topology, source_video)) {<br> ast_stream_topology_free(original_topology);<br> goto cleanup;<br> }<br>@@ -1086,14 +1030,12 @@<br> /* And the original existing streams get marked as removed. This causes the remote side to see<br> * a new stream for the source streams.<br> */<br>- if (remove_all_original_streams(participant_topology, source_video, original_topology)) {<br>- ast_stream_topology_free(participant_topology);<br>+ if (remove_all_original_streams(sc->topology, source_video, original_topology)) {<br> ast_stream_topology_free(original_topology);<br> goto cleanup;<br> }<br> <br>- ast_channel_request_stream_topology_change(participant->chan, participant_topology, NULL);<br>- ast_stream_topology_free(participant_topology);<br>+ ast_channel_request_stream_topology_change(participant->chan, sc->topology, NULL);<br> ast_stream_topology_free(original_topology);<br> }<br> <br>@@ -2144,7 +2086,6 @@<br> { "", 4, { 0, 1, 2, 3 }, },<br> };<br> struct ast_stream_topology *orig = NULL;<br>- struct ast_stream_topology *result = NULL;<br> int i;<br> <br> switch (cmd) {<br>@@ -2168,20 +2109,11 @@<br> for (i = 0; i < ARRAY_LEN(removal_results); ++i) {<br> int j;<br> <br>- result = ast_stream_topology_alloc();<br>- if (!result) {<br>- ast_test_status_update(test, "Unable to allocate result stream topology\n");<br>- goto end;<br>- }<br>+ remove_destination_streams(orig, removal_results[i].channel_name);<br> <br>- if (remove_destination_streams(result, removal_results[i].channel_name, orig)) {<br>- ast_test_status_update(test, "Failure while attempting to remove video streams\n");<br>- goto end;<br>- }<br>-<br>- if (ast_stream_topology_get_count(result) != removal_results[i].num_streams) {<br>+ if (ast_stream_topology_get_count(orig) != removal_results[i].num_streams) {<br> ast_test_status_update(test, "Resulting topology has %d streams, when %d are expected\n",<br>- ast_stream_topology_get_count(result), removal_results[i].num_streams);<br>+ ast_stream_topology_get_count(orig), removal_results[i].num_streams);<br> goto end;<br> }<br> <br>@@ -2190,7 +2122,7 @@<br> struct ast_stream *expected;<br> int orig_index;<br> <br>- actual = ast_stream_topology_get_stream(result, j);<br>+ actual = ast_stream_topology_get_stream(orig, j);<br> <br> orig_index = removal_results[i].params_index[j];<br> expected = ast_stream_topology_get_stream(orig, orig_index);<br>@@ -2221,7 +2153,6 @@<br> <br> end:<br> ast_stream_topology_free(orig);<br>- ast_stream_topology_free(result);<br> return res;<br> }<br> <br>diff --git a/bridges/bridge_softmix/include/bridge_softmix_internal.h b/bridges/bridge_softmix/include/bridge_softmix_internal.h<br>index f93e663..01e65aa 100644<br>--- a/bridges/bridge_softmix/include/bridge_softmix_internal.h<br>+++ b/bridges/bridge_softmix/include/bridge_softmix_internal.h<br>@@ -167,6 +167,8 @@<br> short our_buf[MAX_DATALEN];<br> /*! Data pertaining to talker mode for video conferencing */<br> struct video_follow_talker_data video_talker;<br>+ /*! The ideal stream topology for the channel */<br>+ struct ast_stream_topology *topology;<br> };<br> <br> struct softmix_bridge_data {<br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/6821">change 6821</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/6821"/><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: I58070f421ddeadd2844a33b869b052630cf2e514 </div>
<div style="display:none"> Gerrit-Change-Number: 6821 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Joshua Colp <jcolp@digium.com> </div>