Created attachment 326695 [details] [review]
0001-pad-Handle-offsets-and-sticky-events-more-consistent.patch

I am attaching a patch that solves this issue. Please review.

Pushed a unit test for testing the pad offsets on src pads, which works. The same thing on sink pad fails as expected

commit c8cae6a94bec0534ee46490fffae7c7c2b43f842
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Sat Jun 11 21:37:47 2016 +0300

    pad: Add unit test for pad offset handling on src pads
    
    https://bugzilla.gnome.org/show_bug.cgi?id=765049

The attached patch does not fix it for sink pads though

Created attachment 329624 [details] [review]
pad: Handle offsets and sticky events more consistently

When handling data, always call check_sticky before and after handling it.
Also call check_sticky for sink pads and, in this case, call send_event
instead of push_event.

This patch is solving the issue where, if the offset of a sink pad is changed
inside the pad probe, the change has no effect until the next segment event.

Created attachment 329625 [details] [review]
pad: Add unit test for pad offset handling on sink pads

The test is failing for sinkpads because for srcpads we always have the original segment event stored on the pad, and then modify the original one with the offset. For sinkpads we modify the event and store it, so pad offset changes are accumulating and getting completely wrong over time.

We either have to store the original segment event somewhere (ugly), or revert the previous offset change somehow which requires storing that somewhere as we otherwise don't have it anymore (also ugly). Not sure yet what to do here.

Created attachment 330790 [details] [review]
0001-pad-Handle-offsets-and-sticky-events-more-consistent.patch

Now storing the original event and only passing the copy around. It works, but it fails in the gstfunnel.c check.

In the check it fails after the second gap event that I'm pasting:

  /* push a gap event to srcpad2 to push sticky events */
  fail_unless (gst_pad_push_event (td.mysrc2, gst_event_new_gap (0,
              GST_SECOND)));

  fail_unless (nb_stream_start_event == 2);
  fail_unless (nb_caps_event == 2);
  fail_unless (nb_segment_event == 2);
  fail_unless (nb_gap_event == 2);

  /* push a gap event to srcpad2 */
  fail_unless (gst_pad_push_event (td.mysrc2, gst_event_new_gap (0,
              GST_SECOND)));

  fail_unless (nb_stream_start_event == 2);
  fail_unless (nb_caps_event == 2);
  fail_unless (nb_segment_event == 2);
  fail_unless (nb_gap_event == 3);

Which leads me to this code:
static gboolean
gst_funnel_sink_event (GstPad * pad, GstObject * parent, GstEvent * event)
{
[...]
  } else if (GST_EVENT_TYPE (event) == GST_EVENT_GAP) {
    /* If no data is coming and we receive GAP event, need to forward sticky events. */
    unlock = TRUE;
    GST_PAD_STREAM_LOCK (funnel->srcpad);
    GST_OBJECT_LOCK (funnel);
    gst_object_replace ((GstObject **) & funnel->last_sinkpad,
        GST_OBJECT (pad));
    GST_OBJECT_UNLOCK (funnel);
    gst_pad_sticky_events_foreach (pad, forward_events, funnel->srcpad);


By reading that code, I am not sure why that failing check is there at all, I deduce that the element is now working as expected. Of course, I might have missed something here, so please feedback.

The GstFunnel behavior of forwarding sticky events when no data is coming and a GAP event is received (including the failing test) was introduced in this bug report:

https://bugzilla.gnome.org/show_bug.cgi?id=738202

It might help clarify the issue here.

This commit fixes the failing test:

commit 528fbfe7c10fa63142a42d3f62dcd9d694527a17
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Jul 7 13:15:51 2016 +0300

    funnel: Only forward sticky events on GAP events if needed
    
    That is, if the active pad changed and if forwarding of sticky events is
    requested at all. We otherwise forward events too often.



However it actually uncovered a problem in your patch. The problem is that in gst_pad_push_event_unchecked() you always create a copy of the event (also only do that if there is actually a pad offset). This means that every time the same event goes through there, a new copy is made. And store_sticky_event() (on the sink pad, the peer) then has gst_event_replace() return TRUE (it compares pointers, which are different now), which then causes the GST_PAD_FLAG_PENDING_EVENTS to be set.

Now the next time sticky events are sent, they are all sent because of that flag. Which in the case of funnel meant on every GAP event for example.
(Also the apply_pad_offset() is not only for SEGMENT events but also for others)


We could now either store the original and modified events all the time (and invalidate the modified ones in various situations)... or somehow improve the event comparison to not just look at the to-be-replaced pointer.

Correction: send_event_unchecked(), not push_event_unchecked(). We pass on the copied event (new pointer every time), but store the unmodified one.
So the eventfunc and downstream will always see a new pointer in any case.

Independent of that, get_sticky_event() and sticky_foreach() should probably also return the modified events for sinkpads.


All in all this seems like we need to store the original and modified event for sinkpads.

Created attachment 331010 [details] [review]
pad: Handle offsets and sticky events more consistently

When handling data, always call check_sticky before and after handling it.
Also call check_sticky for sink pads and, in this case, call send_event
instead of push_event.

This patch is solving the issue where, if the offset of a sink pad is changed
inside the pad probe, the change has no effect until the next segment event.

Created attachment 331011 [details] [review]
pad: More consistency fixes with sticky events on sinkpads

Created attachment 331012 [details] [review]
pad: Make sure to only forward sticky events when handling events/queries in downstream direction

Otherwise we might push events downstream while something is being propagated
upstream, which usually leads to deadlocks.

So the patch seems acceptable as-is now, the funnel problem also was solved as part of the new patches.


There is just one open question: We now always store the unmodified event (without pad offset applied). This is the same behaviour as for srcpads but it has the disadvantage that something like gst_pad_sticky_foreach(sinkpad, push_to_src_pad) will copy over unmodified events while it should modified ones. Same goes for gst_pad_get_sticky_event().

I think this boils down to a semantic question. IMHO pad offsets are applied between pads, so a srcpad should not return the modified event when asked for it but a sinkpad should. And also pad probes on srcpads and sinkpad should see the modified event (they do with Vivia's patch).

Main problem with gst_pad_sticky_foreach() modifying the event is... that the callback is allowed to modify the event. Which means that we would have to un-apply the pad offset if the callback modified it and then store that back.

After discussion with Wim on IRC, this should be the way forward:
1) probes on both pads should give the modified events
2) sticky_foreach/get_sticky on both pads should give the modified events
3) in the end, the modified events should be stored on the pads (and unmodified again when the offset is changed)


1) and 2) should be fairly simple, and at that point we have the wanted behaviour. 3) might be trivial, or could require bigger changes. We'll have to try that.

I like that better too (storing/providing the modified events).

So we are in agreement on what to do, someone just needs to FDI?

Yes, I have it somewhere at the top of my todo list... but postpone it again every week because other stuff gets in between. It will be done soonish hopefully :)

See https://bugzilla.gnome.org/show_bug.cgi?id=795330#c8 and the following comments. This basically contradicts what I said in comment 15 here, or rather restricts it a bit.

*** Bug 795330 has been marked as a duplicate of this bug. ***

To summarize the discussion in the other bug, what should happen is that the above should only apply to pads that are *arriving* in pads, not events that are *pushed out* of a pad.

That is, downstream events in sinkpads and upstream events in srcpads should get any modifications from pad probes and pad offsets applied to them before being passed to their event function. And the modified events should also be stored (in case of sticky events in sinkpads) inside the pads.

The patches above will have to be adjusted accordingly.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/168.