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 752746 - harness: allow full control over event forwarding
harness: allow full control over event forwarding
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal major
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 745768
 
 
Reported: 2015-07-22 23:23 UTC by Olivier Crête
Modified: 2015-08-16 13:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
harness: Keep synchronized events in sync with buffers (11.22 KB, patch)
2015-07-22 23:23 UTC, Olivier Crête
none Details | Review
Just disabling the forwarding is annoying because it means the really useful (13.81 KB, patch)
2015-07-23 19:49 UTC, Olivier Crête
none Details | Review
Example audiointerleave tests (9.02 KB, text/plain)
2015-07-24 12:16 UTC, Stian Selnes (stianse)
  Details

Description Olivier Crête 2015-07-22 23:23:00 UTC
While trying to use GstHarness to write a test, I discovered that if you use a src harness, the serialized events get forwarded separately from the buffer. This causes two problems: they are pushed outside the control of the test, and they get out of sync with the buffers.

My solution is to put the serialized buffers in the same GAsyncQueue as the buffers, and then forward the on gst_harness_pull() if there is a pad to forward too, otherwise drop them. This also means that gst_harness_pull_event() will no longer see them, so instead I'm proposing to add a specialized function for that.

Another option is to just put them in the buffres queue is there is a pad to forward to, but it seems less elegant.
Comment 1 Olivier Crête 2015-07-22 23:23:21 UTC
Created attachment 307937 [details] [review]
harness: Keep synchronized events in sync with buffers

This means putting them in the same queue as the buffers
and trying to forward them on a pull. This also means
that synchronized events no longer are in the regular event
queue, but instead must be pulled with specialized functions.
Comment 2 Håvard Graff (hgr) 2015-07-23 09:06:02 UTC
While I appreciate what you are trying to solve, the patch in its current form would break a few hundered of our tests. :)

The main problem is that most events are serialized, and there is an existing event-queue for events, and with this patch the only events ending up in the event-queue would be the OOB ones, hence all tests waiting for specific (custom) events would break.

We have been evolvoing the harness along with the tests we write for it, and the current solution is basically to get the three "necessary" events (Segment, Stream-Start and Caps) pushed so we don't have to worry about them. You can "control" these initial events by when you actually add and/or _play the src-harness. Now if you have a use-case of having more of these events arriving later, serialized with the buffers, I can see the problem.

However, I think I am leaning towards a mechanism of turning off all "convenience-magic" (like the forward-pad is), and then letting the user handle all pulling and pushing of buffers and events manually. This can actually already be acheived by simply having two separate harnesses, and then pulling and pushing between the two "manually".

The alternative would be to do a bigger rewrite where (a bit like you propose) all buffers and events end up in the same queue, have _pull return a GstMiniObject, and have convenience pull_buffer and pull_event doing a check-cast-and-return. This however I feel is moving the scale flexibility<---->convenience to far on the flexibility side, sacrificing some of the ease-of-use and simplicity I believe we have achived quite well currently.
Comment 3 Olivier Crête 2015-07-23 16:28:17 UTC
I'm totally favourable to magically forwarding events, the problem is that the events get forwarded in the streaming thread, so the unit tests is not in control of when they are forwarded, leading to races. The attached patch forwards the event automatically on _pull() if you have a sink_forward_pad, so in that case, it should not change the behaviour.

We could rename the current gst_harness_pull_event() to _pull_event_oob() and make the synchronized one into the main pull_event() function and in it just drop all buffers until an event is received, a bit like it drops or forwards events if you wait for a buffer.

My test sends the original caps event only with the first buffer, not on gst_harnness_play(), and the synchronized events cause the clock id to be unscheduled, which breaks the crank, as I have no way to know if those events have been forwarded or not.

But before we commit to the API for 1.6, I'd really like to fix this thing somehow. I'll fix any tests that have been upstreamed, tests that haven't don't count!
Comment 4 Håvard Graff (hgr) 2015-07-23 18:42:54 UTC
Lets at least try and agree on something! We have currently (quick grep) 1473 tests using GstHarness, and it would be terribly sad if we had to fork it already given all the effort we have put into getting it upstream, and the steady stream of tests we have been providing ever since.

