GNOME Bugzilla – Bug 742684
aggregator: Locking logic should be reviewed, cleaned up, and documented
Last modified: 2015-02-20 00:42:29 UTC
Playing around with the new GstAggregator code, I get deadlocks with the aggregator thread stuck on I see a macro that does: g_mutex_lock() g_cond_broadcast(); g_mutex_unlock(); This is always always always wrong. Because the g_cond_broadcast() may happen between the time when the actual condition is checked and the time when the g_cond_wait() is called, which results in the wait waiting forever.
Actually, all of the locking around flushing and seeking is quite dodgy. There are a bunch of probably useless atomics, where the same variable is read somewhere else without the atomics. The stream lock is taken in the flush_start and released on a flush_stop.. But after a flush start, there is no guarantee you'll get a flush_stop, you could get a state change to NULL instead! I'm not sure I understand why the duplicated gst_aggregator_pad_steal_buffer() in flush_start ? To make it fail: GST_CHECKS=test_flush_start_flush_stop make elements/audiomixer.forever GST_CHECKS=test_live_seeking make elements/audiomixer.forever
We have run those test for nights and have been very careful in the management of those locking issue so this is for sure a regression in recent code. Where in the code did you detect those issues? Putting that bug as blocker. (In reply to comment #1) > Actually, all of the locking around flushing and seeking is quite dodgy. There > are a bunch of probably useless atomics, where the same variable is read > somewhere else without the atomics. The stream lock is taken in the flush_start > and released on a flush_stop.. But after a flush start, there is no guarantee > you'll get a flush_stop, you could get a state change to NULL instead! This is the case only after a flushing seek... Please give more detailed comments because that "it looks wrong" without any detail is not that useful. > I'm not sure I understand why the duplicated > gst_aggregator_pad_steal_buffer() in flush_start ? To handle the case were we had a queued buffer and a thread blocked in _chain waiting for the first buffer to be unqueued. > To make it fail: > > GST_CHECKS=test_flush_start_flush_stop make elements/audiomixer.forever > GST_CHECKS=test_live_seeking make elements/audiomixer.forever
(In reply to comment #0) > g_mutex_lock() > g_cond_broadcast(); > g_mutex_unlock(); > Broadcasting this way is correct. Are you sure this is the example you wanted to show ?
(In reply to comment #3) > (In reply to comment #0) > > g_mutex_lock() > > g_cond_broadcast(); > > g_mutex_unlock(); > > > > Broadcasting this way is correct. Are you sure this is the example you wanted > to show ? It's probably not correct. When using GCond you always need variables additional to the GCond to tell the waiter(s) what happened and that something has happened. And these variables also have to be protected by the same mutex. Reason for that is that there can also be spurious wakeups without a signal()/broadcast() and also you usually have multiple conditions to check and decide on different code paths after waking up. And g_cond_wait() that is not somehow in a loop like this is suspicious and probably wrong: > g_mutex_lock(); > while (!conditions) > g_cond_wait(); > g_mutex_unlock()
(In reply to comment #4) > > g_mutex_lock(); > > while (!conditions) > > g_cond_wait(); > > g_mutex_unlock() This seems like a better way to pinpoint the problem (stating there is no loop around condition would have made this bug report nicer imho). There exist valid use case for the pattern Olivier used, sometimes you just want to make your waiter spin in order to recheck more complex conditions).
Actually, I though the lack of a loop was the problem, but there is one, it's just well hidden by the fact that the aggregate function is called in a loop. The problem is actually that a bunch of the member variables that are used in multiple threads are sometimes modified with some lock held and sometimes not. And every case where the SRC_STREAM_BROADCAST() macro is used is a case where the variable that is checked before waiting on the GCond has been updated without having the same mutex that is used by the GConf held and is therefore racy. Someone really needs to go over every member variable in GstAggregator and document which mutex protects it (or why it doesn't need it!) The only safe pattern to use a GCond is: thread 1: g_mutex_lock(mutex); update variable ..; /* same mutex used to protect changes to the variable as used when reading it and for the GCond ! */ g_cond_broadcast (cond); g_mutex_unlock(mutex); thread 2: g_mutex_lock (mutex); while (variable) { g_cond_wait (cond, mutex); /* it's allow to unlock and relock here ! */ } g_mutex_unlock (mutex) Also, there are probably too many added mutexes in GstAggregator: the event_lock and "stream_lock" should the the Object lock of the pad, the setcaps lock should be the srcpad stream lock (actually should always be called from the streaming thread!).
(In reply to comment #0) > Playing around with the new GstAggregator code, I get deadlocks with the > aggregator thread stuck on I see a macro that does: > It would be useful to have a test / example that shows these deadlocks, as Thibault said we spent quite a lot of time working on thread safety in there and had very intensive test cases running forever. The code has substantially changed since then, and it would be wise that we rerun these specific tests to ensure there has been no regression, but please share failing cases. > g_mutex_lock() > g_cond_broadcast(); > g_mutex_unlock(); > > This is always always always wrong. Because the g_cond_broadcast() may happen > between the time when the actual condition is checked and the time when the > g_cond_wait() is called, which results in the wait waiting forever. I'm not sure why you're saying this, when the doc specifies : It is good practice to lock the same mutex as the waiting threads while calling this function, though not required.
Made some more progress, it seems the audiomixer tests are racy as they expect the flush_stop to be synchronous, but it's not anymore. Also, the use case where you have a gst_element_release_pad() called in the middle of a flushing seek is not well handled now. Example problematic use-case: - upstream flushing seek and forward it upstream to sink_0 and sink_1 - get a flush start on pad sink_0 - get a flush start on pad sink_1 - get a flush stop pad sink_0 - release pad sink_1 - .. the streaming never restarts because the second flush stop never happens Also, it seems that locking around gst_pad_remove() is not sufficient and then we get memory corruption and or that something ues the pads without taking the element's object lock and don't keep a reference. The investigation continues.
I ended up going through all of the locking in GstAggregator and documenting the locking of every member. This resulted in a rather large number of patches which I'm attaching. I also fixed some races in the audiomixer unit tests.
Created attachment 295222 [details] [review] aggregator: Protect data with the same mutex as GCond Whenever a GCond is used, the safest paradigm is to protect the variable which change is signalled by the GCond with the same mutex that the GCond depends on.
Created attachment 295223 [details] [review] aggregator: Replace event lock with pad's object lock Reduce the number of locks simplify code, what is protects is exposed, but the lock was not. Also means adding an _unlocked version of gst_aggregator_pad_steal_buffer().
Created attachment 295224 [details] [review] aggregator: Protect exported pad members with the pad's object lock
Created attachment 295225 [details] [review] videoaggregator: Lock access to members of GstAggregatorPad Take the pad's object lock before accessing members of the GstAggregatorPad structure.
Created attachment 295226 [details] [review] audiomixer: Don't reset caps on flush A flush event doesn't invalidate the previous caps event.
Created attachment 295227 [details] [review] aggregator: Consistently lock some members Some members sometimes used atomic access, sometimes where not locked at all. Instead consistently use a mutex to protect them, also document that.
Created attachment 295228 [details] [review] audiomixer: Clear GstAudioInfo the the caps When clearing the caps, also clear the matching GstAudioInfo
Created attachment 295229 [details] [review] aggregator: Consistenly lock the flow_return state Use the object's lock to protect it.
Created attachment 295230 [details] [review] audiomixer: Make flush start/stop test non-racy The sender must send a new segment event after a flush stop, so drop buffers in the meantime and then re-send the segment event after a flush.
Created attachment 295231 [details] [review] audiomixer: Avoid race in caps negotiation With the current audiomixer, the input caps need to be the same, otherwise there is an unavoidable race in the caps negotiation. So enforce that using capsfilters
Created attachment 295232 [details] [review] aggregator: Protect the tags with the object lock The tags related variables were sometimes protected, sometimes not and sometimes atomic. Put them all under the object lock.
Created attachment 295233 [details] [review] aggregator: Protect the srcpad caps negotiation with the stream lock Instead of adding another lock, use the srcpad stream lock, which is already taken anyway to push out the new caps if needed.
Created attachment 295234 [details] [review] aggregator: Document locking for gst_aggregator_get_latency_unlocked() Renamed it to _unlocked() to make it clear.
Created attachment 295235 [details] [review] aggregator: Protect all latency related members with the object lock The locking was not consistent, now consistently use the object lock.
Created attachment 295236 [details] [review] aggregator: Document how the segment is protected Document that it can only be accessed with the object lock.
Created attachment 295237 [details] [review] aggregator: Document locking of GstAggregatorPrivate members Most of them are protected by the object lock, specify which ones use a different lock.
Created attachment 295238 [details] [review] audiomixer: Replace racy timeout based tested with drain query Using the drain query, we can be certain that the buffer has done going through the aggregator by taking the stream locks.
Created attachment 295245 [details] [review] audiomixer: Make flush start/stop test non-racy The flush stop could have happened between the source trying to push the segment event and the buffer, this would cause a warning. Prevent that by taking the source's stream lock while flushing.
Review of attachment 295222 [details] [review]: ::: gst-libs/gst/base/gstaggregator.c @@ -700,2 @@ gst_pad_stop_task (self->srcpad); - SRC_STREAM_BROADCAST (self); Why do you remove the broadcasting here?? @@ +847,3 @@ * called */ + SRC_STREAM_LOCK (self); Why do you take the lock so early? I can't see any data you protect between that line and the broadcasting. @@ +994,3 @@ GST_INFO_OBJECT (pad, "Removing pad"); + SRC_STREAM_LOCK (self); Why do you take the lock so early? I can't see any data you protect between that line and the broadcasting. @@ +1763,3 @@ } + SRC_STREAM_LOCK (self); Why did you move it here? @@ +1885,2 @@ PAD_LOCK_EVENT (aggpad); + g_atomic_int_set (&aggpad->priv->flushing, FALSE); Why did you move that here? ->flushing is not protected by the event lock.
Review of attachment 295223 [details] [review]: I really do not think it is a good idea to use GST_OBJECT_LOCK here, moreover we have a cond associated with that lock and I can very well imagine deadlock happening because of that approach. Also, I do not see any reason why you would hide GST_OBJECT_LOCK in a new macro instead of using it directly?
Review of attachment 295224 [details] [review]: ::: gst-libs/gst/base/gstaggregator.c @@ +195,2 @@ aggpad->eos = FALSE; aggpad->priv->flushing = FALSE; Flushing should not need locking and should be exclusively used atomically. @@ +362,3 @@ pad = l->data; + PAD_LOCK (pad); Yeah, crazy that was not done!
Review of attachment 295225 [details] [review]: I see why you used the GST_OBJECT_LOCK to replace the EVENT_LOCK now, but I am really afraid it is dangerous (mainly because that lock is associated with the GCond), we should maybe expose a GST_AGGREGATOR_PAD_LOCK or provide getters for those field and hide them in the Private structure?
Review of attachment 295226 [details] [review]: Are you sure about that? I though it forced a new negotiation, doesn't it?
Review of attachment 295227 [details] [review]: Thanks for rationalize those things! You say you documented that but I can't see the documentation in that commit! :)
Review of attachment 295231 [details] [review]: Isn't there code in audiomixer to avoid the use to requiere to do that? I thought there was one.
Review of attachment 295232 [details] [review]: Looks right
Review of attachment 295233 [details] [review]: I did not think about doing that, it might be right indeed. I think slomo had a failling testcase without that patch so it would be nice of him to tell us if it is still good this way :)
Review of attachment 295234 [details] [review]: I do not like much methods that force the user to held a lock, what is the reason for not doing it in the method itself?
Review of attachment 295235 [details] [review]: OK
Review of attachment 295236 [details] [review]: OK
Comment on attachment 295224 [details] [review] aggregator: Protect exported pad members with the pad's object lock >+ PAD_LOCK (pad); >+ if (pad->buffer == NULL && !pad->eos) { >+ GST_OBJECT_UNLOCK (pad); > goto pad_not_ready; >+ } >+ PAD_UNLOCK (pad); Surely the middle bit should be PAD_UNLOCK as well for consistency? (perhaps it'd be nicer with a temp variable)
Review of attachment 295238 [details] [review]: Sounds like a nice trick indeed :)
Review of attachment 295245 [details] [review]: OK
Mathieu would also like to have another look. The code changed a **lot** since our first version and we did not follow well enough the various changes which indeed lead to many inconsistencies.
Thanks for the thoughtful review, I'll try to answer some of your questions/comments below. I've also ran the libs/aggregator, elements/compositor and elements/audiomixer tests in a loop overnight with no failures. I'm getting a timeout after a number of iterators in the nleoperation test, so I'll investigate that. I think the big open question is if it should use the object lock of the pad of have a private lock for the members of GstAggregatorPad (and for the PAD_EVENT* stuff). If it was a private lock, I think it would be much easier for subclasses if that lock was taken after the object lock, and then make every member of GstAggregatorPad private and have accessors. But that makes subclasses a bit more complex. (In reply to comment #28) > Review of attachment 295222 [details] [review]: > > ::: gst-libs/gst/base/gstaggregator.c > @@ -700,2 @@ > gst_pad_stop_task (self->srcpad); > - SRC_STREAM_BROADCAST (self); > > Why do you remove the broadcasting here?? Because the thing waiting on it is in the streaming thread, which has just been stopped, so there is no point in broadcasting. > @@ +847,3 @@ > * called > */ > + SRC_STREAM_LOCK (self); > > Why do you take the lock so early? I can't see any data you protect between > that line and the broadcasting. The SRC_STREAM_WAIT() waits for one of self->priv->running, self->priv->send_eos, pad->buffer or pad->eos, so I included changes to those in the lock that matches it. > @@ +994,3 @@ > GST_INFO_OBJECT (pad, "Removing pad"); > > + SRC_STREAM_LOCK (self); > > Why do you take the lock so early? I can't see any data you protect between > that line and the broadcasting. Same as the previous one. > @@ +1763,3 @@ > } > > + SRC_STREAM_LOCK (self); > > Why did you move it here? To take it before reading the variables that the matching condition cares about. > @@ +1885,2 @@ > PAD_LOCK_EVENT (aggpad); > + g_atomic_int_set (&aggpad->priv->flushing, FALSE); > > Why did you move that here? ->flushing is not protected by the event lock. Actually, which lock is protecteing aggpad->priv->flushing? I didn't go through the locking off the GstAggregatorPadPrivate structure member, it's not consistently using atomics (not in gst_aggregator_pad_flush). And since it's one of the things that PAD_WAIT_EVENT() depends on, it needs to be protected by the same lock. (In reply to comment #29) > Review of attachment 295223 [details] [review]: > > I really do not think it is a good idea to use GST_OBJECT_LOCK here, moreover > we have a cond associated with that lock and I can very well imagine deadlock > happening because of that approach. There could be deadlocks, and we could certainly revert that to be a separate lock, but in this case. The locking order between this lock and the other locks needs to be defined and documented. It also means that the members of the GstAggregatorPad structure should all be > Also, I do not see any reason why you would hide GST_OBJECT_LOCK in a new macro > instead of using it directly? Because macros were already used everywhere and I assume they had been useful for debugging. (In reply to comment #30) > Review of attachment 295224 [details] [review]: > > ::: gst-libs/gst/base/gstaggregator.c > @@ +195,2 @@ > aggpad->eos = FALSE; > aggpad->priv->flushing = FALSE; > > Flushing should not need locking and should be exclusively used atomically. I'm not sure I understand all of the logic around the flushing, but it seems to work fine.. Except that this function wasn't atomic. And since this is used with the PAD_WAIT_EVENT, I'm not sure if it can actually be atomic and be safe. (In reply to comment #31) > Review of attachment 295225 [details] [review]: > > I see why you used the GST_OBJECT_LOCK to replace the EVENT_LOCK now, but I am > really afraid it is dangerous (mainly because that lock is associated with the > GCond), we should maybe expose a GST_AGGREGATOR_PAD_LOCK or provide getters for > those field and hide them in the Private structure? I wouldn't be unfavorable to hidding everything and using a private lock. As long as this lock's order is after the object lock. (In reply to comment #32) > Review of attachment 295226 [details] [review]: > > Are you sure about that? I though it forced a new negotiation, doesn't it? I though that too, but slomo corrected me. If you look for GST_EVENT_FLUSH_STOP in gstpad.c, it only removes EOS & Segment sticky events. (In reply to comment #33) > Review of attachment 295227 [details] [review]: > > Thanks for rationalize those things! > > You say you documented that but I can't see the documentation in that commit! > :) Ooops, should fix the commit message... (In reply to comment #34) > Review of attachment 295231 [details] [review]: > > Isn't there code in audiomixer to avoid the use to requiere to do that? I > thought there was one. There is no audioresample/audioconvert library yet. But I definitely think that we should aim to do like compositor. (In reply to comment #37) > Review of attachment 295234 [details] [review]: > > I do not like much methods that force the user to held a lock, what is the > reason for not doing it in the method itself? Not a big fan of that either, but subclasses seem to hold the object lock for a long time, so it makes it a bit harder to implement without it.
Bad news, the nleoperation tests run with git master of gst-p-bad segfaults in the compositor's blending function when running test_complex_operations_bis. While it seems that with the patches attached here, the test just times out. Further investigation ongoing.
Bad news indeed, but I can't reproduce the timeout with your branch by running this test, been at it for one hour. HEAD is at 412b71c443029c5ed5873dda5e6a8d83b07cc670 : audiomixer: Replace racy timeout based tested with drain query
Nevermind I reproduced, using GST_CHECk= was not very clever, glad you're investigating that :)
To be more specific, with my HEAD as specified in c47, I reproduce the segfault, that is indeed in the blending code. I've reproduced with some logs, and it seems to happen at the beginning of a new run, ie I see http://www.fpaste.org/174035/12398814/ , new run starting at L36 . Interestingly enough at that moment there's only one sink on the compositor anyway, kind of lame to crash here :) Has me wondering if the problem has anything to do with aggregator anyway, could be a subclass issue, not enough info to determine that but I thought I'd share :)
Created attachment 295427 [details] [review] aggregator: Hide GstAggregatorPad buffer and EOS fileds And add a getter for the EOS. The user should always use the various getters to access those fields
Created attachment 295428 [details] [review] aggregator: Make the PAD_LOCK private Instead of using the GST_OBJECT_LOCK we should have a dedicated mutex for the pad as it is also associated with the mutex on the EVENT_MUTEX on which we wait in the _chain function of the pad. The GstAggregatorPad.segment is still protected with the GST_OBJECT_LOCK.
Created attachment 295429 [details] [review] aggregator: More fixes around locking when accessing protected private fields In some more places we were accessing GstAggregator->segment and GstAggregator->seqnum without holding the GST_OBJECT_LOCK
Created attachment 295442 [details] [review] this brings the aggregator a little closer back to the original design
Created attachment 295458 [details] [review] aggregator: Hide GstAggregatorPad buffer and EOS fileds And add a getter for the EOS. The user should always use the various getters to access those fields
Review of attachment 295428 [details] [review]: Can you add a comment in the GstAggregatorPadPrivate explicitly saying which lock to use.
Review of attachment 295458 [details] [review]: OK, I would not lock the pad in the getter though, cause the subclasses methods should always be called in conditions where EOS won't change, but that *shouldn't* hurt anyway
Review of attachment 295429 [details] [review]: OK :)
Review of attachment 295428 [details] [review]: setting patch status
Created attachment 295466 [details] [review] aggregator: Cleanup locking around of AggregatorPad flush related fields And document the locking
Created attachment 295467 [details] [review] aggregator: Cleanup locking around AggregatorPad flush related fields And document the locking
(In reply to comment #55) > Review of attachment 295428 [details] [review]: > > Can you add a comment in the GstAggregatorPadPrivate explicitly saying which > lock to use. I did that in that last 'aggregator: Cleanup locking around AggregatorPad flush related fields' commit
Should probably squash your patches and mine and remove the _unlocked() variant that no longer makes sense if it's not the same lock. Also need to document the locking order between the GstAggregator pad lock and the GstObject lock of the element and the pad. In the STREAM_LOCK(sinkpad)->STREAM_LOCK(srcpad)->OBJECT_LOCK(element)->OBJECT_LOCK(pad) order, I guess it goes between OBJECT_LOCK(element) and OBJECT_LOCK(pad) ? Which is a bit strange, but oh well.
Not closing as we still need to document the locking order and in case we still have things to discuss but I think locking is looking much better now. Attachment 295222 [details] pushed as 0a2dc11 - aggregator: Protect data with the same mutex as GCond Attachment 295223 [details] pushed as cc3f418 - aggregator: Replace event lock with pad's object lock Attachment 295224 [details] pushed as 68ac643 - aggregator: Protect exported pad members with the pad's object lock Attachment 295225 [details] pushed as ee04b09 - videoaggregator: Lock access to members of GstAggregatorPad Attachment 295226 [details] pushed as 067b44e - audiomixer: Don't reset caps on flush Attachment 295227 [details] pushed as cc605f4 - aggregator: Consistently lock some members Attachment 295228 [details] pushed as a2f1aa3 - audiomixer: Clear GstAudioInfo the the caps Attachment 295229 [details] pushed as eddd5fd - aggregator: Consistenly lock the flow_return state Attachment 295231 [details] pushed as 94e2d78 - audiomixer: Avoid race in caps negotiation Attachment 295232 [details] pushed as 4a5882e - aggregator: Protect the tags with the object lock Attachment 295233 [details] pushed as f7070dc - aggregator: Protect the srcpad caps negotiation with the stream lock Attachment 295234 [details] pushed as 41d2667 - aggregator: Document locking for gst_aggregator_get_latency_unlocked() Attachment 295235 [details] pushed as 9df8ac0 - aggregator: Protect all latency related members with the object lock Attachment 295236 [details] pushed as ea76d39 - aggregator: Document how the segment is protected Attachment 295237 [details] pushed as 93d0b51 - aggregator: Document locking of GstAggregatorPrivate members Attachment 295238 [details] pushed as f98e457 - audiomixer: Replace racy timeout based tested with drain query Attachment 295245 [details] pushed as fb6ba27 - audiomixer: Make flush start/stop test non-racy Attachment 295428 [details] pushed as 1a07467 - aggregator: Make the PAD_LOCK private Attachment 295429 [details] pushed as ccf329d - aggregator: More fixes around locking when accessing protected private fields Attachment 295458 [details] pushed as d8eef43 - aggregator: Hide GstAggregatorPad buffer and EOS fileds Attachment 295467 [details] pushed as 71e4c48 - aggregator: Cleanup locking around AggregatorPad flush related fields
Not a blocker bug any more then. Who's going to document the locking order?
Actually, the code is still wrong, doing this fails after a couple iterations: GST_CHECKS=test_live_seeking GST_DEBUG=*agg*:6 CK_FORK=no G_DEBUG=fatal-warnings,fatal-criticals make elements/audiomixer.forever I think the reason is that in the gst_aggregator_wait_and_check() function, it checks on a bunch of stuff before deciding to wait on the condition variable or the clock. But those things can change without the matching "src_lock" being taken.. I think that the PAD_LOCK and the SRC_LOCK should be merged into one, but they're not next to each other in the locking order, so it's not trivial. I documented the locking order, and I also renamed both locks that had "stream" in their name to something less confusing as they really were not what we called "stream locks" in GStreamer. commit c37e82587c57b28337972c2ea5bee26ce9f9e8e3 Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Feb 18 15:53:53 2015 -0500 aggregator: Document locking order https://bugzilla.gnome.org/show_bug.cgi?id=742684 commit 17df37d8cbbf2593b477a6de6014bf64134a3311 Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Feb 18 15:11:14 2015 -0500 aggregator: Rename confusinly named SRC_STREAM_LOCK macros to SRC_LOCK This will match the name of the lock itself. It is also not a stream lock as it not recursive and not held while pushing. https://bugzilla.gnome.org/show_bug.cgi?id=742684 commit 3a3f2b534377c906a6a5d0fc5e5c4a2f14fac845 Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Feb 18 15:06:01 2015 -0500 aggregator: Rename confusingly named stream lock to flush lock This lock is not what is commonly known as a "stream lock" in GStremer, it's not recursive and it's taken from the non-serialized FLUSH_START event. https://bugzilla.gnome.org/show_bug.cgi?id=742684 commit 36ef8f0bd48586111d9db0ca409f009eda3fa675 Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Feb 18 15:04:04 2015 -0500 aggregator: Fix macro indendation Changes no code https://bugzilla.gnome.org/show_bug.cgi?id=742684
Thanks for renaming some of the locks, I was meaning to do that as well, since it was quite confusing like that.
With this commit, I can't make it fail anymore, so mark it as fixed! commit 97f6b7a8aacc526ada14db8bcbd70008d6fc846b Author: Olivier Crête <olivier.crete@collabora.com> Date: Thu Feb 19 18:30:35 2015 -0500 aggregator: Don't try to push tags while flush seeking The downstream segment could have been flushed already, so need to re-send the segment event before re-sending the tags. https://bugzilla.gnome.org/show_bug.cgi?id=742684
Thanks for the time you spent :)