webrtcbin: Support closing of data channels

Support for closing WebRTC data channels as described in RFC
8831 (section 6.7) now fully supported. This means that we can now
reuse data channels that have been closed properly. Previously, an
application that created a lot of short-lived on-demand data channels
would quickly exhaust resources held by lingering non-closed data
channels.

We now use a one-to-one style socket interface to SCTP just like the
Google implementation (i.e. SOCK_STREAM instead of SOCK_SEQPACKET, see
RFC 6458). For some reason the socket interface to use was made
optional through a property "use-sock-stream" even though code wasn't
written to handle the SOCK_SEQPACKET style. Specifically the
SCTP_RESET_STREAMS command wouldn't work without passing the correct
assocation id. Changing the default interface to use from
SOCK_SEQPACKET to SOCK_STREAM now means we don't have to bother about
the association id as there is only one association per socket. For
the SCTP_RESET_STREAMS command we set it to SCTP_ALL_ASSOC just to
match the Google implementation.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/2186>

diff --git a/ext/sctp/sctpassociation.c b/ext/sctp/sctpassociation.c
index fbf5b4a..68c05e6 100644 (file)
--- a/ext/sctp/sctpassociation.c
+++ b/ext/sctp/sctpassociation.c
@@ -234,7 +234,7 @@ gst_sctp_association_init (GstSctpAssociation * self)
 
   self->state = GST_SCTP_ASSOCIATION_STATE_NEW;
 
-  self->use_sock_stream = FALSE;
+  self->use_sock_stream = TRUE;
 
   usrsctp_register_address ((void *) self);
 }
@@ -546,6 +546,7 @@ gst_sctp_association_reset_stream (GstSctpAssociation * self, guint16 stream_id)
 
   length = (socklen_t) (sizeof (struct sctp_reset_streams) + sizeof (guint16));
   srs = (struct sctp_reset_streams *) g_malloc0 (length);
+  srs->srs_assoc_id = SCTP_ALL_ASSOC;
   srs->srs_flags = SCTP_STREAM_RESET_OUTGOING;
   srs->srs_number_streams = 1;
   srs->srs_stream_list[0] = stream_id;

diff --git a/ext/webrtc/gstwebrtcbin.c b/ext/webrtc/gstwebrtcbin.c
index dc4b003..9ef835c 100644 (file)
--- a/ext/webrtc/gstwebrtcbin.c
+++ b/ext/webrtc/gstwebrtcbin.c
@@ -1955,34 +1955,33 @@ _on_data_channel_ready_state (WebRTCDataChannel * channel,
     GParamSpec * pspec, GstWebRTCBin * webrtc)
 {
   GstWebRTCDataChannelState ready_state;
-  guint i;
 
   g_object_get (channel, "ready-state", &ready_state, NULL);
 
   if (ready_state == GST_WEBRTC_DATA_CHANNEL_STATE_OPEN) {
-    gboolean found = FALSE;
-
-    for (i = 0; i < webrtc->priv->pending_data_channels->len; i++) {
-      WebRTCDataChannel *c;
+    gboolean found;
 
-      c = g_ptr_array_index (webrtc->priv->pending_data_channels, i);
-      if (c == channel) {
-        found = TRUE;
-        g_ptr_array_remove_index (webrtc->priv->pending_data_channels, i);
-        break;
-      }
-    }
+    found = g_ptr_array_remove (webrtc->priv->pending_data_channels, channel);
     if (found == FALSE) {
       GST_FIXME_OBJECT (webrtc, "Received open for unknown data channel");
       return;
     }
 
-    g_ptr_array_add (webrtc->priv->data_channels, channel);
+    g_ptr_array_add (webrtc->priv->data_channels, gst_object_ref (channel));
 
     gst_webrtc_bin_update_sctp_priority (webrtc);
 
     g_signal_emit (webrtc, gst_webrtc_bin_signals[ON_DATA_CHANNEL_SIGNAL], 0,
-        gst_object_ref (channel));
+        channel);
+  } else if (ready_state == GST_WEBRTC_DATA_CHANNEL_STATE_CLOSED) {
+    gboolean found;
+
+    found = g_ptr_array_remove (webrtc->priv->pending_data_channels, channel)
+        || g_ptr_array_remove (webrtc->priv->data_channels, channel);
+
+    if (found == FALSE) {
+      GST_FIXME_OBJECT (webrtc, "Received close for unknown data channel");
+    }
   }
 }
 

diff --git a/ext/webrtc/sctptransport.c b/ext/webrtc/sctptransport.c
index 79aaa89..8452198 100644 (file)
--- a/ext/webrtc/sctptransport.c
+++ b/ext/webrtc/sctptransport.c
@@ -32,7 +32,7 @@ GST_DEBUG_CATEGORY_STATIC (GST_CAT_DEFAULT);
 enum
 {
   SIGNAL_0,
-  ON_RESET_STREAM_SIGNAL,
+  ON_STREAM_RESET_SIGNAL,
   LAST_SIGNAL,
 };
 
