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 724571 - In collectpads, the collected function is sometimes called incorrectly.
In collectpads, the collected function is sometimes called incorrectly.
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 724267
 
 
Reported: 2014-02-17 18:53 UTC by Mathieu Duponchelle
Modified: 2014-03-16 17:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
This test highlights the issue. (4.92 KB, patch)
2014-02-17 18:53 UTC, Mathieu Duponchelle
none Details | Review
This patch corrects the observed bug. (2.14 KB, patch)
2014-02-19 01:56 UTC, Mathieu Duponchelle
committed Details | Review
This test highlights the issue correctly. (5.17 KB, patch)
2014-02-19 01:56 UTC, Mathieu Duponchelle
reviewed Details | Review

Description Mathieu Duponchelle 2014-02-17 18:53:33 UTC
Created attachment 269454 [details] [review]
This test highlights the issue.

Have two sinks on an element implementing the collected function, push a seek on its src pad, push flush_start + flush_stop + eos on each of the sinks, collected gets called, that's normal.

Push a new seek on its src pad, flush_start / flush_stop and a buffer on *only one* of the sinks, collected gets called again, that doesn't seem right to me.

The new proposed patch to the collectpads tests is attached.
Comment 1 Mathieu Duponchelle 2014-02-19 01:56:01 UTC
Created attachment 269649 [details] [review]
This patch corrects the observed bug.

This patch corrects the observed bug. The previously attached test case "succeeds" in that instead of failing it hangs on the thread joining as logically gst_pad_push now blocks instead of running collected.

There still are theoretically races with that patch, but it makes the situation better, Sebastian's rework of collectpads should hopefully make the situation better and the code clearer, I'm available to discuss all this on IRC.

I'll attach an updated test case after that.
Comment 2 Mathieu Duponchelle 2014-02-19 01:56:36 UTC
Created attachment 269650 [details] [review]
This test highlights the issue correctly.
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2014-02-19 16:25:45 UTC
Review of attachment 269650 [details] [review]:

::: tests/check/libs/collectpads.c
@@ +1040,3 @@
+  collected = FALSE;
+
+  gst_pad_push_event (srcpad1, gst_event_new_flush_stop (TRUE));

If these parts are independent, please split it into two tests.

@@ +1056,3 @@
+
+  /* Give a little time to make sure collected is not called */
+

meh. Can we do this differently?
Comment 4 Mathieu Duponchelle 2014-02-19 22:00:45 UTC
The first part is necessary to trigger the bug, the two pads must have had an EOS for priv->eospads to be incremented.

As for the sleep, I haven't found an other solution, I am aware that it makes the test theoretically racy, but as the call to gst_pad_push blocks in the thread, one needs to make sure it actually started waiting. I am open to suggestions, I racked my brain but couldn't find.

Even if that test can't be merged, at least it helps showing the problem. It would be useful to merge it though, as that's a case that will need testing when reimplementing collectpads.
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2014-02-22 20:20:16 UTC
As you send a flushing seek, do you need to send the flush_start/stop before sending the EOS? Also I wonder why you are using both gst_pad_push_event and gst_pad_send_event?

Also how does the test actually use collect pads? I don't see a call to gst_collect_pads_add_pad().

Regarding the sleep, can you use a condition var triggered by the chain function on the peer-pad you are pushing the buffer to?
Comment 6 Sebastian Dröge (slomo) 2014-03-16 17:16:07 UTC
Comment on attachment 269649 [details] [review]
This patch corrects the observed bug.

commit d784d59262ebe93d0f7109a5bc2bc59d0f185067
Author: Mathieu Duponchelle <mduponchelle1@gmail.com>
Date:   Wed Feb 19 02:27:36 2014 +0100

    collectpads: When seek flushed, immediately set eospads to 0
    
    This prevents situations where a first branch would get seeked and
    receive a buffer before all branches got seeked, and thus collected
    would get called based on EOS from the previous segment.
    
    As a consequence, during the process of seeking, don't decrease
    the eospads number when a FLUSH_STOP is received.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=724571
Comment 7 Mathieu Duponchelle 2014-03-16 17:19:08 UTC
We send a flushing seek, but there's no one to handle it upstream, so we have to send the flush start / stop ourselves.

The usage of collectpads is implemented by the GstAggregator object, defined at the beginning of the file.

I don't know how I could use the condition var in collectpads, as it's triggered when the code *stops* waiting. Only solution would be to have a special condition for debugging purposes, but I think we don't want that, and Sebastian seems to agree there's no other way but the sleep.
Comment 8 Sebastian Dröge (slomo) 2014-03-16 17:24:15 UTC
Yeah, the problem is that you need to check that something is *not* called. And you can only do that by putting an assertion into that and wait for a while if it doesn't happen.