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 751994 - rtsp : problem when switching from one rtspsrc to another with totem
rtsp : problem when switching from one rtspsrc to another with totem
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.4.5
Other Linux
: Normal normal
: 1.15.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 757624 796886 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-07-05 20:56 UTC by Fabrice Bellet
Modified: 2018-07-28 13:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
partial debug log of a totem session (8.49 KB, text/plain)
2015-07-05 20:56 UTC, Fabrice Bellet
  Details
full log of a totem session, with debug enabled (1.50 MB, text/plain)
2018-07-23 11:23 UTC, Fabrice Bellet
  Details
full log of a totem session, with debug enabled and patch applied (2.88 MB, text/plain)
2018-07-23 11:24 UTC, Fabrice Bellet
  Details
rtspsrc: ensure the TEARDOWN message is sent (1.95 KB, patch)
2018-07-23 12:18 UTC, Fabrice Bellet
none Details | Review
rtspsrc: ensure the TEARDOWN message is sent (1.51 KB, patch)
2018-07-24 13:51 UTC, Fabrice Bellet
none Details | Review
rtspsrc: Add a small configurable teardown delay (6.21 KB, patch)
2018-07-26 11:37 UTC, Jan Schmidt
none Details | Review
rtspsrc: Add a small configurable teardown delay (6.25 KB, patch)
2018-07-26 11:38 UTC, Jan Schmidt
none Details | Review
rtspsrc: Add a small configurable teardown delay (6.27 KB, patch)
2018-07-26 14:08 UTC, Jan Schmidt
none Details | Review

Description Fabrice Bellet 2015-07-05 20:56:14 UTC
Created attachment 306874 [details]
partial debug log of a totem session

When using a playlist in totem containing several rtsp streams, it seems that the transition from one stream to another fails to send the "TEARDOWN" message to the server, which causes problem in my case, because the server consider that the previous stream in just in paused state, and refuses to serve the next one with a "RTSP/1.0 453 Not Enough Bandwidth" error.

For what I understand of the problem, the state changes too quickly from GST_STATE_CHANGE_PLAYING_TO_PAUSED, GST_STATE_CHANGE_PAUSED_TO_READY and GST_STATE_CHANGE_READY_TO_NULL, and the thread has only the time to sent the "PAUSE" command, before closing the connection ?

The attachement shows the debugging log when the stream transition occurs. gst_rtspsrc_close() is called with only_close=1, from gst_rtspsrc_stop(), so no TEARDOWN message is sent.
Comment 1 Edward Hervey 2018-05-12 08:00:40 UTC
Do you still get the issue with current GStreamer ?

If so : can you provide a bigger log (GST_DEBUG=2,*decodeb*:5,*rtsp*:5) ? I have this feeling it's due to something else.
Comment 2 Fabrice Bellet 2018-07-23 11:23:26 UTC
Created attachment 373122 [details]
full log of a totem session, with debug enabled

When skipping to service=372 in totem, the media server fails to start this new stream and replies with a 453 error, "Not Enough Bandwidth".
Comment 3 Fabrice Bellet 2018-07-23 11:24:55 UTC
Created attachment 373123 [details]
full log of a totem session, with debug enabled and patch applied
Comment 4 Fabrice Bellet 2018-07-23 12:18:01 UTC
Created attachment 373124 [details] [review]
rtspsrc: ensure the TEARDOWN message is sent

This possible fix works for me.
Comment 5 Fabrice Bellet 2018-07-24 13:51:41 UTC
Created attachment 373145 [details] [review]
rtspsrc: ensure the TEARDOWN message is sent

Here are some more details.

The problem happens when the transition from state PAUSED to READY (calling gst_rtspsrc_close()) and from READY to NULL (calling gst_rtspsrc_stop()) are too close, and the stop function cancels the still in progress close function.

It is important for the close function to not be cancelled, because it is responsible to send the TEARDOWN message to the server. If this message cannot be sent, it may cause a wrong bandwidth estimation on the server's side, and ultimately prevent another source to be started.
Comment 6 Jan Schmidt 2018-07-26 11:36:01 UTC
I think this is a dup of Bug 757624, and related to the previously fixed Bug 748360.
Comment 7 Jan Schmidt 2018-07-26 11:37:01 UTC
I wrote a similar patch, but with a timeout to prevent waiting forever if the TCP connection stalls. We never want to block shutting down a pipeline for longer than necessary.
Comment 8 Jan Schmidt 2018-07-26 11:37:14 UTC
Created attachment 373169 [details] [review]
rtspsrc: Add a small configurable teardown delay