@@ -102,7 +102,7 @@ _emit_stream_reset (GstWebRTCSCTPTransport * sctp, gpointer user_data)
   guint stream_id = GPOINTER_TO_UINT (user_data);
 
   g_signal_emit (sctp,
-      gst_webrtc_sctp_transport_signals[ON_RESET_STREAM_SIGNAL], 0, stream_id);
+      gst_webrtc_sctp_transport_signals[ON_STREAM_RESET_SIGNAL], 0, stream_id);
 }
 
 static void
@@ -215,6 +215,7 @@ gst_webrtc_sctp_transport_constructed (GObject * object)
   sctp->sctpenc =
       g_object_ref_sink (gst_element_factory_make ("sctpenc", NULL));
   g_object_set (sctp->sctpenc, "sctp-association-id", association_id, NULL);
+  g_object_set (sctp->sctpenc, "use-sock-stream", TRUE, NULL);
 
   g_signal_connect (sctp->sctpdec, "pad-removed",
       G_CALLBACK (_on_sctp_dec_pad_removed), sctp);
@@ -264,11 +265,11 @@ gst_webrtc_sctp_transport_class_init (GstWebRTCSCTPTransportClass * klass)
           0, G_MAXUINT16, 0, G_PARAM_READABLE | G_PARAM_STATIC_STRINGS));
 
   /**
-   * GstWebRTCSCTPTransport::reset-stream:
+   * GstWebRTCSCTPTransport::stream-reset:
    * @object: the #GstWebRTCSCTPTransport
    * @stream_id: the SCTP stream that was reset
    */
-  gst_webrtc_sctp_transport_signals[ON_RESET_STREAM_SIGNAL] =
+  gst_webrtc_sctp_transport_signals[ON_STREAM_RESET_SIGNAL] =
       g_signal_new ("stream-reset", G_TYPE_FROM_CLASS (klass),
       G_SIGNAL_RUN_LAST, 0, NULL, NULL, NULL, G_TYPE_NONE, 1, G_TYPE_UINT);
 }

diff --git a/ext/webrtc/webrtcdatachannel.c b/ext/webrtc/webrtcdatachannel.c
index 5f669dd..7988f0f 100644 (file)
--- a/ext/webrtc/webrtcdatachannel.c
+++ b/ext/webrtc/webrtcdatachannel.c
@@ -281,17 +281,26 @@ static void
 _transport_closed (WebRTCDataChannel * channel)
 {
   GError *error;
+  gboolean both_sides_closed;
 
   GST_WEBRTC_DATA_CHANNEL_LOCK (channel);
   error = channel->stored_error;
   channel->stored_error = NULL;
+
+  both_sides_closed =
+      channel->peer_closed && channel->parent.buffered_amount <= 0;
+  if (both_sides_closed || error) {
+    channel->peer_closed = FALSE;
+  }
   GST_WEBRTC_DATA_CHANNEL_UNLOCK (channel);
 
   if (error) {
     gst_webrtc_data_channel_on_error (GST_WEBRTC_DATA_CHANNEL (channel), error);
     g_clear_error (&error);
   }
-  gst_webrtc_data_channel_on_close (GST_WEBRTC_DATA_CHANNEL (channel));
+  if (both_sides_closed || error) {
+    gst_webrtc_data_channel_on_close (GST_WEBRTC_DATA_CHANNEL (channel));
+  }
 }
 
 static void
