[Asterisk-code-review] bridge: Fix stream topology/participant locking and video mi... (asterisk[master])
Joshua Colp
asteriskteam at digium.com
Sun Aug 6 11:17:22 CDT 2017
Joshua Colp has uploaded this change for review. ( https://gerrit.asterisk.org/6171
Change subject: bridge: Fix stream topology/participant locking and video misrouting.
......................................................................
bridge: Fix stream topology/participant locking and video misrouting.
This change fixes a few locking issues and some video misrouting.
1. When accessing the stream topology of a channel the channel lock
must be held to guarantee the topology remains valid.
2. When a channel was joined to a bridge the bridge specific
implementation for stream mapping was not invoked, causing video
to be misrouted for a brief period of time.
ASTERISK-27182
Change-Id: I5d2f779248b84d41c5bb3896bf22ba324b336b03
---
M bridges/bridge_softmix.c
M main/bridge.c
M main/bridge_channel.c
3 files changed, 51 insertions(+), 22 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/71/6171/1
diff --git a/bridges/bridge_softmix.c b/bridges/bridge_softmix.c
index c5428a8..b359948 100644
--- a/bridges/bridge_softmix.c
+++ b/bridges/bridge_softmix.c
@@ -577,13 +577,18 @@
struct ast_stream_topology *joiner_video = NULL;
struct ast_stream_topology *existing_video = NULL;
struct ast_bridge_channel *participant;
+ int res;
joiner_video = ast_stream_topology_alloc();
if (!joiner_video) {
return;
}
- if (append_source_streams(joiner_video, ast_channel_name(joiner->chan), ast_channel_get_stream_topology(joiner->chan))) {
+ ast_channel_lock(joiner->chan);
+ res = append_source_streams(joiner_video, ast_channel_name(joiner->chan), ast_channel_get_stream_topology(joiner->chan));
+ ast_channel_unlock(joiner->chan);
+
+ if (res) {
goto cleanup;
}
@@ -596,13 +601,18 @@
if (participant == joiner) {
continue;
}
- if (append_source_streams(existing_video, ast_channel_name(participant->chan),
- ast_channel_get_stream_topology(participant->chan))) {
+ ast_channel_lock(participant->chan);
+ res = append_source_streams(existing_video, ast_channel_name(participant->chan),
+ ast_channel_get_stream_topology(participant->chan));
+ ast_channel_unlock(participant->chan);
+ if (res) {
goto cleanup;
}
}
+ ast_channel_lock(joiner->chan);
joiner_topology = ast_stream_topology_clone(ast_channel_get_stream_topology(joiner->chan));
+ ast_channel_unlock(joiner->chan);
if (!joiner_topology) {
goto cleanup;
}
@@ -617,7 +627,9 @@
if (participant == joiner) {
continue;
}
+ ast_channel_lock(participant->chan);
participant_topology = ast_stream_topology_clone(ast_channel_get_stream_topology(participant->chan));
+ ast_channel_unlock(participant->chan);
if (!participant_topology) {
goto cleanup;
}
@@ -753,12 +765,16 @@
continue;
}
+ ast_channel_lock(participant->chan);
remove_destination_streams(participant_topology, ast_channel_name(leaver->chan), ast_channel_get_stream_topology(participant->chan));
+ ast_channel_unlock(participant->chan);
ast_channel_request_stream_topology_change(participant->chan, participant_topology, NULL);
ast_stream_topology_free(participant_topology);
}
+ ast_channel_lock(leaver->chan);
remove_destination_streams(leaver_topology, "", ast_channel_get_stream_topology(leaver->chan));
+ ast_channel_unlock(leaver->chan);
ast_channel_request_stream_topology_change(leaver->chan, leaver_topology, NULL);
ast_stream_topology_free(leaver_topology);
@@ -1657,6 +1673,7 @@
}
ast_bridge_channel_lock(participant);
+ ast_channel_lock(participant->chan);
topology = ast_channel_get_stream_topology(participant->chan);
for (i = 0; i < ast_stream_topology_get_count(topology); ++i) {
@@ -1668,6 +1685,7 @@
break;
}
}
+ ast_channel_unlock(participant->chan);
ast_bridge_channel_unlock(participant);
}
}
@@ -1702,16 +1720,9 @@
/* First traversal: re-initialize all of the participants' stream maps */
AST_LIST_TRAVERSE(&bridge->channels, participant, entry) {
- int size;
-
ast_bridge_channel_lock(participant);
- size = ast_stream_topology_get_count(ast_channel_get_stream_topology(participant->chan));
-
- AST_VECTOR_FREE(&participant->stream_map.to_channel);
- AST_VECTOR_FREE(&participant->stream_map.to_bridge);
-
- AST_VECTOR_INIT(&participant->stream_map.to_channel, size);
- AST_VECTOR_INIT(&participant->stream_map.to_bridge, size);
+ AST_VECTOR_RESET(&participant->stream_map.to_channel, AST_VECTOR_ELEM_CLEANUP_NOOP);
+ AST_VECTOR_RESET(&participant->stream_map.to_bridge, AST_VECTOR_ELEM_CLEANUP_NOOP);
ast_bridge_channel_unlock(participant);
}
@@ -1726,6 +1737,8 @@
AST_LIST_TRAVERSE(&bridge->channels, participant, entry) {
int i;
struct ast_stream_topology *topology;
+
+ ast_channel_lock(participant->chan);
topology = ast_channel_get_stream_topology(participant->chan);
@@ -1766,6 +1779,8 @@
}
ast_bridge_channel_unlock(participant);
}
+
+ ast_channel_unlock(participant->chan);
}
}
diff --git a/main/bridge.c b/main/bridge.c
index a1a1a6f..b732d5f 100644
--- a/main/bridge.c
+++ b/main/bridge.c
@@ -446,7 +446,12 @@
* media types vector. This way all streams map to the same media type index for
* a given channel.
*/
- ast_bridge_channel_stream_map(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);
+ }
}
/*!
diff --git a/main/bridge_channel.c b/main/bridge_channel.c
index 2e94300..1427fd0 100644
--- a/main/bridge_channel.c
+++ b/main/bridge_channel.c
@@ -2344,16 +2344,23 @@
case AST_FRAME_NULL:
break;
default:
- if (fr->stream_num > 0 &&
- (fr->stream_num >= (int)AST_VECTOR_SIZE(&bridge_channel->stream_map.to_channel) ||
- AST_VECTOR_GET(&bridge_channel->stream_map.to_channel, fr->stream_num) == -1)) {
- /* Nowhere to write to, so drop it */
- break;
- }
+ /* Assume that there is no mapped stream for this */
+ num = -1;
- /* Find what stream number to write to for the channel */
- num = fr->stream_num < 0 ? -1 :
- AST_VECTOR_GET(&bridge_channel->stream_map.to_channel, fr->stream_num);
+ if (fr->stream_num > -1) {
+ ast_bridge_channel_lock(bridge_channel);
+ if (fr->stream_num < (int)AST_VECTOR_SIZE(&bridge_channel->stream_map.to_channel)) {
+ num = AST_VECTOR_GET(&bridge_channel->stream_map.to_channel, fr->stream_num);
+ }
+ ast_bridge_channel_unlock(bridge_channel);
+
+ /* If there is no mapped stream after checking the mapping then there is nowhere
+ * to write this frame to, so drop it.
+ */
+ if (num == -1) {
+ break;
+ }
+ }
/* Write the frame to the channel. */
bridge_channel->activity = BRIDGE_CHANNEL_THREAD_SIMPLE;
@@ -2983,7 +2990,9 @@
void ast_bridge_channel_stream_map(struct ast_bridge_channel *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);
}
--
To view, visit https://gerrit.asterisk.org/6171
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5d2f779248b84d41c5bb3896bf22ba324b336b03
Gerrit-Change-Number: 6171
Gerrit-PatchSet: 1
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170806/8520e097/attachment.html>
More information about the asterisk-code-review
mailing list