bridge_channel.c: Fix FRACK when mapping frames to the bridge.
authorRichard Mudgett <rmudgett@digium.com>
Wed, 16 Aug 2017 22:50:18 +0000 (17:50 -0500)
committerRichard Mudgett <rmudgett@digium.com>
Tue, 22 Aug 2017 16:59:49 +0000 (11:59 -0500)
* 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

main/bridge_channel.c

index c465ec1..66292bf 100644 (file)
@@ -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);
 }
 
 /*!
@@ -2458,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;
        }