I have a strong preference to keep pull_event and pull as is, so that buffers and events are separated, and can be individually inspected and pushed onwards if desirable. This is the simplest solution, that also gives full flexibility in writing tests.

Now, since 1.x, there are three events that needs to be present before you can start pushing buffers, the Segment, Caps and Stream-Start. It quickly became tedious to always have to manually pull and push these three events from src-harness to main-harness in every test, so the forward-pad was born.

I completely agree that this has some shortcomings. It currently only forwards STREAM_START, CAPS and SEGMENT, but there could be more of CAPS coming later which probably should not be magically forwarded (but rather put on the event-queue for inspection).

Thinking about it, it *should* be allowed to turn off the forwarding. This allows you to drop, modify or forward the events coming from the src-harness (which could have some applications), and it allows taking control of a case like you describe, where you would simply disable the forwarding, and then use alternating gst_harness_src_push_event and gst_harness_push_from_src to get the sequence right (and deterministic).

So I would suggest adding a gst_harness_disable_forwarding or something like that, and keeping everything else as-is, as this would solve your problem, and not create any new ones for us! :)
Comment 5 Olivier Crête 2015-07-23 19:49:33 UTC
Created attachment 308029 [details] [review]
Just disabling the forwarding is annoying because it means the really useful

multi-push functions can no longer be used and I need to "know" the exact order
in which the src harness produced the buffers and events to reproduce the stream.
So instead I just added a toggle to switch between behaviours.

I'd be curious for you to look at my test in case you have a better idea
https://git.collabora.com/cgit/user/tester/gst-plugins-bad.git/commit/?h=aggregator-queue-3&id=f05415e8946db2616d2acc8f14c2dbcb3f34ae4c from this branch
https://git.collabora.com/cgit/user/tester/gst-plugins-bad.git/log/?h=aggregator-queue-3
Comment 6 Håvard Graff (hgr) 2015-07-23 20:46:37 UTC
I will look closer at the test tomorrow, but the forward-pad is not at all related to the multi-push functions, and they will work exactly the same way. I would also say that if you element is doing something unusual with the initial events (and hence not able to use the simple forwarding), you *should* "know" the exact order :)
Comment 7 Håvard Graff (hgr) 2015-07-23 22:45:52 UTC
Here is a patch with a way to disable forwarding (with tests!)
https://github.com/pexip/gstreamer/commit/2eb0c6da4b1bc04ca82909827f2cd11fb50c1ed6.patch
Comment 8 Stian Selnes (stianse) 2015-07-24 12:16:10 UTC
Created attachment 308076 [details]
Example audiointerleave tests

Olivier, please see two example implementations for test_audiointerleave_2ch_smallbuf. The first uses the current GstHarness, the second uses GstHarness with Havard's patch for disabling event forwarding.
Comment 9 Olivier Crête 2015-07-30 01:34:09 UTC
Interesting take on the tests. Any reason you don't check the type of events? I ended up adding a little utility function to make sure the forwarded events are as expected:

static void
forward_check_event (GstHarness * h, GstHarness * hsrc, GstEventType type)
{
  GstEvent *e;

  e = gst_harness_pull_event (hsrc);
  fail_unless (GST_EVENT_TYPE (e) == type);
  gst_harness_push_event (h, e);
}

If you think the disable forwarding patch is a good idea I'll merge it, otherwise I'll use the version without using the src-harness stuff
Comment 10 Håvard Graff (hgr) 2015-07-30 05:41:56 UTC
I think so, since it can give you full control of what gets passed when.
Comment 11 Tim-Philipp Müller 2015-08-09 15:06:46 UTC
commit 28100e0b6ae556e6abaf7decc45a85c230e58823
Author: Havard Graff <havard.graff@gmail.com>
Date:   Fri Jul 24 00:41:57 2015 +0200

    harness: add _set_forwarding function
    
    To be able to disable the slightly "magic" forwarding of the
    necessary events between the harnesses.
    
    Also introduce a new test-suite for GstHarness, that documents the
    feature, and should hopefully expand into documenting most of the
    features the harness possesses.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=752746