Created attachment 43679 [details] [review] Suggested fix Hi, seems I found a problem with following situation: You have a FsRtpSession with FsRtpStream using FsNiceTransmitter, and you want to remove this session from running conference. However, when you release your references to stream and session calling g_object_unref(), the session is not destroyed. Looking closer what is happening in fs_rtp_session_dispose(), we see that the real dispose is delayed until session's conference has any internal threads running. List of threads is a GPtrArray managed in fs_rtp_conference_handle_message(). Threads are removed from list upon receiving stream status message GST_STREAM_STATUS_TYPE_LEAVE, but the message is never received in this case. Core of the problem seems to be function fs_nice_transmitter_free_gst_stream() where GstNiceSrc elements are first removed from the pipeline and then set to state GST_STATE_NULL. Setting NULL state will trigger posting the GST_STREAM_STATUS_TYPE_LEAVE messages the FsRtpConference is waiting for, but because the source was already removed from pipeline, conference will not get the message and thus FsRtpSession will never enter fs_rtp_session_real_dispose(). I suggest to fix this by rearranging the order of operations in fs_nice_transmitter_free_gst_stream() to first set the nice src to NULL state to allow messages to be propagated, and then to remove GstNiceSrc. Attached minimal example code demonstrates the issue, without the patch applied, there are still GStreamer elements corresponding to the removed session present in the pipeline when program ends. With applied patch, they are properly disposed. (Set GST_DEBUG_DUMP_DOT_DIR environ variable to get the pipeline graphs) I have also one more question relating to this part of Farsight code. In fs_rtp_session_dispose() we wait until ALL conference's internal threads are ended. That means that when we have more than one session inside a conference, any removed session is not completely disposed as long as there are still other running sessions in the conference. Is there a reason to do this? Could not be the session disposed immediately when only threads started by its own gstreamer elements are ended?
Created attachment 43680 [details] Trivial demonstration code
Arg, this is annoying.. Re-ordering doesn't work, as the nicesrc could be restarted by the bin if someone does a state change on the pipeline (or if one adds a sink which will cause a lsot state and playing->paused->playing transitions to happen). Also, the fact that it waits for all threads to go away is nasty.. I think the right solution is to add an explicit _stop() function that must be manually called by the user. Just like we have states in GStreamer. That _stop() function would be exactly the real_dispose() method that we already have. You can simulate that by running g_object_dispose(fssession); directly in your code. If that fixes the problem, I think that's what I'll do, and if you dispose of the session without calling that method first, it would do a g_critical() (just like gst elements that haven't gone to playing). Other ideas welcome.
(In reply to comment #2) Hi Olivier, > Re-ordering doesn't work, as the nicesrc could be restarted by the bin if > someone does a state change on the pipeline (or if one adds a sink which will > cause a lsot state and playing->paused->playing transitions to happen). Please can you describe more closely why the patch will not work? I call gst_element_set_locked_state() on the nicesrc prior setting its state to GST_STATE_NULL. According to doc this "Locks the state of an element, so state changes of the parent don't affect this element anymore." so I do not completely understand how nicesrc can be restarted after this point in code. If I understand it correctly, locking the state should isolate nicesrc from the state changes on the pipeline, so it can be without fear switched to GST_STATE_NULL state and removed. If you have a look, the same lock state - set to NULL - remove operation is used when removing recvonly_filters later in the very same fs_nice_transmitter_free_gst_stream() function. Why it will work in this case, but not with nicesrc? BR Jakub
Actually, the session's internal dispose is not delayed until every thread from the conference has ended. It just checks if fs_rtp_session_dispose() is called from a GstTask-initiated thread from inside the conference. If it is a case, it creates another thread to stop the session from.
(In reply to comment #4) > It just checks if fs_rtp_session_dispose() is called from a GstTask-initiated > thread from inside the conference. If it is a case, it creates another thread > to stop the session from. Thanks for this explanation! Then, I think, the bug is in fs_rtp_conference_is_internal_thread(): gboolean fs_rtp_conference_is_internal_thread (FsRtpConference *self) { guint i; gboolean ret = FALSE; GST_OBJECT_LOCK (self); for (i = 0; i < self->priv->threads->len; i++) { if (g_ptr_array_index (self->priv->threads, i)) { ret = TRUE; break; } } GST_OBJECT_UNLOCK (self); return ret; } I think there should be if (g_ptr_array_index (self->priv->threads, i) == g_thread_self ()) The current function only returns FALSE when the array is full of NULLs After I reverted my previous changes and applied this one, everything started to work correctly, no more leftover elements in GStreamer pipeline.
Yep, I came to the same conclusion and its fixed now.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.