After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 693281 - gstobject: re-enable "notify::parent"
gstobject: re-enable "notify::parent"
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 2.x
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-02-06 21:46 UTC by Stefan Sauer (gstreamer, gtkdoc dev)
Modified: 2018-11-03 12:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
hack to make notify:parent work (1.86 KB, patch)
2013-02-06 21:47 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
rejected Details | Review
don't take the object lock in deep-notify (2.12 KB, patch)
2013-02-06 21:49 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
rejected Details | Review

Description Stefan Sauer (gstreamer, gtkdoc dev) 2013-02-06 21:46:05 UTC
Two attempts to make it possible to listen to changes in parentage.
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2013-02-06 21:47:39 UTC
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.
Comment 2 Stefan Sauer (gstreamer, gtkdoc dev) 2013-02-06 21:49:19 UTC
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?
Comment 3 Tim-Philipp Müller 2013-02-06 22:36:14 UTC
Can you describe how you're using this / why it is needed?
Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2013-02-07 06:35:46 UTC
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).
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2013-02-15 17:36:15 UTC
My workaround for the time being :/

if (gst_bin_add (bin, element)) {
  g_object_notify ((GObject *)element, "parent");
}
Comment 6 Sebastian Dröge (slomo) 2013-07-24 09:26:38 UTC
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.
Comment 7 Stefan Sauer (gstreamer, gtkdoc dev) 2013-07-26 06:23:19 UTC
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.
Comment 8 Sebastian Dröge (slomo) 2013-07-26 06:51:09 UTC
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...
Comment 9 Stefan Sauer (gstreamer, gtkdoc dev) 2013-07-28 18:49:06 UTC
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 :/
Comment 10 Sebastian Dröge (slomo) 2013-07-29 06:31:21 UTC
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.
Comment 11 Sebastian Dröge (slomo) 2014-01-07 16:33:57 UTC
Stefan?
Comment 12 Stefan Sauer (gstreamer, gtkdoc dev) 2014-01-07 17:08:04 UTC
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.
Comment 13 Sebastian Dröge (slomo) 2014-01-08 09:11:06 UTC
What would be the 2.0 solution (FIXME 2.0 IMHO)? Can you do that?
Comment 14 Stefan Sauer (gstreamer, gtkdoc dev) 2014-01-09 12:10:00 UTC
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.
Comment 15 GStreamer system administrator 2018-11-03 12:17:07 UTC
-- 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.