GNOME Bugzilla – Bug 736041
Protect rtsp transport data.
Last modified: 2014-09-16 08:48:38 UTC
Created attachment 285349 [details] [review] gst-rtsp-server patch Fix a crash. possible for more than one thread to manipulate rtsp transports data.
Why is a recursive mutex needed here?
This is necessary because some functions in rtsp-stream-transport.c is calling other functions in rtsp-stream-transport.c . An exampel If calling gst_rtsp_stream_transport_set_active ( take mutex ) this is calling update_transport that is calling gst_rtsp_stream_transport_get_transport ( take mutex )
Maybe there should be internal versions of those functions that don't take the mutexes then
Created attachment 285454 [details] [review] gst-rtsp-server patch New patch
- do the notifies outside of the lock - gst_rtsp_stream_transport_get_stream() does it need the lock? it can never change after creating the transport. - callback such as send_rtp, send_rtcp, keep_alive should be done outside of the lock, if possible.
Created attachment 285710 [details] [review] gst-rtsp-server patch New patch - callback such as send_rtp, send_rtcp, keep_alive should be done outside of the lock, if possible. This is not possible, this is what happens for us.
+ Trace 234074
Thread 4 (Thread 21491)
Thread 11 (Thread 21417)
The do_send_data( send_rtp callback ) is crash because the client is NULL. And if we look at thread 11 f #13 and f #14 we can see that unlink_session_transports (client, session, sessmedia) is called just before gst_rtsp_session_media_set_state. The unlink_session_transports have been called the function unlink_transport ( this is tcp case ) that have called the gst_rtsp_stream_transport_set_callbacks (trans, NULL, NULL, NULL, NULL); So the mutex have to protect the callbacks and userdata.
The problem is in how this code is structured. There is no locking in stream_transport because it is all supposed to be locked from the stream and media objects. Adding those locks is rather pointless because the objects (transport, url) are not refcounted outside of the locks. Adding a lock around the function call is usually done with a recursive mutex. This would add overhead, I think I have a better idea by restructuring the code a little. Let me propose another patch.
I'm thinking of redoing things differently here: http://cgit.freedesktop.org/~wtay/gst-rtsp-server/log/?h=transport-simplify
I tested with patches. client: simplify session transport handlingtransport-simplify stream-transport: make method to handle received data And that have solved our problem.
Wim, are those changes ready to be merged?
commit 0292be09eaf338f16c752aee0c317b1e6b3971e2 Author: Wim Taymans <wtaymans@redhat.com> Date: Tue Sep 9 18:11:39 2014 +0200 client: simplify session transport handling link/unlink of the transport in a session was done to keep track of all TCP transports and to send RTP/RTCP data to the streams. We can simplify that by putting all the TCP transports in a hashtable indexed with the channel number. We also don't need to link/unlink the transports when we pause/resume the streams. The same effect is already achieved when we pause/play the media. Indeed, when we pause the media, the transport is removed from the media and the callbacks will not be called anymore. See https://bugzilla.gnome.org/show_bug.cgi?id=736041 commit ea5d4cfc7e29d010c0d7b6a2f606de76a6d117f4 Author: Wim Taymans <wtaymans@redhat.com> Date: Tue Sep 9 18:10:12 2014 +0200 stream-transport: make method to handle received data Make a method to handle the data received on a channel. It sends the data to the stream of the transport on the RTP or RTCP pads based on the channel number.