[Bug 742684] audiomixer: Un-even locking in GstAggregator causes the tests to fail

Fri Jan 23 13:51:09 PST 2015

https://bugzilla.gnome.org/show_bug.cgi?id=742684
  GStreamer | gst-plugins-bad | git master

--- Comment #45 from Olivier Crête <olivier.crete at ocrete.ca> 2015-01-23 21:51:02 UTC ---
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]:
> 
> ::: 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]:
> 
> 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]:
> 
> ::: 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]:
> 
> 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]:
> 
> 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]:
> 
> 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]:
> 
> 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]:
> 
> 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.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.