GNOME Bugzilla – Bug 548764
New basetransform doesn't lock pad properly when looking at its caps
Last modified: 2008-09-29 19:18:51 UTC
The new basetransform stuff doesn't lock the pad properly, in the right conditions, it causes a race where it sets the old caps on a buffer, not the new ones. I'm attaching a patch that just locks the pad before using the caps, but should it not instead use gst_pad_get_negotiated_caps() ?
Created attachment 117091 [details] [review] Lock the pad before looking at its internal stuff
Created attachment 117094 [details] [review] Check for NULL caps and also unref them Improved patch that doesnt leak them and checks for the null-ness.. Would be simple if we used gst_pad_get_negotiated_caps().. but is it appriopriate?
When exactly can it go wrong? Do you have a testcase or seen it fail? I can only see it fail if someone in the subclass calls a setcaps() on a pad. Normally the caps are protected with the STREAM_LOCK or OBJECT lock on the pads, the code you change is usually called in the STREAM_LOCK.
Ping? Should this be a blocker for the upcoming release? (Marking it as such for now) What did you see fail exactly? Do you have a test case for the problem you've run into? Can we easily reproduce it somehow?
Created attachment 118442 [details] Example of stack trace from crash
Yes, it should really be a blocker. Sadly, I don't have a "simple" test case. The test case involves running all of farsight2. To reproduce: get farsight2 0.0.3 (or the git trunk) and build it cd tests/gui G_DEBUG=fatal-warnings python fs2-gui.py click "New Server" in another term, do the same thing.. but click "Connect" to connect to the server Watch video being transfered between both sides. Try changing the video codec on one side or on the other side.. Enjoy crash. The way I see it is quite simple.. Either GST_PAD_CAPS() is protected by the stream lock or by the object lock. If its the stream lock .. the GstPad should be fixed, if its the object lock, the basetransform should be fixed.
I forgot one thing.. You may need the patches from bug #530935 too.
What's happening with this bug - we need to get it fixed for the release.
What element is after the h264 payloader?
(In reply to comment #9) > What element is after the h264 payloader? capsfilter->dtmfrtpmux->gsrtpbin->tee->udpsink more or less
ok, this is what is obviously wrong in the case of capfilter: - basetransform does outcaps = GST_PAD_CAPS (trans->srcpad) - basetransform calls ->prepare_output_buffer (..., outcaps ,..) - capsfilter does gst_pad_set_caps (trans->srcpad, newcaps) affectively decreasing the refcount we had on outcaps - basetransform continues using outcaps eventhough it might have lost the ref by calling ->prepare_output_buffer. I'll start by applying the patch for this case.
Can you retest to see if the caps mayhem was cause by this: * libs/gst/base/gstbasetransform.c: (gst_base_transform_prepare_output_buffer): Take new caps ref because our old one might have been gone when the subclass performs a gst_pad_set_caps() on the srcpad. See #548764.
Adding refs and locks now and lowering priority. Patch by: Olivier Crete <tester at tester dot ca> * libs/gst/base/gstbasetransform.c: (gst_base_transform_prepare_output_buffer), (gst_base_transform_activate): Temporary fix for blocker bug #548764. Take more caps refs and take the pad lock while doing so. This problem is likely caused by some refcounting problem elsewhere but we don't know where yet.
I refrained from committing the patch I talked about in Comment #13 because I did not understand why it would be needed. Some more debugging revealed a caps refcount error in some other plugin. Then error seems fixed now, closing.