GNOME Bugzilla – Bug 781673
aggregator: always handle synchronized events on output thread
Last modified: 2017-05-20 14:29:06 UTC
Instead of handling synchronized events sometimes on the input threads and sometimes on the output thread, always handle them on the output thread. This required fixing a couple things that were broken by this, but it should make the code more simple and help with #776931 and #780682
Created attachment 350329 [details] [review] aggregator: Only declare first buffer on actual buffer The function needs to be unlocked if any data is received, but only end the first buffer processing on an actual buffer. Not on synchronized events.
Created attachment 350330 [details] [review] aggregator: Simplify clip function The return value was ignored anyway
Created attachment 350331 [details] [review] tests: Test caps using query Sending an event can accepted event if the caps were rejected because the event could be queued and processed later.
Created attachment 350332 [details] [review] aggregator: Only count buffers when declaring queue full
Created attachment 350333 [details] [review] aggregator: Make pad eos as soon as all buffers are processed, dont way for events
Created attachment 350334 [details] [review] aggregator: Delay clipping to output thread This is required because the synchronized events like caps or segments may only be processed on the output thread.
Created attachment 350335 [details] [review] aggregator: Always handle sync'ed events on output thread Having all synchronized events always be handled on the output thread should make synchronization easier.
*** Bug 779945 has been marked as a duplicate of this bug. ***
Go ahead, would be good to get this in as early as possible in the 1.14 release cycle now.
Created attachment 351489 [details] [review] aggregator: Fix indentation
Created attachment 351490 [details] [review] aggregator: Reset pad on init Factor out the pad reset code from the flushing and use it on init as well
Created attachment 351491 [details] [review] aggregator: Reset the pad's first buffer flag with the rest There is not reason to have separate code to reset this one.
Created attachment 351492 [details] [review] aggregator: Set initial position on first buffer Set the initial position on the first buffer, otherwise the queue will grow without limits before the output thread is started.
Created attachment 351493 [details] [review] aggregator: Only declare first buffer on actual buffer The function needs to be unlocked if any data is received, but only end the first buffer processing on an actual buffer, synchronized events don't matter on the first buffer processing.
Created attachment 351494 [details] [review] aggregator: Simplify clip function The return value was ignored anyway
Created attachment 351495 [details] [review] tests: Test caps using query Sending an event can accepted event if the caps were rejected because the event could be queued and processed later.
Created attachment 351496 [details] [review] aggregator: Only count buffers when declaring queue full
Created attachment 351497 [details] [review] aggregator: Make pad eos as soon as all buffers are processed, dont way for events
Created attachment 351498 [details] [review] aggregator: Delay clipping to output thread This is required because the synchronized events like caps or segments may only be processed on the output thread.
Created attachment 351499 [details] [review] aggregator: Always handle sync'ed events on output thread Having all synchronized events always be handled on the output thread should make synchronization easier.
While re-testing this, I found some bugs I had introduced, any kind of review would be welcome otherwise I'll push this as-is
Review of attachment 351490 [details] [review]: . ::: gst-libs/gst/base/gstaggregator.c @@ +246,3 @@ aggpad->priv->tail_time = GST_CLOCK_TIME_NONE; aggpad->priv->time_level = 0; + aggpad->priv->first_buffer = TRUE; This line is different in the refactor. Is that intentional?
Review of attachment 351491 [details] [review]: This makes sense with the previous comment and the changed line should be moved to this patch.
Review of attachment 351492 [details] [review]: Makes sense.
Review of attachment 351493 [details] [review]: This seems to make sense. ::: gst-libs/gst/base/gstaggregator.c @@ +444,3 @@ + if (pad->priv->num_buffers == 0) { + if (g_queue_get_length (&pad->priv->buffers) != 0) Maybe a premature optimization but could be replaced with gst_aggregator_pad_queue_is_empty (pad) like in the original code. @@ +486,3 @@ GST_LOG_OBJECT (pad, "pad not ready to be aggregated yet"); GST_OBJECT_UNLOCK (self); + return have_event; The log message is not quite right here anymore
Review of attachment 351494 [details] [review]: Looks sane. ::: gst-libs/gst/base/gstaggregator.h @@ +159,3 @@ + * clipping it and translating it to the current segment falls + * on the subclass. The function should use the segment of data + * and the negotiated media type on the pad to perform double space! :)
Review of attachment 351495 [details] [review]: Ok.
Review of attachment 351496 [details] [review]: Ok.
Review of attachment 351497 [details] [review]: Ok.
Review of attachment 351498 [details] [review]: This seems mostly sane. I'm not exactly sure about the rationale for storing in the clipped_buffer unless ->clip() is not idempotent and/or not deterministic. ::: gst-libs/gst/base/gstaggregator.c @@ +2494,3 @@ + + if (self == NULL) { + self = GST_AGGREGATOR (gst_pad_get_parent_element (GST_PAD (pad))); Why is this inside the loop? Will the pad get a different parent while the PAD_LOCK is held?
Review of attachment 351499 [details] [review]: Ok.
Created attachment 352207 [details] [review] aggregator: Fix indentation
Created attachment 352208 [details] [review] aggregator: Reset pad on init Factor out the pad reset code from the flushing and use it on init as well
Created attachment 352209 [details] [review] aggregator: Reset the pad's first buffer flag with the rest There is not reason to have separate code to reset this one.
Created attachment 352210 [details] [review] aggregator: Set initial position on first buffer Set the initial position on the first buffer, otherwise the queue will grow without limits before the output thread is started.
Created attachment 352211 [details] [review] aggregator: Only declare first buffer on actual buffer The function needs to be unlocked if any data is received, but only end the first buffer processing on an actual buffer, synchronized events don't matter on the first buffer processing.
Created attachment 352212 [details] [review] aggregator: Simplify clip function The return value was ignored anyway
Created attachment 352213 [details] [review] tests: Test caps using query Sending an event can accepted event if the caps were rejected because the event could be queued and processed later. Also send a drain query in the caps test to make sure that the event has been processed.
Created attachment 352214 [details] [review] aggregator: Only count buffers when declaring queue full
Created attachment 352215 [details] [review] aggregator: Make pad eos as soon as all buffers are processed, dont way for events
Created attachment 352216 [details] [review] aggregator: Delay clipping to output thread This is required because the synchronized events like caps or segments may only be processed on the output thread.
Created attachment 352217 [details] [review] aggregator: Always handle sync'ed events on output thread Having all synchronized events always be handled on the output thread should make synchronization easier.
Review of attachment 352212 [details] [review]: Feel free to push after the doc modifications. ::: gst-libs/gst/base/gstaggregator.h @@ +158,3 @@ + * Called when a buffer is received on a sink pad, the task of + * clipping it and translating it to the current segment falls + * on the subclass. The function should use the segment of data s/ of// ? @@ +162,3 @@ + * clipping of inbuffer. This function takes ownership of + * buf and should output a buffer or return NULL in + * if the buffer should be dropped. s/ in//
Review of attachment 352215 [details] [review]: Typo in commit message: s/way/wait/ Can push once fixed.
Merged: commit d2335bce9b96c95edeb4bafdabcf4ba39def8ade Author: Olivier Crête <olivier.crete@collabora.com> Date: Sat May 14 15:52:37 2016 +0200 aggregator: Always handle sync'ed events on output thread Having all synchronized events always be handled on the output thread should make synchronization easier. https://bugzilla.gnome.org/show_bug.cgi?id=781673 commit 4cec1925e3768227150f82541b12ff61d4537e50 Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Jul 6 16:39:17 2016 -0400 aggregator: Delay clipping to output thread This is required because the synchronized events like caps or segments may only be processed on the output thread. https://bugzilla.gnome.org/show_bug.cgi?id=781673 commit a6710944e81016fdee3a2000c5d98ca06d5d3f24 Author: Olivier Crête <olivier.crete@collabora.com> Date: Thu Jul 7 16:13:57 2016 -0400 aggregator: Make pad eos as soon as all buffers are processed, dont way for events https://bugzilla.gnome.org/show_bug.cgi?id=781673 commit 6e96bebd5bd20ceb8d426958715d539dde67770e Author: Olivier Crête <olivier.crete@collabora.com> Date: Thu Jul 7 11:47:40 2016 -0400 aggregator: Only count buffers when declaring queue full https://bugzilla.gnome.org/show_bug.cgi?id=781673 commit c0849df4aca6b51cc8c79f502bfcdb2171a377f2 Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Jul 6 17:28:11 2016 -0400 tests: Test caps using query Sending an event can accepted event if the caps were rejected because the event could be queued and processed later. Also send a drain query in the caps test to make sure that the event has been processed. https://bugzilla.gnome.org/show_bug.cgi?id=781673 commit 4d408ea9205aedea1b51f571b89ee0cca339315a Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Jul 6 16:41:44 2016 -0400 aggregator: Simplify clip function The return value was ignored anyway https://bugzilla.gnome.org/show_bug.cgi?id=781673 commit 20aee5f5757e2b000d8e55bd1fb5386548526749 Author: Olivier Crête <olivier.crete@collabora.com> Date: Sun May 15 16:04:58 2016 +0300 aggregator: Only declare first buffer on actual buffer The function needs to be unlocked if any data is received, but only end the first buffer processing on an actual buffer, synchronized events don't matter on the first buffer processing. https://bugzilla.gnome.org/show_bug.cgi?id=781673 commit 1ab33d78feb5d27adf229688ea56b92ce4c40222 Author: Olivier Crête <olivier.crete@collabora.com> Date: Tue May 9 20:20:07 2017 -0400 aggregator: Set initial position on first buffer Set the initial position on the first buffer, otherwise the queue will grow without limits before the output thread is started. https://bugzilla.gnome.org/show_bug.cgi?id=781673 commit 82f7b00ea38a6f83d3eb5d9405d25460dacf51e3 Author: Olivier Crête <olivier.crete@collabora.com> Date: Tue May 9 20:06:29 2017 -0400 aggregator: Reset the pad's first buffer flag with the rest There is not reason to have separate code to reset this one. https://bugzilla.gnome.org/show_bug.cgi?id=781673 commit 05374aa22cf730cd876cfb3f127be71ea9271b48 Author: Olivier Crête <olivier.crete@collabora.com> Date: Tue May 9 20:05:55 2017 -0400 aggregator: Reset pad on init Factor out the pad reset code from the flushing and use it on init as well https://bugzilla.gnome.org/show_bug.cgi?id=781673 commit d722e064fd1c3222eb34595ad535a8f9cee10792 Author: Olivier Crête <olivier.crete@collabora.com> Date: Tue May 9 20:13:58 2017 -0400 aggregator: Fix indentation https://bugzilla.gnome.org/show_bug.cgi?id=781673
Comment on attachment 352215 [details] [review] aggregator: Make pad eos as soon as all buffers are processed, dont way for events Fixed and merged
Comment on attachment 352212 [details] [review] aggregator: Simplify clip function Fixed and merged