From dd58ae8cc8f5cda22021cc499503aca4cf61f6ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Wed, 22 Jan 2025 12:32:24 +0200 Subject: [PATCH] netclientclock: Don't ever store failed internal clocks in the cache If starting the internal clock fails we would still store a broken clock in the cache despite it being unusable and never recovering. Not storing it allows the application to simply create a new one at a later time and have starting it retried. Also signal to the application that such a clock is not synced. Part-of: --- .../libs/gst/net/gstnetclientclock.c | 109 +++++++++++------- 1 file changed, 65 insertions(+), 44 deletions(-) diff --git a/subprojects/gstreamer/libs/gst/net/gstnetclientclock.c b/subprojects/gstreamer/libs/gst/net/gstnetclientclock.c index 3fa5e0d508..da1b21692b 100644 --- a/subprojects/gstreamer/libs/gst/net/gstnetclientclock.c +++ b/subprojects/gstreamer/libs/gst/net/gstnetclientclock.c @@ -331,6 +331,7 @@ gst_net_client_internal_clock_constructed (GObject * object) if (!gst_net_client_internal_clock_start (self)) { g_warning ("failed to start clock '%s'", GST_OBJECT_NAME (self)); + self->marked_corrupted = TRUE; } /* all systems go, cap'n */ @@ -1181,41 +1182,44 @@ static void gst_net_client_clock_finalize (GObject * object) { GstNetClientClock *self = GST_NET_CLIENT_CLOCK (object); - GList *l; - if (self->priv->synced_id) - g_signal_handler_disconnect (self->priv->internal_clock, - self->priv->synced_id); - self->priv->synced_id = 0; + if (self->priv->internal_clock) { + GList *l; - G_LOCK (clocks_lock); - for (l = clocks; l; l = l->next) { - ClockCache *cache = l->data; + if (self->priv->synced_id) + g_signal_handler_disconnect (self->priv->internal_clock, + self->priv->synced_id); + self->priv->synced_id = 0; - if (cache->clock == self->priv->internal_clock) { - cache->clocks = g_list_remove (cache->clocks, self); + G_LOCK (clocks_lock); + for (l = clocks; l; l = l->next) { + ClockCache *cache = l->data; - if (cache->clocks) { - update_clock_cache (cache); - } else { - GstClock *sysclock = gst_system_clock_obtain (); - GstClockTime time = gst_clock_get_time (sysclock); - GstNetClientInternalClock *internal_clock = - GST_NET_CLIENT_INTERNAL_CLOCK (cache->clock); + if (cache->clock == self->priv->internal_clock) { + cache->clocks = g_list_remove (cache->clocks, self); - /* only defer deletion if the clock is not marked corrupted */ - if (!internal_clock->marked_corrupted) - time += 60 * GST_SECOND; + if (cache->clocks) { + update_clock_cache (cache); + } else { + GstClock *sysclock = gst_system_clock_obtain (); + GstClockTime time = gst_clock_get_time (sysclock); + GstNetClientInternalClock *internal_clock = + GST_NET_CLIENT_INTERNAL_CLOCK (cache->clock); - cache->remove_id = gst_clock_new_single_shot_id (sysclock, time); - gst_clock_id_wait_async (cache->remove_id, remove_clock_cache, cache, - NULL); - gst_object_unref (sysclock); + /* only defer deletion if the clock is not marked corrupted */ + if (!internal_clock->marked_corrupted) + time += 60 * GST_SECOND; + + cache->remove_id = gst_clock_new_single_shot_id (sysclock, time); + gst_clock_id_wait_async (cache->remove_id, remove_clock_cache, cache, + NULL); + gst_object_unref (sysclock); + } + break; } - break; } + G_UNLOCK (clocks_lock); } - G_UNLOCK (clocks_lock); g_free (self->priv->address); self->priv->address = NULL; @@ -1362,7 +1366,6 @@ static void gst_net_client_clock_constructed (GObject * object) { GstNetClientClock *self = GST_NET_CLIENT_CLOCK (object); - GstClock *internal_clock; GList *l; ClockCache *cache = NULL; @@ -1390,34 +1393,49 @@ gst_net_client_clock_constructed (GObject * object) } if (!cache) { - cache = g_new0 (ClockCache, 1); + GstNetClientInternalClock *internal_clock; - cache->clock = + internal_clock = g_object_new (GST_TYPE_NET_CLIENT_INTERNAL_CLOCK, "address", self->priv->address, "port", self->priv->port, "is-ntp", self->priv->is_ntp, NULL); - gst_object_ref_sink (cache->clock); - clocks = g_list_prepend (clocks, cache); + gst_object_ref_sink (internal_clock); - /* Not actually leaked but is cached for a while before being disposed, - * see gst_net_client_clock_finalize, so pretend it is to not confuse - * tests. */ - GST_OBJECT_FLAG_SET (cache->clock, GST_OBJECT_FLAG_MAY_BE_LEAKED); + if (internal_clock->marked_corrupted) { + GST_WARNING_OBJECT (self, "Internal clock couldn't start"); + gst_object_unref (internal_clock); + } else { + cache = g_new0 (ClockCache, 1); + + cache->clock = GST_CLOCK (internal_clock); + clocks = g_list_prepend (clocks, cache); + + /* Not actually leaked but is cached for a while before being disposed, + * see gst_net_client_clock_finalize, so pretend it is to not confuse + * tests. */ + GST_OBJECT_FLAG_SET (cache->clock, GST_OBJECT_FLAG_MAY_BE_LEAKED); + } } - cache->clocks = g_list_prepend (cache->clocks, self); + if (cache) { + cache->clocks = g_list_prepend (cache->clocks, self); - GST_OBJECT_LOCK (cache->clock); - if (gst_clock_is_synced (cache->clock)) - gst_clock_set_synced (GST_CLOCK (self), TRUE); - self->priv->synced_id = - g_signal_connect (cache->clock, "synced", - G_CALLBACK (gst_net_client_clock_synced_cb), self); - GST_OBJECT_UNLOCK (cache->clock); + GST_OBJECT_LOCK (cache->clock); + if (gst_clock_is_synced (cache->clock)) + gst_clock_set_synced (GST_CLOCK (self), TRUE); + self->priv->synced_id = + g_signal_connect (cache->clock, "synced", + G_CALLBACK (gst_net_client_clock_synced_cb), self); + GST_OBJECT_UNLOCK (cache->clock); + + self->priv->internal_clock = cache->clock; + } G_UNLOCK (clocks_lock); - self->priv->internal_clock = internal_clock = cache->clock; + /* Mark clock as unsynced if creation of the internal clock failed */ + if (!cache) + gst_clock_set_synced (GST_CLOCK (self), FALSE); /* all systems go, cap'n */ } @@ -1427,6 +1445,9 @@ gst_net_client_clock_get_internal_time (GstClock * clock) { GstNetClientClock *self = GST_NET_CLIENT_CLOCK (clock); + if (!self->priv->internal_clock) + return self->priv->base_time; + if (!gst_clock_is_synced (self->priv->internal_clock)) { GstClockTime now = gst_clock_get_internal_time (self->priv->internal_clock); return gst_clock_adjust_with_calibration (self->priv->internal_clock, now,