From 4e4f8b7a7943b8085d38a794fddfed1b81cc0235 Mon Sep 17 00:00:00 2001 From: Eric Date: Fri, 7 Mar 2025 16:05:20 -0500 Subject: [PATCH] webrtc: fix hangup when duplicate sctp association IDs chosen Fixes an issue where the webrtcbin would hangup when finalizing due to the sctpenc hanging up when finalizing. This occurred when the webrtcbin chose to use a sctp association ID already in use. The sctpenc would fail to reach the paused state, but startup a task anyways that was never stopped. This commit modifies the behavior to not choose sctp association IDs randomly, and instead only choose one that is free. It also prevents the sctpenc from starting up that task if it fails to reach the paused state. Fixes: #4188 Part-of: --- .../docs/plugins/gst_plugins_cache.json | 12 ++++ .../gst-plugins-bad/ext/sctp/gstsctpdec.c | 52 ++++++++++++++++- .../gst-plugins-bad/ext/sctp/gstsctpdec.h | 2 + .../gst-plugins-bad/ext/sctp/gstsctpenc.c | 5 +- .../ext/sctp/sctpassociation.c | 56 +++++++++++++++++-- .../ext/sctp/sctpassociation.h | 1 + .../ext/webrtc/webrtcsctptransport.c | 9 +-- 7 files changed, 123 insertions(+), 14 deletions(-) diff --git a/subprojects/gst-plugins-bad/docs/plugins/gst_plugins_cache.json b/subprojects/gst-plugins-bad/docs/plugins/gst_plugins_cache.json index eb9faf50f0..1197ddbde0 100644 --- a/subprojects/gst-plugins-bad/docs/plugins/gst_plugins_cache.json +++ b/subprojects/gst-plugins-bad/docs/plugins/gst_plugins_cache.json @@ -243704,6 +243704,18 @@ } }, "properties": { + "automatic-association-id": { + "blurb": "Whether a SCTP Association ID should be automatically generated.", + "conditionally-available": false, + "construct": false, + "construct-only": false, + "controllable": false, + "default": "false", + "mutable": "null", + "readable": true, + "type": "gboolean", + "writable": true + }, "local-sctp-port": { "blurb": "Local sctp port for the sctp association. The remote port is configured via the GstSctpEnc element.", "conditionally-available": false, diff --git a/subprojects/gst-plugins-bad/ext/sctp/gstsctpdec.c b/subprojects/gst-plugins-bad/ext/sctp/gstsctpdec.c index c40b0847f0..7603912a87 100644 --- a/subprojects/gst-plugins-bad/ext/sctp/gstsctpdec.c +++ b/subprojects/gst-plugins-bad/ext/sctp/gstsctpdec.c @@ -64,6 +64,7 @@ enum PROP_GST_SCTP_ASSOCIATION_ID, PROP_LOCAL_SCTP_PORT, + PROP_AUTOMATIC_ASSOCIATION_ID, NUM_PROPERTIES }; @@ -72,6 +73,7 @@ static GParamSpec *properties[NUM_PROPERTIES]; #define DEFAULT_GST_SCTP_ASSOCIATION_ID 1 #define DEFAULT_LOCAL_SCTP_PORT 0 +#define DEFAULT_AUTOMATIC_ASSOCIATION_ID FALSE #define MAX_SCTP_PORT 65535 #define MAX_GST_SCTP_ASSOCIATION_ID 65535 @@ -207,6 +209,20 @@ gst_sctp_dec_class_init (GstSctpDecClass * klass) 0, MAX_SCTP_PORT, DEFAULT_LOCAL_SCTP_PORT, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS); + /** + * GstSctpDec:automatic-association-id: + * + * Whether a SCTP Association ID should be automatically generated. + * + * Since: 1.28 + */ + properties[PROP_AUTOMATIC_ASSOCIATION_ID] = + g_param_spec_boolean ("automatic-association-id", + "Automatic SCTP Association ID", + "Whether a SCTP Association ID should be automatically generated.", + DEFAULT_AUTOMATIC_ASSOCIATION_ID, + G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS); + g_object_class_install_properties (gobject_class, NUM_PROPERTIES, properties); signals[SIGNAL_RESET_STREAM] = g_signal_new ("reset-stream", @@ -226,7 +242,9 @@ gst_sctp_dec_init (GstSctpDec * self) { self->sctp_association_id = DEFAULT_GST_SCTP_ASSOCIATION_ID; self->local_sctp_port = DEFAULT_LOCAL_SCTP_PORT; + self->automatic_association_id = DEFAULT_AUTOMATIC_ASSOCIATION_ID; + self->automatic_sctp_association = NULL; self->flow_combiner = gst_flow_combiner_new (); self->sink_pad = gst_pad_new_from_static_template (&sink_template, "sink"); @@ -246,11 +264,29 @@ gst_sctp_dec_set_property (GObject * object, guint prop_id, switch (prop_id) { case PROP_GST_SCTP_ASSOCIATION_ID: - self->sctp_association_id = g_value_get_uint (value); + if (self->automatic_association_id) { + GST_WARNING_OBJECT (object, + "Cannot modify association id if automatic association id enabled"); + } else { + self->sctp_association_id = g_value_get_uint (value); + } break; case PROP_LOCAL_SCTP_PORT: self->local_sctp_port = g_value_get_uint (value); break; + case PROP_AUTOMATIC_ASSOCIATION_ID: + self->automatic_association_id = g_value_get_boolean (value); + if (self->automatic_association_id && !self->automatic_sctp_association) { + self->automatic_sctp_association = gst_sctp_association_create (); + if (self->automatic_sctp_association) + self->sctp_association_id = + self->automatic_sctp_association->association_id; + } else if (!self->automatic_association_id + && self->automatic_sctp_association) { + gst_clear_object (&self->automatic_sctp_association); + self->sctp_association_id = DEFAULT_GST_SCTP_ASSOCIATION_ID; + } + break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID (self, prop_id, pspec); break; @@ -270,6 +306,9 @@ gst_sctp_dec_get_property (GObject * object, guint prop_id, GValue * value, case PROP_LOCAL_SCTP_PORT: g_value_set_uint (value, self->local_sctp_port); break; + case PROP_AUTOMATIC_ASSOCIATION_ID: + g_value_set_boolean (value, self->automatic_association_id); + break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID (self, prop_id, pspec); break; @@ -284,6 +323,9 @@ gst_sctp_dec_finalize (GObject * object) gst_flow_combiner_free (self->flow_combiner); self->flow_combiner = NULL; + if (self->automatic_sctp_association) + gst_object_unref (self->automatic_sctp_association); + G_OBJECT_CLASS (parent_class)->finalize (object); } @@ -467,7 +509,13 @@ configure_association (GstSctpDec * self) { gint state; - self->sctp_association = gst_sctp_association_get (self->sctp_association_id); + if (self->automatic_sctp_association) { + self->sctp_association = self->automatic_sctp_association; + self->automatic_sctp_association = NULL; + } else { + self->sctp_association = + gst_sctp_association_get (self->sctp_association_id); + } g_object_get (self->sctp_association, "state", &state, NULL); diff --git a/subprojects/gst-plugins-bad/ext/sctp/gstsctpdec.h b/subprojects/gst-plugins-bad/ext/sctp/gstsctpdec.h index c6c8986571..bcbf471a09 100644 --- a/subprojects/gst-plugins-bad/ext/sctp/gstsctpdec.h +++ b/subprojects/gst-plugins-bad/ext/sctp/gstsctpdec.h @@ -50,7 +50,9 @@ struct _GstSctpDec GstPad *sink_pad; guint sctp_association_id; guint local_sctp_port; + gboolean automatic_association_id; + GstSctpAssociation *automatic_sctp_association; GstSctpAssociation *sctp_association; gulong signal_handler_stream_reset; }; diff --git a/subprojects/gst-plugins-bad/ext/sctp/gstsctpenc.c b/subprojects/gst-plugins-bad/ext/sctp/gstsctpenc.c index 2fcbecebb9..c8b66be0cd 100644 --- a/subprojects/gst-plugins-bad/ext/sctp/gstsctpenc.c +++ b/subprojects/gst-plugins-bad/ext/sctp/gstsctpenc.c @@ -395,8 +395,9 @@ gst_sctp_enc_change_state (GstElement * element, GstStateChange transition) case GST_STATE_CHANGE_NULL_TO_READY: break; case GST_STATE_CHANGE_READY_TO_PAUSED: - gst_pad_start_task (self->src_pad, - (GstTaskFunction) gst_sctp_enc_srcpad_loop, self->src_pad, NULL); + if (ret != GST_STATE_CHANGE_FAILURE) + gst_pad_start_task (self->src_pad, + (GstTaskFunction) gst_sctp_enc_srcpad_loop, self->src_pad, NULL); break; case GST_STATE_CHANGE_PLAYING_TO_PAUSED: break; diff --git a/subprojects/gst-plugins-bad/ext/sctp/sctpassociation.c b/subprojects/gst-plugins-bad/ext/sctp/sctpassociation.c index 772cbc145d..079b84e335 100644 --- a/subprojects/gst-plugins-bad/ext/sctp/sctpassociation.c +++ b/subprojects/gst-plugins-bad/ext/sctp/sctpassociation.c @@ -67,7 +67,22 @@ gst_sctp_association_state_get_type (void) return id; } -G_DEFINE_TYPE (GstSctpAssociation, gst_sctp_association, G_TYPE_OBJECT); +static void +_init_debug (void) +{ + static gsize _init = 0; + + if (g_once_init_enter (&_init)) { + GST_DEBUG_CATEGORY_INIT (gst_sctp_association_debug_category, + "sctpassociation", 0, "debug category for sctpassociation"); + GST_DEBUG_CATEGORY_INIT (gst_sctp_debug_category, "sctplib", 0, + "debug category for messages from usrsctp"); + g_once_init_leave (&_init, 1); + } +} + +G_DEFINE_TYPE_WITH_CODE (GstSctpAssociation, gst_sctp_association, + G_TYPE_OBJECT, _init_debug ()); enum { @@ -361,11 +376,6 @@ gst_sctp_association_get (guint32 association_id) GstSctpAssociation *association; G_LOCK (associations_lock); - GST_DEBUG_CATEGORY_INIT (gst_sctp_association_debug_category, - "sctpassociation", 0, "debug category for sctpassociation"); - GST_DEBUG_CATEGORY_INIT (gst_sctp_debug_category, - "sctplib", 0, "debug category for messages from usrsctp"); - if (!associations) { associations = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, NULL); @@ -386,6 +396,40 @@ gst_sctp_association_get (guint32 association_id) return association; } +GstSctpAssociation * +gst_sctp_association_create (void) +{ + GstSctpAssociation *association; + guint association_id = 0; + + G_LOCK (associations_lock); + if (!associations) { + associations = + g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, NULL); + } + + while (association_id < G_MAXUINT16) { + association = + g_hash_table_lookup (associations, GUINT_TO_POINTER (association_id)); + if (!association) + break; + association_id++; + } + + if (association) { + G_UNLOCK (associations_lock); + return NULL; + } + + association = + g_object_new (GST_SCTP_TYPE_ASSOCIATION, "association-id", association_id, + NULL); + g_hash_table_insert (associations, GUINT_TO_POINTER (association_id), + association); + G_UNLOCK (associations_lock); + return association; +} + gboolean gst_sctp_association_start (GstSctpAssociation * self) { diff --git a/subprojects/gst-plugins-bad/ext/sctp/sctpassociation.h b/subprojects/gst-plugins-bad/ext/sctp/sctpassociation.h index 8133290440..5c4eebe73b 100644 --- a/subprojects/gst-plugins-bad/ext/sctp/sctpassociation.h +++ b/subprojects/gst-plugins-bad/ext/sctp/sctpassociation.h @@ -103,6 +103,7 @@ struct _GstSctpAssociationClass GType gst_sctp_association_get_type (void); GstSctpAssociation *gst_sctp_association_get (guint32 association_id); +GstSctpAssociation *gst_sctp_association_create (void); gboolean gst_sctp_association_start (GstSctpAssociation * self); void gst_sctp_association_set_on_packet_out (GstSctpAssociation * self, diff --git a/subprojects/gst-plugins-bad/ext/webrtc/webrtcsctptransport.c b/subprojects/gst-plugins-bad/ext/webrtc/webrtcsctptransport.c index c65dd19737..895ebbfdb3 100644 --- a/subprojects/gst-plugins-bad/ext/webrtc/webrtcsctptransport.c +++ b/subprojects/gst-plugins-bad/ext/webrtc/webrtcsctptransport.c @@ -193,13 +193,14 @@ static void webrtc_sctp_transport_constructed (GObject * object) { WebRTCSCTPTransport *sctp = WEBRTC_SCTP_TRANSPORT (object); - guint association_id; - - association_id = g_random_int_range (0, G_MAXUINT16); sctp->sctpdec = g_object_ref_sink (gst_element_factory_make ("sctpdec", NULL)); - g_object_set (sctp->sctpdec, "sctp-association-id", association_id, NULL); + g_object_set (sctp->sctpdec, "automatic-association-id", TRUE, NULL); + + guint association_id; + g_object_get (sctp->sctpdec, "sctp-association-id", &association_id, NULL); + sctp->sctpenc = g_object_ref_sink (gst_element_factory_make ("sctpenc", NULL)); g_object_set (sctp->sctpenc, "sctp-association-id", association_id, NULL);