GNOME Bugzilla – Bug 784414
ges-timeline-element: emit 'deep-notify' signal in main thread
Last modified: 2017-07-03 18:37:18 UTC
Created attachment 354756 [details] [review] Patch which ensures deep-notify signals get emitted on the main thread. We should make sure that 'deep-notify' signal gets emitted in the main thread, as other components that connect to it might need to get the callback in the main thread (for Gtk calls for example).
Others might want the notification synchronously though.. You might also be interested in the recently-added gst_element_add_property_deep_notify_watch() which posts messages on the pipeline bus for deep notify signals, messages which you'd then typically pick up from the main thread.
GES is suposdely not multi threaded, I think it makes sense that all our API is happening in the main thread/thread used by GES. Requiring the user handle it on the bus message doesn't seem appropriate here, and gst_element_add_property_deep_notify_watch doesn't allow us to handle it for pad properties. I think simply marshalling in the main maincontext thread is the way to go here.
Created attachment 354758 [details] [review] Updated patch. Emits 'deep-notify' right away if in main thread.
Fair enough, but it will work for pad properties, you just have to set it up on an element (setting it up on a pad doesn't make sense because then you could just use a normal notify signal, there's no 'deep' for the pad after all). It's used in gst-launch-1.0 fwiw.
(In reply to Tim-Philipp Müller from comment #4) > Fair enough, but it will work for pad properties, you just have to set it up > on an element (setting it up on a pad doesn't make sense because then you > could just use a normal notify signal, there's no 'deep' for the pad after > all). It's used in gst-launch-1.0 fwiw. Right, but in GES we have a different notion of deep-notifie for GESTimelineElement, were we do it for only whitelisted properties.
Review of attachment 354758 [details] [review]: Apart from that, looks good. ::: ges/ges-timeline-element.c @@ +1305,3 @@ + + data->child = gst_object_ref (child); + return; Does that work?! It should be g_param_spec_ref/g_param_spec_unref as GParamSpec is not a GObject
Created attachment 354761 [details] [review] Updated patch. Made the required changes.
Review of attachment 354761 [details] [review]: This breaks the following tests: check.gst-editing-services.ges_basic.test_ges_timeline_add_layer: Failed 'Application returned 1' check.gst-editing-services.ges_basic.test_ges_timeline_add_layer_first: Failed 'Application returned 1' check.gst-editing-services.ges_basic.test_ges_timeline_remove_track: Failed 'Application returned 1' check.gst-editing-services.ges_basic.test_ges_scenario: Failed 'Application returned 1' check.gst-editing-services.ges_basic.test_ges_timeline_multiple_tracks: Failed 'Application returned 1' check.gst-editing-services.ges_clip.test_clip_group_ungroup: Failed 'Application returned 1' check.gst-editing-services.ges_clip.test_split_object: Failed 'Application returned 1' check.gst-editing-services.ges_timelineedition.test_timeline_edition_mode: Failed 'Application returned 1' check.gst-editing-services.ges_timelineedition.test_snapping: Failed 'Application returned 1' Example of the issue: ================= Test name: check.gst-editing-services.ges_basic.test_ges_timeline_multiple_tracks Command: '/home/thiblahute/devel/gstreamer/gst-master/build/subprojects/gst-editing-services/tests/check/ges_basic' ================= Running suite(s): ges-basic 0%: Checks: 1, Failures: 1, Errors: 0 ../subprojects/gst-editing-services/tests/check/ges/basic.c:617:F:basic:test_ges_timeline_multiple_tracks:0: trackelement (0x5581c028acf0) refcount is 5 instead of 4
I also think g_idle_add is wrong, as Thibault just mention GES can run on another GMaintContext/Thread. Use g_idle_source_new instead and attach it to the thread default context.
(In reply to Nicolas Dufresne (stormer) from comment #9) > I also think g_idle_add is wrong, as Thibault just mention GES can run on > another GMaintContext/Thread. Use g_idle_source_new instead and attach it to > the thread default context. Well, that would run the callback from the thread where it is emited, really not what we want :-) Most logical thing to handle that case would be to use the default MainContext from the thread where GES has been initialized, but I would say it is not mandatory for now.
I meant GES GMainContext of course. (In reply to Thibault Saunier from comment #2) > GES is suposdely not multi threaded, I think it makes sense that all our API > is happening in the main thread/thread used by GES. > Here you open the door that GES could be running in non-default context (a second context). Using g_idle_add will always reach the default context, that's what I'm saying. If you don't support that, then fine (just like GTK).
(In reply to Nicolas Dufresne (stormer) from comment #11) > I meant GES GMainContext of course. We do not have that concept (yet), if we have a case where it is needed, I think we should had it at that point. I am under the impression that for android we might need it, but I do not see anything blocking us from doing it later when actually needed.
Created attachment 354830 [details] [review] ges_timeline_element::deep-notify emitted on the main thread Fixed tests broken by the previous patch.
Review of attachment 354830 [details] [review]: I guess the same is true with the other tests, please check. ::: tests/check/ges/basic.c @@ -101,0 +113,11 @@ + + /* Let the main loop run so the "deep-notify" signal gets emitted properly. + * We need to do this as "deep-notify" signals get emitted only in the main ... 8 more ... Why does the callback get emited from a GSource and not directly in that test? Check the stack: (gdb) t a a bt
+ Trace 237609
Thread 1 (Thread 0x7ffff7f8c740 (LWP 8868))
We are in the main thread when the element property changed and we should be emitting from the main thread and not on an idle GSource here, please check why it is not the case and do not change the testcase as it should not be needed :)
+ Trace 237610
Created attachment 354844 [details] [review] ges_timeline_element::deep-notify emitted only on the main thread Fixed the condition which checked if we are in the main thread.