and to reinforce how I feel hacking in ext/ogg: http://bit.ly/18heoKh

(In reply to comment #0)
> Created an attachment (id=253094) [details] [review]
> WIP patch
> 
> Here's some initial code that adds flushing seek support to oggmux. Likely not
> 100% correct/complete, but works for the simple case audiotestsrc ! vorbisenc !
> oggmux ! filesink
> 
> The logic is copied from (my limited understanding of) videomixer. Things i'm
> not sure about:
> 
> 1) not sure if what happens in the case upstream fails to seek after
> gst_ogg_mux_handle_src_event has already sent FLUSH_START is correct. In that
> case gst_ogg_mux_collected will send a flush_stop but won't send a new segment.
> Should it?

What happens when a seek fails? Doesn't it just error out anyway?

> 2) FLUSH_STOP aggregation is done by forwarding the first FLUSH_STOP and then
> ignoring the others. That's what videomixer does but what if this case happens:
> one upstream sends FLUSH_STOP; another upstream sends more data belonging to
> the old segment; the other upstream finally sends FLUSH_STOP. Isn't this racy?

I think you should wait until all FLUSH_START of all upstreams are received. After that you can safely forward the very first FLUSH_STOP. Any data that is sent from upstream before the FLUSH_STOP is dropped because the pads are flushing.

> 3) related to 2), the code that handles the _SEEK sets collectpads to flushing,
> grabs GST_COLLECT_PADS_STREAM_LOCK, sets some internal flags, unsets
> collectpads flushing. Is that really enough? shouldn't it wait until upstreams
> have started flushing?

Wait until upstream started flushing until doing what?

> And a final note: the muxer still outputs a start=0, stop=-1 segment. I wonder 
> if it's worth making it output the incoming seek segment given that other
> places in the code make the assumption that timestamps start from 0 (see
> gst_ogg_mux_decorate_buffer for example)

After a seek it should output exactly the seek segment.

(In reply to comment #2)

> > 2) FLUSH_STOP aggregation is done by forwarding the first FLUSH_STOP and then
> > ignoring the others. That's what videomixer does but what if this case happens:
> > one upstream sends FLUSH_STOP; another upstream sends more data belonging to
> > the old segment; the other upstream finally sends FLUSH_STOP. Isn't this racy?
> 
> I think you should wait until all FLUSH_START of all upstreams are received.
> After that you can safely forward the very first FLUSH_STOP. Any data that is
> sent from upstream before the FLUSH_STOP is dropped because the pads are
> flushing.

Actually the waiting shouldn't even be necessary. It should just be ensured that collectpads drop data from all pads as soon as the flushing seek is received (or possibly when the first FLUSH_START is received on any pad). See bug #706441

Created attachment 253344 [details] [review]
new attempt

Here's a better attempt which (i think) implements what we discussed on IRC. 

The steps involved are:

1) when the flushing seek is received, mark that we're flushing and forward the event upstream. Do not set collectpads as flushing here else forwarding the event wouldn't work obviously (and doing set_flushing(TRUE); set_flushing(FALSE); forward; would be racy)

2a) on the first FLUSH_START coming from an upstream pad, flush internal state and set collectpads as flushing. Setting collectpads as flushing means that every pad is marked as FLUSHING until it gets the next FLUSH_STOP

2b) mark that we need to send FLUSH_STOP in the first next _collected call

3) in the first _collected call, send FLUSH_STOP downstream. We flush here since if we're in _collected, it means that all the pads have finished flushing (FLUSH_STOP) and have a buffer queued belonging to the new segment

(and btw: this is how I feel now that I understand oggmux/collectpads a little more: http://bit.ly/12JXa4r)

Created attachment 253349 [details] [review]
new version

changed gst_ogg_mux_flush to send flush_start() downstream to unblock the GST_COLLECT_PADS_STREAM_LOCK before calling gst_collect_pads_set_flushing()

Hm, looks like there's an issue with this approach. Non-serialized events are not blocked by collectpads so sending FLUSH_STOP in _collected doesn't work.

Created attachment 253468 [details] [review]
new patch

New patch that changes the FLUSH_STOP logic a little. Instead of sending FLUSH_STOP from the first _collected() call after a flush, we now send FLUSH_STOP right after flushing. This way oggmux:src and downstream are ready to process events/data immediately. Buffers will still resume traveling downstream again after all upstreams have finished flushing, but events that are not queued by collectpads (like _TAG events), will be able to be pushed by the upstream elements that have finished flushing (possibly while some instead are still flushing), without things failing because oggmux:src is flushing.

...or just look at gst_ogg_mux_flush in the patch.

This patch is functional now and i'll try to add a test. In the meantime, can anyone spot anything obviously wrong?

Review of attachment 253468 [details] [review]:

Maybe there should be some collectpads API to make this more easy, also for other elements?

::: ext/ogg/gstoggmux.c
@@ +369,3 @@
+      if (g_atomic_int_compare_and_exchange (&ogg_mux->pending_flush, TRUE,
+              FALSE)) {
+        gst_ogg_mux_flush (ogg_mux);

This now sends flush-stop from the thread that called flush-start. flush-start is OOB, so can happen from any thread. flush-stop is serialized and must happen from the streaming thread and with the srcpad stream lock.

Created attachment 253576 [details] [review]
new day, new patch

Ok this moves sending FLUSH_STOP to when the first FLUSH_STOP is received from upstream.

If we agree that the logic is alright now, I can try to move it down to collectpads and implement it in videomixer as well.

Review of attachment 253576 [details] [review]:

Looks good :) Also adder and interleave have the same code as videomixer

::: ext/ogg/gstoggmux.c
@@ +384,3 @@
+         * mmkay?
+         */
+        gst_pad_push_event (ogg_mux->srcpad, gst_event_new_flush_stop (TRUE));

Why not just forward the event here? ;)

Alessandro?

Created attachment 255151 [details] [review]
Fixes an issue which prevents the not racy scenario to actually not be racy.

Current implementation uses the public list, which sometimes doesn't contain newly-added pad. It's fairly trivial to highlight this with our test suite. This uses the private list which contains *all* the pads.

Oops, sorry have been traveling and forgot about this. I'll post patches to collectpads/oggmux later today...

Comment on attachment 255151 [details] [review]
Fixes an issue which prevents the not racy scenario to actually not be racy.

Obsolete with Alessandro's collectpads changes

Created attachment 257105 [details] [review]
updated against new collectpads API

Created attachment 257108 [details] [review]
push the correct segment after seeking

This patch makes output timestamps after a seek match the target start/stop timestamps, instead of always starting at ts=0. Attaching as a separate patch as i'm not 100% sure this is allowed in ogg.

Let's close this one for now. Before adding this to oggmux or anything else, we first of all need to define a design how flushing seeks should behave in muxers so we can get consistent and meaningful behaviour.