GNOME Bugzilla – Bug 779945
aggregator: Always call sink_event() from aggregate thread
Last modified: 2017-05-09 12:49:12 UTC
Currently aggregator has this curious code in the sink event handler > if (!gst_aggregator_pad_queue_is_empty (aggpad) && > GST_EVENT_TYPE (event) != GST_EVENT_FLUSH_STOP) { > GST_DEBUG_OBJECT (aggpad, "Store event in queue: %" GST_PTR_FORMAT, > event); > g_queue_push_head (&aggpad->priv->buffers, event); > event = NULL; > SRC_BROADCAST (self); > } If the event is not queued up, it will be directly handled from the event thread. This seems a bit suboptimal as it means that sink_event() will be called from different threads, possible at the same time. At least for all serialized events (except for FLUSH_STOP) we should always queue them up and call them from the aggregate thread IMHO. Easy enough to change, but there probably was a reason for this?
It seems like such a change would reduce a few potential race conditions.
Created attachment 347759 [details] [review] aggregator: Always handle serialized events from the aggregate thread Previously we were handling them from their own streaming thread whenever the queue of the pad was empty, and otherwise queued them up for later handling. This possibly caused the sink_event() vfunc to be called simultaneously from different threads and possibly resulted in race conditions.
Comment on attachment 347759 [details] [review] aggregator: Always handle serialized events from the aggregate thread This breaks the compositor unit test though
Olivier had the exact some change here: https://git.collabora.com/cgit/user/tester/gst-plugins-bad.git/log/?h=agg-neg-2. Sounds like a good idea :).
You want to first get the change to the clipping, look at the other patches before this one in the same branch. I just rebased it over master.
I think the main reason I had not pushed those patches is that the end goal of this branch is to change the way aggregator handles caps, moving it all to the output thread, then add propose_allocation() and decide_allocation(). And I never got to the point where those were mergeable.
*** This bug has been marked as a duplicate of bug 781673 ***