Created attachment 331006 [details] [review]
funnel: Always push all sticky events whenever we forward a serialized event

Otherwise downstream will have an inconsistent set of sticky events at this
point, e.g. when a TAG event is pushed and downstream wants to relate it to
the stream by looking at the current STREAM_START event.

I guess serialized events should indeed be treated like buffers.. Or we can not forward serialized sticky event before a buffer is received on that pad?

What do you mean with that? There's nothing preventing serialized events before buffers, no. Like the initial TAG events you usually see :)

Funnel doesn't forward any sticky event before it gets the first buffer (or GAP event), then it forwards all sticky events. So sticky events are currently sent only for the current pad (which means that the other ones have already been forwarded when the pad was changed).

The only non-sticky serialized event we have are segment-done and flush-stop.. and flushing/seeking is a bit awkward for funnel.. It should probably implement the same logic as the aggregator if we cared about flushing and flushing seeks.

What's the goal of this patch anyway? I'd be tempted to suggest we reject it unless something breaks with the current functionality.

The point is to make it consistent and not just special case GAP events. If anything, GAP and other serialized events should be handled the same here.

Also the sink event function seems to forward sticky events as they arrive?

Sticky events are only forwarded for the current pad:
"
 else if (pad != funnel->last_sinkpad) {
      forward = FALSE;
    }
"

So the only serialized events that are just forwarded are flush_stop and egment_done.

I'm still not sure why the current way is not right, I think we have 3 kinds of serialized things:
 - Buffers and gaps: those are forwarded immediately
 - Sticky events: They describe the surrounding buffers (and are useless on their own), so we only forward them with a gap or buffer
 - Flushes and segment_done: Those would need special handling around seeks, but I don't think anything does seeks through a funnel?

Indeed, I misread the sticky event handling.

The current way is not right because we forward non-sticky serialized events immediately without updating the "context" (the set of sticky events related to the stream). For GAP events this is an obvious problem (but GAP is in no way special), for custom serialized events (or any non-sticky serialized events that we might want to add later) this could very well be a problem and it just seems wrong.

Alternatively we could just drop non-sticky serialized events if they're not for the current pad, but then they're gone obviously ;)

We forward sticky events before GAP events, we probably just want to replace "GST_EVENT_TYPE (event) == GST_EVENT_GAP" with "GST_EVENT_IS_SERIALIZED (event). And also do the same thing on flush-stop. ?

That's exactly what my patch does though :) It forwards all serialized events if the code above considers them forwardable (that is: all non-sticky serialized events and all sticky events that are considered forwardable above, and flush-stop which has forward=true).

Olivier, any further opinions? :)

Comment on attachment 331006 [details] [review]
funnel: Always push all sticky events whenever we forward a serialized event

Going to merge after 1.10

Attachment 331006 [details] pushed as c56e1d1 - funnel: Always push all sticky events whenever we forward a serialized event