After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 763326 - gst-launch of 8-channel deinterleave pipeline stalls
gst-launch of 8-channel deinterleave pipeline stalls
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-03-08 14:50 UTC by Heinrich Fink
Modified: 2017-02-22 07:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
level 6 debug log (323.57 KB, application/x-xz)
2016-03-08 14:52 UTC, Heinrich Fink
  Details
pipeline graph (42.51 KB, image/svg+xml)
2016-03-08 14:52 UTC, Heinrich Fink
  Details
level 6 debug log (282.95 KB, application/x-xz)
2016-03-08 15:01 UTC, Heinrich Fink
  Details
pipeline graph (19.98 KB, image/svg+xml)
2016-03-08 15:02 UTC, Heinrich Fink
  Details
gdb stacktrace of stalled pipeline (16.21 KB, text/plain)
2016-03-08 15:21 UTC, Heinrich Fink
  Details
gdb stacktrace of stalled pipeline (full) (32.67 KB, text/plain)
2016-03-08 15:21 UTC, Heinrich Fink
  Details
0001-deinterleave-Use-GstIterator-for-iterating-all-pads-.patch (5.49 KB, patch)
2016-03-16 18:30 UTC, Sebastian Dröge (slomo)
committed Details | Review
0002-deinterleave-Return-the-current-caps-on-the-srcpads-.patch (1.67 KB, patch)
2016-03-16 18:30 UTC, Sebastian Dröge (slomo)
committed Details | Review
0001-dont-intersect-if-peercaps-is-empty_new (505 bytes, patch)
2017-02-09 12:30 UTC, abhimanyu.v
none Details | Review
new patch for fixing empty peercaps usecase (1.04 KB, patch)
2017-02-14 09:59 UTC, abhimanyu.v
needs-work Details | Review

Description Heinrich Fink 2016-03-08 14:50:41 UTC
The following launch line hang on my machine, i.e. never transitions to PLAYING: 

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 \
  deinter.src_2 ! queue ! fakesink \
  deinter.src_3 ! queue ! fakesink \
  deinter.src_4 ! queue ! fakesink \
  deinter.src_5 ! queue ! fakesink \
  deinter.src_6 ! queue ! fakesink \
  deinter.src_7 ! queue ! fakesink

This has been observed on a 12-core CPU. Limiting the amount of CPU cores used to four or less (i.e. taskset -c 0-3 gst-launch-1.0 ...) makes everything work again. This suggests the bug to be a race condition.
Comment 1 Heinrich Fink 2016-03-08 14:52:01 UTC
Created attachment 323401 [details]
level 6 debug log
Comment 2 Heinrich Fink 2016-03-08 14:52:28 UTC
Created attachment 323402 [details]
pipeline graph
Comment 3 Sebastian Dröge (slomo) 2016-03-08 14:57:40 UTC
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 :)
Comment 4 Sebastian Dröge (slomo) 2016-03-08 14:59:00 UTC
This pipeline never creates any buffer it seems.
Comment 5 Heinrich Fink 2016-03-08 15:01:18 UTC
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.
Comment 6 Heinrich Fink 2016-03-08 15:01:51 UTC
Created attachment 323404 [details]
level 6 debug log
Comment 7 Heinrich Fink 2016-03-08 15:02:51 UTC
Created attachment 323405 [details]
pipeline graph
Comment 8 Sebastian Dröge (slomo) 2016-03-08 15:05:18 UTC
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.
Comment 9 Heinrich Fink 2016-03-08 15:16:52 UTC
<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)
Comment 10 Heinrich Fink 2016-03-08 15:21:39 UTC
Created attachment 323408 [details]
gdb stacktrace of stalled pipeline
Comment 11 Heinrich Fink 2016-03-08 15:21:56 UTC
Created attachment 323409 [details]
gdb stacktrace of stalled pipeline (full)
Comment 12 Sebastian Dröge (slomo) 2016-03-08 15:35:11 UTC
Ok so it's a deadlock
Comment 13 Sebastian Dröge (slomo) 2016-03-16 18:30:38 UTC
Created attachment 324129 [details] [review]
0001-deinterleave-Use-GstIterator-for-iterating-all-pads-.patch
Comment 14 Sebastian Dröge (slomo) 2016-03-16 18:30:53 UTC
Created attachment 324130 [details] [review]
0002-deinterleave-Return-the-current-caps-on-the-srcpads-.patch
Comment 15 Sebastian Dröge (slomo) 2016-03-16 18:32:19 UTC
The second patch is not actually needed but simplifies all the caps query logic a bit.
Comment 16 Heinrich Fink 2016-03-17 09:36:56 UTC
Thanks, your patches fix my deadlock, i.e. I wasn't able to reproduce the deadlock anymore.
Comment 17 Sebastian Dröge (slomo) 2016-03-17 19:14:42 UTC
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
Comment 18 abhimanyu.v 2017-02-09 08:14:20 UTC
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?
Comment 19 abhimanyu.v 2017-02-09 08:18:25 UTC
> 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?
Comment 21 Sebastian Dröge (slomo) 2017-02-09 11:37:40 UTC
Is anything left to be done here?
Comment 22 abhimanyu.v 2017-02-09 11:46:27 UTC
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?
Comment 23 Sebastian Dröge (slomo) 2017-02-09 11:53:49 UTC
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 :)
Comment 24 abhimanyu.v 2017-02-09 12:30:41 UTC
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 25 Sebastian Dröge (slomo) 2017-02-10 11:11:28 UTC
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.
Comment 26 abhimanyu.v 2017-02-14 09:59:08 UTC
Created attachment 345712 [details] [review]
new patch for fixing empty peercaps usecase

New patch created from master
Comment 27 abhimanyu.v 2017-02-14 10:00:48 UTC
>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.
Comment 28 Sebastian Dröge (slomo) 2017-02-14 10:29:29 UTC
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
Comment 29 abhimanyu.v 2017-02-14 13:03:43 UTC
>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?
Comment 30 abhimanyu.v 2017-02-17 12:35:10 UTC
any update on this?
Comment 31 Tim-Philipp Müller 2017-02-21 14:39:54 UTC
Probably best if you clone this bug and/or open a new bug for your patch.
Comment 32 abhimanyu.v 2017-02-22 07:42:43 UTC
Thanks tim, create bugid 779055
Comment 33 abhimanyu.v 2017-02-22 07:43:05 UTC
link https://bugzilla.gnome.org/show_bug.cgi?id=779055