<p>Joshua Colp has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/6531">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">bridge: Change participant SFU streams when source streams change.<br><br>Some endpoints do not like a stream being reused for a new<br>media stream. The frame/jitterbuffer can rely on underlying<br>attributes of the media stream in order to order the packets.<br>When a new stream takes its place without any notice the<br>buffer can get confused and the media ends up getting dropped.<br><br>This change uses the SSRC change to determine that a new source<br>is reusing an existing stream and then bridge_softmix renegotiates<br>each participant such that they see a new media stream. This<br>causes the frame/jitterbuffer to start fresh and work as expected.<br><br>ASTERISK-27277<br><br>Change-Id: I30ccbdba16ca073d7f31e0e59ab778c153afae07<br>---<br>M bridges/bridge_softmix.c<br>M channels/chan_iax2.c<br>M funcs/func_frame_trace.c<br>M include/asterisk/frame.h<br>M include/asterisk/res_pjsip_session.h<br>M main/channel.c<br>M res/res_pjsip_sdp_rtp.c<br>M res/res_pjsip_session.c<br>8 files changed, 159 insertions(+), 9 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/31/6531/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/bridges/bridge_softmix.c b/bridges/bridge_softmix.c<br>index 59b16b7..ff2b124 100644<br>--- a/bridges/bridge_softmix.c<br>+++ b/bridges/bridge_softmix.c<br>@@ -79,7 +79,7 @@<br> <br> struct softmix_translate_helper_entry {<br> int num_times_requested; /*!< Once this entry is no longer requested, free the trans_pvt<br>- and re-init if it was usable. */<br>+ and re-init if it was usable. */<br> struct ast_format *dst_format; /*!< The destination format for this helper */<br> struct ast_trans_pvt *trans_pvt; /*!< the translator for this slot. */<br> struct ast_frame *out_frame; /*!< The output frame from the last translation */<br>@@ -494,18 +494,16 @@<br> struct ast_stream *stream;<br> struct ast_stream *stream_clone;<br> char *stream_clone_name;<br>- size_t stream_clone_name_len;<br> <br> stream = ast_stream_topology_get_stream(source, i);<br> if (!is_video_source(stream)) {<br> continue;<br> }<br> <br>- /* The +3 is for the two underscore separators and null terminator */<br>- stream_clone_name_len = SOFTBRIDGE_VIDEO_DEST_LEN + strlen(channel_name) + strlen(ast_stream_get_name(stream)) + 3;<br>- stream_clone_name = ast_alloca(stream_clone_name_len);<br>- snprintf(stream_clone_name, stream_clone_name_len, "%s_%s_%s", SOFTBRIDGE_VIDEO_DEST_PREFIX,<br>- channel_name, ast_stream_get_name(stream));<br>+ if (ast_asprintf(&stream_clone_name, "%s_%s_%s", SOFTBRIDGE_VIDEO_DEST_PREFIX,<br>+ channel_name, ast_stream_get_name(stream)) < 0) {<br>+ return -1;<br>+ }<br> <br> stream_clone = ast_stream_clone(stream, stream_clone_name);<br> if (!stream_clone) {<br>@@ -987,6 +985,118 @@<br> }<br> }<br> <br>+static int remove_all_original_streams(struct ast_stream_topology *dest,<br>+ const struct ast_stream_topology *source,<br>+ const struct ast_stream_topology *original)<br>+{<br>+ int i;<br>+<br>+ for (i = 0; i < ast_stream_topology_get_count(source); ++i) {<br>+ struct ast_stream *stream;<br>+ int original_index;<br>+<br>+ stream = ast_stream_topology_get_stream(source, i);<br>+<br>+ /* Mark the existing stream as removed so we get a new one, this will get<br>+ * reused on a subsequent renegotiation.<br>+ */<br>+ for (original_index = 0; original_index < ast_stream_topology_get_count(original); ++original_index) {<br>+ struct ast_stream *original_stream = ast_stream_topology_get_stream(original, original_index);<br>+<br>+ if (!strcmp(ast_stream_get_name(stream), ast_stream_get_name(original_stream))) {<br>+ struct ast_stream *removed;<br>+<br>+ /* Since the participant is still going to be in the bridge we<br>+ * change the name so that routing does not attempt to route video<br>+ * to this stream.<br>+ */<br>+ removed = ast_stream_clone(stream, "removed");<br>+ if (!removed) {<br>+ return -1;<br>+ }<br>+<br>+ ast_stream_set_state(removed, AST_STREAM_STATE_REMOVED);<br>+<br>+ /* The destination topology can only ever contain the same, or more,<br>+ * streams than the original so this is safe.<br>+ */<br>+ ast_stream_topology_set_stream(dest, original_index, removed);<br>+<br>+ break;<br>+ }<br>+ }<br>+ }<br>+<br>+ return 0;<br>+}<br>+<br>+static void sfu_topologies_on_source_change(struct ast_bridge_channel *source, struct ast_bridge_channels_list *participants)<br>+{<br>+ struct ast_stream_topology *source_video = NULL;<br>+ struct ast_bridge_channel *participant;<br>+ int res;<br>+<br>+ source_video = ast_stream_topology_alloc();<br>+ if (!source_video) {<br>+ return;<br>+ }<br>+<br>+ ast_channel_lock(source->chan);<br>+ res = append_source_streams(source_video, ast_channel_name(source->chan), ast_channel_get_stream_topology(source->chan));<br>+ ast_channel_unlock(source->chan);<br>+<br>+ if (res) {<br>+ goto cleanup;<br>+ }<br>+<br>+ AST_LIST_TRAVERSE(participants, participant, entry) {<br>+ struct ast_stream_topology *original_topology;<br>+ struct ast_stream_topology *participant_topology;<br>+<br>+ if (participant == source) {<br>+ continue;<br>+ }<br>+<br>+ ast_channel_lock(participant->chan);<br>+ original_topology = ast_stream_topology_clone(ast_channel_get_stream_topology(participant->chan));<br>+ ast_channel_unlock(participant->chan);<br>+ if (!original_topology) {<br>+ goto cleanup;<br>+ }<br>+<br>+ participant_topology = ast_stream_topology_clone(original_topology);<br>+ if (!participant_topology) {<br>+ ast_stream_topology_free(original_topology);<br>+ goto cleanup;<br>+ }<br>+<br>+ /* We add all the source streams back in, if any removed streams are already present they will<br>+ * get used first followed by appending new ones.<br>+ */<br>+ if (append_all_streams(participant_topology, source_video)) {<br>+ ast_stream_topology_free(participant_topology);<br>+ ast_stream_topology_free(original_topology);<br>+ goto cleanup;<br>+ }<br>+<br>+ /* And the original existing streams get marked as removed. This causes the remote side to see<br>+ * a new stream for the source streams.<br>+ */<br>+ if (remove_all_original_streams(participant_topology, source_video, original_topology)) {<br>+ ast_stream_topology_free(participant_topology);<br>+ ast_stream_topology_free(original_topology);<br>+ goto cleanup;<br>+ }<br>+<br>+ ast_channel_request_stream_topology_change(participant->chan, participant_topology, NULL);<br>+ ast_stream_topology_free(participant_topology);<br>+ ast_stream_topology_free(original_topology);<br>+ }<br>+<br>+cleanup:<br>+ ast_stream_topology_free(source_video);<br>+}<br>+<br> /*!<br> * \internal<br> * \brief Determine what to do with a control frame.<br>@@ -1016,6 +1126,11 @@<br> softmix_data->last_video_update = ast_tvnow();<br> }<br> break;<br>+ case AST_CONTROL_STREAM_TOPOLOGY_SOURCE_CHANGED:<br>+ if (bridge->softmix.video_mode.mode == AST_BRIDGE_VIDEO_MODE_SFU) {<br>+ sfu_topologies_on_source_change(bridge_channel, &bridge->channels);<br>+ }<br>+ break;<br> default:<br> break;<br> }<br>diff --git a/channels/chan_iax2.c b/channels/chan_iax2.c<br>index 490c4ce..04aa228 100644<br>--- a/channels/chan_iax2.c<br>+++ b/channels/chan_iax2.c<br>@@ -1433,6 +1433,7 @@<br> /* Intended only for internal stream topology manipulation. */<br> case AST_CONTROL_STREAM_TOPOLOGY_CHANGED:<br> /* Intended only for internal stream topology change notification. */<br>+ case AST_CONTROL_STREAM_TOPOLOGY_SOURCE_CHANGED:<br> case AST_CONTROL_STREAM_STOP:<br> case AST_CONTROL_STREAM_SUSPEND:<br> case AST_CONTROL_STREAM_RESTART:<br>diff --git a/funcs/func_frame_trace.c b/funcs/func_frame_trace.c<br>index 49abfdf..e88cafa 100644<br>--- a/funcs/func_frame_trace.c<br>+++ b/funcs/func_frame_trace.c<br>@@ -342,6 +342,9 @@<br> case AST_CONTROL_STREAM_TOPOLOGY_CHANGED:<br> ast_verbose("SubClass: STREAM_TOPOLOGY_CHANGED\n");<br> break;<br>+ case AST_CONTROL_STREAM_TOPOLOGY_SOURCE_CHANGED:<br>+ ast_verbose("SubClass: STREAM_TOPOLOGY_SOURCE_CHANGED\n");<br>+ break;<br> case AST_CONTROL_STREAM_STOP:<br> ast_verbose("SubClass: STREAM_STOP\n");<br> break;<br>diff --git a/include/asterisk/frame.h b/include/asterisk/frame.h<br>index 8f0dacc..eb6a647 100644<br>--- a/include/asterisk/frame.h<br>+++ b/include/asterisk/frame.h<br>@@ -301,6 +301,7 @@<br> AST_CONTROL_MASQUERADE_NOTIFY = 34, /*!< A masquerade is about to begin/end. (Never sent as a frame but directly with ast_indicate_data().) */<br> AST_CONTROL_STREAM_TOPOLOGY_REQUEST_CHANGE = 35, /*!< Channel indication that a stream topology change has been requested */<br> AST_CONTROL_STREAM_TOPOLOGY_CHANGED = 36, /*!< Channel indication that a stream topology change has occurred */<br>+ AST_CONTROL_STREAM_TOPOLOGY_SOURCE_CHANGED = 37, /*!< Channel indication that one of the source streams has changed its source */<br> <br> /*<br> * WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING<br>diff --git a/include/asterisk/res_pjsip_session.h b/include/asterisk/res_pjsip_session.h<br>index d5b6fa1..fcb14b7 100644<br>--- a/include/asterisk/res_pjsip_session.h<br>+++ b/include/asterisk/res_pjsip_session.h<br>@@ -109,6 +109,8 @@<br> char mslabel[AST_UUID_STR_LEN];<br> /*! \brief Track label */<br> char label[AST_UUID_STR_LEN];<br>+ /*! \brief The underlying session has been changed in some fashion */<br>+ unsigned int changed;<br> };<br> <br> /*!<br>diff --git a/main/channel.c b/main/channel.c<br>index 74de9ca..ecc771c 100644<br>--- a/main/channel.c<br>+++ b/main/channel.c<br>@@ -4228,6 +4228,7 @@<br> case AST_CONTROL_MASQUERADE_NOTIFY:<br> case AST_CONTROL_STREAM_TOPOLOGY_REQUEST_CHANGE:<br> case AST_CONTROL_STREAM_TOPOLOGY_CHANGED:<br>+ case AST_CONTROL_STREAM_TOPOLOGY_SOURCE_CHANGED:<br> case AST_CONTROL_STREAM_STOP:<br> case AST_CONTROL_STREAM_SUSPEND:<br> case AST_CONTROL_STREAM_REVERSE:<br>@@ -4528,6 +4529,7 @@<br> case AST_CONTROL_UPDATE_RTP_PEER:<br> case AST_CONTROL_STREAM_TOPOLOGY_REQUEST_CHANGE:<br> case AST_CONTROL_STREAM_TOPOLOGY_CHANGED:<br>+ case AST_CONTROL_STREAM_TOPOLOGY_SOURCE_CHANGED:<br> case AST_CONTROL_STREAM_STOP:<br> case AST_CONTROL_STREAM_SUSPEND:<br> case AST_CONTROL_STREAM_REVERSE:<br>diff --git a/res/res_pjsip_sdp_rtp.c b/res/res_pjsip_sdp_rtp.c<br>index e095f06..88b94ee 100644<br>--- a/res/res_pjsip_sdp_rtp.c<br>+++ b/res/res_pjsip_sdp_rtp.c<br>@@ -1022,6 +1022,19 @@<br> continue;<br> }<br> <br>+ /* If we are currently negotiating as a result of the remote side renegotiating then<br>+ * determine if the source for this stream has changed.<br>+ */<br>+ if (pjmedia_sdp_neg_get_state(session->inv_session->neg) == PJMEDIA_SDP_NEG_STATE_REMOTE_OFFER &&<br>+ session->active_media_state) {<br>+ struct ast_rtp_instance_stats stats = { 0, };<br>+<br>+ if (!ast_rtp_instance_get_stats(session_media->rtp, &stats, AST_RTP_INSTANCE_STAT_REMOTE_SSRC) &&<br>+ stats.remote_ssrc != ssrc) {<br>+ session_media->changed = 1;<br>+ }<br>+ }<br>+<br> ast_rtp_instance_set_remote_ssrc(session_media->rtp, ssrc);<br> }<br> }<br>diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c<br>index 64416a0..ad0cb4e 100644<br>--- a/res/res_pjsip_session.c<br>+++ b/res/res_pjsip_session.c<br>@@ -765,6 +765,7 @@<br> {<br> int i;<br> struct ast_stream_topology *topology;<br>+ unsigned int changed = 0;<br> <br> for (i = 0; i < local->media_count; ++i) {<br> struct ast_sip_session_media *session_media;<br>@@ -802,6 +803,9 @@<br> if (handle_negotiated_sdp_session_media(session_media, session, local, remote, i, stream)) {<br> return -1;<br> }<br>+<br>+ changed |= session_media->changed;<br>+ session_media->changed = 0;<br> }<br> <br> /* Apply the pending media state to the channel and make it active */<br>@@ -858,7 +862,13 @@<br> <br> ast_channel_unlock(session->channel);<br> <br>- ast_queue_frame(session->channel, &ast_null_frame);<br>+ if (changed) {<br>+ struct ast_frame f = { AST_FRAME_CONTROL, .subclass.integer = AST_CONTROL_STREAM_TOPOLOGY_SOURCE_CHANGED };<br>+<br>+ ast_queue_frame(session->channel, &f);<br>+ } else {<br>+ ast_queue_frame(session->channel, &ast_null_frame);<br>+ }<br> <br> return 0;<br> }<br>@@ -1476,7 +1486,10 @@<br> ast_stream_set_formats(stream, joint_cap);<br> }<br> <br>- ++type_streams[ast_stream_get_type(stream)];<br>+ /* We only count streams that have not been removed */<br>+ if (ast_stream_get_state(stream) != AST_STREAM_STATE_REMOVED) {<br>+ ++type_streams[ast_stream_get_type(stream)];<br>+ }<br> }<br> <br> if (session->active_media_state->topology) {<br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/6531">change 6531</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/6531"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: I30ccbdba16ca073d7f31e0e59ab778c153afae07 </div>
<div style="display:none"> Gerrit-Change-Number: 6531 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Joshua Colp <jcolp@digium.com> </div>