[Asterisk-code-review] bridge/core unreal: Fix SFU bugs with forwarding frames. (asterisk[master])
Jenkins2
asteriskteam at digium.com
Mon Jul 17 17:59:32 CDT 2017
Jenkins2 has submitted this change and it was merged. ( https://gerrit.asterisk.org/5994 )
Change subject: bridge/core_unreal: Fix SFU bugs with forwarding frames.
......................................................................
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
---
M bridges/bridge_softmix.c
M include/asterisk/channel.h
M main/bridge_channel.c
M main/channel.c
M main/core_unreal.c
5 files changed, 52 insertions(+), 18 deletions(-)
Approvals:
Joshua Colp: Verified
Kevin Harwell: Looks good to me, but someone else must approve
Matthew Fredrickson: Looks good to me, approved
Jenkins2: Approved for Submit
diff --git a/bridges/bridge_softmix.c b/bridges/bridge_softmix.c
index ae877eb..21f5190 100644
--- a/bridges/bridge_softmix.c
+++ b/bridges/bridge_softmix.c
@@ -600,7 +600,7 @@
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 197cc99..55126b4 100644
--- a/include/asterisk/channel.h
+++ b/include/asterisk/channel.h
@@ -2030,6 +2030,26 @@
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.
* \param chan destination channel of the frame
diff --git a/main/bridge_channel.c b/main/bridge_channel.c
index e8ab8a8..2e94300 100644
--- a/main/bridge_channel.c
+++ b/main/bridge_channel.c
@@ -998,21 +998,6 @@
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 @@
}
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 811826f..23bb74f 100644
--- a/main/channel.c
+++ b/main/channel.c
@@ -4180,6 +4180,11 @@
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 5da7408..3db6a4d 100644
--- a/main/core_unreal.c
+++ b/main/core_unreal.c
@@ -323,6 +323,19 @@
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);
--
To view, visit https://gerrit.asterisk.org/5994
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib7445722c3219951d6740802a0feddf2908c18c8
Gerrit-Change-Number: 5994
Gerrit-PatchSet: 1
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170717/5fb55109/attachment.html>
More information about the asterisk-code-review
mailing list