GNOME Bugzilla – Bug 755732
rtmpsrc: Slow teardown of connection when pipeline is interrupted
Last modified: 2018-11-03 13:40:56 UTC
The RTMP plugin can be slow to stop when interrupted (with SIGINT) as it waits for the network socket to timeout before stopping. librtmp has this timeout set by default at 30 seconds. Here is an example output with timestamps: Sep 25 16:38:13 Setting pipeline to PAUSED ... Sep 25 16:38:13 Pipeline is PREROLLING ... Sep 25 16:38:15 Redistribute latency... Sep 25 16:38:15 Pipeline is PREROLLED ... Sep 25 16:38:15 Setting pipeline to PLAYING ... Sep 25 16:38:15 New clock: GstSystemClock Sep 25 16:38:16 handling interrupt. <-------------- interrupt with SIGINT Sep 25 16:38:16 Interrupt: Stopping pipeline ... Sep 25 16:38:16 Execution ended after 0:00:00.951296656 Sep 25 16:38:16 Setting pipeline to PAUSED ... Sep 25 16:38:16 Setting pipeline to READY ... Sep 25 16:38:46 Setting pipeline to NULL ... <-------------- 30 seconds delay Sep 25 16:38:46 Freeing pipeline ... The log shows going from READY->NULL took 30 seconds.
Created attachment 312302 [details] [review] Patch to use GstPoll on sockets I have created a patch (0002-rtmpsrc-Poll-changes-on-RTMP-socket.patch) for this which uses GstPoll so that we don't block the plugin inside librtmp. Using GstPoll means we can block inside gst_rtmp_src_create() which gives us control to unblock quickly when unlocking the plugin. I think this might also help with https://bugzilla.gnome.org/show_bug.cgi?id=729099 and unsafe multi-threading of librtmp. Note there is an existing indentation problem on line 437 of gstrtmpsrc.c that gst-indent fixes. I've got this in a separate patch (0001-rtmpsrc-Fix-indentation-with-gst-indent.patch).
Created attachment 312303 [details] [review] Fix indentation in gstrtmpsrc.c with gst-indent
Review of attachment 312302 [details] [review]: ::: ext/rtmp/gstrtmpsrc.c @@ +343,3 @@ + * to timeout on its recv call. */ + poll_state = gst_poll_wait (src->poll, GST_CLOCK_TIME_NONE); + if ((poll_state == -1) && (errno == EBUSY)) { The extra parenthesis are not needed here :) @@ +661,2 @@ if (src->rtmp) { + gst_poll_set_flushing (src->poll, FALSE); Shouldn't this be flushing in stop(), and non-flushing in start()?
>@@ +661,2 @@ > if (src->rtmp) { >+ gst_poll_set_flushing (src->poll, FALSE); > > Shouldn't this be flushing in stop(), and non-flushing in start()? My assumption was that create and stop are in the same thread so we can't simultaneously by blocked in create and in stop. Where as unlock was a separate thread (and can flush the polling). Is this correct view of how it works? I put this non-flushing in stop() to just reset the poll to a good state as this is where we clear up src->rtmp and close any sockets. I could also do the same in start() to ensure the the poll is non-flushing before we start to use it.
create() and stop() are not in the same thread
Created attachment 313114 [details] [review] Patch to use GstPoll on sockets Made updates to patch as suggested: * Poll is flushed in stop and non-flushing in start. * Removed extra parenthesis.
Comment on attachment 313114 [details] [review] Patch to use GstPoll on sockets Now only misses the set_flushing(FALSE) in unlock_stop(). The other question is, if RTMP_Read() does anything else than reading... like writing to the socket for keep-alive or similar things if no data is available for reading.
John, are you going to update the patch?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/306.