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 721289 - pad: using multiple blocking probes doesn't work as expected
pad: using multiple blocking probes doesn't work as expected
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other All
: Normal normal
: 1.2.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-12-31 20:16 UTC by Todd Agulnick
Modified: 2014-03-05 17:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Testcase for the bug (3.99 KB, text/plain)
2014-01-01 12:36 UTC, Pedro Corte-Real
  Details
Potential patch to gstpad.c (1.23 KB, patch)
2014-01-01 13:39 UTC, Pedro Corte-Real
rejected Details | Review

Description Todd Agulnick 2013-12-31 20:16:08 UTC
Here's a scenario:

Add blocking Probe A.
Probe A's callback gets invoked and returns GST_PAD_PROBE_OK, leaving the block in place.

Add blocking Probe B.
Probe B's callback doesn't get invoked, as pad is already blocked.

Remove blocking probe A.
Probe B's callback still doesn't get invoked.

Pad remains blocked because of Probe B's existence, but the callback associated with the Probe B was never invoked.

A more desirable implementation would track which probe had blocked the pad (in the example above, that would be Probe A). When that pad is removed, it unblocks and invokes the callback of any remaining blocking probes (in this example, Probe B). If the callback leaves the blocking probe in place, then Probe B becomes the de facto blocker and the pad remains blocked until it is removed.
Comment 1 Pedro Corte-Real 2013-12-31 20:51:55 UTC
On IRC we had discussed the workaround for there not being a gst_pad_probe_unblock() of adding the probe a second time and then removing the original version of the probe. This bug makes that unworkable as the removal doesn't unblock the probe because there's already another blocking probe installed.

It seems the API needs to pick either one of two options:

1) The block is of the probe and not of the pad so:
  - All callbacks should be called on matching events even if a previous one has blocked. A pad may become blocked by two different probes
  - There should be a gst_pad_probe_unblock(pad, probe) that unblocks only that single probe and potentially leaves the pad blocked if there's another blocking probe in effect.

2) The block is of the pad so:
  - We need a gst_pad_unblock(pad) that unblocks the pad whatever the blocking probe was

Options 1) seems more flexible but isn't backwards compatible as it requires all callbacks to be called even if the first has blocked the pad.
Comment 2 Sebastian Dröge (slomo) 2014-01-01 09:27:19 UTC
Could you provide a testcase for this to make it easier to reproduce and debug? :)
Comment 3 Pedro Corte-Real 2014-01-01 12:36:33 UTC
Created attachment 265095 [details]
Testcase for the bug

Here's a reduced testcase extracted from my app.
Comment 4 Pedro Corte-Real 2014-01-01 13:39:02 UTC
Created attachment 265097 [details] [review]
Potential patch to gstpad.c

I attempted to create a patch to add gst_pad_unblock(pad) to fix this. It seems to kind of work in my app although on the second file change I seem to be getting a second EOS that closes the pipeline. I'm running the unblock from the bus callback which is probably wrong.
Comment 5 Sebastian Dröge (slomo) 2014-01-02 09:48:46 UTC
What should happen here is:

1) Add probe A
2) blocked due to probe A
3) add probe B, not called but pad still blocked
4) remove probe A
5) probe B is called and can decide to block or not

Should be possible to implement that easily with the cookies :)
Comment 6 Sebastian Dröge (slomo) 2014-01-02 09:50:37 UTC
Comment on attachment 265097 [details] [review]
Potential patch to gstpad.c

This should be independent of this bug, can you open a new one for it :)

The API should be:
void gst_pad_unblock(pad, probe_id, probe_return)
Comment 7 Sebastian Dröge (slomo) 2014-01-02 12:36:21 UTC
Please test if this fixes all your problems

commit 9e125e7bab92259d2d3317c2bf47da081c03ba8a
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Jan 2 13:34:52 2014 +0100

    pad: Add unit test for adding/removing blocking probes while a pad is blocked
    
    And make sure that these new probes are actually called if they should
    instead of silently blocking the pad forever.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=721289

commit 93ce90f83320e845867d28b9cbeea5b596a1ef04
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Jan 2 13:33:20 2014 +0100

    pad: Check if new probes need to be called when adding/removing some
    
    This allows blocking a pad, add a new blocking probe, removing
    the first probe and then having the second probe called. Which
    could then decide that data-flow should actually continue
    instead of blocking now.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=721289