Merge changes from topic 'ASTERISK-27212' into 15

* changes:
  bridge_channel.c: Fix FRACK when mapping frames to the bridge.
  bridge: Fix softmix bridge deadlock.

bridges/bridge_softmix.c

@@ -1686,7 +1686,8 @@ static void map_source_to_destinations(const char *source_stream_name, const cha
 	}
 }
 
-/*\brief stream_topology_changed callback
+/*!
+ * \brief stream_topology_changed callback
  *
  * For most video modes, nothing beyond the ordinary is required.
  * For the SFU case, though, we need to completely remap the streams
@@ -1724,34 +1725,52 @@ static void softmix_bridge_stream_topology_changed(struct ast_bridge *bridge, st
 
 	/* Second traversal: Map specific video channels from their source to their destinations.
 	 *
-	 * This is similar to what is done in ast_stream_topology_map(), except that
-	 * video channels are handled differently. Each video source has it's own
-	 * unique index on the bridge. this way, a particular channel's source video
-	 * can be distributed to the appropriate destination streams on the other
-	 * channels
+	 * This is similar to what is done in ast_stream_topology_map(),
+	 * except that video channels are handled differently.  Each video
+	 * source has it's own unique index on the bridge.  This way, a
+	 * particular channel's source video can be distributed to the
+	 * appropriate destination streams on the other channels.
 	 */
 	AST_LIST_TRAVERSE(&bridge->channels, participant, entry) {
 		int i;
 		struct ast_stream_topology *topology;
 
+		ast_bridge_channel_lock(participant);
 		ast_channel_lock(participant->chan);
 
 		topology = ast_channel_get_stream_topology(participant->chan);
+		if (topology) {
+			/*
+			 * Sigh.  We have to clone to avoid deadlock in
+			 * map_source_to_destinations() because topology
+			 * is not an ao2 object.
+			 */
+			topology = ast_stream_topology_clone(topology);
+		}
+		if (!topology) {
+			/* Oh, my, we are in trouble. */
+			ast_channel_unlock(participant->chan);
+			ast_bridge_channel_unlock(participant);
+			continue;
+		}
 
 		for (i = 0; i < ast_stream_topology_get_count(topology); ++i) {
 			struct ast_stream *stream = ast_stream_topology_get_stream(topology, i);
-			ast_bridge_channel_lock(participant);
+
 			if (is_video_source(stream)) {
 				AST_VECTOR_APPEND(&media_types, AST_MEDIA_TYPE_VIDEO);
 				AST_VECTOR_REPLACE(&participant->stream_map.to_bridge, i, AST_VECTOR_SIZE(&media_types) - 1);
 				AST_VECTOR_REPLACE(&participant->stream_map.to_channel, AST_VECTOR_SIZE(&media_types) - 1, -1);
-				/* Unlock the participant to prevent potential deadlock
-				 * in map_source_to_destinations
+				/*
+				 * Unlock the channel and participant to prevent
+				 * potential deadlock in map_source_to_destinations().
 				 */
+				ast_channel_unlock(participant->chan);
 				ast_bridge_channel_unlock(participant);
 				map_source_to_destinations(ast_stream_get_name(stream), ast_channel_name(participant->chan),
 					AST_VECTOR_SIZE(&media_types) - 1, &bridge->channels);
 				ast_bridge_channel_lock(participant);
+				ast_channel_lock(participant->chan);
 			} else if (is_video_dest(stream, NULL, NULL)) {
 				/* We expect to never read media from video destination channels, but just
 				 * in case, we should set their to_bridge value to -1.
@@ -1773,10 +1792,12 @@ static void softmix_bridge_stream_topology_changed(struct ast_bridge *bridge, st
 				AST_VECTOR_REPLACE(&participant->stream_map.to_bridge, i, index);
 				AST_VECTOR_REPLACE(&participant->stream_map.to_channel, index, i);
 			}
-			ast_bridge_channel_unlock(participant);
 		}
 
+		ast_stream_topology_free(topology);
+
 		ast_channel_unlock(participant->chan);
+		ast_bridge_channel_unlock(participant);
 	}
 }
 

include/asterisk/bridge_channel.h

@@ -716,19 +716,24 @@ void ast_bridge_channel_feature_digit(struct ast_bridge_channel *bridge_channel,
  * \brief Maps a channel's stream topology to and from the bridge
  * \since 15.0.0
  *
- * When a channel joins a bridge or its associated stream topology is updated, each stream
- * in the topology needs to be mapped according to its media type to the bridge. Calling
- * this method creates a mapping of each stream on the channel indexed to the bridge's
- * supported media types and vice versa (i.e. bridge's media types indexed to channel
- * streams).
- *
- * The first channel to join the bridge creates the initial order for the bridge's media
- * types (e.g. a one to one mapping is made). Subsequently added channels are mapped to
- * that order adding more media types if/when the newly added channel has more streams
- * and/or media types specified by the bridge.
+ * \details
+ * When a channel joins a bridge or its associated stream topology is
+ * updated, each stream in the topology needs to be mapped according
+ * to its media type to the bridge.  Calling this method creates a
+ * mapping of each stream on the channel indexed to the bridge's
+ * supported media types and vice versa (i.e. bridge's media types
+ * indexed to channel streams).
+ *
+ * The first channel to join the bridge creates the initial order for
+ * the bridge's media types (e.g. a one to one mapping is made).
+ * Subsequently added channels are mapped to that order adding more
+ * media types if/when the newly added channel has more streams and/or
+ * media types specified by the bridge.
  *
  * \param bridge_channel Channel to map
  *
+ * \note The bridge_channel's bridge must be locked prior to calling this function.
+ *
  * \return Nothing
  */
 void ast_bridge_channel_stream_map(struct ast_bridge_channel *bridge_channel);

include/asterisk/bridge_technology.h

@@ -164,20 +164,30 @@ struct ast_bridge_technology {
 	/*!
 	 * \brief Callback for when a request has been made to change a stream topology on a channel
 	 *
-	 * This is called when a bridge receives a request to change the topology on the channel. A bridge
-	 * technology should define a handler for this callback if it needs to update internals or intercept
-	 * the request and not pass it on to other channels. This can be done by returning a nonzero value.
+	 * \details
+	 * This is called when a bridge receives a request to change the
+	 * topology on the channel.  A bridge technology should define a
+	 * handler for this callback if it needs to update internals or
+	 * intercept the request and not pass it on to other channels.
+	 * This can be done by returning a nonzero value.
 	 *
-	 * \retval 0 Frame accepted by the bridge.
-	 * \retval -1 Frame rejected.
+	 * \retval 0 Frame can pass to the bridge technology.
+	 * \retval non-zero Frame intercepted by the bridge technology.
+	 *
+	 * \note On entry, bridge is already locked.
 	 */
 	int (*stream_topology_request_change)(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel);
 	/*!
 	 * \brief Callback for when a stream topology changes on the channel
 	 *
-	 * This is called when a bridge receives an indication that a topology has been changed on a channel
-	 * and the new topology has been mapped to the bridge. A bridge technology should define a handler for
-	 * this callback if it needs to update internals due to a channel's topology changing.
+	 * \details
+	 * This is called when a bridge receives an indication that a
+	 * topology has been changed on a channel and the new topology has
+	 * been mapped to the bridge.  A bridge technology should define a
+	 * handler for this callback if it needs to update internals due
+	 * to a channel's topology changing.
+	 *
+	 * \note On entry, bridge is already locked.
 	 */
 	void (*stream_topology_changed)(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel);
 	/*! TRUE if the bridge technology is currently suspended. */

main/bridge_channel.c

@@ -626,11 +626,11 @@ void ast_bridge_channel_kick(struct ast_bridge_channel *bridge_channel, int caus
 
 /*!
  * \internal
- * \brief Write an \ref ast_frame onto the bridge channel
+ * \brief Write an \ref ast_frame into the bridge
  * \since 12.0.0
  *
- * \param bridge_channel Which channel to queue the frame onto.
- * \param frame The frame to write onto the bridge_channel
+ * \param bridge_channel Which channel is queueing the frame.
+ * \param frame The frame to write into the bridge
  *
  * \retval 0 on success.
  * \retval -1 on error.
@@ -638,19 +638,73 @@ void ast_bridge_channel_kick(struct ast_bridge_channel *bridge_channel, int caus
 static int bridge_channel_write_frame(struct ast_bridge_channel *bridge_channel, struct ast_frame *frame)
 {
 	const struct ast_control_t38_parameters *t38_parameters;
+	int unmapped_stream_num;
 	int deferred;
 
 	ast_assert(frame->frametype != AST_FRAME_BRIDGE_ACTION_SYNC);
 
 	ast_bridge_channel_lock_bridge(bridge_channel);
 
+	/* Map the frame to the bridge. */
+	if (ast_channel_is_multistream(bridge_channel->chan)) {
+		unmapped_stream_num = frame->stream_num;
+		switch (frame->frametype) {
+		case AST_FRAME_VOICE:
+		case AST_FRAME_VIDEO:
+		case AST_FRAME_TEXT:
+		case AST_FRAME_IMAGE:
+			/* Media frames need to be mapped to an appropriate write stream */
+			if (frame->stream_num < 0) {
+				/* Map to default stream */
+				frame->stream_num = -1;
+				break;
+			}
+			ast_bridge_channel_lock(bridge_channel);
+			if (frame->stream_num < (int)AST_VECTOR_SIZE(&bridge_channel->stream_map.to_bridge)) {
+				frame->stream_num = AST_VECTOR_GET(
+					&bridge_channel->stream_map.to_bridge, frame->stream_num);
+				if (0 <= frame->stream_num) {
+					ast_bridge_channel_unlock(bridge_channel);
+					break;
+				}
+			}
+			ast_bridge_channel_unlock(bridge_channel);
+			ast_bridge_unlock(bridge_channel->bridge);
+			/*
+			 * Ignore frame because we don't know how to map the frame
+			 * or the bridge is not expecting any media from that
+			 * stream.
+			 */
+			return 0;
+		case AST_FRAME_DTMF_BEGIN:
+		case AST_FRAME_DTMF_END:
+			/*
+			 * XXX It makes sense that DTMF could be on any audio stream.
+			 * For now we will only put it on the default audio stream.
+			 */
+		default:
+			frame->stream_num = -1;
+			break;
+		}
+	} else {
+		unmapped_stream_num = -1;
+		frame->stream_num = -1;
+	}
+
 	deferred = bridge_channel->bridge->technology->write(bridge_channel->bridge, bridge_channel, frame);
 	if (deferred) {
 		struct ast_frame *dup;
 
 		dup = ast_frdup(frame);
 		if (dup) {
+			/*
+			 * We have to unmap the deferred frame so it comes back
+			 * in like a new frame.
+			 */
+			dup->stream_num = unmapped_stream_num;
+			ast_bridge_channel_lock(bridge_channel);
 			AST_LIST_INSERT_HEAD(&bridge_channel->deferred_queue, dup, frame_list);
+			ast_bridge_channel_unlock(bridge_channel);
 		}
 	}
 
@@ -760,12 +814,14 @@ void bridge_channel_queue_deferred_frames(struct ast_bridge_channel *bridge_chan
 {
 	struct ast_frame *frame;
 
+	ast_bridge_channel_lock(bridge_channel);
 	ast_channel_lock(bridge_channel->chan);
 	while ((frame = AST_LIST_REMOVE_HEAD(&bridge_channel->deferred_queue, frame_list))) {
 		ast_queue_frame_head(bridge_channel->chan, frame);
 		ast_frfree(frame);
 	}
 	ast_channel_unlock(bridge_channel->chan);
+	ast_bridge_channel_unlock(bridge_channel);
 }
 
 /*!
@@ -2434,6 +2490,7 @@ static const char *controls[] = {
 static void bridge_handle_trip(struct ast_bridge_channel *bridge_channel)
 {
 	struct ast_frame *frame;
+	int blocked;
 
 	if (!ast_strlen_zero(ast_channel_call_forward(bridge_channel->chan))) {
 		/* TODO If early bridging is ever used by anything other than ARI,
@@ -2457,13 +2514,8 @@ static void bridge_handle_trip(struct ast_bridge_channel *bridge_channel)
 		return;
 	}
 
-	if (ast_channel_is_multistream(bridge_channel->chan) &&
-	    (frame->frametype == AST_FRAME_IMAGE || frame->frametype == AST_FRAME_TEXT ||
-	     frame->frametype == AST_FRAME_VIDEO || frame->frametype == AST_FRAME_VOICE)) {
-		/* Media frames need to be mapped to an appropriate write stream */
-		frame->stream_num = AST_VECTOR_GET(
-			&bridge_channel->stream_map.to_bridge, frame->stream_num);
-	} else {
+	if (!ast_channel_is_multistream(bridge_channel->chan)) {
+		/* This may not be initialized by non-multistream channel drivers */
 		frame->stream_num = -1;
 	}
 
@@ -2485,10 +2537,16 @@ static void bridge_handle_trip(struct ast_bridge_channel *bridge_channel)
 			ast_channel_publish_dial(NULL, bridge_channel->chan, NULL, controls[frame->subclass.integer]);
 			break;
 		case AST_CONTROL_STREAM_TOPOLOGY_REQUEST_CHANGE:
-			if (bridge_channel->bridge->technology->stream_topology_request_change &&
-			    bridge_channel->bridge->technology->stream_topology_request_change(
-				    bridge_channel->bridge, bridge_channel)) {
-				/* Topology change was denied so drop frame */
+			ast_bridge_channel_lock_bridge(bridge_channel);
+			blocked = bridge_channel->bridge->technology->stream_topology_request_change
+				&& bridge_channel->bridge->technology->stream_topology_request_change(
+					bridge_channel->bridge, bridge_channel);
+			ast_bridge_unlock(bridge_channel->bridge);
+			if (blocked) {
+				/*
+				 * Topology change was intercepted by the bridge technology
+				 * so drop frame.
+				 */
 				bridge_frame_free(frame);
 				return;
 			}
@@ -2498,12 +2556,14 @@ static void bridge_handle_trip(struct ast_bridge_channel *bridge_channel)
 			 * If a stream topology has changed then the bridge_channel's
 			 * media mapping needs to be updated.
 			 */
+			ast_bridge_channel_lock_bridge(bridge_channel);
 			if (bridge_channel->bridge->technology->stream_topology_changed) {
 				bridge_channel->bridge->technology->stream_topology_changed(
 					bridge_channel->bridge, bridge_channel);
 			} else {
 				ast_bridge_channel_stream_map(bridge_channel);
 			}
+			ast_bridge_unlock(bridge_channel->bridge);
 			break;
 		default:
 			break;
@@ -2990,9 +3050,11 @@ struct ast_bridge_channel *bridge_channel_internal_alloc(struct ast_bridge *brid
 
 void ast_bridge_channel_stream_map(struct ast_bridge_channel *bridge_channel)
 {
+	ast_bridge_channel_lock(bridge_channel);
 	ast_channel_lock(bridge_channel->chan);
 	ast_stream_topology_map(ast_channel_get_stream_topology(bridge_channel->chan),
 		&bridge_channel->bridge->media_types, &bridge_channel->stream_map.to_bridge,
 		&bridge_channel->stream_map.to_channel);
 	ast_channel_unlock(bridge_channel->chan);
+	ast_bridge_channel_unlock(bridge_channel);
 }