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 596366 - proxy_getcaps reverses direction of getcaps
proxy_getcaps reverses direction of getcaps
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal critical
: 0.10.26
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-09-25 19:48 UTC by Stefan Sauer (gstreamer, gtkdoc dev)
Modified: 2009-12-11 09:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
backtrace for the lockup (17.62 KB, text/plain)
2009-09-25 19:48 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
  Details
add ghostpad test for setting target again after pad is linked (2.44 KB, patch)
2009-09-26 20:48 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
add ghostpad test for setting target again after pad is linked (2.46 KB, patch)
2009-09-27 19:34 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
standalone test that uses adder too (1.98 KB, text/plain)
2009-09-27 19:36 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
  Details
possible patch (1.87 KB, patch)
2009-09-28 20:46 UTC, Wim Taymans
none Details | Review
backtrace for the deeper lockup (17.71 KB, text/plain)
2009-09-29 12:04 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
  Details
test case to reproduce the problem (1.74 KB, text/x-csrc)
2009-09-30 21:15 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
  Details
break the getcaps cycle (413 bytes, patch)
2009-10-02 14:55 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review

Description Stefan Sauer (gstreamer, gtkdoc dev) 2009-09-25 19:48:52 UTC
Created attachment 144026 [details]
backtrace for the lockup

  • #3 pthread_mutex_lock
    from /lib/libpthread.so.0
  • #4 gst_proxy_pad_get_target
    at gstghostpad.c line 366
  • #5 gst_proxy_pad_do_getcaps
    at gstghostpad.c line 221
  • #6 gst_pad_get_caps_unlocked
    at gstpad.c line 2122
  • #7 gst_pad_get_caps
    at gstpad.c line 2206
  • #215 gst_pad_get_caps_unlocked
    at gstpad.c line 2122
  • #216 gst_pad_link_prepare
    at gstpad.c line 1772
  • #217 gst_pad_link
    at gstpad.c line 2009
  • #218 gst_ghost_pad_set_target
    at gstghostpad.c line 1208
  • #219 bt_sink_bin_update
    at sink-bin.c line 680

problem is that gst_proxy_pad_get_target() in #4 tries to GST_PROXY_LOCK
(0x8188058);, which is already locked by gst_ghost_pad_set_target() in #218

just releasing the lock for the gst_pad_link() is not enough.
Comment 1 Jan Schmidt 2009-09-25 22:27:35 UTC
Is this a regression? If not, I'd like to punt it and make releases... now.
Comment 2 Sebastian Dröge (slomo) 2009-09-26 05:44:15 UTC
Stefan, do you see another possibility to fix this other than making the GST_PROXY_LOCK recursive?
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2009-09-26 19:46:40 UTC
It worked before. I hit it when recording a song in buzzard. I tried git bisecting it, but failed as things fail to run for older core versions. I am now trying to write a unit test to narrow it down.

If I don't hit it in buzztard for very special reasons, I would consider it as quite critical.
Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2009-09-26 20:12:30 UTC
The lokup happens when the ghostpad is linked. This is a workaround:

    if((peer_pad=gst_pad_get_peer(ghost_pad))) {
      gst_pad_unlink(peer_pad,ghost_pad);
    }
    
    if(!gst_ghost_pad_set_target(GST_GHOST_PAD(ghost_pad),new_target_pad)) {
      GST_WARNING("failed to link internal pads");
    }
    
    if(peer_pad) {
      gst_pad_link(peer_pad,ghost_pad);
      gst_object_unref(peer_pad);
    }
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2009-09-26 20:48:07 UTC
Created attachment 144087 [details] [review]
add ghostpad test for setting target again after pad is linked

