webrtcbin: try harder not to pick duplicate media ids

On renegotiation, or when the user has specified a mid for
a transceiver, we need to avoid picking a duplicate mid for
a transceiver that doesn't yet have one.

Also assign the mid we created to the transceiver, that doesn't
fix a specific bug but seems to make sense to me.

Change-Id: I6c2399a352f2ab05b469914a348512bb2b5b4a34
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/1902>

diff --git a/ext/webrtc/gstwebrtcbin.c b/ext/webrtc/gstwebrtcbin.c
index 019ca59..f285658 100644 (file)
--- a/ext/webrtc/gstwebrtcbin.c
+++ b/ext/webrtc/gstwebrtcbin.c
@@ -2456,7 +2456,7 @@ static gboolean
 sdp_media_from_transceiver (GstWebRTCBin * webrtc, GstSDPMedia * media,
     GstWebRTCRTPTransceiver * trans, GstWebRTCSDPType type, guint media_idx,
     GString * bundled_mids, guint bundle_idx, gchar * bundle_ufrag,
-    gchar * bundle_pwd, GArray * reserved_pts)
+    gchar * bundle_pwd, GArray * reserved_pts, GHashTable * all_mids)
 {
   /* TODO:
    * rtp header extensions
@@ -2598,10 +2598,18 @@ sdp_media_from_transceiver (GstWebRTCBin * webrtc, GstSDPMedia * media,
   if (trans->mid) {
     gst_sdp_media_add_attribute (media, "mid", trans->mid);
   } else {
-    sdp_mid = g_strdup_printf ("%s%u", gst_sdp_media_get_media (media),
-        webrtc->priv->media_counter++);
-    gst_sdp_media_add_attribute (media, "mid", sdp_mid);
-    g_free (sdp_mid);
+    /* Make sure to avoid mid collisions */
+    while (TRUE) {
+      sdp_mid = g_strdup_printf ("%s%u", gst_sdp_media_get_media (media),
+          webrtc->priv->media_counter++);
+      if (g_hash_table_contains (all_mids, (gpointer) sdp_mid)) {
+        g_free (sdp_mid);
+      } else {
+        gst_sdp_media_add_attribute (media, "mid", sdp_mid);
+        g_hash_table_insert (all_mids, sdp_mid, NULL);
+        break;
+      }
+    }
   }
 
   /* TODO:
@@ -2665,7 +2673,7 @@ gather_reserved_pts (GstWebRTCBin * webrtc)
 static gboolean
 _add_data_channel_offer (GstWebRTCBin * webrtc, GstSDPMessage * msg,
     GstSDPMedia * media, GString * bundled_mids, guint bundle_idx,
-    gchar * bundle_ufrag, gchar * bundle_pwd)
+    gchar * bundle_ufrag, gchar * bundle_pwd, GHashTable * all_mids)
 {
   GstSDPMessage *last_offer = _get_latest_self_generated_sdp (webrtc);
   gchar *ufrag, *pwd, *sdp_mid;
@@ -2726,10 +2734,18 @@ _add_data_channel_offer (GstWebRTCBin * webrtc, GstSDPMessage * msg,
 
     gst_sdp_media_add_attribute (media, "mid", mid);
   } else {
-    sdp_mid = g_strdup_printf ("%s%u", gst_sdp_media_get_media (media),
-        webrtc->priv->media_counter++);
-    gst_sdp_media_add_attribute (media, "mid", sdp_mid);
-    g_free (sdp_mid);
+    /* Make sure to avoid mid collisions */
+    while (TRUE) {
+      sdp_mid = g_strdup_printf ("%s%u", gst_sdp_media_get_media (media),
+          webrtc->priv->media_counter++);
+      if (g_hash_table_contains (all_mids, (gpointer) sdp_mid)) {
+        g_free (sdp_mid);
+      } else {
+        gst_sdp_media_add_attribute (media, "mid", sdp_mid);
+        g_hash_table_insert (all_mids, sdp_mid, NULL);
+        break;
+      }
+    }
   }
 
   if (bundled_mids) {
@@ -2754,11 +2770,14 @@ static GstSDPMessage *
 _create_offer_task (GstWebRTCBin * webrtc, const GstStructure * options,
     GError ** error)
 {
-  GstSDPMessage *ret;
+  GstSDPMessage *ret = NULL;
   GString *bundled_mids = NULL;
   gchar *bundle_ufrag = NULL;
   gchar *bundle_pwd = NULL;
   GArray *reserved_pts = NULL;
+  GHashTable *all_mids =
+      g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
+
   GstSDPMessage *last_offer = _get_latest_self_generated_sdp (webrtc);
   GList *seen_transceivers = NULL;
   guint media_idx = 0;
@@ -2834,6 +2853,7 @@ _create_offer_task (GstWebRTCBin * webrtc, const GstStructure * options,
 
           if (trans->mid && g_strcmp0 (trans->mid, last_mid) == 0) {
             GstSDPMedia *media;
+            const gchar *mid;
 
             g_assert (!g_list_find (seen_transceivers, trans));
 
@@ -2845,13 +2865,22 @@ _create_offer_task (GstWebRTCBin * webrtc, const GstStructure * options,
             gst_sdp_media_copy (last_media, &media);
             _media_replace_direction (media, trans->direction);
 
-            if (bundled_mids) {
-              const gchar *mid = gst_sdp_media_get_attribute_val (media, "mid");
+            mid = gst_sdp_media_get_attribute_val (media, "mid");
+            g_assert (mid);
 
-              g_assert (mid);
-              g_string_append_printf (bundled_mids, " %s", mid);
+            if (g_hash_table_contains (all_mids, mid)) {
+              gst_sdp_media_free (media);
+              g_set_error (error, GST_WEBRTC_BIN_ERROR,
+                  GST_WEBRTC_BIN_ERROR_FAILED,
+                  "Duplicate mid %s when creating offer", mid);
+              goto duplicate_mid;
             }
 
+            g_hash_table_insert (all_mids, g_strdup (mid), NULL);
+
+            if (bundled_mids)
+              g_string_append_printf (bundled_mids, " %s", mid);
+
             gst_sdp_message_add_media (ret, media);
             media_idx++;
 
@@ -2865,7 +2894,7 @@ _create_offer_task (GstWebRTCBin * webrtc, const GstStructure * options,
         GstSDPMedia media = { 0, };
         gst_sdp_media_init (&media);
         if (_add_data_channel_offer (webrtc, ret, &media, bundled_mids, 0,
-                bundle_ufrag, bundle_pwd)) {
+                bundle_ufrag, bundle_pwd, all_mids)) {
           gst_sdp_message_add_media (ret, &media);
           media_idx++;
         } else {
@@ -2875,6 +2904,26 @@ _create_offer_task (GstWebRTCBin * webrtc, const GstStructure * options,
     }
   }
 
+  /* First, go over all transceivers and gather existing mids */
+  for (i = 0; i < webrtc->priv->transceivers->len; i++) {
+    GstWebRTCRTPTransceiver *trans;
+
+    trans = g_ptr_array_index (webrtc->priv->transceivers, i);
+
+    if (g_list_find (seen_transceivers, trans))
+      continue;
+
+    if (trans->mid) {
+      if (g_hash_table_contains (all_mids, trans->mid)) {
+        g_set_error (error, GST_WEBRTC_BIN_ERROR, GST_WEBRTC_BIN_ERROR_FAILED,
+            "Duplicate mid %s when creating offer", trans->mid);
+        goto duplicate_mid;
+      }
+
+      g_hash_table_insert (all_mids, g_strdup (trans->mid), NULL);
+    }
+  }
+
   /* add any extra streams */
   for (i = 0; i < webrtc->priv->transceivers->len; i++) {
     GstWebRTCRTPTransceiver *trans;
@@ -2901,7 +2950,7 @@ _create_offer_task (GstWebRTCBin * webrtc, const GstStructure * options,
 
     if (sdp_media_from_transceiver (webrtc, &media, trans,
             GST_WEBRTC_SDP_TYPE_OFFER, media_idx, bundled_mids, 0, bundle_ufrag,
-            bundle_pwd, reserved_pts)) {
+            bundle_pwd, reserved_pts, all_mids)) {
       gst_sdp_message_add_media (ret, &media);
       media_idx++;
     } else {
@@ -2910,12 +2959,14 @@ _create_offer_task (GstWebRTCBin * webrtc, const GstStructure * options,
 
     if (webrtc->bundle_policy == GST_WEBRTC_BUNDLE_POLICY_NONE) {
       g_array_free (reserved_pts, TRUE);
+      reserved_pts = NULL;
     }
     seen_transceivers = g_list_prepend (seen_transceivers, trans);
   }
 
   if (webrtc->bundle_policy != GST_WEBRTC_BUNDLE_POLICY_NONE) {
     g_array_free (reserved_pts, TRUE);
+    reserved_pts = NULL;
   }
 
   /* add a data channel if exists and not renegotiated */
@@ -2923,7 +2974,7 @@ _create_offer_task (GstWebRTCBin * webrtc, const GstStructure * options,
     GstSDPMedia media = { 0, };
     gst_sdp_media_init (&media);
     if (_add_data_channel_offer (webrtc, ret, &media, bundled_mids, 0,
-            bundle_ufrag, bundle_pwd)) {
+            bundle_ufrag, bundle_pwd, all_mids)) {
       gst_sdp_message_add_media (ret, &media);
       media_idx++;
     } else {
@@ -2938,18 +2989,11 @@ _create_offer_task (GstWebRTCBin * webrtc, const GstStructure * options,
 
     gst_sdp_message_add_attribute (ret, "group", mids);
     g_free (mids);
+    bundled_mids = NULL;
   }
 
-  if (bundle_ufrag)
-    g_free (bundle_ufrag);
-
-  if (bundle_pwd)
-    g_free (bundle_pwd);
-
   /* FIXME: pre-emptively setup receiving elements when needed */
 
-  g_list_free (seen_transceivers);
-
   if (webrtc->priv->last_generated_answer)
     gst_webrtc_session_description_free (webrtc->priv->last_generated_answer);
   webrtc->priv->last_generated_answer = NULL;
@@ -2962,7 +3006,29 @@ _create_offer_task (GstWebRTCBin * webrtc, const GstStructure * options,
         gst_webrtc_session_description_new (GST_WEBRTC_SDP_TYPE_OFFER, copy);
   }
 
+out:
+  if (reserved_pts)
+    g_array_free (reserved_pts, TRUE);
+
+  g_hash_table_unref (all_mids);
+
+  g_list_free (seen_transceivers);
+
+  if (bundle_ufrag)
+    g_free (bundle_ufrag);
+
+  if (bundle_pwd)
+    g_free (bundle_pwd);
+
+  if (bundled_mids)
+    g_string_free (bundled_mids, TRUE);
+
   return ret;
+
+duplicate_mid:
+  gst_sdp_message_uninit (ret);
+  ret = NULL;
+  goto out;
 }
 
 static void