Do you still have the gdb stack trace?  I remember seeing something like that, but I don't remember how I got it to happen.

Created attachment 334928 [details] [review]
gstclock: Keep weak reference to underlying clock

Can you try with this patch?

Comment on attachment 334928 [details] [review]
gstclock: Keep weak reference to underlying clock

While this patch makes sense (except for the change in common/, please omit that :) ), it shouldn't be necessary according to the comments in the clock code.

Question is what and why the clock is gone before the clock ids are gone. This sounds like an element getting rid of the clock but not its clock ids.

Hello,
the patch also makes sense from my point of view.

@slomo, now we are using a data structure to keep the ClockId until we need it, but the Clock is unrefered after creating the ClockId as you can see in the next code.
Without the @GstBlub's patch, should we keep the Clock ref in the data structure until the ClockId is unshceduled?

ClockId creation:
  GstClock *clock;
  clock = gst_system_clock_obtain ();
  init_time = gst_clock_get_time (clock);
  clock_id = gst_clock_new_periodic_id (data->clock, init_time, 2 * GST_SECOND);
  g_object_unref (clock);

Then, when the ClockId is not longer needed:
  gst_clock_id_unschedule (data->clock_id);
  gst_clock_id_unref (data->clock_id);

Keeping the clock around is AFAIU how that API is intended to be used (API with a gun to shoot yourself into the feet included *sigh*).

The problem with not keeping the clock around is that you can never be sure if your callback is actually ever going to be called, which is also problematic. OTOH you're not guaranteed that either otherwise (e.g. when an audio clock is used and we're down in PAUSED).

I think the patch makes sense (slightly worried about the overhead of the weak ref) but there also seems to be a bigger problem here

@slomo, thank you for the explanation!!

So, from my point of view:
- Or we include the @GstBlub's patch.
- Or we should update de doc of gst_clock_new_periodic_id and gst_clock_new_single_shot_id

What do you think?

(In reply to Sebastian Dröge (slomo) from comment #3)
> Comment on attachment 334928 [details] [review] [review]
> gstclock: Keep weak reference to underlying clock
> 
> While this patch makes sense (except for the change in common/, please omit
> that :) ), it shouldn't be necessary according to the comments in the clock
> code.

Oops, that change was not intentional (switching branches too often I guess)

> Question is what and why the clock is gone before the clock ids are gone.
> This sounds like an element getting rid of the clock but not its clock ids.

Yeah, sounds like an element is doing something wrong, BUT at the same time the Clock IDs should either ref the clock so it never goes away until all IDs are gone, or they should at least hold a weak reference.  I chose the latter as holding a strong reference would change behaviour.  Keeping a week reference handles the incorrect use more gracefully, though at the expense of some overhead.

Created attachment 335003 [details] [review]
gstclock: Keep weak reference to underlying clock

Updated the patch.

Yes, I think generally having this patch is a good idea. The elements should also be fixed though :)

Review of attachment 335003 [details] [review]:

::: gst/gstclock.h
@@ -347,3 @@
- * Get the owner clock of the entry
- */
-#define GST_CLOCK_ENTRY_CLOCK(entry)    ((entry)->clock)

Don't remove public API

@@ +558,3 @@
 gint                    gst_clock_id_compare_func       (gconstpointer id1, gconstpointer id2);
 
+gboolean                gst_clock_id_is_clock           (GstClockID id, GstClock * clock);

Maybe gst_clock_id_belongs_to_clock()? gst_clock_id_get_clock()?

