Created attachment 323401 [details]
level 6 debug log

Created attachment 323402 [details]
pipeline graph

The pipeline graph is not very useful. Would be better to get the one when the pipeline stalled. This now is before deinterleave received any buffer at all :)

This pipeline never creates any buffer it seems.

the pipeline already fails with two or more downstream connections (a single one works), i.e. that will stall already

gst-launch-1.0 -e \
  audiotestsrc is-live=TRUE ! "audio/x-raw,rate=48000,channels=8" ! queue ! deinterleave name=deinter \
  deinter.src_0 ! queue ! fakesink \
  deinter.src_1 ! queue ! fakesink

I am updating the debug log with the output of this pipeline, might be easier to read with less elements in the play.

Created attachment 323404 [details]
level 6 debug log

Created attachment 323405 [details]
pipeline graph

The audiotestsrc task is blocked by waiting for the ALLOCATION query to return from the queue. And the queue task seems to be busy forever with caps query things.

<slomo> put a tee before deinterleave
still stalls, but less often

<slomo> add a tee after audiotestsrc directly?
still stalls, but less often

not using GST_DEBUG_DUMP_DOT_DIR in the environment also makes it less likely to hang (but still happens)

Created attachment 323408 [details]
gdb stacktrace of stalled pipeline

Created attachment 323409 [details]
gdb stacktrace of stalled pipeline (full)

Ok so it's a deadlock

Created attachment 324129 [details] [review]
0001-deinterleave-Use-GstIterator-for-iterating-all-pads-.patch

Created attachment 324130 [details] [review]
0002-deinterleave-Return-the-current-caps-on-the-srcpads-.patch

The second patch is not actually needed but simplifies all the caps query logic a bit.

Thanks, your patches fix my deadlock, i.e. I wasn't able to reproduce the deadlock anymore.

Comment on attachment 324129 [details] [review]
0001-deinterleave-Use-GstIterator-for-iterating-all-pads-.patch

commit 605175b8c4ca07932f15a5035072a31314dfa5a6
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Mar 16 20:18:41 2016 +0200

    deinterleave: Use GstIterator for iterating all pads instead of manually iterating them while holding the object lock all the time
    
    Doing queries while holding the object lock is a bit dangerous, and in this
    case causes deadlocks.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=763326

Review of attachment 324129 [details] [review]:

::: gst/interleave/deinterleave.c
@@ -568,42 +569,62 @@
-    GstPad *ourpad = GST_PAD (l->data);
-    GstCaps *peercaps = NULL, *ourcaps;
-    GstCaps *templ_caps = gst_pad_get_pad_template_caps (ourpad);
... 39 more ...
+    switch (res) {
+          /* Only ask for peer caps for other pads than pad
+        ourcaps = gst_caps_copy (templ_caps);
... 59 more ...

Shouldnt this line be not present as ret is already initialised with any caps. Also if we keep empty then in case of resync, it try to intersect and result in always empty.

In our setup, which uses appsrc with deinterleave element, this line causes caps to return empty (sometime) which in turn in  gstbasetranform.c:2913, cause it to fail the check which then return empty caps and caps negotiation fails. Is changing to empty instead of any is correct here?

> gstbasetranform.c:2913
at this line, should we also check for empty peercaps also along with peercaps not NULL check? Is this scenario possible or is it always expected that element should return either NULL caps or valid?

i see it is already fixed here, sorry for spam
https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/gst/interleave/deinterleave.c?id=4f426f6f54987dc28c59b52faf419cd6ac5b13f2

Is anything left to be done here?

No, but what do you think of this?
> gstbasetranform.c:745
at this line, should we also check for empty peercaps also along with peercaps not NULL check? Is this scenario possible or is it always expected that element should return either NULL caps or valid?

If they're empty, they stay empty, no? Might just be a fast-path to error out faster here. If you want to provide a patch, just do so :)

Created attachment 345310 [details] [review]
0001-dont-intersect-if-peercaps-is-empty_new

Check of empty peercaps as it should be similar to null peercaps.

Comment on attachment 345310 [details] [review]
0001-dont-intersect-if-peercaps-is-empty_new

This does not apply to latest GIT master, and would also cause the empty peercaps to not be unreffed and leaked.

Created attachment 345712 [details] [review]
new patch for fixing empty peercaps usecase

New patch created from master

>and would also cause the empty peercaps to not be unreffed and leaked.
This seems not the case on master as at line 724, peercaps is being freed. All path seems to go through it.

Review of attachment 345712 [details] [review]:

::: libs/gst/base/gstbasetransform.c
@@ +672,3 @@
     gst_caps_unref (peerfilter);
 
+  if (peercaps && !gst_caps_is_empty (peercaps)) {

This is not equivalent though. If the caps are empty, now we would work with the template caps. Before we would work with the empty caps and error out.

I think you want to add a "if (gst_caps_is_empty(peercaps)) goto done;" above instead

>I think you want to add a "if (gst_caps_is_empty(peercaps)) goto done;" above instead
This will cause caps to return NULL which will still fail linking. we would need to return some caps either it could be peerfilter as in case of peerfilter empty or we return template as it is now. I think empty caps is kind of similar to NULL caps practically?

any update on this?

Probably best if you clone this bug and/or open a new bug for your patch.

Thanks tim, create bugid 779055

link https://bugzilla.gnome.org/show_bug.cgi?id=779055