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 721096 - pad: Pad BLOCKING probe callback issues
pad: Pad BLOCKING probe callback issues
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other All
: Normal normal
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-12-26 18:36 UTC by Andrey Utkin
Modified: 2013-12-31 13:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
pad: Don't ignore probe callback return value when immediately calling IDLE probe (3.49 KB, patch)
2013-12-30 09:04 UTC, Sebastian Dröge (slomo)
committed Details | Review
WIP: pad: Take the stream-lock while calling the IDLE probe callback immediately (1.10 KB, patch)
2013-12-30 09:05 UTC, Sebastian Dröge (slomo)
rejected Details | Review

Description Andrey Utkin 2013-12-26 18:36:36 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).
Comment 1 Sebastian Dröge (slomo) 2013-12-30 09:04:07 UTC
Created attachment 265023 [details] [review]
pad: Don't ignore probe callback return value when immediately calling IDLE probe
Comment 2 Sebastian Dröge (slomo) 2013-12-30 09:05:49 UTC
Created attachment 265025 [details] [review]
WIP: pad: Take the stream-lock while calling the IDLE probe callback immediately

This is still racy.
Comment 3 Sebastian Dröge (slomo) 2013-12-30 09:08:20 UTC
For 1), I'm not entirely sure yet how that happens. But there's definitely something fishy in here :)
Comment 4 Sebastian Dröge (slomo) 2013-12-30 09:52:54 UTC
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.
Comment 5 Wim Taymans 2013-12-30 09:56:27 UTC
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.
Comment 6 Andrey Utkin 2013-12-30 11:00:35 UTC
(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.
Comment 7 Wim Taymans 2013-12-30 11:12:26 UTC
(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.
Comment 8 Andrey Utkin 2013-12-31 13:28:47 UTC
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.