[Bug 760532] bin, pipeline, sink: state change / async-done propagation issue with repeated rtspsrc seeks

[GStreamer (GNOME Bugzilla)]

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

--- Comment #23 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
(In reply to Sebastian Dröge (slomo) from comment #21)
> Created attachment 326878 [details] [review]
> rtspsrc: Don't propagate spurious state change returns from internal
> elements further
> 
> We handle them inside rtspsrc and override them in all other cases anyway, so
> do the same for "internal" state changes like PAUSED->PAUSED and
> PLAYING->PLAYING.
> 
> This keeps unexpected NO_PREROLL to confuse state changes in GstBin.

This patch alone fixes the testcase... half. There seems to be an unrelated
problem with it freezeing every ~100 seeks for a few 10 seconds, just to
continue properly again afterwards. Which is probably something wrong on the
RTSP side.


Independent of that, there are at least two other problems with state changes.
One of which I understand fully now, the other needs further investigation:

1) Pipelines with a NO_PREROLL and ASYNC element. The initial state change to
PLAYING will succeed just fine, as the pending state of everything will be
PLAYING. The bin will go to PLAYING immediately, the sink will follow soonish
afterwards (commit_state there will go to PLAYING). So far so good.

Now once the sink loses the PLAYING state, it will get pending=PAUSED. If the
bin forgot the NO_PREROLL (because PAUSED->PLAYING returned SUCCESS from the
NO_PREROLL element), everything is fine. The bin will go to pending=PAUSED too
due to the ASYNC_START message and once the sink has prerolled will move
everything over to PLAYING again.

Question 1: is it intentional that a) the bin forgets about NO_PREROLL and b)
the bin goes to PAUSED too if the sink loses state... but would go to PLAYING
immediately without waiting for the sink during the initial state change to
PLAYING? It seems inconsistent at best.

Now the actual problem is if the bin did not forget about NO_PREROLL because of
timing related reasons (which the rtspsrc fix works around for this specific
case). The bin will then ignore the ASYNC_START message, the sink will preroll
at some point and send ASYNC_DONE which is also ignored ("nothing pending). And
that's it, in commit_state in basesink pending=PAUSED, so we just go to PAUSED
and wait there for someone else to move us to PLAYING. Usually the bin would be
responsible for lifting the element from PAUSED to PLAYING again but the bin
ignored ASYNC_DONE (and ASYNC_START to begin with) so won't do it.

Question 2: should the bin check on ASYNC_DONE if a) itself has no state change
pending but b) the element that posted ASYNC_DONE is not in the target state
(note: pending state is PAUSED here, target state is PLAYING) yet, and if so
lift the element to PLAYING without affecting any other elements?


2) ASYNC_START being ignored if a state change is already happening. For
ASYNC_DONE there is code for handling this case already: if polling==TRUE it
will remember that there were ASYNC_DONE pending, and if so will handle the
ASYNC_DONE before setting polling to FALSE again.

For ASYNC_START there are two cases where we ignore them. If polling==TRUE and
if GST_STATE_PENDING() is not VOID_PENDING. The first case can be solved
similar to ASYNC_DONE, the second case seems not solveable easily as
GST_STATE_PENDING() is outside the control of the bin. E.g.
gst_element_state_continue() (as called from gst_element_change_state()) will
call this. But from my understanding, the GST_STATE_PENDING() check also does
not make sense: if a state is pending, we either would not modify it or
rightfully lower the pending state (from PLAYING to PAUSED) because a new async
state change is necessary.
I think this is how these should be solved, but I'm struggling a bit with that.


And 3) is a potential problem I'm not 100% sure about yet.
gst_bin_continue_func() and handle_async_start() seem to be racy. If
ASYNC_START is received between scheduling gst_bin_continue_func() and actually
doing gst_bin_continue_func(), I think we might miss an asynchronous state
change altogether.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.