GNOME Bugzilla – Bug 721096
pad: Pad BLOCKING probe callback issues
Last modified: 2013-12-31 13:28:47 UTC
I see there a race condition and plain defect: 1) with GST_PAD_PROBE_TYPE_BLOCKING, inside of which i do gst_pad_remove_probe (pad, GST_PAD_PROBE_INFO_ID (info)), is still entered more than once; 2) in gstpad.c:1270 the return value of callback is ignored. This is in fresh git master checkout, HEAD b03361ac38f020bac6f06c6092adc691b3419bdf. It is reasonable to assume that application maker should guard his callback procedure with his own mutex, but it's pity that a feature of return value GST_PAD_PROBE_REMOVE does not guarantee that callback will be never more entered (i.e. there is a indeterminate delay). You can check these issues with my sketch app https://github.com/krieger-od/gstreamer_test To build, run ./build.sh To launch test app infinitely, run ./loop_forever.sh (problems show up not every time, so this is necessary).
Created attachment 265023 [details] [review] pad: Don't ignore probe callback return value when immediately calling IDLE probe
Created attachment 265025 [details] [review] WIP: pad: Take the stream-lock while calling the IDLE probe callback immediately This is still racy.
For 1), I'm not entirely sure yet how that happens. But there's definitely something fishy in here :)
Comment on attachment 265025 [details] [review] WIP: pad: Take the stream-lock while calling the IDLE probe callback immediately Taking the stream lock should not happen here. Because IDLE probes are blocking too there's no chance that the pad becomes non-idle while we call the callback here.
If there are multiple threads all doing things on the pad, the probe will be called multiple times from these multiple threads. GStreamer does not try to serialize the probe callbacks because that would always create a bottleneck. If the app can't deal with concurrent probe callbacks, it needs to serialize itself, like: int id; probe_callback () { mutex_lock() if (id != 0) { /* do stuff */ .... remove_probe (id) id = 0; } mutex_unlock() } id = add_probe() Simply checking if the probe was not removed when the callback is called should be enough.
(In reply to comment #5) > Simply checking if the probe was not removed when the callback is called should > be enough. Is there a chance that probe id be re-taken in small amount of time, or newly leased ids only go up? If the latter, then ok. I am OK with no serialization out of box, but this should be mentioned in docs.
(In reply to comment #6) > (In reply to comment #5) > > Simply checking if the probe was not removed when the callback is called should > > be enough. > > Is there a chance that probe id be re-taken in small amount of time, or newly > leased ids only go up? If the latter, then ok. The id is a gluong that is incremented for each probe that is added. it will be resued after it wraps around but that is not going to happen in a small amount of time. > > > I am OK with no serialization out of box, but this should be mentioned in docs. OK.
BTW Only statically allocated mutex can be safely used in this situation. Because if the mutex is automatically or dynamically allocated, the mutex inevitably gets freed somewhere. And since we have no guarantees against late spare calls to our probe callback, mutex can be invalid at that moment.