This test doe not prove the bug yet. In my backtrace one sees lots of basetransform elements and e.g. adders. I think the issue is caused by gst_pad_proxy_getcaps() checking the downstream caps again.
Comment 6 Stefan Sauer (gstreamer, gtkdoc dev) 2009-09-27 17:33:25 UTC
(In reply to comment #2)
> Stefan, do you see another possibility to fix this other than making the
> GST_PROXY_LOCK recursive?

It a GMutex and imho there is no recursive variant for that (in glib).
Comment 7 Stefan Sauer (gstreamer, gtkdoc dev) 2009-09-27 19:34:42 UTC
Created attachment 144130 [details] [review]
add ghostpad test for setting target again after pad is linked

Add a test that actually builds
Comment 8 Stefan Sauer (gstreamer, gtkdoc dev) 2009-09-27 19:36:25 UTC
Created attachment 144131 [details]
standalone test that uses adder too

core tests cannot use adder. here is a standalone test app that uses adder and it still does not reproduce the issue. I also checked adder changes and they are not causing it.
Comment 9 Sebastian Dröge (slomo) 2009-09-28 05:52:17 UTC
(In reply to comment #6)
> (In reply to comment #2)
> > Stefan, do you see another possibility to fix this other than making the
> > GST_PROXY_LOCK recursive?
> 
> It a GMutex and imho there is no recursive variant for that (in glib).

Right, but we could use a GStaticRecMutex instead. (Edward will love that :) )

But really, why is there no GRecMutex in GLib? That really shouldn't be hard to implement...
Comment 10 Wim Taymans 2009-09-28 20:46:44 UTC
Created attachment 144200 [details] [review]
possible patch

The locking was a bit too eager, we can release them before we attempt to relink the new target.
Comment 11 Jan Schmidt 2009-09-29 09:36:07 UTC
Stefan, could you test this patch with Buzztard?
Comment 12 Stefan Sauer (gstreamer, gtkdoc dev) 2009-09-29 12:04:18 UTC
Created attachment 144257 [details]
backtrace for the deeper lockup

I still locks up, but now its a different lock :/

  • #4 gst_pad_get_caps
    at gstpad.c line 2202
  • #5 gst_proxy_pad_do_getcaps
    at gstghostpad.c line 227
  • #6 gst_pad_get_caps_unlocked
    at gstpad.c line 2122
  • #215 gst_pad_get_caps_unlocked
    at gstpad.c line 2122
  • #216 gst_pad_link_prepare
    at gstpad.c line 1772
  • #217 gst_pad_link
    at gstpad.c line 2009

gst_pad_link_prepare() in #216 takes the object lock on the pad and gst_pad_get_caps() in #4 tries to take it again.

What I really don't understand is that why is the chain of _get_caps calls 'comming back' to the ghostpad.
Comment 13 Stefan Sauer (gstreamer, gtkdoc dev) 2009-09-30 21:15:42 UTC
Created attachment 144442 [details]
test case to reproduce the problem

simplified the problem is :

src ! tee ! queue ! fx ! adder ! sink
          ! queue      !

tee uses gst_pad_proxy_getcaps() and that send the getcaps back and hits sink again.
Comment 14 Stefan Sauer (gstreamer, gtkdoc dev) 2009-09-30 21:35:54 UTC
tee does not have substantial changes. I will try bisecting adder changes next.
Comment 15 Stefan Sauer (gstreamer, gtkdoc dev) 2009-10-01 14:27:15 UTC
Its not adder either. I bisected all changes of this year.

I was trying this in sgstpad.c, but its causing more pain :/
 GstCaps *
 gst_pad_get_caps (GstPad * pad)
 {
   GstCaps *result = NULL;

   g_return_val_if_fail (GST_IS_PAD (pad), NULL);

+  if (G_UNLIKELY (GST_OBJECT_FLAG_SET (pad, GST_PAD_IN_GETCAPS))) {
+    return gst_caps_new_empty ();
+  }
Comment 16 Stefan Sauer (gstreamer, gtkdoc dev) 2009-10-02 14:55:43 UTC
Created attachment 144598 [details] [review]
break the getcaps cycle

If we allow gst_pad_proxy_getcaps to do getcaps on all other pads, we need a way to flag all pads on our way (GST_PAD_IN_GETCAPS). Then we need to check for that (ideally without taking a lock). If the flag is not set take the lock, if the flag is set return ANY caps. I see no way to do the flag check and taking the lock. We would need another lock for the flag.

The attached patch simply does a TRYLOCK on the pad and returns ANY if the lock cannot be taken. 

It fixes the testapp and also my bigger case. It does not break any tests in core and base for me. Still, is this the right thing to do?
Comment 17 Sebastian Dröge (slomo) 2009-10-02 16:59:00 UTC
I don't think that's OK, you really want to take the lock there and wait in most situations. Only in case of the cycles you don't want that... so that situation has to be flagged somehow, yes.
Comment 18 Stefan Sauer (gstreamer, gtkdoc dev) 2009-10-03 09:51:26 UTC
Any possible scenario where this could cause problems?

Ans any idea how to fix it beside having another lock (e.g. capslock) and lock both with lock order. I mean do a trylock for the capslock and if we got it take the object lock too. If taking the capslock fails, return any caps.
Comment 19 Wim Taymans 2009-10-06 20:42:11 UTC
commit 3ac4a08383216a3a0908aaac8b68b21c050fb8e7
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Mon Sep 28 22:43:51 2009 +0200

    ghostpad: take locks around smaller section
    
    We don't need the hold the proxy mutex locked for getting the internal pad and
    for linking the new target pad when we retarget. So take the lock a little later
    and release it earlier.
    
    Fixes #596366
Comment 20 Wim Taymans 2009-12-01 16:12:37 UTC
a getcaps should never reverse the direction. if proxy_getcaps does that then it needs fixing. For the tee case, the caps comming out of the particular pad only depend on the upstream allowed caps, not on the downstream ones of the other branch.
Comment 21 Sebastian Dröge (slomo) 2009-12-08 15:22:53 UTC
gst_pad_proxy_get_caps() did this before latest GIT, I don't see how gst_proxy_pad_do_getcaps() could do this.
Comment 22 Sebastian Dröge (slomo) 2009-12-10 14:19:29 UTC
Should be fixed by this, the direction inversion seem to come from tee using gst_pad_proxy_get_caps(), which is now fixed.

http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=c6f2a9477750be536924bf8e70a830ddec4c1389
Comment 23 Stefan Sauer (gstreamer, gtkdoc dev) 2009-12-11 09:17:19 UTC
Confirmed, its fixed. Many thanks!