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 761211 - pad: blocking pull probe during pull_range doesn't work
pad: blocking pull probe during pull_range doesn't work
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other All
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-01-28 00:21 UTC by Matej Knopp
Modified: 2016-03-29 03:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch (1.03 KB, patch)
2016-01-28 00:21 UTC, Matej Knopp
rejected Details | Review
Add test for blocking pull probe (3.00 KB, patch)
2016-01-28 15:25 UTC, Matej Knopp
accepted-commit_now Details | Review
pad: PULL probes are called without a buffer so don't require any of the data flags to be set (1017 bytes, patch)
2016-02-17 14:59 UTC, Sebastian Dröge (slomo)
rejected Details | Review
pad: Add test for blocking pull probe (3.02 KB, patch)
2016-02-17 14:59 UTC, Sebastian Dröge (slomo)
committed Details | Review
pad: rework probe's hook_marshall function (1.85 KB, patch)
2016-03-24 20:38 UTC, Thiago Sousa Santos
none Details | Review
tests: pad: extra test for pad pull buffer probe (3.50 KB, patch)
2016-03-24 20:38 UTC, Thiago Sousa Santos
none Details | Review
tests: pad: extra tests for pad pull probes (5.44 KB, patch)
2016-03-28 14:15 UTC, Thiago Sousa Santos
committed Details | Review
pad: rework probe's hook_marshall function (2.00 KB, patch)
2016-03-28 14:24 UTC, Thiago Sousa Santos
committed Details | Review

Description Matej Knopp 2016-01-28 00:21:31 UTC
Created attachment 319884 [details] [review]
Patch

PROBE_PULL macro is missing GST_PAD_PROBE_TYPE_BUFFER, so the probe is never matched
Comment 1 Sebastian Dröge (slomo) 2016-01-28 08:07:11 UTC
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.
Comment 2 Matej Knopp 2016-01-28 11:00:01 UTC
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.
Comment 3 Matej Knopp 2016-01-28 11:01:32 UTC
Btw, looking at the code, gst_pad_get_range_unchecked seems to have same problem.
Comment 4 Matej Knopp 2016-01-28 11:12:40 UTC
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?
Comment 5 Tim-Philipp Müller 2016-01-28 11:19:08 UTC
You might have a buffer because the caller can provide one to pull_range() but ..
Comment 6 Sebastian Dröge (slomo) 2016-01-28 12:24:33 UTC
I guess the check in probe_hook_marshal is wrong here.
Comment 7 Matej Knopp 2016-01-28 12:51:59 UTC
Would would the right check be?
Comment 8 Sebastian Dröge (slomo) 2016-01-28 12:53:14 UTC
For PULL you could allow no other flags to be set.
Comment 9 Matej Knopp 2016-01-28 15:25:07 UTC
Created attachment 319944 [details] [review]
Add test for blocking pull probe
Comment 10 Sebastian Dröge (slomo) 2016-01-29 10:05:20 UTC
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.
Comment 11 Matej Knopp 2016-01-29 11:42:09 UTC
No argument from me here. I'm just waiting for Tim to implement it :)
Comment 12 Sebastian Dröge (slomo) 2016-01-29 14:23:22 UTC
Didn't you show me a patch on a pastebin for that yesterday? :)
Comment 13 Matej Knopp 2016-01-29 14:24:22 UTC
I don't think so, the flags matching code is giving me headaches :)
Comment 14 Sebastian Dröge (slomo) 2016-02-17 14:59:54 UTC
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
Comment 15 Sebastian Dröge (slomo) 2016-02-17 14:59:59 UTC
Created attachment 321500 [details] [review]
pad: Add test for blocking pull probe
Comment 16 Sebastian Dröge (slomo) 2016-02-17 15:01:58 UTC
Matej, can you check if this fixes everything for you?
Comment 17 Matej Knopp 2016-02-17 21:26:16 UTC
Yeah, this seems to do the trick. Thanks!
Comment 18 Sebastian Dröge (slomo) 2016-02-18 07:45:45 UTC
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
Comment 19 Sebastian Dröge (slomo) 2016-02-18 09:47:58 UTC
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?
Comment 20 Thiago Sousa Santos 2016-03-24 20:02:58 UTC
    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.
Comment 21 Thiago Sousa Santos 2016-03-24 20:38:16 UTC
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.
Comment 22 Thiago Sousa Santos 2016-03-24 20:38:21 UTC
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.
Comment 23 Thiago Sousa Santos 2016-03-24 20:38:56 UTC
Please review with care, I might have overlooked something.

All tests in core/base/good/ugly/bad still pass for me.
Comment 24 Thiago Sousa Santos 2016-03-28 04:01:36 UTC
I'll write some more tests for this before merging to make sure we get this right for more scenarios.
Comment 25 Thiago Sousa Santos 2016-03-28 14:15:49 UTC
Created attachment 324872 [details] [review]
tests: pad: extra tests for pad pull probes

Now also has a test for idle probes
Comment 26 Thiago Sousa Santos 2016-03-28 14:24:38 UTC
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 27 Sebastian Dröge (slomo) 2016-03-28 18:56:07 UTC
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?
Comment 28 Thiago Sousa Santos 2016-03-29 03:53:03 UTC
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