@@ -299,6 +308,9 @@ _close_sctp_stream (WebRTCDataChannel * channel, gpointer user_data)
 {
   GstPad *pad, *peer;
 
+  GST_INFO_OBJECT (channel, "Closing outgoing SCTP stream %i label \"%s\"",
+      channel->parent.id, channel->parent.label);
+
   pad = gst_element_get_static_pad (channel->appsrc, "src");
   peer = gst_pad_get_peer (pad);
   gst_object_unref (pad);
@@ -321,31 +333,44 @@ _close_procedure (WebRTCDataChannel * channel, gpointer user_data)
 {
   /* https://www.w3.org/TR/webrtc/#data-transport-closing-procedure */
   GST_WEBRTC_DATA_CHANNEL_LOCK (channel);
-  if (channel->parent.ready_state == GST_WEBRTC_DATA_CHANNEL_STATE_CLOSED
-      || channel->parent.ready_state == GST_WEBRTC_DATA_CHANNEL_STATE_CLOSING) {
+  if (channel->parent.ready_state == GST_WEBRTC_DATA_CHANNEL_STATE_CLOSED) {
     GST_WEBRTC_DATA_CHANNEL_UNLOCK (channel);
     return;
-  }
-  channel->parent.ready_state = GST_WEBRTC_DATA_CHANNEL_STATE_CLOSING;
-  GST_WEBRTC_DATA_CHANNEL_UNLOCK (channel);
-  g_object_notify (G_OBJECT (channel), "ready-state");
+  } else if (channel->parent.ready_state ==
+      GST_WEBRTC_DATA_CHANNEL_STATE_CLOSING) {
+    _channel_enqueue_task (channel, (ChannelTask) _transport_closed, NULL,
+        NULL);
+  } else if (channel->parent.ready_state == GST_WEBRTC_DATA_CHANNEL_STATE_OPEN) {
+    channel->parent.ready_state = GST_WEBRTC_DATA_CHANNEL_STATE_CLOSING;
+    GST_WEBRTC_DATA_CHANNEL_UNLOCK (channel);
+    g_object_notify (G_OBJECT (channel), "ready-state");
 
-  GST_WEBRTC_DATA_CHANNEL_LOCK (channel);
-  if (channel->parent.buffered_amount <= 0) {
-    _channel_enqueue_task (channel, (ChannelTask) _close_sctp_stream,
-        NULL, NULL);
+    GST_WEBRTC_DATA_CHANNEL_LOCK (channel);
+    if (channel->parent.buffered_amount <= 0) {
+      _channel_enqueue_task (channel, (ChannelTask) _close_sctp_stream,
+          NULL, NULL);
+    }
   }
 
   GST_WEBRTC_DATA_CHANNEL_UNLOCK (channel);
 }
 
 static void
-_on_sctp_reset_stream (GstWebRTCSCTPTransport * sctp, guint stream_id,
+_on_sctp_stream_reset (GstWebRTCSCTPTransport * sctp, guint stream_id,
     WebRTCDataChannel * channel)
 {
-  if (channel->parent.id == stream_id)
-    _channel_enqueue_task (channel, (ChannelTask) _transport_closed,
+  if (channel->parent.id == stream_id) {
+    GST_INFO_OBJECT (channel,
+        "Received channel close for SCTP stream %i label \"%s\"",
+        channel->parent.id, channel->parent.label);
+
+    GST_WEBRTC_DATA_CHANNEL_LOCK (channel);
+    channel->peer_closed = TRUE;
+    GST_WEBRTC_DATA_CHANNEL_UNLOCK (channel);
+
+    _channel_enqueue_task (channel, (ChannelTask) _close_procedure,
         GUINT_TO_POINTER (stream_id), NULL);
+  }
 }
 
 static void
@@ -439,7 +464,7 @@ _parse_control_packet (WebRTCDataChannel * channel, guint8 * data,
     channel->opened = TRUE;
 
     GST_INFO_OBJECT (channel, "Received channel open for SCTP stream %i "
-        "label %s protocol %s ordered %s", channel->parent.id,
+        "label \"%s\" protocol %s ordered %s", channel->parent.id,
         channel->parent.label, channel->parent.protocol,
         channel->parent.ordered ? "true" : "false");
 
@@ -673,7 +698,7 @@ webrtc_data_channel_start_negotiation (WebRTCDataChannel * channel)
   buffer = construct_open_packet (channel);
 
   GST_INFO_OBJECT (channel, "Sending channel open for SCTP stream %i "
-      "label %s protocol %s ordered %s", channel->parent.id,
+      "label \"%s\" protocol %s ordered %s", channel->parent.id,
       channel->parent.label, channel->parent.protocol,
       channel->parent.ordered ? "true" : "false");
 
@@ -991,7 +1016,7 @@ _data_channel_set_sctp_transport (WebRTCDataChannel * channel,
       GST_OBJECT (sctp));
 
   if (sctp) {
-    g_signal_connect (sctp, "stream-reset", G_CALLBACK (_on_sctp_reset_stream),
+    g_signal_connect (sctp, "stream-reset", G_CALLBACK (_on_sctp_stream_reset),
         channel);
     g_signal_connect (sctp, "notify::state", G_CALLBACK (_on_sctp_notify_state),
         channel);

diff --git a/ext/webrtc/webrtcdatachannel.h b/ext/webrtc/webrtcdatachannel.h
index 7ca3c0d..53c11f2 100644 (file)
--- a/ext/webrtc/webrtcdatachannel.h
+++ b/ext/webrtc/webrtcdatachannel.h
@@ -51,6 +51,7 @@ struct _WebRTCDataChannel
   gboolean                          opened;
   gulong                            src_probe;
   GError                           *stored_error;
+  gboolean                          peer_closed;
 
   gpointer                          _padding[GST_PADDING];
 };