GNOME Bugzilla – Bug 761211
pad: blocking pull probe during pull_range doesn't work
Last modified: 2016-03-29 03:53:50 UTC
Created attachment 319884 [details] [review] Patch PROBE_PULL macro is missing GST_PAD_PROBE_TYPE_BUFFER, so the probe is never matched
Comment on attachment 319884 [details] [review] Patch Not sure if that is intended. With BUFFER, the assumption could be that the GstPadProbeInfo always contains a valid buffer. What is exactly the problem you see here? Shouldn't a probe with just GST_PAD_PROBE_TYPE_PULL | GST_PAD_PROBE_TYPE_BLOCK work? I also wonder why there's no non-blocking usage of the PROBE_PULL macro.
No, GST_PAD_PROBE_TYPE_PULL | GST_PAD_PROBE_TYPE_BLOCK doesn't work. The probe callback is never invoked, because this check in probe_hook_marshal fails /* one of the data types for non-idle probes */ if ((type & GST_PAD_PROBE_TYPE_IDLE) == 0 && (flags & GST_PAD_PROBE_TYPE_ALL_BOTH & type) == 0) goto no_match; The second check (flags & GST_PAD_PROBE_TYPE_ALL_BOTH & type) == 0) means, that both type (argument to PROBE_PULL) and flags (probe flags) must have at least one bit from GST_PAD_PROBE_TYPE_ALL_BOTH set. Otherwise you get always no_match.
Btw, looking at the code, gst_pad_get_range_unchecked seems to have same problem.
Also, on the other hand, I understand that you don't have a buffer yet, so you don't get buffer in probe even though you specify GST_PAD_PROBE_TYPE_BUFFER; So maybe the check in probe_hook_marshal is wrong in this case?
You might have a buffer because the caller can provide one to pull_range() but ..
I guess the check in probe_hook_marshal is wrong here.
Would would the right check be?
For PULL you could allow no other flags to be set.
Created attachment 319944 [details] [review] Add test for blocking pull probe
Review of attachment 319884 [details] [review]: ::: gst/gstpad.c @@ +4492,3 @@ + PROBE_PULL (pad, + GST_PAD_PROBE_TYPE_PULL | GST_PAD_PROBE_TYPE_BUFFER | + GST_PAD_PROBE_TYPE_BLOCK, res_buf, offset, size, probe_stopped); As discussed yesterday, it should work differently and the check should be handled like for IDLE. Basically allow PULL without any other flags being set.
No argument from me here. I'm just waiting for Tim to implement it :)
Didn't you show me a patch on a pastebin for that yesterday? :)
I don't think so, the flags matching code is giving me headaches :)
Created attachment 321499 [details] [review] pad: PULL probes are called without a buffer so don't require any of the data flags to be set
Created attachment 321500 [details] [review] pad: Add test for blocking pull probe
Matej, can you check if this fixes everything for you?
Yeah, this seems to do the trick. Thanks!
Attachment 321499 [details] pushed as b89fa47 - pad: PULL probes are called without a buffer so don't require any of the data flags to be set Attachment 321500 [details] pushed as 17d30e9 - pad: Add test for blocking pull probe
Had to be reverted as it breaks various tests. The problem is that probes without SCHEDULING or DATA flags will get all of them set in gst_pad_add_probe(). So we would now always get probes with GST_PAD_PROBE_TYPE_EVENT_DOWNSTREAM called whenever a pull happens. Not sure how to handle this other than setting the BUFFER flag for the PULL probes like you suggested... and hoping that nobody ever created a probe that assumes there is a non-NULL buffer whenever the BUFFER flag is set. Opinions?
if ((type & GST_PAD_PROBE_TYPE_IDLE) == 0 && (flags & GST_PAD_PROBE_TYPE_ALL_BOTH & type) == 0) goto no_match; IDLE/BLOCK occurrences are reversed for PUSH/PULL modes. I think do_hook_marshall needs to handle PUSH and PULL differently for some of those checks.
Created attachment 324725 [details] [review] pad: rework probe's hook_marshall function PUSH and PULL mode have opposite scenarios for IDLE and BLOCK probes. For PUSH it will BLOCK with some data type and IDLE won't have a type. For PULL it will BLOCK before getting some data and will be IDLE when some data is obtained. The check in hook_marshall was specific for PUSH mode and would cause PULL probes to fail to be called. Adding different checks for the mode to fix this issue.
Created attachment 324726 [details] [review] tests: pad: extra test for pad pull buffer probe Makes sure it is only called when there is a buffer.
Please review with care, I might have overlooked something. All tests in core/base/good/ugly/bad still pass for me.
I'll write some more tests for this before merging to make sure we get this right for more scenarios.
Created attachment 324872 [details] [review] tests: pad: extra tests for pad pull probes Now also has a test for idle probes
Created attachment 324873 [details] [review] pad: rework probe's hook_marshall function Allow both IDLE and BLOCK probes to be executed without a data type for PULL mode. This also makes it work for IDLE probes. The remaining question is where should the BLOCK probe happen for PULL mode. Before getrange is called or when it returns with some data (a buffer).
Comment on attachment 324872 [details] [review] tests: pad: extra tests for pad pull probes Might also want to merge my tests then :) Can you do that?
commit 368ee8a336d0c868d81fdace54b24431a8b48cbf Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Thu Mar 24 17:34:20 2016 -0300 pad: rework probe's hook_marshall function PUSH and PULL mode have opposite scenarios for IDLE and BLOCK probes. For PUSH it will BLOCK with some data type and IDLE won't have a type. For PULL it will BLOCK before getting some data and will be IDLE when some data is obtained. The check in hook_marshall was specific for PUSH mode and would cause PULL probes to fail to be called. Adding different checks for the mode to fix this issue. https://bugzilla.gnome.org/show_bug.cgi?id=761211 commit 1967d56aea6910b7ee238dda905b549a6be575af Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Thu Mar 24 17:34:40 2016 -0300 tests: pad: extra tests for pad pull probes For BUFFER and IDLE probes https://bugzilla.gnome.org/show_bug.cgi?id=761211 commit a7813adb1d87e01b7746bb0b9dc822728a200eda Author: Matej Knopp <matej.knopp@gmail.com> Date: Thu Jan 28 16:22:17 2016 +0100 pad: Add test for blocking pull probe https://bugzilla.gnome.org/show_bug.cgi?id=761211