This causes rtspsrc to send a teardown and wait on
PAUSED->READY transition, with a configurable delay.
Otherwise, typically teardown never gets sent in
playbin / uridecodebin where the transition back to NULL
happens too quickly.

The timeout is set to 100ms default.
Comment 9 Jan Schmidt 2018-07-26 11:38:24 UTC
Created attachment 373170 [details] [review]
rtspsrc: Add a small configurable teardown delay

This causes rtspsrc to send a teardown and wait on
PAUSED->READY transition, with a configurable delay.
Otherwise, typically teardown never gets sent in
playbin / uridecodebin where the transition back to NULL
happens too quickly.

The timeout is set to 100ms default.
Comment 10 Fabrice Bellet 2018-07-26 13:57:58 UTC
It works for me, thanks! Just add the src object when calling GST_WARNING_OBJECT().
Comment 11 Jan Schmidt 2018-07-26 14:07:01 UTC
Oops sorry -last second change to replace an errant g_print in the first patch :)
Comment 12 Jan Schmidt 2018-07-26 14:08:22 UTC
Created attachment 373173 [details] [review]
rtspsrc: Add a small configurable teardown delay

This causes rtspsrc to send a teardown and wait on
PAUSED->READY transition, with a configurable delay.
Otherwise, typically teardown never gets sent in
playbin / uridecodebin where the transition back to NULL
happens too quickly.

The timeout is set to 100ms default.
Comment 13 Jan Schmidt 2018-07-26 14:42:56 UTC
Pushed to master:

commit f067b50dd6d0344f134b0aa6d2453879e73caccb (HEAD -> master)
Author: Jan Schmidt <jan@centricular.com>
Date:   Fri Jul 27 00:41:57 2018 +1000

    rtspsrc: Add a small configurable teardown delay
    
    This causes rtspsrc to send a teardown and wait on
    PAUSED->READY transition, with a configurable delay.
    Otherwise, typically teardown never gets sent in
    playbin / uridecodebin where the transition back to NULL
    happens too quickly.
    
    The timeout is set to 100ms default.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=751994
Comment 14 Jan Schmidt 2018-07-26 14:45:29 UTC
*** Bug 757624 has been marked as a duplicate of this bug. ***
Comment 15 Olivier Crête 2018-07-26 15:52:52 UTC
Blocking in paused->ready on the application thread is really nasty. Can we please avoid that? I think we somehow instead need a way to do async state transitions?
Comment 16 Jan Schmidt 2018-07-26 16:00:32 UTC
Downward state transitions are never async, we guarantee that (for GStreamer 1.0 at least), so I couldn't see a way to avoid it. I guess neither can anyone else, which is why we've had broken TEARDOWN behaviour for years and 3 year old bugs.

The line has always been "don't block on downward state transitions", but various elements always have. Closing a file might block while data is written out, for example.

This seems like a less-bad solution in that it at least has a time limit, and the default is small enough that I don't think it'll bother anyone.
Comment 17 Tim-Philipp Müller 2018-07-26 16:04:22 UTC
I think it's quite acceptable in this case given the small timeout and it fixes a real issue that comes up a LOT.

We block in other cases too (e.g. filesrc, filesink) and there it's not even interruptable. So you're not wrong but it seems better to me to do this than leave it broken like it is now.

If you have ideas how to do it better, let's open a bug about that and we can then change the implementation over to that once it lands?
Comment 18 Sebastian Dröge (slomo) 2018-07-27 07:45:39 UTC
(In reply to Jan Schmidt from comment #16)
> Downward state transitions are never async, we guarantee that (for GStreamer
> 1.0 at least), so I couldn't see a way to avoid it.

PLAYING->PAUSED in basesink is async (and that's generally unexpected and sometimes causes problems).
Comment 19 Niels De Graef 2018-07-27 14:22:29 UTC
Seems both thaytan and me were working on the same thing at the same time. I have another fix that does not have the 100ms delay, but not so sure of any side effects. Anyone care to see if the fix in bug 796886 might also be a possibility?
Comment 20 Jan Schmidt 2018-07-28 13:06:42 UTC
*** Bug 796886 has been marked as a duplicate of this bug. ***