GNOME Bugzilla – Bug 747852
pad: idle probe doesn't block pad from pushing data
Last modified: 2015-04-16 14:47:32 UTC
When the idle probe is called immediately it won't actually block the pad from pushing data. Is one supposed to use GST_PAD_PROBE_TYPE_BLOCKING instead of _IDLE in such cases? But then the pad might block on a non-idle scenario and dynamic pipeline operations might not be successful.
Created attachment 301550 [details] [review] tests: pad: test that idle probe will block Test to show the behavior
No, it should really block the pad. The problem seems to be that the probe checks in the beginning of the functions (chain_data_unchecked(), etc) are not checking for IDLE but only for BLOCK (and not for BLOCKING==IDLE|BLOCK).
Created attachment 301553 [details] [review] pad: avoid letting data pass when pad is idle-blocked It can happen that when a idle pad probe is added the pad is idle and the idle callback is immediatelly called. Meanwhile the element can try to push data and the probe callback routines should check not only for BLOCK but for BLOCK + IDLE (BLOCKING) to avoid passing data while the callback is still running
Created attachment 301571 [details] [review] tests: pad: test that idle probe will block Updated test case
Created attachment 301572 [details] [review] pad: block data flow when idle probe is running Updated patch that does wait before iterating over probes when an idle probe is still running from the gst_pad_add_probe function. The reason to wait there is that the idle probe would block, otherwise, after the push so it would be before everything runs again. One side effect is that even flushes are held here until the idle probe finishes running.
Review of attachment 301572 [details] [review]: ::: gst/gstpad.c @@ +1395,3 @@ /* Keep another ref, the callback could destroy the pad */ gst_object_ref (pad); + pad->priv->idle_running++; Can this ever be bigger than 1? @@ +3449,3 @@ (info->type & GST_PAD_PROBE_TYPE_BLOCK) == GST_PAD_PROBE_TYPE_BLOCK; + do_pad_idle_probe_wait (pad); Maybe you want to do something with the return value here, but otherwise this makes sense :)
(In reply to Sebastian Dröge (slomo) from comment #6) > Review of attachment 301572 [details] [review] [review]: > > ::: gst/gstpad.c > @@ +1395,3 @@ > /* Keep another ref, the callback could destroy the pad */ > gst_object_ref (pad); > + pad->priv->idle_running++; > > Can this ever be bigger than 1? Theoretically you can gst_pad_add_probe from some thread while another thread is running your idle probe from another call to gst_pad_add_probe. Not sure there is a use case for this happening in the same pad but it would be annoying to debug so better protect from the beginning. > > @@ +3449,3 @@ > (info->type & GST_PAD_PROBE_TYPE_BLOCK) == GST_PAD_PROBE_TYPE_BLOCK; > > + do_pad_idle_probe_wait (pad); > > Maybe you want to do something with the return value here, but otherwise > this makes sense :) Good point, should check for FLUSHING
This is not exactly as trivial as it seems. When the idle probe is called after data has passed the pad, no data is flowing from the pad as it is running the idle probe code. As the idle probe doesn't do any blocking by itself, it happens that all data pushed from the idle probe callback is allowed to pass. The user has full control of what happens in the idle probe. When the idle probe is called directly from the gst_pad_add_probe the streaming thread can try to push data and we need to block that but without disabling the idle probe from doing what it wants. For example, relinking that will cause reconfigure events to be sent. Or any smarter decision involving caps queries. One possible solution is to allow data from the idle probe thread to unconditionally pass.
Maybe an IDLE probe should only block serialized events/queries and buffers? That seems like the expected behaviour to me
Created attachment 301662 [details] [review] pad: block data flow when idle probe is running When idle probe runs directly from the gst_pad_add_probe() function we need to make sure that no data flow happens as idle probe is a blocking probe. The idle probe will prevent that any buffer, bufferlist or serialized events and queries are not flowing while it is running.
Review of attachment 301662 [details] [review]: Looks generally good to me, let's get this in and see what happens? ::: gst/gstpad.c @@ +3435,3 @@ + ( \ + (((i)->type & (GST_PAD_PROBE_TYPE_EVENT_DOWNSTREAM | \ + GST_PAD_PROBE_TYPE_EVENT_FLUSH)) && \ Not sure about FLUSH here
Pushed. Thanks for the review. commit d4d161a28223f3d8dfb81623672c9a13dbf4cf23 Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Tue Apr 14 10:47:20 2015 -0300 tests: pad: test that idle probe will block This tests add an idle probe on an idle pad from a separate thread so that the callback is called immediatelly. This callback will sit still and then we try to push a buffer on this same pad. It verifies that the idle probe blocks data passing https://bugzilla.gnome.org/show_bug.cgi?id=747852 commit 3d8de8a4f90e464e814c19c128776aa08e6be1b7 Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Tue Apr 14 17:06:36 2015 -0300 pad: block data flow when idle probe is running When idle probe runs directly from the gst_pad_add_probe() function we need to make sure that no data flow happens as idle probe is a blocking probe. The idle probe will prevent that any buffer, bufferlist or serialized events and queries are not flowing while it is running. https://bugzilla.gnome.org/show_bug.cgi?id=747852