Created attachment 235330 [details] [review]
hack to make notify:parent work

I guess, this one is bad as it temporarily changes the behavior for the class.

Created attachment 235331 [details] [review]
don't take the object lock in deep-notify

Here we don't take the lock (and don't ref). How likely is it that objects get destroying while a property is getting set?

Can you describe how you're using this / why it is needed?

A song in buzztard might not be fully connected at all times. The UI gives a visual clue which elements are active (connected to the sink) by shading them. The engine in buzztard is keeping a track of the graph of the song and the graph in gstreamer. Whenever components are linked/unlinked, subgraphs might get connected/disconneted on the gstreamer side. As a consequece the engine add new connected parts to the running pipeline and removes disconnected parts from it. The UI components simply watch the parent state to know if a component is in the running pipeline or not.

If we can find a acceptable solution, I can always add such a signal on the buzztard api level.

Regarding the gstreamer part - alternatves would be a recursive mutex, or caching the list of parents.
Blocking signal handlers is not enough (as the signal emission is not he issue, but the traversal of the parent hierarchy).

My workaround for the time being :/

if (gst_bin_add (bin, element)) {
  g_object_notify ((GObject *)element, "parent");
}

Review of attachment 235331 [details] [review]:

::: gst/gstobject.c
@@ +451,2 @@
   /* now let the parent dispatch those, too */
+  parent = gst_object->parent;

Maybe just special case the "parent" property here? If it's the parent property and it is for this very specific object, don't take the lock. Otherwise (i.e. also for the parents of the newly parented object) take the lock.

This should be safe as we know that when the parent is changed the lock is taken. However emitting a signal while some object lock is taken is not ideal, it can easily cause deadlocks if the application does something from this signal.

There are two cases. After g_object_notify_queue_thaw() this is called for multiple properties. On a normal g_object_notify() it is called for one pspec.
If n_pspecs==1 and *pspec->name=="parent" we can savely bypass the lock. But what if n_pspecs>1?

We could simply not take the lock here (as in the patch) and say that modifying the parentage from the signal-handlers is not allowed.

Ah I see... not holding the lock in any case if the parent property is notified doesn't seem right though. Other properties could be protected by the object lock too, but the thaw() might've been called without holding the object lock.

Also it's not only the child's object lock we hold here, it's the parent's too, right? Or actually, isn't it just the parent's object lock? Maybe we could unlock that while doing the g_object_notify() and then lock it again? Might make the parent's locking a bit unhappy though...

I think the GstObject.lock should be a GRecMutex. The problem is that gst_object_dispatch_properties_changed() walks a chain of obj->parent->grand_parent->...->root, where we need to avoid to lock parent in some cases.
Now gst_object_set_parent() which should call g_object_notify_by_pspec() does not know whether parent is locked or not.

Should we add a gst_object_set_parent_locked() which would unlock parent and call gst_object_set_parent() - not pretty either :/

And we can't change it to a GRecMutex without breaking ABI unfortunately. Otherwise it currently looks like the only sensible solution to this.


How would a gst_object_set_parent_locked() help here though? I think the problem is more that outside set_parent() a lock is taken, and we don't even know on how many levels of parents it is taken and on which.

Stefan?

I think the best we can do is to add a FIXME-1.99 (or whatever the convention for 2.0 is) + mention that it is broken in the docs.
I have a workaround in my app, so I am not blocked here right now.

What would be the 2.0 solution (FIXME 2.0 IMHO)? Can you do that?

commit 46b18f7a8b5765d934ce6034ef31e2d273264b17
Author: Stefan Sauer <ensonic@users.sf.net>
Date:   Thu Jan 9 07:56:55 2014 +0100

    gstobject: add FIXME and docs for the disabled notify on parent
    
    We haven't found a way to re-enable emitting notify and deep-notify for parent
    changes. Add a FIXME-2.0 and a doc blob on the property. See #693281.

-- 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/34.