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 585708 - [adder] Wrong handling of flushing seeks
[adder] Wrong handling of flushing seeks
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal major
: 0.10.24
Assigned To: Wim Taymans
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-06-14 07:23 UTC by Edward Hervey
Modified: 2009-07-30 16:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
new test for adder (4.15 KB, patch)
2009-06-14 21:04 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review

Description Edward Hervey 2009-06-14 07:23:59 UTC
When doing a *flushing* seek on an element/pipeline one expects the following to happen synchronously:
* the event is sent
* some element will handle the seek
  * Sends out a FLUSH_START
  * Streaming stops
  * Figures out if it can handle the seek and if so configures itself accordingly
  * Sends out a FLUSH_STOP
  * Returns True if the streaming thread will push data according to the requested seek, else False
* The seek event push returns to the user with either True/False

If the seek is successfull, the streaming threads start pushing the newsegment, tags and buffers (in that order, tags being optional).


This means that if one does the following on an element:
1. asynchronously blocks the source pad,
2. Sends a flushing seek which succeeds,
3. asynchronoulsy un-blocks the source pad.

... we expect the first thing to come out of the pad to be a new_segment.



With adder this is not the case, since the FLUSH_STOP event is sent from the streaming thread and not from the thread that did the flushing seek (i.e. it does not send those events synchronously).

Because of this... we end up with adder pushing out a stray FLUSH_STOP event... and breaks the whole synchronization when used in GNonLin.

Adder (or maybe collectpads?) should be modified to collect the flush_start/flush_stop events coming into its sink pads (which are received synchronously) and send out FLUSH_STOP once all incoming streams have stopped flushing in a synchronous fashion.


Because of this issue, using adder in GNonLin operations fail, which results in any gnonlin-based software (pitivi and other various scripts) not being able to do proper audio mixing.


I have tried to figure out what's wrong within adder in vain. But some commits do look suspicious.
* 4228ba0c6b095da4cd24e76a576aff5733b6c91e. This commit message and the comment in the code states that flush_stop should be sent (from the streaming threads) if the seek failed... yet the code is doing it ... if the seek succeeds ! Are the two comments wrong and the code correct... or the opposite ?
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2009-06-14 17:56:25 UTC
Regarding the 2nd of my commits, it should be if (!result). Let me explain the problem at the time of the change:

If one conncted a live-src to adder and send a flushing seek, it did a flush start, the seek failed and no one did a flush stop. Thus the pipeline never played. With git HEAD it does not happen anymore. I am writing tests for the scenarios right now and then will see if I can use git bisect to find what fixed it and when.
Comment 2 Stefan Sauer (gstreamer, gtkdoc dev) 2009-06-14 21:04:38 UTC
Created attachment 136580 [details] [review]
new test for adder

GST_CHECKS="test_live_seeking" GST_DEBUG_NO_COLOR=1 GST_DEBUG="*:2,base*:4,adder:4,check:4" make elements/adder.check 2>debug.log

This is the problem. Adders src pad ends up beeing in flushing.

0:00:00.434794025  2194  0x81e79c0 INFO                 basesrc gstbasesrc.c:2291:gst_base_src_loop:<src1> pausing after gst_pad_push() = wrong-state
0:00:00.434814138  2194  0x81e79c0 DEBUG                basesrc gstbasesrc.c:2317:gst_base_src_loop:<src1> pausing task, reason wrong-state

But this seems to be not related to adder anymore. Need to stop for now, its getting too late :)
Comment 3 Edward Hervey 2009-06-15 05:34:11 UTC
(In reply to comment #1)
> Regarding the 2nd of my commits, it should be if (!result). Let me explain the
> problem at the time of the change:
> 
> If one conncted a live-src to adder and send a flushing seek, it did a flush
> start, the seek failed and no one did a flush stop. Thus the pipeline never
> played. With git HEAD it does not happen anymore. I am writing tests for the
> scenarios right now and then will see if I can use git bisect to find what
> fixed it and when.
> 

The fact that live sources didn't do the corresponding flush stop was definitely a bug. So if that's fixed it's all good now and that hack should go away.

The question is ... how should adder react if upstream failed to seek ? imho... it should stop all seeking and return False downstream.

In any case.. it should not break behaviour for the cases where you only have seekable sources upstream.

BTW, if you're using live sources... wouldn't liveadder from -bad be a better choice ?
Comment 4 Wim Taymans 2009-06-15 10:32:53 UTC
Since the live non-seekable source drops the seek event, no flush-start or flush stop is sent, leaving the adder sinkpads in the flushing state. It should likely unset the flushing state on the particular sinkpad when the seek fails.
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2009-06-16 19:32:44 UTC
I filed a separate bug regarding the live-source seeking behaviour as bug #586033.

Regarding live-adder. I don't expect missing buffers in my usecases, so it won't help.
Comment 6 Wim Taymans 2009-06-17 09:27:16 UTC
this fixes the live seeking test in Comment #2

commit 85dbf9351515b9ad9e5e9c01fa811af2fb71943c
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Wed Jun 17 11:22:51 2009 +0200

    adder: more seeking fixes.
    
    When a seek failed upstream, make sure the adder sinkpad is set unflushing again
    so that streaming can continue.
    We only have a pending segment when we flushed.
    Set the flush_stop_pending flag inside the appropriate locks and before we
    attempt to perform the upstream seek.
    Add some more comments.
    Use the right lock to protect the flags in flush_stop.
    
    See #585708

Comment 7 Edward Hervey 2009-07-30 13:23:44 UTC
Now behaves consistently for me. Stefan, are we good to close this now ?
Comment 8 Stefan Sauer (gstreamer, gtkdoc dev) 2009-07-30 15:14:28 UTC
Yes, lets close this one.