[Asterisk-code-review] bridge channel.c: Fix FRACK when mapping frames to the bridge. (asterisk[15.0])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/6281 )

Change subject: bridge_channel.c: Fix FRACK when mapping frames to the bridge.
......................................................................

bridge_channel.c: Fix FRACK when mapping frames to the bridge.

* Add protection checks when mapping streams to the bridge.  The channel
and bridge may be in the process of updating the stream mapping when a
media frame comes in so we may not be able to map the frame at the time.

* We need to map the streams to the bridge's stream numbers right before
they are written into the bridge.  That way we don't have to keep
locking/unlocking the bridge and we won't have any synchronization
problems before the frames actually go into the bridge.

* Protect the deferred queue with the bridge_channel lock.

ASTERISK-27212

Change-Id: Id6860dd61b594b90c8395f6e2c0150219094c21a
---
M main/bridge_channel.c
1 file changed, 61 insertions(+), 10 deletions(-)

Approvals:
  Joshua Colp: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved
  Jenkins2: Approved for Submit



diff --git a/main/bridge_channel.c b/main/bridge_channel.c
index c465ec1..66292bf 100644
--- a/main/bridge_channel.c
+++ b/main/bridge_channel.c
@@ -626,11 +626,11 @@
 
 /*!
  * \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,11 +638,58 @@
 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) {
@@ -650,7 +697,14 @@
 
 		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 @@
 {
 	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);
 }
 
 /*!
@@ -2458,13 +2514,8 @@
 		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;
 	}
 

-- 
To view, visit https://gerrit.asterisk.org/6281
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 15.0
Gerrit-MessageType: merged
Gerrit-Change-Id: Id6860dd61b594b90c8395f6e2c0150219094c21a
Gerrit-Change-Number: 6281
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170823/8faecd25/attachment-0001.html>