(In reply to Sebastian Dröge (slomo) from comment #10)
> Review of attachment 335003 [details] [review] [review]:
> 
> ::: gst/gstclock.h
> @@ -347,3 @@
> - * Get the owner clock of the entry
> - */
> -#define GST_CLOCK_ENTRY_CLOCK(entry)    ((entry)->clock)
> 
> Don't remove public API

The problem with keeping it is that it would have to be change to obtain a strong reference, which means that the user of this API would have to unref it.  I would rather remove this broken API, no one should really be using it anyway.

> 
> @@ +558,3 @@
>  gint                    gst_clock_id_compare_func       (gconstpointer id1,
> gconstpointer id2);
>  
> +gboolean                gst_clock_id_is_clock           (GstClockID id,
> GstClock * clock);
> 
> Maybe gst_clock_id_belongs_to_clock()? gst_clock_id_get_clock()?

How about gst_clock_id_uses_clock()?

We could add gst_clock_id_get_clock() as a replacement for GST_CLOCK_ENTRY_CLOCK(), although the semantics would be slightly different.

(In reply to Sebastian Dröge (slomo) from comment #9)
> Yes, I think generally having this patch is a good idea. The elements should
> also be fixed though :)

The question is which element is it?  I know I ran into a crash like this before, but I don't recall if it was my own fault or if it was some gstreamer element.  Maybe Miguel can shed some light.

(In reply to GstBlub from comment #11)
> (In reply to Sebastian Dröge (slomo) from comment #10)
> > Review of attachment 335003 [details] [review] [review] [review]:
> > 
> > ::: gst/gstclock.h
> > @@ -347,3 @@
> > - * Get the owner clock of the entry
> > - */
> > -#define GST_CLOCK_ENTRY_CLOCK(entry)    ((entry)->clock)
> > 
> > Don't remove public API
> 
> The problem with keeping it is that it would have to be change to obtain a
> strong reference, which means that the user of this API would have to unref
> it.  I would rather remove this broken API, no one should really be using it
> anyway.

Deprecate this one (and in #ifndef GST_DISABLE_DEPRECATED guards), and add a function that gives a strong ref :)

> > 
> > @@ +558,3 @@
> >  gint                    gst_clock_id_compare_func       (gconstpointer id1,
> > gconstpointer id2);
> >  
> > +gboolean                gst_clock_id_is_clock           (GstClockID id,
> > GstClock * clock);
> > 
> > Maybe gst_clock_id_belongs_to_clock()? gst_clock_id_get_clock()?
> 
> How about gst_clock_id_uses_clock()?

Ack
 
> We could add gst_clock_id_get_clock() as a replacement for
> GST_CLOCK_ENTRY_CLOCK(), although the semantics would be slightly different.

Yes, add a new function for that and deprecate the macro

(In reply to Sebastian Dröge (slomo) from comment #13)
> (In reply to GstBlub from comment #11)
> > The problem with keeping it is that it would have to be change to obtain a
> > strong reference, which means that the user of this API would have to unref
> > it.  I would rather remove this broken API, no one should really be using it
> > anyway.
> 
> Deprecate this one (and in #ifndef GST_DISABLE_DEPRECATED guards), and add a
> function that gives a strong ref :)

What should the deprecated macro do?  Leak a reference each time it is called, or return an unsafe pointer to the GstClock (essentially temporarily obtain a strong reference, unreffing it again, and returning the raw pointer)?

The same it does now, and potentially crash

Created attachment 335014 [details] [review]
gstclock: Keep weak reference to underlying clock

Updated patch.

(In reply to GstBlub from comment #12)

> The question is which element is it?  I know I ran into a crash like this
> before, but I don't recall if it was my own fault or if it was some
> gstreamer element.  Maybe Miguel can shed some light.

In my case it was my fault because I assumed that keeping the clock ref was not needed.

Review of attachment 335014 [details] [review]:

::: gst/gstclock.h
@@ +394,3 @@
   gint                  refcount;
   /*< protected >*/
+  GWeakRef               clock;

This will still break ABI: everything currently using GST_CLOCK_ENTRY_CLOCK() would need to be recompiled or would do weird things.

You will have to keep this as is, and add the GWeakRef into the padding at the end, e.g.

union {
  gpointer _gst_reserved[GST_PADDING];
  GWeakRef clock;
} ABI;


And then you can also leave the macro above as is

Created attachment 335121 [details] [review]
gstclock: Keep weak reference to underlying clock

Updated the patch once again.

Comment on attachment 335121 [details] [review]
gstclock: Keep weak reference to underlying clock

>--- a/gst/gstclock.c
>+++ b/gst/gstclock.c
>@@ -247,7 +247,10 @@ gst_clock_entry_new (GstClock * clock, GstClockTime time,
>       "created entry %p, time %" GST_TIME_FORMAT, entry, GST_TIME_ARGS (time));
> 
>   entry->refcount = 1;
>+#ifndef GST_DISABLE_DEPRECATED
>   entry->clock = clock;
>+#endif
>+  g_weak_ref_init (&entry->ABI.clock, clock);

Not sure this is right, we should keep setting it for backwards compat. I think you probably wanted a

#ifndef GST_REMOVE_DEPRECATED

here instead (and also in the header).

 
> /**
>+ * gst_clock_id_get_clock:
>+ * @id: a #GstClockID
>+ *
>+ * This function returns the underlying clock.
>+ *
>+ * Returns: (transfer full) (nullable): a #GstClock or %NULL when the
>+ *     underlying clock has been freed.  Unref after usage.
>+ *
>+ * MT safe.

Please add a Since: 1.12  here and for uses_clock()

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/187.