[Asterisk-code-review] bridge_softmix/sfu_topologies_on_join: Ignore topology change failures (asterisk[18])

[Joshua Colp]

Joshua Colp has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/14968 )

Change subject: bridge_softmix/sfu_topologies_on_join: Ignore topology change failures
......................................................................

bridge_softmix/sfu_topologies_on_join: Ignore topology change failures

When a channel joins a bridge, we do topology change requests on all
existing channels to add the new participant to them.  However the
announcer channel will return an error because it doesn't support
topology in the first place.  Unfortunately, there doesn't seem to be a
reliable way to tell if the error is expected or not so the error is
ignored for all channels.  If the request fails on a "real" channel,
that channel just won't get the new participant's video.

Change-Id: Ic95db4683f27d224c1869fe887795d6b9fdea4f0
---
M bridges/bridge_softmix.c
1 file changed, 24 insertions(+), 19 deletions(-)

Approvals:
  Kevin Harwell: Looks good to me, but someone else must approve
  Benjamin Keith Ford: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved; Approved for Submit



diff --git a/bridges/bridge_softmix.c b/bridges/bridge_softmix.c
index 8d2d67c..5acd794 100644
--- a/bridges/bridge_softmix.c
+++ b/bridges/bridge_softmix.c
@@ -621,16 +621,16 @@
 static void sfu_topologies_on_join(struct ast_bridge *bridge,
 	struct ast_bridge_channel *joiner)
 {
-	struct ast_stream_topology *joiner_video = NULL;
+	RAII_VAR(struct ast_stream_topology *, joiner_video, NULL, ast_stream_topology_free);
 	struct ast_bridge_channels_list *participants = &bridge->channels;
 	struct ast_bridge_channel *participant;
 	int res;
 	struct softmix_channel *sc;
+	SCOPE_ENTER(3, "%s: \n", ast_channel_name(joiner->chan));
 
 	joiner_video = ast_stream_topology_alloc();
 	if (!joiner_video) {
-		ast_log(LOG_ERROR, "%s: Couldn't alloc topology\n", ast_channel_name(joiner->chan));
-		return;
+		SCOPE_EXIT_LOG_RTN(LOG_ERROR, "%s: Couldn't alloc topology\n", ast_channel_name(joiner->chan));
 	}
 
 	sc = joiner->tech_pvt;
@@ -643,30 +643,30 @@
 	ast_channel_unlock(joiner->chan);
 
 	if (res || !sc->topology) {
-		ast_log(LOG_ERROR, "%s: Couldn't append source streams\n", ast_channel_name(joiner->chan));
-		goto cleanup;
+		SCOPE_EXIT_LOG_RTN(LOG_ERROR, "%s: Couldn't append source streams\n", ast_channel_name(joiner->chan));
 	}
 
 	AST_LIST_TRAVERSE(participants, participant, entry) {
 		if (participant == joiner) {
 			continue;
 		}
+		ast_trace(-1, "%s: Appending existing participant %s\n", ast_channel_name(joiner->chan),
+			ast_channel_name(participant->chan));
 		ast_channel_lock(participant->chan);
 		res = append_source_streams(sc->topology, ast_channel_name(participant->chan),
 			bridge->softmix.send_sdp_label ? ast_channel_uniqueid(participant->chan) : NULL,
 			ast_channel_get_stream_topology(participant->chan));
 		ast_channel_unlock(participant->chan);
 		if (res) {
-			ast_log(LOG_ERROR, "%s/%s: Couldn't append source streams\n",
+			SCOPE_EXIT_LOG_RTN(LOG_ERROR, "%s/%s: Couldn't append source streams\n",
 				ast_channel_name(participant->chan), ast_channel_name(joiner->chan));
-			goto cleanup;
 		}
 	}
 
+	ast_trace(-1, "%s: Requesting topology change.\n", ast_channel_name(joiner->chan));
 	res = ast_channel_request_stream_topology_change(joiner->chan, sc->topology, NULL);
 	if (res) {
-		ast_debug(3, "%s: Couldn't request topology change\n", ast_channel_name(joiner->chan));
-		goto cleanup;
+		SCOPE_EXIT_LOG_RTN(LOG_ERROR, "%s: Couldn't request topology change\n", ast_channel_name(joiner->chan));
 	}
 
 	AST_LIST_TRAVERSE(participants, participant, entry) {
@@ -675,21 +675,22 @@
 		}
 
 		sc = participant->tech_pvt;
+		ast_trace(-1, "%s: Appending joiner %s\n", ast_channel_name(participant->chan),
+			ast_channel_name(joiner->chan));
+
 		if (append_all_streams(sc->topology, joiner_video)) {
-			ast_log(LOG_ERROR, "%s/%s: Couldn't apopend streams\n",
+			SCOPE_EXIT_LOG_RTN(LOG_ERROR, "%s/%s: Couldn't append streams\n",
 				ast_channel_name(participant->chan), ast_channel_name(joiner->chan));
-			goto cleanup;
 		}
+		ast_trace(-1, "%s: Requesting topology change\n", ast_channel_name(participant->chan));
 		res = ast_channel_request_stream_topology_change(participant->chan, sc->topology, NULL);
 		if (res) {
-			ast_debug(3, "%s/%s: Couldn't request topology change\n",
+			ast_trace(-1, "%s/%s: Couldn't request topology change\n",
 				ast_channel_name(participant->chan), ast_channel_name(joiner->chan));
-			goto cleanup;
 		}
 	}
 
-cleanup:
-	ast_stream_topology_free(joiner_video);
+	SCOPE_EXIT();
 }
 
 /*! \brief Function called when a channel is joined into the bridge */
@@ -706,15 +707,16 @@
 	int pos_id;
 	int is_announcement = 0;
 	int samplerate_change;
+	SCOPE_ENTER(3, "%s:\n", ast_channel_name(bridge_channel->chan));
 
 	softmix_data = bridge->tech_pvt;
 	if (!softmix_data) {
-		return -1;
+		SCOPE_EXIT_RTN_VALUE(-1, "No tech_pvt\n");
 	}
 
 	/* Create a new softmix_channel structure and allocate various things on it */
 	if (!(sc = ast_calloc(1, sizeof(*sc)))) {
-		return -1;
+		SCOPE_EXIT_RTN_VALUE(-1, "Couldn't alloc tech_pvt\n");
 	}
 
 	samplerate_change = softmix_data->internal_rate;
@@ -739,7 +741,7 @@
 						"Could not allocate enough memory.\n", bridge->uniqueid,
 						ast_channel_name(bridge_channel->chan));
 				ast_free(sc);
-				return -1;
+				SCOPE_EXIT_RTN_VALUE(-1, "Couldn't do binaural join\n");
 			}
 		}
 	}
@@ -768,7 +770,7 @@
 	}
 
 	softmix_poke_thread(softmix_data);
-	return 0;
+	SCOPE_EXIT_RTN_VALUE(0);
 }
 
 static int remove_destination_streams(struct ast_stream_topology *topology,
@@ -2329,6 +2331,9 @@
 			ast_trace(-1, "%s: Stream %d:%s changed state from %s to %s\n",  ast_channel_name(bridge_channel->chan),
 				index, ast_stream_get_name(old_stream), ast_stream_state2str(ast_stream_get_state(old_stream)),
 				ast_stream_state2str(ast_stream_get_state(new_stream)));
+		} else {
+			ast_trace(-1, "%s: Stream %d:%s didn't do anything\n",  ast_channel_name(bridge_channel->chan),
+				index, ast_stream_get_name(old_stream));
 		}
 		SCOPE_EXIT();
 	}

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/14968
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 18
Gerrit-Change-Id: Ic95db4683f27d224c1869fe887795d6b9fdea4f0
Gerrit-Change-Number: 14968
Gerrit-PatchSet: 2
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200922/fb293db4/attachment.html>