GNOME Bugzilla – Bug 735195
bus: Missing API to remove a bus watch
Last modified: 2014-10-14 08:46:05 UTC
When adding a bus watch, using gst_bus_add_watch(), to a non default main context, there are no API to remove this watch. The only reference to this watch is it's id returned by g_source_attach(). With an id it's only possible to remove sources from the default main context. I think an API should be exposed to make it possible to use the feature of adding watches to something that is not the default main context, using gst_bus_add_watch(). Or remove that possiblity.
gst_bus_add_watch() only adds the watch to the default main context, so you remove it with g_source_remove(). If you want to add a watch to non-default main context, you need to create it with gst_bus_create_watch(), then attach it with g_source_attach(). If you want to remove it, you use g_source_destroy(). And then to not leak it, you unref it with g_source_unref() as you were given a new reference by gst_bus_create_watch().
> gst_bus_add_watch() only adds the watch to the default main context, (...) > > If you want to add a watch to non-default main context, you need to create it > with gst_bus_create_watch(), then attach it with g_source_attach(). Not quite, see the documentation for gst_bus_add_watch() at http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstBus.html#gst-bus-add-watch
Created attachment 284151 [details] [review] gst_bus_add_watch can return 0 if a watch is already attached.
Damn, you are fast with replies. I made a patch that introduces a remove function for bus watches. I don't know if it's a good idea or not, but I'll attach it soon.
Created attachment 284152 [details] [review] API function to remove a bus watch Maybe a function looking something like this could be useful.
Ohh, I didn't notice we had the clever idea of making our thing non-standard... Sorry.
Not sure what you mean, it is (or at least was) the new standard...
In GLib, all of the thing that add a GSource to a mainloop and return a id all deal with the global default main loop (_idle_add, g_timeout_add, etc). They all ignore the thread local main loop. In GIO, to add to a specific main loop, a _create_source() function is used and the GSource is manipulated directly.
Olivier, g_main_context_get_thread_default() is widely used in gio, and was added for it. (Anyway...) Linus, does this already fix your issues? commit b1f23598099da82835df60bc2fb09501233f9118 Author: Philippe Normand <philn@igalia.com> Date: Wed Aug 13 14:12:00 2014 +0200 bus: destroy signal watch from the context it was mapped to Don't rely on g_source_remove() because it operates on the main context. If a signal watch was added to a new thread-default context g_source_remove() would have no effect. So simply use g_source_destroy() to avoid this problem. Additionally the source_id was removed from GstBusPrivate because it was redundant with the signal watch GSource also stored in that structure. https://bugzilla.gnome.org/show_bug.cgi?id=734716
(In reply to comment #8) > In GLib, all of the thing that add a GSource to a mainloop and return a id all > deal with the global default main loop (_idle_add, g_timeout_add, etc). They > all ignore the thread local main loop. In GIO, to add to a specific main loop, > a _create_source() function is used and the GSource is manipulated directly. Yes, to not break backwards compatibility.
commit b0fff643dcbec1a7018e10ea2763dcc4ebd378cf Author: Linus Svensson <linusp.svensson@gmail.com> Date: Tue Jul 15 16:06:49 2014 +0200 bus: gst_bus_add_watch() can return 0 on error Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=735195
Is anything else left to be done here? The other patch that adds API does not seem right to me.
Created attachment 286732 [details] [review] Removed id from the remove API Perhaps you like this better. I think there should be some sort of API to remove bus watches that was added to a thread default main context.
Comment on attachment 286732 [details] [review] Removed id from the remove API Seems ok to me, any other opinions? :)
Review of attachment 286732 [details] [review]: ::: gst/gstbus.c @@ +977,3 @@ + watch_id = bus->priv->signal_watch; + + GST_OBJECT_UNLOCK (bus); You'll have to set priv->signal_watch to NULL here before unlocking
(In reply to comment #0) > When adding a bus watch, using gst_bus_add_watch(), to a non default main > context, there are no API to remove this watch. The only reference to this > watch is it's id returned by g_source_attach(). With an id it's only possible > to remove sources from the default main context. Just a correction: tmc = g_main_context_get_thread_default (); source = g_main_context_find_source_by_id (sid); g_source_unref (source); > > I think an API should be exposed to make it possible to use the feature of > adding watches to something that is not the default main context, using > gst_bus_add_watch(). Or remove that possiblity. I do think that GLib API isn't very friendly with custom thread context. Still, I'm myself in favour of adding a friendlier API.
* obviously, I'm missing a parameter to find_source_by_id, and missing a call to g_source_remote(), but I think you got the point.
(In reply to comment #15) > You'll have to set priv->signal_watch to NULL here before unlocking It looks like that is already taken care of in gst_bus_source_finalize. If you look at gst_bus_remove_signal_watch, priv->signal_watch is not set to NULL. Shouldn't gst_bus_remove_watch and gst_bus_remove_signal_watch should be implemented in the same manner?
Created attachment 288283 [details] [review] A testcase for removing a bus watch I implemented a unit test for testing gst_bus_remove_watch. Two GST_ERROR message are printed when running the testcase (when trying to remove non-existing bus watches). I don't know if that's a problem.
commit bf8e36a7685a34a878276b5b463645dffebfdba7 Author: Linus Svensson <linusp.svensson@gmail.com> Date: Sat Oct 11 19:28:21 2014 +0200 tests: Add a test for removing a bus watch https://bugzilla.gnome.org/show_bug.cgi?id=735195 commit c8b512d2f0a468eba940bede3c7d9ce485a2176a Author: Linus Svensson <linusp.svensson@gmail.com> Date: Tue Aug 19 23:28:52 2014 +0200 bus: Add a function to remove a bus watch If a bus watch is added to the non default main context it's not possible to remove it using g_source_remove(). Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=735195