<p>Joshua Colp <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/6530">View Change</a></p><div style="white-space:pre-wrap">Approvals:
  Richard Mudgett: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved
  Joshua Colp: Approved for Submit

</div><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 channels/chan_pjsip.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>9 files changed, 162 insertions(+), 9 deletions(-)<br><br></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 bae0b8c..b4b2cad 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>@@ -493,21 +493,21 @@<br>     for (i = 0; i < ast_stream_topology_get_count(source); ++i) {<br>              struct ast_stream *stream;<br>            struct ast_stream *stream_clone;<br>-             char *stream_clone_name;<br>-             size_t stream_clone_name_len;<br>+                char *stream_clone_name = NULL;<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>+                 ast_free(stream_clone_name);<br>+                 return -1;<br>+           }<br> <br>          stream_clone = ast_stream_clone(stream, stream_clone_name);<br>+          ast_free(stream_clone_name);<br>          if (!stream_clone) {<br>                  return -1;<br>            }<br>@@ -987,6 +987,120 @@<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>+                          if (ast_stream_topology_set_stream(dest, original_index, removed)) {<br>+                                 ast_stream_free(removed);<br>+                                    return -1;<br>+                           }<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>+ 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 +1130,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/channels/chan_pjsip.c b/channels/chan_pjsip.c<br>index f86ef00..8f46f65 100644<br>--- a/channels/chan_pjsip.c<br>+++ b/channels/chan_pjsip.c<br>@@ -1731,6 +1731,8 @@<br>          break;<br>        case AST_CONTROL_STREAM_TOPOLOGY_CHANGED:<br>             break;<br>+       case AST_CONTROL_STREAM_TOPOLOGY_SOURCE_CHANGED:<br>+             break;<br>        case -1:<br>              res = -1;<br>             break;<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 5f49c82..3495748 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 77cd807..99b51bd 100644<br>--- a/res/res_pjsip_sdp_rtp.c<br>+++ b/res/res_pjsip_sdp_rtp.c<br>@@ -1021,6 +1021,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 b6fa8bb..d8cdcc0 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></pre><p>To view, visit <a href="https://gerrit.asterisk.org/6530">change 6530</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/6530"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15.0 </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: I30ccbdba16ca073d7f31e0e59ab778c153afae07 </div>
<div style="display:none"> Gerrit-Change-Number: 6530 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean.bright@gmail.com> </div>