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 562170 - GstBus watch doesn't work with non-default main context
GstBus watch doesn't work with non-default main context
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
0.10.21
Other Linux
: Normal blocker
: 0.10.22
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2008-11-24 20:17 UTC by Justin Karneges
Modified: 2009-01-17 21:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
possible fix, more or less untested (3.54 KB, patch)
2008-11-25 14:50 UTC, Tim-Philipp Müller
committed Details | Review
Patch fixing GstBusSource inheritance and unit test which fails w/o this patch (3.37 KB, patch)
2009-01-16 19:52 UTC, Tim-Philipp Müller
committed Details | Review

Description Justin Karneges 2008-11-24 20:17:05 UTC
Some events are not delivered to a watcher when using a non-default GMainContext, like this:

GMainContext *mainContext = g_main_context_new();
GMainLoop *mainLoop = g_main_loop_new(mainContext, FALSE);
...
GSource *source = gst_bus_create_watch(bus);
g_source_attach(source, mainContext);
g_source_set_callback(source, (GSourceFunc)cb_bus_call, data, NULL);

Many events are received once the pipeline runs, just not EOS or SEGMENT_DONE.

If, instead, the default context is used, then it works fine:

GMainLoop *mainLoop = g_main_loop_new(NULL, FALSE);
GMainContext *mainContext = g_main_loop_get_context(mainLoop);
...
GSource *source = gst_bus_create_watch(bus);
g_source_attach(source, mainContext);
g_source_set_callback(source, (GSourceFunc)cb_bus_call, data, NULL);

The callback will receive all of the other events as usual, but this time EOS and SEGMENT_DONE will also be delivered too.

An extensive workaround is to write your own GSource, which is described here: http://article.gmane.org/gmane.comp.video.gstreamer.devel/21665

I currently use the above method with success, but probably GStreamer should be fixed too.
Comment 1 Tim-Philipp Müller 2008-11-25 13:47:58 UTC
I think this is a known issue of sorts - we only wake up the default main context when a message is posted on the bus. It's a bit tricky to know which contexts to wake up, since the source doesn't know when it gets attached to a main context, or which, so we have to query and keep track of that actively ourselves. I'll have a look at this.
Comment 2 Tim-Philipp Müller 2008-11-25 14:50:03 UTC
Created attachment 123365 [details] [review]
possible fix, more or less untested

Does this help at all?
Comment 3 Justin Karneges 2008-11-25 17:40:42 UTC
I patched this on 10.21 which took hand-correction, and it doesn't work.  I can try to get a CVS build set up to test, if that would make any difference.

For your own testing purposes, it is very easy to make the two line mainContext initialization change to one of the example playback programs and see whether you still get EOS or not.

Thanks for looking at this.  When I examined the patch and saw the GMainContext in global space, plus the "FIXME"s, it is clear the existing code is quite wrong. :)
Comment 4 Tim-Philipp Müller 2008-11-25 21:11:28 UTC
Okay, guess I'll have to go and actually test this with a non-default context then :)


> An extensive workaround is to write your own GSource, which is described here:
> http://article.gmane.org/gmane.comp.video.gstreamer.devel/21665
> 
> I currently use the above method with success, but probably GStreamer should be
> fixed too.

I think the reason this seems to work correctly is that the source_prepare function implicitly sets/keeps *timeout=0, which means 'I'm ready to be dispatched now'. You could probably achieve the same effect by adding an idle or timeout source to your main context and use the standard GstBusSource.
Comment 5 Wim Taymans 2008-11-25 21:15:38 UTC
Why not simply start a thread that pops and dispatches messages? why does it have to be a mainloop?
Comment 6 Justin Karneges 2008-11-25 23:16:23 UTC
(In reply to comment #5)
> Why not simply start a thread that pops and dispatches messages? why does it
> have to be a mainloop?

Yes, maybe that's a better workaround than the one I found on gstreamer-devel.

But I think the natural way to use GstBus is with a mainloop, right?
Comment 7 Tim-Philipp Müller 2008-12-27 17:47:30 UTC
I've committed this now, along with a unit test:

 2008-12-27  Tim-Philipp Müller  <tim.muller at collabora co uk>

	* gst/gstbus.c: (gst_bus_dispose), (gst_bus_get_property),
	  (gst_bus_wakeup_main_context), (gst_bus_set_main_context),
	  (gst_bus_post), (gst_bus_source_prepare), (gst_bus_source_finalize),
	  (gst_bus_create_watch):
	  Make GstBusSource work with non-default main contexts (#562170).

	* tests/check/gst/gstbus.c: (message_func_eos), (message_func_app),
	  (test_watch), (test_watch_with_custom_context), (gst_bus_suite):
	  Add test case for GstBusSource with a non-default main context.


> I patched this on 10.21 which took hand-correction, and it doesn't work. I
> can try to get a CVS build set up to test, if that would make any difference.

Are you sure your application was actually using the patched libgstreamer? Maybe you could attach a GST_DEBUG=GST_BUS:5 log?


> But I think the natural way to use GstBus is with a mainloop, right?

Using a main loop only makes sense if there are multiple event sources being watched (e.g. GStreamer bus, Gtk+/X events etc.). If you're only ever waiting for GstBus messages anyway, you may just as well use gst_bus_*pop* directly.
Comment 8 Tim-Philipp Müller 2009-01-16 19:52:25 UTC
Created attachment 126601 [details] [review]
Patch fixing GstBusSource inheritance and unit test which fails w/o this patch

Rookie mistake: GstBusSource didn't inherit from GSource directly, which overwrites bussource->inited when GLib initialises what it thinks is the GSource member at the very beginning of the struct. This leads to non-default main contexts not working in some or all circumstances (no idea why it's not triggered with the existing unit test).

This should most likely go into the release, even though I don't think it actually causes bad memory access of any kind.

Patch also contains a unit test which demonstrates the bug (hangs until timeout because the custom context is never woken up when the eos message is posted on the bus, because it's not set, which is because bsrc->inited is already non-zero as described above).
Comment 9 Justin Karneges 2009-01-16 21:16:11 UTC
confirmed patch works
Comment 10 Jan Schmidt 2009-01-17 20:35:22 UTC
Agree this should go in... please commit.
Comment 11 Tim-Philipp Müller 2009-01-17 21:06:45 UTC
Committed, thanks! (And thanks for the test case Justin!)

 2009-01-17  Tim-Philipp Müller  <tim.muller at collabora co uk>

	* gst/gstbus.c: (gst_bus_set_main_context), (gst_bus_create_watch):
	  Fix order of members in GstBusSource structure - the first member
	  must be the parent structure ie. GSource. Should make bus sources
	  attached to non-default main contexts work in all cases now (ie.
	  primarily in cases where the callback has a non-NULL user data
	  argument). Fixes #562170.

	* tests/check/gst/gstbus.c: (test_custom_main_context):
	  Add unit test for the above, based on code by
	  Justin Karneges <justin at affinix com>.