From 7f09fd2c2f1501aeccc7737be4dd0e52fd656913 Mon Sep 17 00:00:00 2001 From: Joshua Colp Date: Tue, 11 Jul 2017 19:33:44 +0000 Subject: [PATCH] bridge/core_unreal: Fix SFU bugs with forwarding frames. This change fixes a few things uncovered during SFU testing. 1. Unreal channels incorrectly forwarded video frames when no video stream was present on them. This caused a crash when they were read as the core requires a stream to exist for the underlying media type. The Unreal channel will now ensure a stream exists for the media type before forwarding the frame and if no stream exists then the frame is dropped. 2. Mapping of frames during bridging from the stream number of the underlying channel to the stream number of the bridge was done in the wrong location. This resulted in the frame getting dropped. This mapping now occurs on reading of the frame from the channel. 3. Bridging was using the wrong ast_read function resulting in it living in a non-multistream world. 4. In bridge_softmix when adding new streams to existing channels the wrong stream topology was copied resulting in no streams being added. Change-Id: Ib7445722c3219951d6740802a0feddf2908c18c8 --- bridges/bridge_softmix.c | 2 +- include/asterisk/channel.h | 20 ++++++++++++++++++++ main/bridge_channel.c | 30 +++++++++++++----------------- main/channel.c | 5 +++++ main/core_unreal.c | 13 +++++++++++++ 5 files changed, 52 insertions(+), 18 deletions(-) diff --git a/bridges/bridge_softmix.c b/bridges/bridge_softmix.c index ae877eb6e3..21f5190e57 100644 --- a/bridges/bridge_softmix.c +++ b/bridges/bridge_softmix.c @@ -600,7 +600,7 @@ static void sfu_topologies_on_join(struct ast_bridge_channel *joiner, struct ast if (participant == joiner) { continue; } - participant_topology = ast_stream_topology_clone(ast_channel_get_stream_topology(joiner->chan)); + participant_topology = ast_stream_topology_clone(ast_channel_get_stream_topology(participant->chan)); if (!participant_topology) { goto cleanup; } diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h index 197cc990db..55126b472f 100644 --- a/include/asterisk/channel.h +++ b/include/asterisk/channel.h @@ -2029,6 +2029,26 @@ struct ast_frame *ast_read_stream(struct ast_channel *chan); */ struct ast_frame *ast_read_noaudio(struct ast_channel *chan); +/*! + * \brief Reads a frame, but does not filter to just the default streams, + * returning AST_FRAME_NULL frame if audio. + * + * \param chan channel to read a frame from + * + * \return Returns a frame, or NULL on error. If it returns NULL, you + * best just stop reading frames and assume the channel has been + * disconnected. + * + * \note This function will not perform any filtering and will return + * media frames from all streams on the channel. To determine which + * stream a frame originated from the stream_num on it can be + * examined. + * + * \note Audio is replaced with AST_FRAME_NULL to avoid + * transcode when the resulting audio is not necessary. + */ +struct ast_frame *ast_read_stream_noaudio(struct ast_channel *chan); + /*! * \brief Write a frame to a channel * This function writes the given frame to the indicated channel. diff --git a/main/bridge_channel.c b/main/bridge_channel.c index e8ab8a8980..2e943000cc 100644 --- a/main/bridge_channel.c +++ b/main/bridge_channel.c @@ -998,21 +998,6 @@ int ast_bridge_channel_queue_frame(struct ast_bridge_channel *bridge_channel, st return 0; } - if (ast_channel_is_multistream(bridge_channel->chan) && - (fr->frametype == AST_FRAME_IMAGE || fr->frametype == AST_FRAME_TEXT || - fr->frametype == AST_FRAME_VIDEO || fr->frametype == AST_FRAME_VOICE)) { - /* Media frames need to be mapped to an appropriate write stream */ - dup->stream_num = AST_VECTOR_GET( - &bridge_channel->stream_map.to_bridge, fr->stream_num); - if (dup->stream_num == -1) { - ast_bridge_channel_unlock(bridge_channel); - bridge_frame_free(dup); - return 0; - } - } else { - dup->stream_num = -1; - } - AST_LIST_INSERT_TAIL(&bridge_channel->wr_queue, dup, frame_list); if (ast_alertpipe_write(bridge_channel->alert_pipe)) { ast_log(LOG_ERROR, "We couldn't write alert pipe for %p(%s)... something is VERY wrong\n", @@ -2455,15 +2440,26 @@ static void bridge_handle_trip(struct ast_bridge_channel *bridge_channel) } if (bridge_channel->features->mute) { - frame = ast_read_noaudio(bridge_channel->chan); + frame = ast_read_stream_noaudio(bridge_channel->chan); } else { - frame = ast_read(bridge_channel->chan); + frame = ast_read_stream(bridge_channel->chan); } if (!frame) { ast_bridge_channel_kick(bridge_channel, 0); 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 { + frame->stream_num = -1; + } + switch (frame->frametype) { case AST_FRAME_CONTROL: switch (frame->subclass.integer) { diff --git a/main/channel.c b/main/channel.c index 811826f1cf..23bb74f082 100644 --- a/main/channel.c +++ b/main/channel.c @@ -4180,6 +4180,11 @@ struct ast_frame *ast_read_noaudio(struct ast_channel *chan) return __ast_read(chan, 1, 1); } +struct ast_frame *ast_read_stream_noaudio(struct ast_channel *chan) +{ + return __ast_read(chan, 1, 0); +} + int ast_indicate(struct ast_channel *chan, int condition) { return ast_indicate_data(chan, condition, NULL, 0); diff --git a/main/core_unreal.c b/main/core_unreal.c index 5da7408770..3db6a4dbdd 100644 --- a/main/core_unreal.c +++ b/main/core_unreal.c @@ -323,6 +323,19 @@ int ast_unreal_write(struct ast_channel *ast, struct ast_frame *f) return -1; } + /* If we are told to write a frame with a type that has no corresponding + * stream on the channel then drop it. + */ + if (f->frametype == AST_FRAME_VOICE) { + if (!ast_channel_get_default_stream(ast, AST_MEDIA_TYPE_AUDIO)) { + return 0; + } + } else if (f->frametype == AST_FRAME_VIDEO) { + if (!ast_channel_get_default_stream(ast, AST_MEDIA_TYPE_VIDEO)) { + return 0; + } + } + /* Just queue for delivery to the other side */ ao2_ref(p, 1); ao2_lock(p);